DWARF: Add error handling for unexpected CFA opcodes

Signed-off-by: Sumera Priyadarsini <[email protected]>

cmd/eh-frame/main.go

@@ -55,7 +55,7 @@ func main() {
 	}
 
 	if flags.Final {
-		ut, arch, err := unwind.GenerateCompactUnwindTable(logger, executablePath)
+		ut, arch, err := unwind.GenerateCompactUnwindTable(executablePath)
 		if err != nil {
 			// nolint:forbidigo
 			fmt.Println("failed with:", err)
@@ -70,7 +70,7 @@ func main() {
 	}
 
 	ptb := unwind.NewUnwindTableBuilder(logger)
-	err := ptb.PrintTable(logger, os.Stdout, executablePath, flags.Compact, pc)
+	err := ptb.PrintTable(os.Stdout, executablePath, flags.Compact, pc)
 	if err != nil {
 		// nolint:forbidigo
 		fmt.Println("failed with:", err)

internal/dwarf/frame/table.go

@@ -4,10 +4,7 @@ package frame
 import (
 	"bytes"
 	"encoding/binary"
-	"strings"
-
-	"github.com/go-kit/log"
-	"github.com/go-kit/log/level"
+	"fmt"
 
 	"github.com/parca-dev/parca-agent/internal/dwarf/util"
 )
@@ -87,26 +84,29 @@ func (ici *InstructionContextIterator) HasNext() bool {
 	return !ici.done
 }
 
-func (ici *InstructionContextIterator) Next() *InstructionContext {
+func (ici *InstructionContextIterator) Next() (*InstructionContext, error) {
 	for ici.ctx.buf.Len() > 0 {
 		lastPcBefore := ici.ctx.lastInsCtx.loc
-		executeDWARFInstruction(ici.ctx)
+		err := executeDWARFInstruction(ici.ctx)
+		if err != nil {
+			return ici.ctx.lastInsCtx, err
+		}
 		lastPcAfter := ici.ctx.lastInsCtx.loc
 		// We are at an instruction boundary when there's a program counter change.
 		if lastPcBefore != lastPcAfter {
-			return ici.ctx.lastInsCtx
+			return ici.ctx.lastInsCtx, nil
 		}
 	}
 
 	// Account for the last instruction boundary.
 	if !ici.lastReached {
 		ici.lastReached = true
-		return ici.ctx.currInsCtx
+		return ici.ctx.currInsCtx, nil
 	}
 
 	// We are done iterating.
 	ici.done = true
-	return nil
+	return nil, nil //nolint:nilnil
 }
 
 // RowState is a stack where `DW_CFA_remember_state` pushes
@@ -147,10 +147,8 @@ type Context struct {
 	lastInsCtx      *InstructionContext
 	rememberedState *StateStack
 	// The buffer where we store the dwarf unwind entries to be parsed for this function.
-	buf                *bytes.Reader
-	order              binary.ByteOrder
-	fullExecutablePath string
-	logger             log.Logger
+	buf   *bytes.Reader
+	order binary.ByteOrder
 }
 
 func (ctx *Context) currentInstruction() *InstructionContext {
@@ -258,40 +256,48 @@ const low_6_offset = 0x3f
 
 type instruction func(ctx *Context)
 
-func NewContext(logger log.Logger, fullExecutablePath string) *Context {
+func NewContext() *Context {
 	return &Context{
-		currInsCtx:         &InstructionContext{},
-		lastInsCtx:         &InstructionContext{},
-		buf:                &bytes.Reader{},
-		rememberedState:    newStateStack(),
-		fullExecutablePath: fullExecutablePath,
-		logger:             log.With(logger, "component", "dwarf unwind table context"),
+		currInsCtx:      &InstructionContext{},
+		lastInsCtx:      &InstructionContext{},
+		buf:             &bytes.Reader{},
+		rememberedState: newStateStack(),
 	}
 }
 
-func executeCIEInstructions(cie *CommonInformationEntry, context *Context) *Context {
+func executeCIEInstructions(cie *CommonInformationEntry, context *Context) (*Context, error) {
 	if context == nil {
-		context = NewContext(context.logger, context.fullExecutablePath)
+		context = NewContext()
 	}
 
 	context.reset(cie)
-	context.executeDWARFProgram()
-	return context
+	err := context.executeDWARFProgram()
+	if err != nil {
+		return context, err
+	}
+	return context, nil
 }
 
 // ExecuteDWARFProgram evaluates the unwind opcodes for a function.
-func ExecuteDWARFProgram(fde *FrameDescriptionEntry, context *Context) *InstructionContextIterator {
-	ctx := executeCIEInstructions(fde.CIE, context)
+func ExecuteDWARFProgram(fde *FrameDescriptionEntry, context *Context) (*InstructionContextIterator, error) {
+	ctx, err := executeCIEInstructions(fde.CIE, context)
+	if err != nil {
+		return nil, err
+	}
 	ctx.order = fde.order
 	frame := ctx.currentInstruction()
 	frame.loc = fde.Begin()
-	return ctx.Execute(fde.Instructions)
+	return ctx.Execute(fde.Instructions), nil
 }
 
-func (ctx *Context) executeDWARFProgram() {
+func (ctx *Context) executeDWARFProgram() error {
 	for ctx.buf.Len() > 0 {
-		executeDWARFInstruction(ctx)
+		err := executeDWARFInstruction(ctx)
+		if err != nil {
+			return err
+		}
 	}
+	return nil
 }
 
 // Execute execute dwarf instructions.
@@ -303,26 +309,26 @@ func (ctx *Context) Execute(instructions []byte) *InstructionContextIterator {
 	}
 }
 
-func executeDWARFInstruction(ctx *Context) {
+func executeDWARFInstruction(ctx *Context) error {
 	instruction, err := ctx.buf.ReadByte()
 	if err != nil {
 		panic("Could not read from instruction buffer")
 	}
 
 	if instruction == DW_CFA_nop {
-		return
+		return nil
 	}
 
-	fn := lookupFunc(instruction, ctx)
+	fn, err := lookupFunc(instruction, ctx)
+	if err != nil {
+		return fmt.Errorf(" DWARF CFA rule is not valid. This should never happen :%w", err)
+	}
 	fn(ctx)
-}
 
-// getPid gets the Process ID as a string from the fullExecutablePath provided
-func getPid(fullExecutablePath string) string {
-	return strings.Split(fullExecutablePath, "/")[2]
+	return nil
 }
 
-func lookupFunc(instruction byte, ctx *Context) instruction {
+func lookupFunc(instruction byte, ctx *Context) (instruction, error) {
 	const high_2_bits = 0xc0
 	var restoreOpcode bool
 
@@ -413,9 +419,9 @@ func lookupFunc(instruction byte, ctx *Context) instruction {
 	case DW_CFA_GNU_window_save:
 		fn = gnuwindowsave
 	default:
-		level.Error(ctx.logger).Log("msg", "encountered an unexpected DWARF CFA opcode", "opcode instruction", instruction, "PID", getPid(ctx.fullExecutablePath), "comm", ctx.fullExecutablePath)
+		return nil, fmt.Errorf("encountered an unexpected DWARF CFA opcode instruction %d", instruction)
 	}
-	return fn
+	return fn, nil
 }
 
 // TODO(sylfrena): Reuse types.

pkg/profiler/cpu/bpf/maps/maps.go

@@ -1556,7 +1556,7 @@ func (m *Maps) setUnwindTableForMapping(buf *profiler.EfficientBuffer, pid int,
 		// Generate the unwind table.
 		// PERF(javierhonduco): Not reusing a buffer here yet, let's profile and decide whether this
 		// change would be worth it.
-		ut, arch, err := unwind.GenerateCompactUnwindTable(m.logger, fullExecutablePath)
+		ut, arch, err := unwind.GenerateCompactUnwindTable(fullExecutablePath)
 		level.Debug(m.logger).Log("msg", "found unwind entries", "executable", mapping.Executable, "len", len(ut))
 
 		if err != nil {

pkg/stack/unwind/compact_unwind_table.go

@@ -20,8 +20,6 @@ import (
 	"fmt"
 	"sort"
 
-	"github.com/go-kit/log"
-
 	"github.com/parca-dev/parca-agent/internal/dwarf/frame"
 )
 
@@ -137,9 +135,9 @@ func (t CompactUnwindTable) RemoveRedundant() CompactUnwindTable {
 
 // BuildCompactUnwindTable produces a compact unwind table for the given
 // frame description entries.
-func BuildCompactUnwindTable(logger log.Logger, fdes frame.FrameDescriptionEntries, arch elf.Machine, fullExecutablePath string) (CompactUnwindTable, error) {
+func BuildCompactUnwindTable(fdes frame.FrameDescriptionEntries, arch elf.Machine) (CompactUnwindTable, error) {
 	table := make(CompactUnwindTable, 0, 4*len(fdes)) // heuristic: we expect each function to have ~4 unwind entries.
-	context := frame.NewContext(logger, fullExecutablePath)
+	context := frame.NewContext()
 	lastFunctionPc := uint64(0)
 	for _, fde := range fdes {
 		// Add a synthetic row at the end of the function but only
@@ -155,8 +153,21 @@ func BuildCompactUnwindTable(logger log.Logger, fdes frame.FrameDescriptionEntri
 			})
 		}
 
-		frameContext := frame.ExecuteDWARFProgram(fde, context)
-		for insCtx := frameContext.Next(); frameContext.HasNext(); insCtx = frameContext.Next() {
+		frameContext, err := frame.ExecuteDWARFProgram(fde, context)
+		if err != nil {
+			return CompactUnwindTable{}, err
+		}
+
+		var err2 error
+
+		for insCtx, err1 := frameContext.Next(); frameContext.HasNext(); insCtx, err2 = frameContext.Next() {
+			if err1 != nil {
+				return CompactUnwindTable{}, err1
+			}
+			if err2 != nil {
+				return CompactUnwindTable{}, err2
+			}
+
 			row := unwindTableRow(insCtx)
 			compactRow, err := rowToCompactRow(row, arch)
 			if err != nil {
@@ -268,7 +279,7 @@ func CompactUnwindTableRepresentation(unwindTable UnwindTable, arch elf.Machine)
 
 // GenerateCompactUnwindTable produces the compact unwind table for a given
 // executable.
-func GenerateCompactUnwindTable(logger log.Logger, fullExecutablePath string) (CompactUnwindTable, elf.Machine, error) {
+func GenerateCompactUnwindTable(fullExecutablePath string) (CompactUnwindTable, elf.Machine, error) {
 	var ut CompactUnwindTable
 
 	// Fetch FDEs.
@@ -282,9 +293,9 @@ func GenerateCompactUnwindTable(logger log.Logger, fullExecutablePath string) (C
 	sort.Sort(fdes)
 
 	// Generate the compact unwind table.
-	ut, err = BuildCompactUnwindTable(logger, fdes, arch, fullExecutablePath)
+	ut, err = BuildCompactUnwindTable(fdes, arch)
 	if err != nil {
-		return ut, arch, err
+		return ut, arch, fmt.Errorf("build compact unwind table for executable %q: %w", fullExecutablePath, err)
 	}
 
 	// This should not be necessary, as per the sorting above, but

pkg/stack/unwind/compact_unwind_table_test.go

@@ -20,7 +20,6 @@ import (
 	"testing"
 
 	"github.com/parca-dev/parca-agent/internal/dwarf/frame"
-	"github.com/parca-dev/parca-agent/pkg/logger"
 
 	"github.com/stretchr/testify/require"
 )
@@ -456,10 +455,8 @@ func TestIsSorted(t *testing.T) {
 	matches, err := filepath.Glob("../../../testdata/vendored/x86/*")
 	require.NoError(t, err)
 
-	logger := logger.NewLogger("error", logger.LogFormatLogfmt, "compact-unwind-table-tests")
-
 	for _, match := range matches {
-		ut, _, err := GenerateCompactUnwindTable(logger, match)
+		ut, _, err := GenerateCompactUnwindTable(match)
 		require.NoError(t, err)
 		requireSorted(t, ut)
 	}
@@ -470,10 +467,8 @@ func TestNoRepeatedPCs(t *testing.T) {
 	matches, err := filepath.Glob("../../../testdata/vendored/x86/*")
 	require.NoError(t, err)
 
-	logger := logger.NewLogger("error", logger.LogFormatLogfmt, "compact-unwind-table-tests")
-
 	for _, match := range matches {
-		ut, _, err := GenerateCompactUnwindTable(logger, match)
+		ut, _, err := GenerateCompactUnwindTable(match)
 		require.NoError(t, err)
 		requireNoDuplicatedPC(t, ut)
 	}
@@ -483,10 +478,8 @@ func TestNoRedundantRows(t *testing.T) {
 	matches, err := filepath.Glob("../../../testdata/vendored/x86/*")
 	require.NoError(t, err)
 
-	logger := logger.NewLogger("error", logger.LogFormatLogfmt, "compact-unwind-table-tests")
-
 	for _, match := range matches {
-		ut, _, err := GenerateCompactUnwindTable(logger, match)
+		ut, _, err := GenerateCompactUnwindTable(match)
 		require.NoError(t, err)
 		requireNoRedundantRows(t, ut)
 	}
@@ -499,12 +492,10 @@ func BenchmarkGenerateCompactUnwindTable(b *testing.B) {
 
 	b.ReportAllocs()
 
-	logger := logger.NewLogger("error", logger.LogFormatLogfmt, "compact-unwind-table-tests")
-
 	var cut CompactUnwindTable
 	var err error
 	for n := 0; n < b.N; n++ {
-		cut, _, err = GenerateCompactUnwindTable(logger, objectFilePath)
+		cut, _, err = GenerateCompactUnwindTable(objectFilePath)
 	}
 
 	require.NoError(b, err)

pkg/stack/unwind/unwind_table.go

@@ -79,7 +79,7 @@ func registerToString(reg uint64, arch elf.Machine) string {
 }
 
 // PrintTable is a debugging helper that prints the unwinding table to the given io.Writer.
-func (ptb *UnwindTableBuilder) PrintTable(logger log.Logger, writer io.Writer, path string, compact bool, pc *uint64) error {
+func (ptb *UnwindTableBuilder) PrintTable(writer io.Writer, path string, compact bool, pc *uint64) error {
 	fdes, arch, err := ReadFDEs(path)
 	if err != nil {
 		return err
@@ -92,7 +92,7 @@ func (ptb *UnwindTableBuilder) PrintTable(logger log.Logger, writer io.Writer, p
 		}
 	}()
 
-	unwindContext := frame.NewContext(logger, path)
+	unwindContext := frame.NewContext()
 	for _, fde := range fdes {
 		if pc != nil {
 			if fde.Begin() > *pc || *pc > fde.End() {
@@ -102,8 +102,18 @@ func (ptb *UnwindTableBuilder) PrintTable(logger log.Logger, writer io.Writer, p
 
 		fmt.Fprintf(writer, "=> Function start: %x, Function end: %x\n", fde.Begin(), fde.End())
 
-		frameContext := frame.ExecuteDWARFProgram(fde, unwindContext)
-		for insCtx := frameContext.Next(); frameContext.HasNext(); insCtx = frameContext.Next() {
+		frameContext, err := frame.ExecuteDWARFProgram(fde, unwindContext)
+		if err != nil {
+			return err
+		}
+		var err2 error
+		for insCtx, err1 := frameContext.Next(); frameContext.HasNext(); insCtx, err2 = frameContext.Next() {
+			if err1 != nil {
+				return err1
+			}
+			if err2 != nil {
+				return err2
+			}
 			unwindRow := unwindTableRow(insCtx)
 
 			if unwindRow == nil {
@@ -222,17 +232,27 @@ func ReadFDEs(path string) (frame.FrameDescriptionEntries, elf.Machine, error) {
 	return fdes, arch, nil
 }
 
-func BuildUnwindTable(ctx *frame.Context, fdes frame.FrameDescriptionEntries) UnwindTable {
+func BuildUnwindTable(fdes frame.FrameDescriptionEntries) (UnwindTable, error) {
 	// The frame package can raise in case of malformed unwind data.
 	table := make(UnwindTable, 0, 4*len(fdes)) // heuristic
 
 	for _, fde := range fdes {
-		frameContext := frame.ExecuteDWARFProgram(fde, ctx)
-		for insCtx := frameContext.Next(); frameContext.HasNext(); insCtx = frameContext.Next() {
+		frameContext, err := frame.ExecuteDWARFProgram(fde, nil)
+		if err != nil {
+			return UnwindTable{}, err
+		}
+		var err2 error
+		for insCtx, err1 := frameContext.Next(); frameContext.HasNext(); insCtx, err2 = frameContext.Next() {
+			if err1 != nil {
+				return UnwindTable{}, err1
+			}
+			if err2 != nil {
+				return UnwindTable{}, err2
+			}
 			table = append(table, *unwindTableRow(insCtx))
 		}
 	}
-	return table
+	return table, nil
 }
 
 // UnwindTableRow represents a single row in the unwind table.