Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GetCaller for use in error helpers #108

Merged
merged 4 commits into from
Nov 10, 2024
Merged

Add GetCaller for use in error helpers #108

merged 4 commits into from
Nov 10, 2024

Conversation

prashantv
Copy link
Contributor

@prashantv prashantv commented Nov 10, 2024

Fixes #70

errtrace.Wrap captures the immediate caller, which doesn't compose
with existing error-wrapping helpers. To support this use-case, add
errtrace.GetCaller() that helpers can use to capture their caller
and add that to the error trace.

This approach was chosen over the typical skip callers as:

  • Skip requires a more complex assembly implementation to skip an
    arbitrary number of frames, which is more error-prone, e.g.,
    handling the skip argument being too large.

  • Faster as each call only skips a fixed (and predictable) number of
    frames. This is especially important since it's expected that
    errtrace is used to annotate all frames in the return path.

  • Avoids skip calculations, and generally more flexible to pass around
    compared to skipping frames, which is tied to the stack.

Regardless of which approach is chosen, there is a limitation:
incompatibility with inlined callers. Go tracks inlined code
and PCs of inlined code is correctly mapped back, but the stack
does not contain inlined frames, so the skip calculation done
cannot correctly skip inlined frames. This approach only requires
the error helper to have a stack frame, which is typically the case
when calling errtrace.GetCaller followed by Wrap, but callers
should use go:noinline for correct stack frames.

@prashantv prashantv requested a review from abhinav November 10, 2024 03:38
@prashantv
Copy link
Contributor Author

prashantv commented Nov 10, 2024

This talk on mid-stack inliningwas helpful in understanding how Go tracks inlined PCs:
https://docs.google.com/presentation/d/1Wcblp3jpfeKwA0Y4FOmj63PW52M_qmNqlQkNaLj0P5o/edit#slide=id.p

runtime.Callers skips correctly even in the presence of inline frames using the inlining tables.

(and for reference, how the trace unwinder unwinds)

@prashantv prashantv changed the base branch from main to prashant/version November 10, 2024 04:12
@prashantv
Copy link
Contributor Author

This change is part of the following stack:

Change managed by git-spice.

wrap_caller.go Outdated Show resolved Hide resolved
wrap_caller.go Outdated Show resolved Hide resolved
wrap_caller.go Show resolved Hide resolved
Copy link
Contributor

@abhinav abhinav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm besides comments

Base automatically changed from prashant/version to main November 10, 2024 05:14
`errtrace.Wrap` captures the immediate caller, which doesn't compose
with existing error-wrapping helpers. To support this use-case, add
`errtrace.GetCaller()` that helpers can use to capture their caller
and add that to the error trace.

This approach was chosen over the typical `skip` callers as:

 * Skip requires a more complex assembly implementation to skip an
   arbitrary number of frames, which is more error-prone, e.g.,
   handling the skip argument being too large.

 * Faster as each call only skips a fixed (and predictable) number of
   frames. This is especially important since it's expected that
   `errtrace` is used to annotate all frames in the return path.

 * Avoids skip calculations, and generally more flexible to pass around
   compared to skipping frames, which is tied to the stack.

Regardless of which approach is chosen, there is a limitation:
incompatibility with inlined callers. Go tracks inlined code
and PCs of inlined code is correctly mapped back, but the stack
does not contain inlined frames, so the skip calculation done
cannot correctly skip inlined frames. This approach only requires
the error helper to have a stack frame, which is typically the case
when calling `errtrace.GetCaller` followed by `Wrap`, but callers
should use `go:noinline` for correct stack frames.
@prashantv prashantv merged commit 3ad5434 into main Nov 10, 2024
26 checks passed
@prashantv prashantv deleted the prashant/pc-caller branch November 10, 2024 06:08
prashantv added a commit that referenced this pull request Nov 10, 2024
Follow-up to #108, missed the changelog update in that PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SkipCaller(int) to support helper function around errtrace
2 participants