Skip to content

Conversation

korniltsev
Copy link
Contributor

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)

@korniltsev korniltsev force-pushed the korniltsev/lookup_fde branch from ed6f9ee to 0896d47 Compare June 23, 2025 13:42
@@ -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 {
Copy link
Contributor

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) {
Copy link
Contributor

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)
Copy link
Contributor

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.

@@ -0,0 +1,96 @@
// Copyright The OpenTelemetry Authors
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Comment on lines 21 to 24
// 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 {
Copy link
Contributor

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?

Comment on lines 66 to 68
if err != nil {
return FDE{}, err
}
Copy link
Contributor

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.

Comment on lines 34 to 41
hook := lookupHook{}
ee := elfExtractor{
ref: nil,
file: ef,
hooks: &hook,
deltas: nil,
allowGenericRegs: false,
}
Copy link
Contributor

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.

Comment on lines 87 to 96
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) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

These should go away.

Comment on lines 205 to 207
elfRef := pfelf.NewReference(filename, pfelf.SystemOpener)
defer elfRef.Close()
elf, err := elfRef.GetELF()
Copy link
Contributor

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.

Comment on lines 202 to 203
filename := filepath.Join(t.TempDir(), "dwarf_extract_elf_")
err = os.WriteFile(filename, buffer, 0o600)
Copy link
Contributor

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.

Copy link
Contributor Author

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

@korniltsev korniltsev force-pushed the korniltsev/lookup_fde branch from 35cc31b to 3a7d2a7 Compare June 25, 2025 09:50
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Not directly related to this PR, but it seems like, there is a possible endless loop.
There are cases, e.g. here, here or here, where (*reader) parseHDR(..) returns 0 as fdeLen/hlen. This could cause a potential endless loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@korniltsev korniltsev force-pushed the korniltsev/lookup_fde branch from bebae53 to 1709ab4 Compare June 25, 2025 10:20
Copy link
Contributor

@fabled fabled left a 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.

Comment on lines 314 to 315
// formatLen returns the length of a field encoded with enc encoding.
func formatLen(enc encoding) int {
Copy link
Contributor

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?

Comment on lines 906 to 907
// parses first fields of FDE, specifically PC Begin, PC Range
func parseFDEHDR(r *reader, ef *pfelf.File, ipStart uintptr,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 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,

Comment on lines 31 to 35
idx--
if idx < 0 {
return FDE{}, errors.New("FDE not found")
}
t.position(idx)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
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)

Comment on lines 19 to 21
// 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)
Copy link
Contributor

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.

Copy link
Contributor

@fabled fabled left a 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?

Comment on lines +103 to +107
// 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)
}
Copy link
Contributor

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) {
Copy link
Contributor

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.

Copy link
Contributor

@florianl florianl left a 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 👍

@korniltsev korniltsev force-pushed the korniltsev/lookup_fde branch from eb9458f to e8d9533 Compare June 26, 2025 07:59
@fabled fabled merged commit 0ec6c0b into open-telemetry:main Jun 26, 2025
25 checks passed
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.

3 participants