Skip to content

Add GetCaller for use in error helpers - #108

Merged
prashantv merged 4 commits into
mainfrom
prashant/pc-caller
Nov 10, 2024
Merged

prashantv merged 4 commits into
mainfrom
prashant/pc-caller

Conversation

@prashantv

@prashantv prashantv commented Nov 10, 2024 •

Copy link
Copy Markdown
Contributor

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

prashantv commented Nov 10, 2024 •

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
Contributor Author

This change is part of the following stack:

Change managed by git-spice.

Comment thread wrap_caller.go Outdated
Comment thread wrap_caller.go Outdated
Comment thread wrap_caller.go

@abhinav abhinav left a comment

Copy link
Copy Markdown
Contributor

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
@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