feat(elfunwindinfo): add LookupFDE by korniltsev · Pull Request #545 · open-telemetry/opentelemetry-ebpf-profiler

korniltsev · 2025-06-23T13:41:50Z

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)

fabled · 2025-06-24T19:21:19Z

 }

+// formatLen returns the length of a field encoded with enc encoding.
+func (r *reader) formatLen(enc encoding) int {


Seems r is not used at all, could this be made just a regular function helper?

fabled · 2025-06-24T19:24:41Z

-	// 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) {


Seems ee is not used at all. Could this be turned to a regular function instead? This would allow also simplifying callers.

fabled · 2025-06-24T19:32:32Z

+			fdeAddr, ehFrameSec.vaddr)
+	}
+	fr := ehFrameSec.reader(fdeAddr-ehFrameSec.vaddr, false)
+	_, err = ee.parseFDE(&fr, ef, ipStart, hp.cieCache, true)


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.

fabled · 2025-06-24T19:34:51Z

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


The filename should probably be elfehframelookup.go or similar.

These filenames in this packages are extremely hard to read. Including the one you suggest.

fabled · 2025-06-24T19:37:50Z

+// 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 {


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?

fabled · 2025-06-24T19:42:25Z

+	if err != nil {
+		return FDE{}, err
+	}


Will become redundant when the sort lambda actually parses only the binary index.

fabled · 2025-06-24T19:43:48Z

+	hook := lookupHook{}
+	ee := elfExtractor{
+		ref:              nil,
+		file:             ef,
+		hooks:            &hook,
+		deltas:           nil,
+		allowGenericRegs: false,
+	}


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.

fabled · 2025-06-24T19:44:02Z

+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)                              {}


These should go away.

fabled · 2025-06-24T19:45:37Z

+	elfRef := pfelf.NewReference(filename, pfelf.SystemOpener)
+	defer elfRef.Close()
+	elf, err := elfRef.GetELF()


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.

fabled · 2025-06-24T19:47:56Z

+	filename := filepath.Join(t.TempDir(), "dwarf_extract_elf_")
+	err = os.WriteFile(filename, buffer, 0o600)


This created file is never removed.

They are removed by the testing package at the test cleanup.
I've replaced it with bytes buffer as you suggested anyways

florianl · 2025-06-25T10:08:49Z

+	fde = fdeInfo{}
+	fdeLen, fde.ciePos, err = r.parseHDR(false)
 	if err != nil {
 		// parseHDR returns unconditionally the CIE/FDE entry length.


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.

there is a special case

https://github.com/grafana/opentelemetry-ebpf-profiler/blob/3a7d2a71b522d806be947a6c6dc798bae28b9fb2/nativeunwind/elfunwindinfo/elfehframe.go#L1229-L1231

fabled

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.

fabled · 2025-06-26T04:53:39Z

+// formatLen returns the length of a field encoded with enc encoding.
+func formatLen(enc encoding) int {


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?

fabled · 2025-06-26T04:59:17Z

+// parses first fields of FDE, specifically PC Begin, PC Range
+func parseFDEHDR(r *reader, ef *pfelf.File, ipStart uintptr,


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,

fabled · 2025-06-26T05:07:13Z

+	idx--
+	if idx < 0 {
+		return FDE{}, errors.New("FDE not found")
+	}
+	t.position(idx)


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)

fabled · 2025-06-26T05:13:45Z

+// 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)


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.

fabled

LGTM with few last minute nits. Pre-approving. @florianl or @christos68k would you have time to look at this too?

fabled · 2025-06-26T06:42:53Z

+// 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)
+}


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.

fabled · 2025-06-26T06:44:07Z

 	require.Equal(t, data.Deltas[:len(firstDeltas)], firstDeltas)
 }
+
+func TestLookupFDE(t *testing.T) {


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.

florianl

Just comments on the style. Functionality works fine for me 👍

Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>

korniltsev requested review from a team as code owners June 23, 2025 13:41

korniltsev mentioned this pull request Jun 23, 2025

feat(elfunwindinfo): allow capturing an FDE during deltas extraction #544

Closed

feat(elfunwindinfo): add LookupFDE

0896d47

korniltsev force-pushed the korniltsev/lookup_fde branch from ed6f9ee to 0896d47 Compare June 23, 2025 13:42

fabled reviewed Jun 24, 2025

View reviewed changes

florianl reviewed Jun 25, 2025

View reviewed changes

Comment thread nativeunwind/elfunwindinfo/elfehframe.go

review fixes

3a7d2a7

korniltsev force-pushed the korniltsev/lookup_fde branch from 35cc31b to 3a7d2a7 Compare June 25, 2025 09:50

florianl reviewed Jun 25, 2025

View reviewed changes

Comment thread nativeunwind/elfunwindinfo/elfehframelookup.go Outdated

Update nativeunwind/elfunwindinfo/elfehframelookup.go

1709ab4

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

fabled reviewed Jun 26, 2025

View reviewed changes

korniltsev added 2 commits June 26, 2025 12:47

review fixes

5a6295f

lint

fa19621

fabled approved these changes Jun 26, 2025

View reviewed changes

florianl approved these changes Jun 26, 2025

View reviewed changes

Comment thread nativeunwind/elfunwindinfo/elfehframetable.go Outdated

Comment thread nativeunwind/elfunwindinfo/elfehframetable.go Outdated

korniltsev and others added 4 commits June 26, 2025 14:43

Update nativeunwind/elfunwindinfo/elfehframetable.go

fad438c

Co-authored-by: Florian Lehner <florianl@users.noreply.github.com>

Update nativeunwind/elfunwindinfo/elfehframetable.go

4990073

elfehframetable_test.go

e9163f1

add concurrency notice

e8d9533

korniltsev force-pushed the korniltsev/lookup_fde branch from eb9458f to e8d9533 Compare June 26, 2025 07:59

fabled merged commit 0ec6c0b into open-telemetry:main Jun 26, 2025
25 checks passed

This was referenced Jul 8, 2025

Update with changes from upstream parca-dev/opentelemetry-ebpf-profiler#74

Merged

fix jits parca-dev/opentelemetry-ebpf-profiler#75

Closed

This was referenced Aug 14, 2025

fix golabels tests parca-dev/opentelemetry-ebpf-profiler#92

Closed

fix golabels test parca-dev/opentelemetry-ebpf-profiler#97

Closed

		filename := filepath.Join(t.TempDir(), "dwarf_extract_elf_")
		err = os.WriteFile(filename, buffer, 0o600)

		// formatLen returns the length of a field encoded with enc encoding.
		func formatLen(enc encoding) int {

		// parses first fields of FDE, specifically PC Begin, PC Range
		func parseFDEHDR(r reader, ef pfelf.File, ipStart uintptr,

Conversation

korniltsev commented Jun 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fabled left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabled left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florianl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants