Improve elfehframe handling

nativeunwind/elfunwindinfo/elfehframe.go

@@ -38,6 +38,8 @@ var errEmptyEntry = errors.New("FDE/CIE empty")
 
 // ehframeHooks interface provides hooks for filtering and debugging eh_frame parsing
 type ehframeHooks interface {
+	// fdeUnsorted is called if FDE entries from unsorted area are found.
+	fdeUnsorted()
 	// fdeHook is called for each FDE. Returns false if the FDE should be filtered out.
 	fdeHook(cie *cieInfo, fde *fdeInfo) bool
 	// deltaHook is called for each stack delta found
@@ -369,7 +371,6 @@ type fdeInfo struct {
 	ciePos  uint64
 	ipLen   uintptr
 	ipStart uintptr
-	sorted  bool
 }
 
 const (
@@ -976,7 +977,6 @@ func (ee *elfExtractor) parseFDE(r *reader, ef *pfelf.File, ipStart uintptr,
 		return uintptr(fdeLen), err
 	}
 	st := state{cie: cie, cur: cie.initialState}
-	fde.sorted = sorted
 
 	// Process the FDE opcodes
 	if !ee.hooks.fdeHook(st.cie, &fde) {
@@ -1006,6 +1006,7 @@ func (ee *elfExtractor) parseFDE(r *reader, ef *pfelf.File, ipStart uintptr,
 			}
 			ee.hooks.deltaHook(ip, &st.cur, delta)
 			ee.deltas.AddEx(delta, sorted)
+			sorted = true
 			hint = sdtypes.UnwindHintNone
 		}
 
@@ -1028,11 +1029,11 @@ func (ee *elfExtractor) parseFDE(r *reader, ef *pfelf.File, ipStart uintptr,
 
 	// Add end-of-function stop delta. This might later get removed if there is
 	// another function starting on this address.
-	ee.deltas.AddEx(sdtypes.StackDelta{
+	ee.deltas.Add(sdtypes.StackDelta{
 		Address: uint64(fde.ipStart + fde.ipLen),
 		Hints:   sdtypes.UnwindHintGap,
 		Info:    info,
-	}, sorted)
+	})
 
 	return uintptr(fdeLen), nil
 }
@@ -1178,16 +1179,13 @@ func (ee *elfExtractor) walkBinSearchTable(ef *pfelf.File, ehFrameHdrSec *elfReg
 	if err != nil {
 		return err
 	}
+	r := t.entryAt(0)
 	for f := uintptr(0); f < t.fdeCount; f++ {
-		var (
-			ipStart uintptr
-			fr      reader
-		)
-		ipStart, fr, entryErr := t.parseHdrEntry()
+		ipStart, fr, entryErr := t.decodeEntry(&r)
 		if entryErr != nil {
-			return err
+			return entryErr
 		}
-		_, err = ee.parseFDE(&fr, ef, ipStart, t.cieCache, true)
+		_, err = ee.parseFDE(&fr, ef, ipStart, t.cieCache, f > 0)
 		if err != nil && !errors.Is(err, errEmptyEntry) {
 			return fmt.Errorf("failed to parse FDE: %v", err)
 		}
@@ -1204,6 +1202,8 @@ func (ee *elfExtractor) walkFDEs(ef *pfelf.File, ehFrameSec *elfRegion, debugFra
 		return err
 	}
 
+	ee.hooks.fdeUnsorted()
+
 	// Walk the section, and process each FDE it contains
 	var entryLen uintptr
 	for f := uintptr(0); f < uintptr(len(ehFrameSec.data)); f += entryLen {

nativeunwind/elfunwindinfo/elfehframe_test.go

@@ -20,6 +20,9 @@ type ehtester struct {
 	found int
 }
 
+func (e *ehtester) fdeUnsorted() {
+}
+
 func (e *ehtester) fdeHook(cie *cieInfo, fde *fdeInfo) bool {
 	e.t.Logf("FDE ciePos %x, ip %x...%x, ipLen %d (enc %x, cf %d, df %d, ra %d)",
 		fde.ciePos, fde.ipStart, fde.ipStart+fde.ipLen, fde.ipLen,

nativeunwind/elfunwindinfo/elfehframetable.go

@@ -21,17 +21,21 @@ type FDE struct {
 }
 
 type EhFrameTable struct {
-	r             reader
-	hdr           *ehFrameHdr
-	fdeCount      uintptr
-	tableStartPos uintptr
-	ehFrameSec    *elfRegion
-	efm           elf.Machine
-	cieCache      *lru.LRU[uint64, *cieInfo]
+	fdeCount       uintptr
+	tableStartPos  uintptr
+	tableEntrySize int
+	tableEnc       encoding
+	ehFrameHdrSec  *elfRegion
+	ehFrameSec     *elfRegion
+	efm            elf.Machine
+
+	// cieCache holds the CIEs decoded so far. This is the only piece that is
+	// not concurrent safe, and could be made into a sync lru if needed.
+	cieCache *lru.LRU[uint64, *cieInfo]
 }
 
-// NewEhFrameTable creates a new EhFrameTable from the given pfelf.File
-// The returned EhFrameTable must not be used concurrently
+// NewEhFrameTable creates a new EhFrameTable from the given pfelf.File.
+// The returned EhFrameTable is not concurrent safe.
 func NewEhFrameTable(ef *pfelf.File) (*EhFrameTable, error) {
 	ehFrameHdrSec, ehFrameSec, err := findEhSections(ef)
 	if err != nil {
@@ -49,15 +53,14 @@ func NewEhFrameTable(ef *pfelf.File) (*EhFrameTable, error) {
 // LookupFDE performs a binary search in .eh_frame_hdr for an FDE covering the given addr.
 func (e *EhFrameTable) LookupFDE(addr libpf.Address) (FDE, error) {
 	idx := sort.Search(e.count(), func(idx int) bool {
-		e.position(idx)
-		ipStart, _, _ := e.parseHdrEntry() // ignoring error, check bounds later
+		r := e.entryAt(idx)
+		ipStart, _ := r.ptr(e.tableEnc) // ignoring error, check bounds later
 		return ipStart > uintptr(addr)
 	})
 	if idx <= 0 {
 		return FDE{}, errors.New("FDE not found")
 	}
-	e.position(idx - 1)
-	ipStart, fr, entryErr := e.parseHdrEntry()
+	ipStart, fr, entryErr := e.decodeEntryAt(idx - 1)
 	if entryErr != nil {
 		return FDE{}, entryErr
 	}
@@ -78,45 +81,50 @@ func (e *EhFrameTable) LookupFDE(addr libpf.Address) (FDE, error) {
 func newEhFrameTableFromSections(ehFrameHdrSec *elfRegion,
 	ehFrameSec *elfRegion, efm elf.Machine,
 ) (hp *EhFrameTable, err error) {
-	hp = &EhFrameTable{
-		hdr: (*ehFrameHdr)(unsafe.Pointer(&ehFrameHdrSec.data[0])),
-		r:   ehFrameHdrSec.reader(unsafe.Sizeof(ehFrameHdr{}), false),
-	}
-	if _, err = hp.r.ptr(hp.hdr.ehFramePtrEnc); err != nil {
-		return hp, err
+	hdr := (*ehFrameHdr)(unsafe.Pointer(&ehFrameHdrSec.data[0]))
+
+	r := ehFrameHdrSec.reader(unsafe.Sizeof(ehFrameHdr{}), false)
+	if _, err = r.ptr(hdr.ehFramePtrEnc); err != nil {
+		return nil, err
 	}
-	if hp.fdeCount, err = hp.r.ptr(hp.hdr.fdeCountEnc); err != nil {
-		return hp, err
+	fdeCount, err := r.ptr(hdr.fdeCountEnc)
+	if err != nil {
+		return nil, err
 	}
-	if hp.cieCache, err = lru.New[uint64, *cieInfo](cieCacheSize, hashUint64); err != nil {
-		return hp, err
+	cieCache, err := lru.New[uint64, *cieInfo](cieCacheSize, hashUint64)
+	if err != nil {
+		return nil, err
 	}
-	hp.ehFrameSec = ehFrameSec
-	hp.tableStartPos = hp.r.pos
-	hp.efm = efm
-	return hp, nil
+	return &EhFrameTable{
+		fdeCount:       fdeCount,
+		tableStartPos:  r.pos,
+		tableEntrySize: formatLen(hdr.tableEnc) * 2,
+		tableEnc:       hdr.tableEnc,
+		ehFrameHdrSec:  ehFrameHdrSec,
+		ehFrameSec:     ehFrameSec,
+		efm:            efm,
+		cieCache:       cieCache,
+	}, nil
 }
 
 // returns FDE count
 func (e *EhFrameTable) count() int {
 	return int(e.fdeCount)
 }
 
-// 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)
+// entryAt returns a reader for the binary search table at given index.
+func (e *EhFrameTable) entryAt(idx int) reader {
+	return e.ehFrameHdrSec.reader(e.tableStartPos+uintptr(e.tableEntrySize*idx), false)
 }
 
-// parseHdrEntry parsers an entry in the .eh_frame_hdr binary search table and the corresponding
-// entry in the .eh_frame section
-func (e *EhFrameTable) parseHdrEntry() (ipStart uintptr, fr reader, err error) {
-	ipStart, err = e.r.ptr(e.hdr.tableEnc)
+// decodeEntry decodes one entry of the binary search table from the reader.
+func (e *EhFrameTable) decodeEntry(r *reader) (ipStart uintptr, fr reader, err error) {
+	ipStart, err = r.ptr(e.tableEnc)
 	if err != nil {
 		return 0, reader{}, err
 	}
 	var fdeAddr uintptr
-	fdeAddr, err = e.r.ptr(e.hdr.tableEnc)
+	fdeAddr, err = r.ptr(e.tableEnc)
 	if err != nil {
 		return 0, reader{}, err
 	}
@@ -125,10 +133,15 @@ func (e *EhFrameTable) parseHdrEntry() (ipStart uintptr, fr reader, err error) {
 			fdeAddr, e.ehFrameSec.vaddr)
 	}
 	fr = e.ehFrameSec.reader(fdeAddr-e.ehFrameSec.vaddr, false)
-
 	return ipStart, fr, err
 }
 
+// decodeEntryAt decodes the entry from given index.
+func (e *EhFrameTable) decodeEntryAt(idx int) (ipStart uintptr, fr reader, err error) {
+	r := e.entryAt(idx)
+	return e.decodeEntry(&r)
+}
+
 // formatLen returns the length of a field encoded with enc encoding.
 func formatLen(enc encoding) int {
 	switch enc & encFormatMask {

nativeunwind/elfunwindinfo/elfehframetable_test.go

@@ -4,14 +4,11 @@
 package elfunwindinfo
 
 import (
-	"bytes"
-	"encoding/base64"
 	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"go.opentelemetry.io/ebpf-profiler/libpf"
-	"go.opentelemetry.io/ebpf-profiler/libpf/pfelf"
 )
 
 func TestLookupFDE(t *testing.T) {
@@ -51,9 +48,7 @@ func TestLookupFDE(t *testing.T) {
 		{at: 0x1000, expected: FDE{}},
 		{at: 0xcafe000, expected: FDE{}},
 	}
-	buffer, err := base64.StdEncoding.DecodeString(usrBinVolname)
-	require.NoError(t, err)
-	elf, err := pfelf.NewFile(bytes.NewReader(buffer), 0, false)
+	elf, err := getUsrBinPfelf()
 	require.NoError(t, err)
 	t.Cleanup(func() {
 		err = elf.Close()

nativeunwind/elfunwindinfo/stackdeltaextraction.go

@@ -39,14 +39,15 @@ type extractionFilter struct {
 
 var _ ehframeHooks = &extractionFilter{}
 
+func (f *extractionFilter) fdeUnsorted() {
+	f.unsortedFrames = true
+}
+
 // fdeHook filters out .eh_frame data that is superseded by .gopclntab data
 func (f *extractionFilter) fdeHook(_ *cieInfo, fde *fdeInfo) bool {
-	if !fde.sorted {
-		// Seems .debug_frame sometimes has broken FDEs for zero address
-		if fde.ipStart == 0 {
-			return false
-		}
-		f.unsortedFrames = true
+	// Seems .debug_frame sometimes has broken FDEs for zero address
+	if f.unsortedFrames && fde.ipStart == 0 {
+		return false
 	}
 	// Parse functions outside the gopclntab area
 	if fde.ipStart < f.start || fde.ipStart > f.end {
@@ -85,9 +86,7 @@ type elfExtractor struct {
 	allowGenericRegs bool
 }
 
-func (ee *elfExtractor) extractDebugDeltas() error {
-	var err error
-
+func (ee *elfExtractor) extractDebugDeltas() (err error) {
 	// Attempt finding the associated debug information file with .debug_frame,
 	// but ignore errors if it's not available; many production systems
 	// do not intentionally have debug packages installed.
@@ -122,7 +121,13 @@ func ExtractELF(elfRef *pfelf.Reference, interval *sdtypes.IntervalData) error {
 	if err != nil {
 		return err
 	}
+	return extractFile(elfFile, elfRef, interval)
+}
 
+// extractFile extracts the elfFile stack deltas and uses the optional elfRef to resolve
+// debug link references if needed.
+func extractFile(elfFile *pfelf.File, elfRef *pfelf.Reference,
+	interval *sdtypes.IntervalData) (err error) {
 	// Parse the stack deltas from the ELF
 	filter := extractionFilter{}
 	deltas := sdtypes.StackDeltaArray{}
@@ -143,7 +148,7 @@ func ExtractELF(elfRef *pfelf.Reference, interval *sdtypes.IntervalData) error {
 	if err = ee.parseDebugFrame(elfFile); err != nil {
 		return fmt.Errorf("failure to parse debug_frame stack deltas: %v", err)
 	}
-	if len(deltas) < numIntervalsToOmitDebugLink {
+	if ee.ref != nil && len(deltas) < numIntervalsToOmitDebugLink {
 		// There is only few stack deltas. See if we find the .gnu_debuglink
 		// debug information for additional .debug_frame stack deltas.
 		if err = ee.extractDebugDeltas(); err != nil {

nativeunwind/elfunwindinfo/stackdeltaextraction_test.go

@@ -4,11 +4,12 @@
 package elfunwindinfo
 
 import (
+	"bytes"
 	"encoding/base64"
-	"os"
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"go.opentelemetry.io/ebpf-profiler/libpf/pfelf"
 	sdtypes "go.opentelemetry.io/ebpf-profiler/nativeunwind/stackdeltatypes"
 )
 
@@ -133,23 +134,20 @@ var firstDeltas = sdtypes.StackDeltaArray{
 	{Address: 0x8e7, Info: deltaRSP(80, 16)},
 }
 
-func TestExtractStackDeltasFromFilename(t *testing.T) {
+func getUsrBinPfelf() (*pfelf.File, error) {
 	buffer, err := base64.StdEncoding.DecodeString(usrBinVolname)
+	if err != nil {
+		return nil, err
+	}
+	return pfelf.NewFile(bytes.NewReader(buffer), 0, false)
+}
+
+func TestExtractStackDeltasFromFilename(t *testing.T) {
+	elf, err := getUsrBinPfelf()
 	require.NoError(t, err)
-	// Write the executable file to a temporary file, and the symbol
-	// file, too.
-	exeFile, err := os.CreateTemp("/tmp", "dwarf_extract_elf_")
-	require.NoError(t, err)
-	defer exeFile.Close()
-	_, err = exeFile.Write(buffer)
-	require.NoError(t, err)
-	err = exeFile.Sync()
-	require.NoError(t, err)
-	defer os.Remove(exeFile.Name())
-	filename := exeFile.Name()
 
 	var data sdtypes.IntervalData
-	err = Extract(filename, &data)
+	err = extractFile(elf, nil, &data)
 	require.NoError(t, err)
 	for _, delta := range data.Deltas {
 		t.Logf("%#v", delta)