-
Notifications
You must be signed in to change notification settings - Fork 323
feat(elfunwindinfo): add LookupFDE #545
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
Conversation
ed6f9ee
to
0896d47
Compare
@@ -311,6 +311,20 @@ func (r *reader) expression() ([]dwarfExpression, error) { | |||
return expr, nil | |||
} | |||
|
|||
// formatLen returns the length of a field encoded with enc encoding. | |||
func (r *reader) formatLen(enc encoding) int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems r
is not used at all, could this be made just a regular function helper?
// already previously validated in `validateEhFrameHdr`. | ||
r := ehFrameHdrSec.reader(unsafe.Sizeof(*h), false) | ||
// newHdrParser returns a new ehFrameHdrParser | ||
func (ee *elfExtractor) newHdrParser(ehFrameHdrSec *elfRegion) (hp ehFrameHdrParser, err error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ee
is not used at all. Could this be turned to a regular function instead? This would allow also simplifying callers.
fdeAddr, ehFrameSec.vaddr) | ||
} | ||
fr := ehFrameSec.reader(fdeAddr-ehFrameSec.vaddr, false) | ||
_, err = ee.parseFDE(&fr, ef, ipStart, hp.cieCache, true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this here seems quite over kill. ee.parseFDE
is called for LookupFDE
also and doing a ton of extra work. I think the parseHdrEntry
should be untangled from elfExtractor
and be made member of ehFrameHdrParser
. It should only parse and return ipStart
and fdeAddr
pointers.
The binary lookup needs only this ipStart
for the comparison. And in the final stage it can the get the FDE corresponding fdeAddr
. For the binary lookup, it might make sense to write a specific minimal FDE extractor that just gets the ipStart
and ipLen
members without decoding anything else.
nativeunwind/elfunwindinfo/lookup.go
Outdated
@@ -0,0 +1,96 @@ | |||
// Copyright The OpenTelemetry Authors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename should probably be elfehframelookup.go
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These filenames in this packages are extremely hard to read. Including the one you suggest.
nativeunwind/elfunwindinfo/lookup.go
Outdated
// LookupFDE performs a binary search in .eh_frame_hdr for an FDE covering the given addr. | ||
func LookupFDE(ef *pfelf.File, addr uintptr) (FDE, error) { | ||
ehFrameHdrSec, ehFrameSec, err := findEhSections(ef) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be wrapped in a NewEhFrameTable
similar to NewGopclntab
to avoid multiple findEhSections
calls in case of multiple lookups in future?
nativeunwind/elfunwindinfo/lookup.go
Outdated
if err != nil { | ||
return FDE{}, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will become redundant when the sort lambda actually parses only the binary index.
nativeunwind/elfunwindinfo/lookup.go
Outdated
hook := lookupHook{} | ||
ee := elfExtractor{ | ||
ref: nil, | ||
file: ef, | ||
hooks: &hook, | ||
deltas: nil, | ||
allowGenericRegs: false, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the objects used by this function should not involve elfExtractor
. This allows removing the lookupHook
too, and simplifying and speeding up this code quite a bit.
nativeunwind/elfunwindinfo/lookup.go
Outdated
type lookupHook struct { | ||
fde fdeInfo | ||
} | ||
|
||
func (f *lookupHook) fdeHook(_ *cieInfo, fde *fdeInfo) bool { | ||
f.fde = *fde | ||
return false | ||
} | ||
func (f *lookupHook) deltaHook(_ uintptr, _ *vmRegs, _ sdtypes.StackDelta) {} | ||
func (f *lookupHook) golangHook(_, _ uintptr) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should go away.
elfRef := pfelf.NewReference(filename, pfelf.SystemOpener) | ||
defer elfRef.Close() | ||
elf, err := elfRef.GetELF() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to use reference here? Just do pfelf.Open
?
Or even better, do something: elf, err := pfelf.NewFile(bytes.NewReader(buffer), 0, false)
to completely avoid using temporary files and work on the memory object itself.
filename := filepath.Join(t.TempDir(), "dwarf_extract_elf_") | ||
err = os.WriteFile(filename, buffer, 0o600) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This created file is never removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are removed by the testing package at the test cleanup.
I've replaced it with bytes buffer as you suggested anyways
35cc31b
to
3a7d2a7
Compare
fde := fdeInfo{sorted: sorted} | ||
fde.len, fde.ciePos, err = r.parseHDR(false) | ||
fde = fdeInfo{} | ||
fdeLen, fde.ciePos, err = r.parseHDR(false) | ||
if err != nil { | ||
// parseHDR returns unconditionally the CIE/FDE entry length. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bebae53
to
1709ab4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting to look pretty good! Thanks! Some nits, and final change of requesting to make/move LookupFDE
to be a member of ehFrameTable
. And then make the ehFrameTable
and its constructor from pfelf.File
public. This will make the exposed API more flexible and easier to extend in the future.
// formatLen returns the length of a field encoded with enc encoding. | ||
func formatLen(enc encoding) int { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Even though this is generic code, this is only ever used by the elfehframetable.go
code, perhaps it would be better to move it there?
// parses first fields of FDE, specifically PC Begin, PC Range | ||
func parseFDEHDR(r *reader, ef *pfelf.File, ipStart uintptr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// parses first fields of FDE, specifically PC Begin, PC Range | |
func parseFDEHDR(r *reader, ef *pfelf.File, ipStart uintptr, | |
// parsesFDEHeader parses first fields of FDE, specifically the CIE, PC Begin and the PC Range. | |
func parsesFDEHeader(r *reader, ef *pfelf.File, ipStart uintptr, |
idx-- | ||
if idx < 0 { | ||
return FDE{}, errors.New("FDE not found") | ||
} | ||
t.position(idx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
idx-- | |
if idx < 0 { | |
return FDE{}, errors.New("FDE not found") | |
} | |
t.position(idx) | |
if idx <= 0 { | |
return FDE{}, errors.New("FDE not found") | |
} | |
t.position(idx - 1) |
// LookupFDE performs a binary search in .eh_frame_hdr for an FDE covering the given addr. | ||
func LookupFDE(ef *pfelf.File, addr libpf.Address) (FDE, error) { | ||
t, err := newEhFrameTable(ef) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think LookupFDE
should be a member of ehFrameTable
. And make newEhFrameTable
and ehFrameTable
public.
This would allow the user to make multiple queries to the table without parsing the headers again.
I know currently there is no use case for it, but this would be symmetric with the way gopclntab API works, and allow for the possibilty in the future. I would not be surprised if some interpreters wanted to chase FDE blocks, or even walk the full FDE table in future. Having the pre-processed table header exposed as struct
would also allow extending this API more easier in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with few last minute nits. Pre-approving. @florianl or @christos68k would you have time to look at this too?
// position adjusts the reader position to point at the table entry with idx index | ||
func (e *EhFrameTable) position(idx int) { | ||
tableEntrySize := formatLen(e.hdr.tableEnc) * 2 | ||
e.r.pos = e.tableStartPos + uintptr(tableEntrySize*idx) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last minute nit: this makes EhFrameTable
stateful and non-concurrent. Not a problem currently, but would be worth documenting, or fixing this. Simple fix would be to remove this and add index
to parseHdrEntry
.
@@ -157,3 +160,63 @@ func TestExtractStackDeltasFromFilename(t *testing.T) { | |||
} | |||
require.Equal(t, data.Deltas[:len(firstDeltas)], firstDeltas) | |||
} | |||
|
|||
func TestLookupFDE(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last minute nit: perhaps worth a a separate elfehframetable_test.go
? Or just remove the whole file to ehframe_test.go
since now its testing more than stack deltas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just comments on the style. Functionality works fine for me 👍
Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>
eb9458f
to
e8d9533
Compare
This is a helper for #416 alternative to #544
The new functionality is useful if someone knows an address in the middle of a function but does not know the range of the function and would like to find out the range of the function. (for example _PyEval_EvalFrameDefault.cold function, see the linked issue and comments)