Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions nativeunwind/elfunwindinfo/elfehframe_aarch64.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,19 +144,11 @@ func (regs *vmRegs) getUnwindInfoARM() sdtypes.UnwindInfo {
info.FPOpcode = support.UnwindOpcodeBaseLR
info.FPParam = 0
case regCFA:
if regs.cfa.off != 0 {
// In ARM64, nothing can be assumed regarding RA location, it is
// simply somewhere on the stack, its detailed location needs to
// be extracted from FDE record.
// In our approach, RA offset part of stack delta always points
// to RA location no matter whether CFA is evaluated with respect
// to SP or FP.
// Use same opcode as for CFA:
info.FPOpcode = info.Opcode
// Convert CFA base to SP / FP base in order to keep
// offset to RA from frame bottom (FP based heuristic).
// CFA offset needs to be added to the one denoting RA location.
info.FPParam = int32(regs.cfa.off) + int32(regs.ra.off)
info.FPParam = int32(regs.ra.off)
if regs.fp.reg == regCFA && regs.fp.off+8 == regs.ra.off {
info.FPOpcode = support.UnwindOpcodeBaseCFAFrame
} else {
info.FPOpcode = support.UnwindOpcodeBaseCFA
}
}

Expand Down
35 changes: 18 additions & 17 deletions support/ebpf/native_stack_trace.ebpf.c
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,9 @@ static EBPF_INLINE u64 unwind_register_address(UnwindState *state, u64 cfa, u8 o

// Resolve the 'BASE' register, and fetch the CFA/FP/SP value.
switch (opcode & ~UNWIND_OPCODEF_DEREF) {
#if defined(__aarch64__)
case UNWIND_OPCODE_BASE_CFA_FRAME:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See below on making this a flag, but if you prefer not to do that, this should be #ifdef __aarch64__ for now to not bloat the x86-64 version.

#endif
case UNWIND_OPCODE_BASE_CFA: addr = cfa; break;
case UNWIND_OPCODE_BASE_FP: addr = state->fp; break;
case UNWIND_OPCODE_BASE_SP: addr = state->sp; break;
Expand Down Expand Up @@ -342,6 +345,9 @@ static EBPF_INLINE u64 unwind_register_address(UnwindState *state, u64 cfa, u8 o
#ifdef OPTI_DEBUG
switch (opcode) {
case UNWIND_OPCODE_BASE_CFA: DEBUG_PRINT("unwind: cfa+%d", preDeref); break;
#if defined(__aarch64__)
case UNWIND_OPCODE_BASE_CFA_FRAME: DEBUG_PRINT("unwind (fp+ra): cfa+%d", preDeref - 8); break;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

similar to https://github.com/open-telemetry/opentelemetry-ebpf-profiler/pull/1048/changes#r2674201404

Suggested change
case UNWIND_OPCODE_BASE_CFA_FRAME: DEBUG_PRINT("unwind (fp+ra): cfa+%d", preDeref - 8); break;
#if defined(__aarch64__)
case UNWIND_OPCODE_BASE_CFA_FRAME: DEBUG_PRINT("unwind (fp+ra): cfa+%d", preDeref - 8); break;
#endif

#endif
case UNWIND_OPCODE_BASE_FP: DEBUG_PRINT("unwind: fp+%d", preDeref); break;
case UNWIND_OPCODE_BASE_SP: DEBUG_PRINT("unwind: sp+%d", preDeref); break;
case UNWIND_OPCODE_BASE_CFA | UNWIND_OPCODEF_DEREF:
Expand Down Expand Up @@ -569,7 +575,18 @@ static EBPF_INLINE ErrorCode unwind_one_frame(struct UnwindState *state, bool *s
DEBUG_PRINT("RA: %016llX", (u64)ra);

// read the value of RA from stack
if (bpf_probe_read_user(&state->pc, sizeof(state->pc), (void *)ra)) {
int err;
u64 fpra[2];
fpra[0] = state->fp;
if (info->fpOpcode == UNWIND_OPCODE_BASE_CFA_FRAME) {
err = bpf_probe_read_user(fpra, sizeof(fpra), (void *)(ra - 8));
} else {
err = bpf_probe_read_user(&fpra[1], sizeof(fpra[0]), (void *)ra);
}
if (!err) {
state->fp = fpra[0];
state->pc = fpra[1];
} else {
// error reading memory, mark RA as invalid
ra = 0;
}
Expand All @@ -586,22 +603,6 @@ static EBPF_INLINE ErrorCode unwind_one_frame(struct UnwindState *state, bool *s
return ERR_NATIVE_PC_READ;
}

// Try to resolve frame pointer
// simple heuristic for FP based frames
// the GCC compiler usually generates stack frame records in such a way,
// so that FP/RA pair is at the bottom of a stack frame (stack frame
// record at lower addresses is followed by stack vars at higher ones)
// this implies that if no other changes are applied to the stack such
// as alloca(), following the prolog SP/FP points to the frame record
// itself, in such a case FP offset will be equal to 8
if (info->fpParam == 8) {
// we can assume the presence of frame pointers
if (info->fpOpcode != UNWIND_OPCODE_BASE_LR) {
// FP precedes the RA on the stack (Aarch64 ABI requirement)
bpf_probe_read_user(&state->fp, sizeof(state->fp), (void *)(ra - 8));
}
}

state->sp = cfa;
unwinder_mark_nonleaf_frame(state);
frame_ok:
Expand Down
17 changes: 10 additions & 7 deletions support/ebpf/stackdeltatypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,22 @@
#define OPTI_STACKDELTATYPES_H

// Command without arguments, the argument is instead an UNWIND_COMMAND_* value
#define UNWIND_OPCODE_COMMAND 0x00
#define UNWIND_OPCODE_COMMAND 0x00
// Expression with base value being the Canonical Frame Address (CFA)
#define UNWIND_OPCODE_BASE_CFA 0x01
#define UNWIND_OPCODE_BASE_CFA 0x01
// Expression with base value being the Stack Pointer
#define UNWIND_OPCODE_BASE_SP 0x02
#define UNWIND_OPCODE_BASE_SP 0x02
// Expression with base value being the Frame Pointer
#define UNWIND_OPCODE_BASE_FP 0x03
#define UNWIND_OPCODE_BASE_FP 0x03
// Expression with base value being the Link Register (ARM64)
#define UNWIND_OPCODE_BASE_LR 0x04
#define UNWIND_OPCODE_BASE_LR 0x04
// Expression with base value being a Generic Register
#define UNWIND_OPCODE_BASE_REG 0x05
#define UNWIND_OPCODE_BASE_REG 0x05
// Expression for RA with base value being the CFA, and
// also indicating that the FP immediately precedes the RA (ARM64).
#define UNWIND_OPCODE_BASE_CFA_FRAME 0x06
Comment on lines +16 to +18
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, how about making this a flag? UNWIND_OPCODEF_FRAME and perhaps have some UNWIND_OPCODEF_MASK to contain all flags.
This might become handy with other opcodes later, and currently it would probably reduce code size by eliminating the new case in the switch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I prefer not to make this a flag, because we only expect to apply it to CFA. If we start applying it to more opcodes then we can make it a flag. (Also it is slightly nerror prone, since there are several places where we need to change logic that currently looks at opcode or opcode & ~UNWIND_OPCODEF_DEREF that would need to be changed).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'll look into doing this as a follow up if it makes sense.

// An opcode flag to indicate that the value should be dereferenced
#define UNWIND_OPCODEF_DEREF 0x80
#define UNWIND_OPCODEF_DEREF 0x80

// Unsupported or no value for the register
#define UNWIND_COMMAND_INVALID 0
Expand Down
Binary file modified support/ebpf/tracer.ebpf.amd64
Binary file not shown.
Binary file modified support/ebpf/tracer.ebpf.arm64
Binary file not shown.
15 changes: 8 additions & 7 deletions support/types.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 8 additions & 7 deletions support/types_def.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,13 +146,14 @@ const (

const (
// UnwindOpcodes from the C header file
UnwindOpcodeCommand uint8 = C.UNWIND_OPCODE_COMMAND
UnwindOpcodeBaseCFA uint8 = C.UNWIND_OPCODE_BASE_CFA
UnwindOpcodeBaseSP uint8 = C.UNWIND_OPCODE_BASE_SP
UnwindOpcodeBaseFP uint8 = C.UNWIND_OPCODE_BASE_FP
UnwindOpcodeBaseLR uint8 = C.UNWIND_OPCODE_BASE_LR
UnwindOpcodeBaseReg uint8 = C.UNWIND_OPCODE_BASE_REG
UnwindOpcodeFlagDeref uint8 = C.UNWIND_OPCODEF_DEREF
UnwindOpcodeCommand uint8 = C.UNWIND_OPCODE_COMMAND
UnwindOpcodeBaseCFA uint8 = C.UNWIND_OPCODE_BASE_CFA
UnwindOpcodeBaseSP uint8 = C.UNWIND_OPCODE_BASE_SP
UnwindOpcodeBaseFP uint8 = C.UNWIND_OPCODE_BASE_FP
UnwindOpcodeBaseLR uint8 = C.UNWIND_OPCODE_BASE_LR
UnwindOpcodeBaseReg uint8 = C.UNWIND_OPCODE_BASE_REG
UnwindOpcodeBaseCFAFrame uint8 = C.UNWIND_OPCODE_BASE_CFA_FRAME
UnwindOpcodeFlagDeref uint8 = C.UNWIND_OPCODEF_DEREF

// UnwindCommands from the C header file
UnwindCommandInvalid int32 = C.UNWIND_COMMAND_INVALID
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@
"node+0x17f07fb",
"node+0x90848b",
"libc.so.6+0x8202f",
"libc.so.6+0x8202f",
"libc.so.6+0xebf1b"
]
},
Expand All @@ -70,7 +69,6 @@
"node+0x17f07fb",
"node+0x90848b",
"libc.so.6+0x8202f",
"libc.so.6+0x8202f",
"libc.so.6+0xebf1b"
]
},
Expand All @@ -82,7 +80,6 @@
"node+0x17f07fb",
"node+0x90848b",
"libc.so.6+0x8202f",
"libc.so.6+0x8202f",
"libc.so.6+0xebf1b"
]
},
Expand All @@ -94,7 +91,6 @@
"node+0x17f07fb",
"node+0x90848b",
"libc.so.6+0x8202f",
"libc.so.6+0x8202f",
"libc.so.6+0xebf1b"
]
},
Expand All @@ -106,7 +102,6 @@
"node+0x17e1abb",
"node+0x90d2ff",
"libc.so.6+0x8202f",
"libc.so.6+0x8202f",
"libc.so.6+0xebf1b"
]
}
Expand Down
5 changes: 0 additions & 5 deletions tools/coredump/testdata/arm64/node1600-inlining.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@
"node+0x14efc2f",
"node+0xb6add7",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Removal of these mystery frames is great!

