diff --git a/cmd/eh-frame/main.go b/cmd/eh-frame/main.go index 7351e7eabe..ea63945ae8 100644 --- a/cmd/eh-frame/main.go +++ b/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) diff --git a/internal/dwarf/frame/table.go b/internal/dwarf/frame/table.go index 46fcdb88b2..e2367dd738 100644 --- a/internal/dwarf/frame/table.go +++ b/internal/dwarf/frame/table.go @@ -4,10 +4,10 @@ package frame import ( "bytes" "encoding/binary" + "fmt" "strings" "github.com/go-kit/log" - "github.com/go-kit/log/level" "github.com/parca-dev/parca-agent/internal/dwarf/util" ) @@ -258,40 +258,49 @@ const low_6_offset = 0x3f type instruction func(ctx *Context) -func NewContext(logger log.Logger, fullExecutablePath string) *Context { +func NewContext(fullExecutablePath string) *Context { return &Context{ currInsCtx: &InstructionContext{}, lastInsCtx: &InstructionContext{}, buf: &bytes.Reader{}, rememberedState: newStateStack(), fullExecutablePath: fullExecutablePath, - logger: log.With(logger, "component", "dwarf unwind table context"), } } -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.fullExecutablePath) } 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) ctx.order = fde.order frame := ctx.currentInstruction() frame.loc = fde.Begin() - return ctx.Execute(fde.Instructions) + if err != nil { + return ctx.Execute(fde.Instructions), err + } + 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,18 +312,22 @@ 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) fn(ctx) + if err != nil { + return fmt.Errorf(" DWARF CFA rule is not valid. This should never happen :%w", err) + } + return nil } // getPid gets the Process ID as a string from the fullExecutablePath provided @@ -322,7 +335,7 @@ func getPid(fullExecutablePath string) string { return strings.Split(fullExecutablePath, "/")[2] } -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 +426,10 @@ 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) + pid := getPid(ctx.fullExecutablePath) + return nil, fmt.Errorf("encountered an unexpected DWARF CFA opcode instruction %d for executable: %s with PID: %s", instruction, ctx.fullExecutablePath, pid) } - return fn + return fn, nil } // TODO(sylfrena): Reuse types. diff --git a/pkg/profiler/cpu/bpf/maps/maps.go b/pkg/profiler/cpu/bpf/maps/maps.go index 506bc4766d..bf6c465826 100644 --- a/pkg/profiler/cpu/bpf/maps/maps.go +++ b/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 { diff --git a/pkg/stack/unwind/compact_unwind_table.go b/pkg/stack/unwind/compact_unwind_table.go index 13c19cf71f..26f2250997 100644 --- a/pkg/stack/unwind/compact_unwind_table.go +++ b/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, fullExecutablePath string) (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(fullExecutablePath) lastFunctionPc := uint64(0) for _, fde := range fdes { // Add a synthetic row at the end of the function but only @@ -155,7 +153,10 @@ func BuildCompactUnwindTable(logger log.Logger, fdes frame.FrameDescriptionEntri }) } - frameContext := frame.ExecuteDWARFProgram(fde, context) + frameContext, err := frame.ExecuteDWARFProgram(fde, context) + if err != nil { + return CompactUnwindTable{}, err + } for insCtx := frameContext.Next(); frameContext.HasNext(); insCtx = frameContext.Next() { row := unwindTableRow(insCtx) compactRow, err := rowToCompactRow(row, arch) @@ -268,7 +269,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,7 +283,7 @@ 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, fullExecutablePath) if err != nil { return ut, arch, err } diff --git a/pkg/stack/unwind/compact_unwind_table_test.go b/pkg/stack/unwind/compact_unwind_table_test.go index de881a190a..ba4a60cce2 100644 --- a/pkg/stack/unwind/compact_unwind_table_test.go +++ b/pkg/stack/unwind/compact_unwind_table_test.go @@ -470,10 +470,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 +481,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 +495,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) diff --git a/pkg/stack/unwind/unwind_table.go b/pkg/stack/unwind/unwind_table.go index c07ebcccd5..f4a011b566 100644 --- a/pkg/stack/unwind/unwind_table.go +++ b/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(path) for _, fde := range fdes { if pc != nil { if fde.Begin() > *pc || *pc > fde.End() { @@ -102,7 +102,10 @@ 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) + frameContext, err := frame.ExecuteDWARFProgram(fde, unwindContext) + if err != nil { + return err + } for insCtx := frameContext.Next(); frameContext.HasNext(); insCtx = frameContext.Next() { unwindRow := unwindTableRow(insCtx) @@ -222,17 +225,20 @@ func ReadFDEs(path string) (frame.FrameDescriptionEntries, elf.Machine, error) { return fdes, arch, nil } -func BuildUnwindTable(ctx *frame.Context, fdes frame.FrameDescriptionEntries) UnwindTable { +func BuildUnwindTable(ctx *frame.Context, 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) + frameContext, err := frame.ExecuteDWARFProgram(fde, ctx) + if err != nil { + return UnwindTable{}, err + } for insCtx := frameContext.Next(); frameContext.HasNext(); insCtx = frameContext.Next() { table = append(table, *unwindTableRow(insCtx)) } } - return table + return table, nil } // UnwindTableRow represents a single row in the unwind table. diff --git a/pkg/stack/unwind/unwind_table_test.go b/pkg/stack/unwind/unwind_table_test.go index febb0f8bd7..014c63ddfe 100644 --- a/pkg/stack/unwind/unwind_table_test.go +++ b/pkg/stack/unwind/unwind_table_test.go @@ -15,10 +15,8 @@ package unwind import ( - "os" "testing" - "github.com/go-kit/log" "github.com/stretchr/testify/require" "github.com/parca-dev/parca-agent/internal/dwarf/frame" @@ -29,11 +27,10 @@ func TestBuildUnwindTable(t *testing.T) { fdes, _, err := ReadFDEs("../../../testdata/out/x86/basic-cpp") require.NoError(t, err) - logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)) - logger = log.With(logger, "component", "unwind table test") - unwindContext := frame.NewContext(logger, "../../../testdata/out/x86/basic-cpp") + unwindContext := frame.NewContext("../../../testdata/out/x86/basic-cpp") - unwindTable := BuildUnwindTable(unwindContext, fdes) + unwindTable, err := BuildUnwindTable(unwindContext, fdes) + require.NoError(t, err) require.Len(t, unwindTable, 38) require.Equal(t, uint64(0x401020), unwindTable[0].Loc) @@ -55,16 +52,14 @@ func TestSpecialOpcodes(t *testing.T) { }, } - logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)) - logger = log.With(logger, "component", "unwind table test") - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { fdes, _, err := ReadFDEs(tt.executable) require.NoError(t, err) - ctx := frame.NewContext(logger, tt.executable) - unwindTable := BuildUnwindTable(ctx, fdes) + ctx := frame.NewContext(tt.executable) + unwindTable, err := BuildUnwindTable(ctx, fdes) + require.NoError(t, err) require.NotEmpty(t, unwindTable) }) } @@ -78,18 +73,18 @@ func benchmarkParsingDWARFUnwindInformation(b *testing.B, executable string) { var rbpOffset int64 - logger := log.NewLogfmtLogger(log.NewSyncWriter(os.Stderr)) - logger = log.With(logger, "component", "unwind table test") - for n := 0; n < b.N; n++ { fdes, _, err := ReadFDEs(executable) if err != nil { panic("could not read FDEs") } - unwindContext := frame.NewContext(logger, executable) + unwindContext := frame.NewContext(executable) for _, fde := range fdes { - frameContext := frame.ExecuteDWARFProgram(fde, unwindContext) + frameContext, err := frame.ExecuteDWARFProgram(fde, unwindContext) + if err != nil { + panic("unable to evaluate DWARF unwind codes") + } for insCtx := frameContext.Next(); frameContext.HasNext(); insCtx = frameContext.Next() { unwindRow := unwindTableRow(insCtx) if unwindRow.RBP.Rule == frame.RuleUndefined || unwindRow.RBP.Offset == 0 {