Add UnwrapFrame: reverse of Wrap

[abhinav]

Per discussion in #71, add an UnwrapFrame function
that wraps the outermost errtrace frame from an error.
As a result of this change,
it's possible to implement buildTraceTree or a variant of it
using exported functionality exclusively.

This makes it possible for users to use a different trace formatting
mechanism than the trees generated by FormatString.

Resolves #71

[prashantv]

this is a good place to start, just wanted to think about a couple of things:

• should we return a frame with the func/line as frields, or should we instead return a type that wraps the pc and lazily pulls the frame as needed.
• I wonder if there's a performance benefit in CallersFrames when multiple pcs are passed.

I don't think this function is expected to be used in any performance-sensitive paths, so don't think it's worth optimizing for it yet.

In the future, if batching is faster, we may want to switch to doing that internally (in which case we couldn't use this function), which is still OK.

[abhinav]

Yeah, I was unsure about expose Frame with decoded information or just a hidden pc that loads the frame lazily.
I started with exposed frame since that's what we had for traceFrame, and it's the most "here's just data,"
but I think lazy decoding leaves room for frame iteration in the future, should we want that.

I wonder if there's a performance benefit in CallersFrames when multiple pcs are passed.

Doesn't look like it. https://cs.opensource.google/go/go/+/refs/tags/go1.22.2:src/runtime/symtab.go;l=91
You pay for one caller at a time, although it pays for two frames for the initial Next() call if there are multiple frames.

[prashantv]

Re: lazy vs eager, I'm leaning towards the eager approach like you have it for simplicity. That said, what do you think our own Frame type vs exposing the runtime type?

Re: performance benefit of batching,

I wrote a benchmark for an error with ~6 frames and compared a for loop that builds up []uintptr of PCs, and uses CallerFrames to then generate a []string of function names, and compared doing the same using UnwrapFrame. The time is similar, though it is more allocation-friendly to batch CallerFrames calls,

BenchmarkUnwrapBatch 	 2007859	       578.3 ns/op	     464 B/op	       3 allocs/op
BenchmarkUnwrapFunc  	 1301568	       924.6 ns/op	    1552 B/op	      13 allocs/op

So if we want to optimize the tree building, batching may be worth it, but I don't think that's worth changing the API for.

[cbandy]

• should we return a frame with the func/line as frields, or should we instead return a type that wraps the pc and lazily pulls the frame as needed.

Are there any plans to not use PCs in the errtrace implementation?

In https://go.dev/issue/63358, a few people have said that []uintptr seems like a good way to represent a trace, though https://go.dev/issue/60873 may be more appropriate for that discussion.

Personally, I do not mind creating runtime.Frames myself from one or more uintptr. 🤔 I think I just want some way to read the unexported pc field.

[abhinav]

Are there any plans to not use PCs in the errtrace implementation?

No, no plans to use non-PC sources of information.
[]uintptr for a trace seems like a good idea, but those discussions do not seem close to resolving.
I think we could expose these frames as-is for now, and add a means of exposing the []uintptr in the future.
That may happen anyway if we implement #104.

[prashantv]

optional: should we use runtime.Frame here?

[prashantv]

Overall, I like using UnwrapFrame as the API, and seeing it simplify our internal tree building gives me confidence in the level of abstraction.

For exposing frames, I think the current API is fine, though wanted to capture alternatives and tradeoffs. Do you think there's much value in our own type over the runtime type? A couple of scenarios it could be useful:

• If we want to allow custom wrapping, where we allow callers to specify the function / file:line even if it doesn't correspond to a real PC, but I'm not sure if we want to support that use-case.
• If we want to use our own PC --> func/file:line logic for speed or other reasons, rather than coupling to runtime.

[abhinav]

Do you think there's much value in our own type over the runtime type?

My main reason for that was to get the custom String() function, but I'm not tied to that. We can use runtime.Frame.
That has a side benefit of exposing the pc as well for folks like #102 (comment).

[abhinav]

I've added a runtime.Frame-based version in a new commit just to see how it looks.
CC @prashantv

[prashantv]

My main reason for that was to get the custom String() function, but I'm not tied to that. We can use runtime.Frame. That has a side benefit of exposing the pc as well for folks like #102 (comment).

Yeah there's a few options here:

• Some underlying method on the error that returns the pc uintptr directly (or maybe []uintptr), that can be used as an interface that other types can implement
• Method to extract a single frame like we have that could work with other types if they match an interface
  • Whether the frame is our own type or the runtime type

I'm leaning more towards exposing the pc uintptr directly in the future if there's some standard method, and using the runtime.Frame for now to minimize code in errtrace, but don't feel strongly either way (our own type vs runtime type).

[abhinav]

I'm leaning more towards [..] using the runtime.Frame for now

Excellent. That's what we're doing now!

[prashantv]

I've added a runtime.Frame-based version in a new commit just to see how it looks. CC @prashantv

Nice, my stamp was from before I saw the latest commit, but I'm fine with either with a slight preference for runtime.Frame -- I do like that it exposes the pc (if we want to do that eventually anyway) without having to decide what interface/method we should use the expose the PC directly (though that'd still be nice for compatibility with other errors that have a pc in the future)