"libc.so.6+0xe5d9b"
]
},
Expand All @@ -84,7 +83,6 @@
"node+0x14fd073",
"node+0xb66667",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -96,7 +94,6 @@
"node+0x14fd073",
"node+0xb66667",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -108,7 +105,6 @@
"node+0x14fd073",
"node+0xb66667",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -120,7 +116,6 @@
"node+0x14fd073",
"node+0xb66667",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand Down
5 changes: 0 additions & 5 deletions tools/coredump/testdata/arm64/node16110-inlining.json
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@
"node+0x14bb6bf",
"node+0xb5a0b7",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -82,7 +81,6 @@
"node+0x14c8bfb",
"node+0xb55947",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -94,7 +92,6 @@
"node+0x14c8bfb",
"node+0xb55947",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -106,7 +103,6 @@
"node+0x14c8bfb",
"node+0xb55947",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -118,7 +114,6 @@
"node+0x14c8bfb",
"node+0xb55947",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand Down
5 changes: 0 additions & 5 deletions tools/coredump/testdata/arm64/node1640-inlining.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@
"node+0x151c917",
"node+0xb7a817",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -84,7 +83,6 @@
"node+0x1529d5b",
"node+0xb760a7",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -96,7 +94,6 @@
"node+0x1529d5b",
"node+0xb760a7",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -108,7 +105,6 @@
"node+0x1529d5b",
"node+0xb760a7",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -120,7 +116,6 @@
"node+0x1529d5b",
"node+0xb760a7",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand Down
5 changes: 0 additions & 5 deletions tools/coredump/testdata/arm64/node1660-inlining.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@
"node+0x14a292f",
"node+0xb5630f",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -84,7 +83,6 @@
"node+0x14afd73",
"node+0xb51b9f",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -96,7 +94,6 @@
"node+0x14afd73",
"node+0xb51b9f",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -108,7 +105,6 @@
"node+0x14afd73",
"node+0xb51b9f",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand All @@ -120,7 +116,6 @@
"node+0x14afd73",
"node+0xb51b9f",
"libc.so.6+0x7d5c7",
"libc.so.6+0x7d5c7",
"libc.so.6+0xe5d9b"
]
},
Expand Down
Loading