diff --git a/.superpowers/sdd/unresolved-frame-modules-report.md b/.superpowers/sdd/unresolved-frame-modules-report.md new file mode 100644 index 00000000..e699226c --- /dev/null +++ b/.superpowers/sdd/unresolved-frame-modules-report.md @@ -0,0 +1,311 @@ +# Unresolved frames name their module + +Branch: `feat/unresolved-frame-modules`. One commit. Not pushed, no PR. + +Goal: a stack frame that was unwound correctly but could not be given a +symbol should read `libcuda.so.1+0x1b71c6`, not `0x7f2c945b2c2b`. + +--- + +## Where the module was being lost, and why + +Two independent losses, both real, both required for the fix. Neither is +the guess in the brief ("the pprof builder's unconditional default +mapping") on its own — that default is a *symptom* of the second. + +### 1. blazesym returns nothing at all for an address it cannot name + +This is the answer to the question the brief asked to settle first: +**no, blazesym does not populate `Frame.Module` when it fails.** + +`blazesym/src/symbolize/symbolizer.rs` produces `Symbolized::Unknown(reason)` +for a miss — a variant that carries a `Reason` and *nothing else*. The +mapping was known one stack frame earlier (`handle_entry_addr` has the +`MapsEntry` in hand, path and all) and is discarded on the way out. The C +API then does this, `blazesym/capi/src/symbolize.rs:1105`: + +```rust +Symbolized::Unknown(reason) => { + // Unknown symbols/addresses are just represented with all + // fields set to zero (except for reason). + let () = unsafe { syms_last.write_bytes(0, 1) }; + sym_ref.reason = reason.into(); +} +``` + +So `blaze_sym.module` is NULL and `offset` is 0 for exactly the frames that +need them. `symbolize/local.go`'s `fromBlazesymSym` faithfully copies that +nothing into `Frame.Module`, writes the hex PC into `Name` so the location +renders as *something*, and sets `Reason = FailureMissingSymbols`. + +The module was never lost downstream. It never arrived. + +### 2. The GPU pipeline builds its profile with no `procmap.Resolver` + +`pprof.ProfileBuilder.addLocation` has only one way to reach a real +`profile.Mapping`: `p.resolver.Lookup(pid, addr)`. `cmd/gpu-cuda-profile` +and `cmd/gpu-stub-profile` construct their builders as +`pprof.BuildersOptions{SampleRate: 1}` — no `Resolver`. Every frame therefore +fell through to `addLocationByFallback` on `p.Profile.Mapping[0]`, the +unconditional `{ID: 1}` default. + +`go tool pprof -raw /home/diego/gpu-cuda-45.pb.gz` confirms it exactly: + +``` +Locations + 9: 0x0 M=1 cudaLaunchKernel :0:0 s=0() + 10: 0x0 M=1 0x7f2c958b71c6 :0:0 s=0() + ... + 16: 0x0 M=1 0x7f2c944de06b :0:0 s=0() +Mappings +1: 0x0/0x0/0x0 +``` + +Note `0x0 M=1` on **every** location, including the resolved ones — the +address column is zero because the fallback path never sets `Location.Address`. +So the GPU path had no mapping for any frame, resolved or not. + +### Why wiring a Resolver into the GPU builder is not the fix + +It cannot work. The GPU tools build the profile *after* `cmd.Wait()` — the +workload has exited and `/proc//maps` is gone. A build-time lookup finds +nothing. The mapping has to be captured **while the target is alive**, which +is the symbolization moment, not the build moment. + +That settles the layering: **the fix is in the symbolizer**, and the frame +carries the mapping forward to the builder. + +--- + +## What was built + +``` +blazesym miss -> symbolize.attachModules (NEW, needs a live process) + fills Module/BuildID/MapStart/MapLimit/MapOff + -> symbolize.ToProfFrames (carries them + Unresolved bit) + -> pprof.addLocation branch 3b (NEW: frame-carried mapping) + -> pprof.addLocationByAddr (renames, only if Unresolved) +``` + +- **`internal/framename`** (new) owns the one textual form, + `Format(modulePath, off)` and `IsAddressOnly(name)`. Three packages have to + agree on it (pprof writes it, foldedstacks counts it, flamegraph colours it); + a private copy in each is how the honesty counters drift apart. +- **`symbolize.Frame`** gains `MapStart/MapLimit/MapOff` and a + `ModuleOffset() (uint64, bool)` accessor. +- **`symbolize/module.go`** (new): `ModuleIndex` interface (`*procmap.Resolver` + satisfies it) and `attachModules`, which runs **only** over frames with + `Reason != FailureNone && Module == ""`. A fully symbolized stack does zero + lookups. +- **`symbolize.NewLocalSymbolizer`** takes options; `WithModuleIndex(idx)` + supplies the index. Without one, nothing changes at all. +- **`pprof.Frame`** gains `Unresolved bool`, set by `ToProfFrames` from + `Frame.Reason`. It has to be carried: after the conversion, `0x4017c2` is + indistinguishable from a function genuinely called `0x4017c2`, and pprof has + no unsymbolized bit. +- Wired at: `cmd/gpu-cuda-profile`, `cmd/gpu-stub-profile`, + `perfagent/agent.go` (`chooseSymbolizer`), and the debuginfod symbolizer's + local fallback. +- **`gpuprobe.Stats.StackFramesModuleOnly`** (new counter): the subset of + `StackFramesUnresolved` that recovered a module. Zero-while-the-total-is-large + is the "no index wired / target exited first" failure, and it is now visible + instead of hidden inside the total. + +--- + +## What an unresolved frame renders as + +### pprof + +`Function.Name` becomes `libcuda.so.550.54.14+0x14b71c6`, and `Location.Address` +is the same number, and `Location.Mapping` is a real mapping with the file, +start, limit, offset and build-id. + +**Why `Function.Name` and not the flame graph's presentation layer:** the same +`.pb.gz` is read by `go tool pprof`, which renders `Function.Name` and knows +nothing about perf-agent's conventions. A name only the flame graph understood +would help half the audience. Putting it in the name serves both, and the real +`Mapping` serves a third audience — anything that wants to re-symbolize the +profile later. + +**The offset is module-relative, not absolute.** It is `Address - MapStart + +MapOff`, i.e. the offset into the backing file — the same number pprof already +stores in `Location.Address`, stable across runs, and directly usable with +`addr2line -e libcuda.so.550.54.14 0x14b71c6`. + +`Mapping.HasFunctions` is deliberately **not** set by an unresolved frame. The +mapping is in the profile because its symbols are missing; claiming otherwise +would mislead every downstream reader. + +### Flame graph + +The label is the same string. The colour needed a change, and this is the one +non-obvious edit in the diff: + +`flamegraph.Classify` checked module-derived rules *before* the +`isUnsymbolized(name)` rule. That order was safe only because GPU profiles had +no modules. Once frames carry `libcuda.so.1`, an unnamed frame matches +`isVendorModule` and would have been painted as ordinary vendor code — +silently destroying `DomainUnsymbolized`, whose label is literally +*"vendor, no symbols"* and whose hatch is the only thing on the page saying no +symbol table named this address. The domain would have become unreachable for +vendor libraries, i.e. the graph would look cleanest exactly when +symbolization worked worst. + +So `isUnsymbolized` now moves **above** the module rules (and recognizes the +new form), with `module == "[kernel]"` still above it so a raw kernel address +stays orange. Both grades of unresolved share the domain and are told apart by +their label — `libcuda.so.1+0x1b71c6` (module known) vs `0x7f2c945b2c2b` +(nothing known). + +`foldedstacks.isAddressOnly` likewise recognizes the new form, so +`Result.AddressOnlyFrames` and the "N of M frame slots have no symbol" warning +report the identical gap before and after. A test asserts that equality +directly. + +--- + +## The no-mapping case + +A PC that no file-backed executable mapping covers stays a bare `0x...`, on +the default mapping, and is never merged with the has-module case. Four +independent guards: + +1. `attachModules` only writes fields on a `Lookup` hit with a non-empty path; + `procmap`'s parser already drops anonymous, non-executable and bracketed + pseudo-file lines, so a hit is always a real file. +2. `pprof.frameMapping` additionally requires `MapStart <= Address < MapLimit`. + A frame whose carried range does not contain its own address is refused + outright rather than interned at a nonsense offset under a confident file name. +3. `framename.Format` returns `""` for `""`, `[kernel]` and `[jit]`, so the + sentinel mappings can never become a module name. +4. It is *counted*, not just rendered: `LocalStats.ModulesAttached` / + `ModulesBare`, and `gpuprobe.Stats.StackFramesModuleOnly` against + `StackFramesUnresolved`. + +One thing this cannot rule out, stated rather than hidden: a symbol whose whole +demangled name is an identifier, `+0x`, and hex digits — no parenthesis, colon, +space or bracket anywhere — is indistinguishable from the module form once the +profile is written, because the `Unresolved` bit does not survive into the +file. None has been observed. The consequence would be a frame drawn as +unsymbolized and counted in the gap warning: over-reporting the gap, never +hiding it. `TestKnownAmbiguity` pins it so nobody reads the strictness tests as +a proof it is impossible. + +--- + +## What changes for the CPU profilers (`--profile`, `--offcpu`) + +They share `pprof`, `symbolize` and the flame graph, so: + +1. **Unresolved frames gain module names.** `perfagent/agent.go` now passes its + `procmap.Resolver` to `NewLocalSymbolizer` as a module index, so a hex frame + in a stripped library reads `libssl.so.3+0x4a120`. This is the intended + benefit, arriving for free. +2. **`Mapping.HasFunctions` is no longer set by an unresolved frame.** Behaviour + change. Previously any non-empty `Name` — including a hex address — set it. + A mapping with at least one resolved frame still sets it. `TestMappingFlags` + (which uses a named frame) passes unchanged. +3. **Flame graph colouring** of a hex-named frame with a known vendor/system + module moves from yellow/red to the hatched unsymbolized domain (see above). + A hex-named *kernel* frame is unaffected. +4. **No change to resolved frames**, anywhere. Branch 3 (the builder's own + Resolver) still runs first and still wins, so existing mapping attribution + is untouched; the new branch 3b only fires where branch 3 found nothing. + `TestResolvedFrameIsNeverRenamed` and the byte-for-byte comparison in + `TestGPUStack_After` pin this. +5. **No new hot-path cost.** The index is consulted per *failed* frame only. + +Not changed, and worth knowing: neither `profile/` nor `offcpu/` ever calls +`Resolver.Invalidate`, so a PID that exits and is reused within one run keeps +the first process's mappings. That window is pre-existing — the builders +already attribute every user frame through the same cache — and this change +neither widens nor narrows it. Fixing it is separate work. + +--- + +## Verification + +``` +go build ./... && go vet ./... && go test ./... -count=1 # all pass +~/go/bin/golangci-lint run --timeout=5m # 0 issues +cd test && go vet ./... # separate module, clean +``` + +New tests: + +- `internal/framename/framename_test.go` — format, recognition, the + round-trip property that everything `Format` emits `IsAddressOnly` accepts, + and the acknowledged ambiguity. +- `symbolize/module_test.go` — module attached; **resolved frames not touched + and not even looked up**; a module the symbolizer already knew is not + overwritten; unmapped address stays bare with all fields zero; nil index + counted as bare; `ModuleOffset` rejects an address outside its own mapping; + `ToProfFrames` carries the fields and the `Unresolved` bit, and never marks + an inlined frame. +- `pprof/unresolved_test.go` — the rename, the mapping, name/`Location.Address` + agreement, `HasFunctions` false, no-mapping stays bare on mapping 1, + inconsistent carried range refused, resolved frame never renamed, `[kernel]` + never used as a module name, Resolver still wins, two offsets in one module + stay two frames. +- `internal/foldedstacks/gpu_stack_e2e_test.go` — the whole pipeline over the + real stack (below). +- `internal/flamegraph/domain_test.go` — unsymbolized beats module; kernel + still beats unsymbolized. + +### What the new pipeline produces for the real 16-frame stack + +The stack is transcribed from `go tool pprof -raw` on +`/home/diego/gpu-cuda-45.pb.gz` (locations 4..18 then 2). `TestGPUStack_Before` +reproduces the file's current output; `TestGPUStack_After` shows the new one: + +``` +_start _start +__libc_start_main_alias_1 __libc_start_main_alias_1 +__libc_start_call_main __libc_start_call_main +main main +__device_stub__Z14perfagent_axpy... __device_stub__Z14perfagent_axpy... +cudaLaunchKernel cudaLaunchKernel +0x7f2c958b71c6 -> libcuda.so.550.54.14+0x14b71c6 +0x7f2c945ace62 -> libcuda.so.550.54.14+0x1ace62 +0x7f2c945acc75 -> libcuda.so.550.54.14+0x1acc75 +0x7f2c945b2dfb -> libcuda.so.550.54.14+0x1b2dfb +0x7f2c945b2c2b -> libcuda.so.550.54.14+0x1b2c2b +0x7f2c945bbf6f -> libcuda.so.550.54.14+0x1bbf6f +0x7f2c944de06b -> libcuda.so.550.54.14+0xde06b +(anonymous namespace)::on_callback(...) (anonymous namespace)::on_callback(...) +[gpu:launch] [gpu:launch] +[gpu:kernel:_Z14perfagent_axpyfPKfPfi] [gpu:kernel:_Z14perfagent_axpyfPKfPfi] + +Mappings: 1: 0x0/0x0/0x0 -> + libcuda.so.550.54.14 @ 0x7f2c94400000 +``` + +The mapping *ranges* in that test are a labelled fixture: the real run's +`/proc//maps` was never recorded, so the range is chosen to contain the +observed addresses. It is not a claim about where libcuda sat on that machine. +What is real is the stack, the frame names, the seven addresses, and the +arithmetic. + +`TestGPUStack_SymbolizationGapStillReported` asserts `AddressOnlyFrames == 7` +in **both** columns. + +### Cannot verify end to end + +**This has not been confirmed against a live GPU.** This box has `CapEff: 0` +and no GPU; the existing capture cannot be re-rendered because the mapping data +was never written into it, which is the whole point of the bug. Everything +above is unit-level proof of the mechanism plus a reconstruction of the real +stack. + +A real confirmation needs a fresh capture on the RTX 3090, which the controller +will run. What to look for: + +1. `go tool pprof -raw .pb.gz` shows **more than one** mapping, with + libcuda's real path, start, limit and build-id, and locations reading + `0x M=` rather than `0x0 M=1`. +2. The seven frames render as `libcuda.so...+0x...` / `libcupti.so...+0x...`. +3. The run's stats line shows `StackFramesModuleOnly` close to + `StackFramesUnresolved`. If it is 0 while the latter is large, the module + index is not reaching the symbolizer, or the workload is exiting before its + stacks are drained. +4. The flame graph's symbolization-gap warning still reports roughly 15%, not 0%. diff --git a/cmd/gpu-cuda-profile/main.go b/cmd/gpu-cuda-profile/main.go index 14ae1c4d..84e9714d 100644 --- a/cmd/gpu-cuda-profile/main.go +++ b/cmd/gpu-cuda-profile/main.go @@ -24,6 +24,7 @@ import ( "github.com/dpsoft/perf-agent/internal/gpuabi" "github.com/dpsoft/perf-agent/pprof" "github.com/dpsoft/perf-agent/symbolize" + "github.com/dpsoft/perf-agent/unwind/procmap" ) func main() { @@ -54,7 +55,16 @@ func main() { // Without a symbolizer the sampled launch stacks still arrive and are // still accounted for, but every one of them degrades to no stack — the // profile would then be honest and useless, all GPU time unattributed. - sym, err := symbolize.NewLocalSymbolizer() + // The maps index the symbolizer falls back on for addresses blazesym + // cannot name. It is consulted DURING the run, while the workload is + // alive; by the time this tool builds the profile the workload has + // exited and /proc//maps is gone, so a lookup at build time would + // find nothing. Without this, every frame inside a stripped vendor + // library (libcuda, libcupti - NVIDIA ships no symbols for their + // internals) renders as a bare ASLR'd address. + modules := procmap.NewResolver() + defer modules.Close() + sym, err := symbolize.NewLocalSymbolizer(symbolize.WithModuleIndex(modules)) if err != nil { log.Fatalf("symbolizer: %v", err) } diff --git a/cmd/gpu-stub-profile/main.go b/cmd/gpu-stub-profile/main.go index eafd2289..d8eb94ce 100644 --- a/cmd/gpu-stub-profile/main.go +++ b/cmd/gpu-stub-profile/main.go @@ -14,6 +14,7 @@ import ( "github.com/dpsoft/perf-agent/internal/gpuabi" "github.com/dpsoft/perf-agent/pprof" "github.com/dpsoft/perf-agent/symbolize" + "github.com/dpsoft/perf-agent/unwind/procmap" ) func main() { @@ -23,7 +24,16 @@ func main() { // Without a symbolizer the sampled launch stacks still arrive and are // still accounted for, but every one of them degrades to no stack — the // profile would then be honest and useless, all GPU time unattributed. - sym, err := symbolize.NewLocalSymbolizer() + // The maps index the symbolizer falls back on for addresses blazesym + // cannot name. It is consulted DURING the run, while the workload is + // alive; by the time this tool builds the profile the workload has + // exited and /proc//maps is gone, so a lookup at build time would + // find nothing. Without this, every frame inside a stripped vendor + // library (libcuda, libcupti - NVIDIA ships no symbols for their + // internals) renders as a bare ASLR'd address. + modules := procmap.NewResolver() + defer modules.Close() + sym, err := symbolize.NewLocalSymbolizer(symbolize.WithModuleIndex(modules)) if err != nil { log.Fatalf("symbolizer: %v", err) } diff --git a/gpuprobe/consumer.go b/gpuprobe/consumer.go index c62a7c2b..cd0d82d9 100644 --- a/gpuprobe/consumer.go +++ b/gpuprobe/consumer.go @@ -935,6 +935,20 @@ type Stats struct { // well-resolved stack lands here. It is the ratio against // StacksResolved's frame count that is diagnostic, not the raw number. StackFramesUnresolved uint64 + // StackFramesModuleOnly is the subset of StackFramesUnresolved that at + // least knows which file the address fell in, and therefore renders as + // "libcuda.so.1+0x1b71c6" instead of "0x7f2c945b2c2b". NVIDIA ships no + // symbols for the libcuda/libcupti internals a launch stack is full of, + // so the name is genuinely unrecoverable while the module is not, and + // knowing a frame is seven deep inside libcuda is most of the answer. + // + // Healthy: close to StackFramesUnresolved. Worst: zero while + // StackFramesUnresolved is large - no ModuleIndex is wired into the + // symbolizer, or the target exits before its stacks are drained, and + // every unresolved frame is an ASLR'd address that means nothing across + // runs. The two are counted apart precisely so that case cannot hide + // inside the total. + StackFramesModuleOnly uint64 // StackDeleteFailed counts gpu_stacks entries the consumer read but could // not delete. See freeStackLocked: deletion is what stops the map // filling, so a rising count here is the early warning for capture @@ -2575,14 +2589,23 @@ func (c *Consumer) resolveStackLocked(pid uint32, stackID int32) ([]pp.Frame, bo // indistinguishable from a function genuinely called "0x4017c2". // // Bounded and allocation-free: one pass over the frames already in hand, - // two integer increments, nothing retained. - var resolved int + // a few integer increments, nothing retained. + var resolved, moduleOnly int for i := range frames { if frames[i].Reason == symbolize.FailureNone { resolved++ + continue + } + // Unresolved, but the symbolizer placed it in a mapping - see + // symbolize.attachModules. Counted apart from the plain total so a + // run that recovers no modules at all cannot read the same as one + // that recovers them for every frame. + if _, ok := frames[i].ModuleOffset(); ok { + moduleOnly++ } } c.stats.StackFramesUnresolved += uint64(len(frames) - resolved) + c.stats.StackFramesModuleOnly += uint64(moduleOnly) if resolved == 0 && len(frames) > 0 { c.stats.StacksUnresolved++ } diff --git a/internal/flamegraph/domain.go b/internal/flamegraph/domain.go index 25b02ea2..b8919b8a 100644 --- a/internal/flamegraph/domain.go +++ b/internal/flamegraph/domain.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/dpsoft/perf-agent/internal/foldedstacks" + "github.com/dpsoft/perf-agent/internal/framename" ) // Domain is what a frame *is*, not how deep it sits or what its name hashes @@ -41,6 +42,9 @@ import ( // - DomainUnsymbolized. These frames were unwound correctly; no symbol // table could name them. That is a symbolization gap, not a hole in the // stack, and the reader must be able to tell the difference at a glance. +// Two grades of it share the domain and are told apart by the label: +// "libcuda.so.1+0x1b71c6" means the module is known and only the symbol +// is missing, "0x7f2c945b2c2b" means not even the module is. // They keep the CPU band's warm hue drained to a pale sand, and keep the // hatch: the layer is known, the name is not. Warm-but-colourless, so it // never reads as one of the named CPU layers and never steals the pure @@ -114,7 +118,7 @@ var domainInfo = [numDomains]DomainInfo{ }, DomainUnsymbolized: { Key: "unsym", Label: "vendor, no symbols", Fill: "var(--fill-unsym)", Overlay: "var(--hatch-gap)", - Desc: "Unwound correctly; no symbol table could name it. The depth is real, the names are missing — usually a stripped vendor library with no exported symbols. The CPU band's hue, drained: right layer, no name.", + Desc: "Unwound correctly; no symbol table could name it. The depth is real, the names are missing — usually a stripped vendor library with no exported symbols. Labelled module+offset (libcuda.so.1+0x1b71c6) where the profile knows which file the address fell in, and as a bare address where it does not. The CPU band's hue, drained: right layer, no name.", }, DomainProfilerShim: { Key: "shim", Label: "perf-agent", Fill: "var(--fill-shim)", Stroke: "var(--edge-shim)", @@ -163,6 +167,21 @@ func Classify(name, module string) Domain { // --- module-derived, i.e. taken from the profile rather than guessed --- case module == "[kernel]": return DomainKernel + + // --- no symbol, whether or not the module is known --- + // + // This sits ABOVE the remaining module rules on purpose. Once frames + // carry their mapping, an unnamed frame inside libcuda.so.1 matches + // isVendorModule, and colouring it as ordinary vendor code would erase + // the only signal saying no symbol table could name it - which is what + // DomainUnsymbolized exists to show ("vendor, no symbols"). Its label + // now carries the module too ("libcuda.so.1+0x1b71c6"), so the layer is + // still legible; the hatch is what says the name is not. + // + // Kernel stays above this, so a hex-named kernel frame is still orange. + case isUnsymbolized(name): + return DomainUnsymbolized + case isShimModule(module): return DomainProfilerShim case isVendorModule(module): @@ -171,8 +190,6 @@ func Classify(name, module string) Domain { return DomainSystem // --- name-derived --- - case isUnsymbolized(name): - return DomainUnsymbolized case isShimSymbol(name): return DomainProfilerShim case isVendorSymbol(name): @@ -280,19 +297,8 @@ func isSystemSymbol(name string) bool { } // isUnsymbolized reports whether a frame name carries no symbol: a bare -// address, or the placeholder the folder writes for a location with neither. +// address, the module-relative "libcuda.so.1+0x1b71c6" form, or the +// placeholder the folder writes for a location with neither. func isUnsymbolized(name string) bool { - return name == foldedstacks.UnknownFrame || (strings.HasPrefix(name, "0x") && isHex(name[2:])) -} - -func isHex(s string) bool { - if s == "" { - return false - } - for _, r := range s { - if (r < '0' || r > '9') && (r < 'a' || r > 'f') && (r < 'A' || r > 'F') { - return false - } - } - return true + return name == foldedstacks.UnknownFrame || framename.IsAddressOnly(name) } diff --git a/internal/flamegraph/domain_test.go b/internal/flamegraph/domain_test.go index e95db992..1a7d6500 100644 --- a/internal/flamegraph/domain_test.go +++ b/internal/flamegraph/domain_test.go @@ -87,3 +87,30 @@ func TestBoundaryAndUnattributedBoundaryLookDifferent(t *testing.T) { assert.Empty(t, a.Overlay) assert.NotEmpty(t, b.Overlay, "unattributed GPU time must be hatched") } + +// Once frames carry their mapping, an unnamed frame inside libcuda matches +// isVendorModule. It must still read as unsymbolized: the module is a fact the +// profile now supplies, but no symbol table named this address, and the hatch +// is the only thing on the page that says so. +func TestUnsymbolizedWinsOverModule(t *testing.T) { + const mod = "/usr/lib/x86_64-linux-gnu/libcuda.so.1" + assert.Equal(t, DomainUnsymbolized, Classify("libcuda.so.1+0x1b71c6", mod)) + assert.Equal(t, DomainUnsymbolized, Classify("0x7f2c945b2c2b", mod)) + // The resolved frame in the same library is unaffected. + assert.Equal(t, DomainVendorRuntime, Classify("cuLaunchKernel", mod)) +} + +// The bare form and the module form share a domain on purpose - both are +// "unwound, unnamed" - and are told apart by their label, which is the only +// place the difference belongs. +func TestBothUnresolvedFormsShareTheDomainButNotTheLabel(t *testing.T) { + assert.Equal(t, DomainUnsymbolized, Classify("libcupti.so.12+0x0", "/usr/lib/libcupti.so.12")) + assert.Equal(t, DomainUnsymbolized, Classify("0xdeadbeef", "")) + assert.NotEqual(t, "libcupti.so.12+0x0", "0xdeadbeef") +} + +// Kernel classification is checked before the unsymbolized rule, so a raw +// kernel address is still drawn as kernel. +func TestKernelStillBeatsUnsymbolized(t *testing.T) { + assert.Equal(t, DomainKernel, Classify("0xffffffffc0201234", "[kernel]")) +} diff --git a/internal/foldedstacks/fold.go b/internal/foldedstacks/fold.go index db1fa890..2f34ee65 100644 --- a/internal/foldedstacks/fold.go +++ b/internal/foldedstacks/fold.go @@ -33,6 +33,8 @@ import ( "strings" "github.com/google/pprof/profile" + + "github.com/dpsoft/perf-agent/internal/framename" ) // StackOrder describes how a profile stores frames within Sample.Location. @@ -415,34 +417,46 @@ func lineName(loc *profile.Location, ln profile.Line) string { return addressName(loc) } +// addressName names a Location that has no usable Function. The mapping form +// is preferred over the bare address: Location.Address is already relative to +// the mapping, so "libcuda.so.1+0x1b71c6" is the same number with the file it +// belongs to attached, and it survives ASLR where the bare form does not. +// +// (The bare form was previously unreachable-first here: the Address!=0 test +// came before the mapping test, so a location with both always printed the +// address alone.) func addressName(loc *profile.Location) string { - if loc != nil && loc.Address != 0 { - return fmt.Sprintf("0x%x", loc.Address) + if loc == nil { + return UnknownFrame } - if loc != nil && loc.Mapping != nil && loc.Mapping.File != "" { - return fmt.Sprintf("%s+0x%x", loc.Mapping.File, loc.Address) + if loc.Mapping != nil { + if n := framename.Format(loc.Mapping.File, loc.Address); n != "" { + return n + } + } + if loc.Address != 0 { + return fmt.Sprintf("0x%x", loc.Address) } return UnknownFrame } // isAddressOnly reports whether a frame name carries no symbol. perf-agent's -// symbolizer already formats unresolved PCs as "0x7f2c945ace62" and missing -// frames as "[unknown]", so a name-shaped test is the only way to count them -// once the profile is written; there is no separate "unsymbolized" bit in -// pprof to consult. +// symbolizer formats an unresolved PC as "0x7f2c945ace62" when nothing is +// known about it, as "libcuda.so.1+0x1b71c6" when the module is known but the +// symbol is not, and missing frames as "[unknown]"; a name-shaped test is the +// only way to count them once the profile is written, because there is no +// separate "unsymbolized" bit in pprof to consult. +// +// The module-relative form counts here too. Naming the library is a real +// improvement, but it is not a symbol, and AddressOnlyFrames is the number +// the warning banner reports as the profile's symbolization gap. Excluding +// it would have made that gap read 0% on exactly the runs where it is +// largest. func isAddressOnly(name string) bool { if name == UnknownFrame || name == "" { return true } - if !strings.HasPrefix(name, "0x") || len(name) < 3 { - return false - } - for _, r := range name[2:] { - if (r < '0' || r > '9') && (r < 'a' || r > 'f') && (r < 'A' || r > 'F') { - return false - } - } - return true + return framename.IsAddressOnly(name) } func matchesInexact(labels map[string][]string, rules []InexactRule) bool { diff --git a/internal/foldedstacks/gpu_stack_e2e_test.go b/internal/foldedstacks/gpu_stack_e2e_test.go new file mode 100644 index 00000000..3734694a --- /dev/null +++ b/internal/foldedstacks/gpu_stack_e2e_test.go @@ -0,0 +1,318 @@ +package foldedstacks_test + +import ( + "bytes" + "strings" + "testing" + + gprofile "github.com/google/pprof/profile" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dpsoft/perf-agent/internal/flamegraph" + "github.com/dpsoft/perf-agent/internal/foldedstacks" + pp "github.com/dpsoft/perf-agent/pprof" + "github.com/dpsoft/perf-agent/symbolize" + "github.com/dpsoft/perf-agent/unwind/procmap" +) + +// This is the 16-frame joined stack from the real RTX 3090 capture in +// gpu-cuda-45.pb.gz, transcribed from `go tool pprof -raw` on that file: +// locations 4..18 then 2, with seven consecutive libcuda internals rendering +// as bare hex because NVIDIA ships no symbols for them. +// +// What that raw dump also shows is the loss this change is about. Every +// location reads "0x0 M=1" - address zero, mapping one - and the whole file +// has one mapping, "1: 0x0/0x0/0x0". The frames are missing not merely a +// symbol but the module. +// +// The capture cannot be re-rendered: the mapping data was never written into +// it. So this drives the NEW pipeline over the same stack, starting where the +// old one starts - symbolize.Frames straight out of blazesym, which for a +// miss returns name "", module "", offset 0 and nothing but a Reason +// (capi/src/symbolize.rs zeroes the whole blaze_sym) - and ending where a +// reader looks: the pprof Function names and the folded stacks. +// +// The mapping ranges below are a FIXTURE. The real run's /proc//maps was +// never recorded, so the ranges are chosen to contain the observed addresses; +// they are not a claim about where libcuda actually sat on that machine. What +// is real here is the stack, the addresses, and the arithmetic. +var ( + cudaMap = procmap.Mapping{ + Path: "/usr/lib/x86_64-linux-gnu/libcuda.so.550.54.14", + // Contains all seven unresolved addresses, 0x7f2c944de06b..0x7f2c958b71c6. + Start: 0x7f2c94400000, Limit: 0x7f2c96000000, Offset: 0x0, + BuildID: "0bd1a2f4c0de", IsExec: true, + } + appMap = procmap.Mapping{ + Path: "/home/diego/work/cuda_workload", + Start: 0x400000, Limit: 0x480000, Offset: 0x0, + BuildID: "1234abcd", IsExec: true, + } +) + +type mapsFixture struct{ maps []procmap.Mapping } + +func (f mapsFixture) Lookup(_ uint32, addr uint64) (procmap.Mapping, bool) { + for _, m := range f.maps { + if addr >= m.Start && addr < m.Limit { + return m, true + } + } + return procmap.Mapping{}, false +} + +// unresolvedPCs are the seven, in stack order, exactly as the capture has +// them (raw locations 10..16). +var unresolvedPCs = []uint64{ + 0x7f2c958b71c6, + 0x7f2c945ace62, + 0x7f2c945acc75, + 0x7f2c945b2dfb, + 0x7f2c945b2c2b, + 0x7f2c945bbf6f, + 0x7f2c944de06b, +} + +// blazesymOutput is the CPU half of the stack as the symbolizer receives it, +// root-first: raw locations 4..17. The two synthetic GPU frames above it +// ([gpu:launch], [gpu:kernel:...]) are added by gpu/projection.go, not by the +// symbolizer, and are appended in buildProfile. +func blazesymOutput() []symbolize.Frame { + named := func(addr uint64, name string) symbolize.Frame { + return symbolize.Frame{Address: addr, Name: name, Reason: symbolize.FailureNone} + } + out := []symbolize.Frame{ + named(0x401060, "_start"), + named(0x7f2c98029d90, "__libc_start_main_alias_1"), + named(0x7f2c98029e40, "__libc_start_call_main"), + named(0x4023a0, "main"), + named(0x402150, "__device_stub__Z14perfagent_axpyfPKfPfi(float, float const*, float*, int)"), + named(0x7f2c96a12340, "cudaLaunchKernel"), + } + for _, pc := range unresolvedPCs { + // This is all blazesym gives back for an address it cannot name. + // symbolize/local.go then writes the hex address into Name so the + // location renders as something rather than . + out = append(out, symbolize.Frame{ + Address: pc, + Name: "0x" + hexs(pc), + Reason: symbolize.FailureMissingSymbols, + }) + } + return append(out, + named(0x7f2c92a04510, "(anonymous namespace)::on_callback(void*, CUpti_CallbackDomain, unsigned int, void const*)"), + ) +} + +func hexs(v uint64) string { + const digits = "0123456789abcdef" + if v == 0 { + return "0" + } + var buf [16]byte + i := len(buf) + for v > 0 { + i-- + buf[i] = digits[v&0xf] + v >>= 4 + } + return string(buf[i:]) +} + +// buildProfile runs the frames through the three steps the GPU pipeline does: +// attach modules (inside the symbolizer, while the target is still alive) -> +// ToProfFrames -> the pprof builder, then appends the two synthetic frames +// gpu/projection.go nests above the CPU stack. Passing idx==nil reproduces +// today's behaviour. +// +// attachModules is unexported, so the loop below stands in for it; what it +// does is the contract symbolize/module_test.go pins directly. +func buildProfile(t *testing.T, idx symbolize.ModuleIndex) *gprofile.Profile { + t.Helper() + frames := blazesymOutput() + if idx != nil { + for i := range frames { + if frames[i].Reason == symbolize.FailureNone { + continue + } + m, ok := idx.Lookup(4242, frames[i].Address) + if !ok { + continue + } + frames[i].Module = m.Path + frames[i].BuildID = m.BuildID + frames[i].MapStart, frames[i].MapLimit, frames[i].MapOff = m.Start, m.Limit, m.Offset + } + } + + stack := symbolize.ToProfFrames(frames) + stack = append(stack, + pp.FrameFromName("[gpu:launch]"), + pp.FrameFromName("[gpu:kernel:_Z14perfagent_axpyfPKfPfi]"), + ) + + bs := pp.NewProfileBuilders(pp.BuildersOptions{SampleRate: 1}) + bs.AddSample(&pp.ProfileSample{ + Pid: 4242, SampleType: pp.SampleTypeGpu, Value: 1536, Stack: stack, + }) + + var buf bytes.Buffer + for _, b := range bs.Builders { + _, err := b.Write(&buf) + require.NoError(t, err) + break + } + p, err := gprofile.Parse(&buf) + require.NoError(t, err) + return p +} + +func names(p *gprofile.Profile) []string { + var out []string + for _, loc := range p.Sample[0].Location { + for _, ln := range loc.Line { + out = append(out, ln.Function.Name) + } + } + return out +} + +// TestGPUStack_Before reproduces gpu-cuda-45.pb.gz: no module index, so seven +// frames are bare hex and the whole profile has the one default mapping. +func TestGPUStack_Before(t *testing.T) { + p := buildProfile(t, nil) + + require.Len(t, p.Mapping, 1, "the profile has exactly one mapping") + assert.Equal(t, uint64(0), p.Mapping[0].Start) + assert.Equal(t, uint64(0), p.Mapping[0].Limit) + assert.Empty(t, p.Mapping[0].File, "0x0/0x0/0x0") + + got := names(p) + require.Len(t, got, 16) + assert.Equal(t, []string{ + "0x7f2c958b71c6", "0x7f2c945ace62", "0x7f2c945acc75", "0x7f2c945b2dfb", + "0x7f2c945b2c2b", "0x7f2c945bbf6f", "0x7f2c944de06b", + }, got[6:13], "the seven frames as the real capture renders them") +} + +// TestGPUStack_After is what the new pipeline produces for the same stack. +func TestGPUStack_After(t *testing.T) { + idx := mapsFixture{maps: []procmap.Mapping{appMap, cudaMap}} + p := buildProfile(t, idx) + + got := names(p) + require.Len(t, got, 16) + + want := []string{ + "_start", + "__libc_start_main_alias_1", + "__libc_start_call_main", + "main", + "__device_stub__Z14perfagent_axpyfPKfPfi(float, float const*, float*, int)", + "cudaLaunchKernel", + // The seven. Each offset is its PC minus cudaMap.Start. + "libcuda.so.550.54.14+0x14b71c6", + "libcuda.so.550.54.14+0x1ace62", + "libcuda.so.550.54.14+0x1acc75", + "libcuda.so.550.54.14+0x1b2dfb", + "libcuda.so.550.54.14+0x1b2c2b", + "libcuda.so.550.54.14+0x1bbf6f", + "libcuda.so.550.54.14+0xde06b", + "(anonymous namespace)::on_callback(void*, CUpti_CallbackDomain, unsigned int, void const*)", + "[gpu:launch]", + "[gpu:kernel:_Z14perfagent_axpyfPKfPfi]", + } + assert.Equal(t, want, got) + + // Every frame that already had a name renders byte-for-byte as before - + // including the two synthetic GPU frames, which carry no address and must + // never acquire a module. + before := names(buildProfile(t, nil)) + for _, i := range []int{0, 1, 2, 3, 4, 5, 13, 14, 15} { + assert.Equal(t, before[i], got[i], "frame %d changed", i) + } + + // The profile now describes a real file, and the Location addresses are + // the same module-relative offsets the names carry - so `go tool pprof + // -raw` shows "0x14b71c6 M=2" where it used to show "0x0 M=1", and either + // number can be fed to addr2line -e libcuda.so.550.54.14. + var cuda *gprofile.Mapping + for _, m := range p.Mapping { + if m.File == cudaMap.Path { + cuda = m + } + } + require.NotNil(t, cuda, "libcuda mapping interned") + assert.Equal(t, cudaMap.Start, cuda.Start) + assert.Equal(t, cudaMap.Limit, cuda.Limit) + assert.Equal(t, cudaMap.BuildID, cuda.BuildID) + assert.False(t, cuda.HasFunctions, "the mapping must not claim symbols it does not have") + + for _, loc := range p.Sample[0].Location { + if loc.Mapping != cuda { + continue + } + assert.Equal(t, "libcuda.so.550.54.14+0x"+hexs(loc.Address), loc.Line[0].Function.Name, + "the name and Location.Address must be the same number") + } +} + +// A mapping index that covers only the application leaves the libcuda frames +// exactly as they are today. Recovering the module is not all-or-nothing, and +// the frames that miss must stay bare rather than borrow a neighbour's file. +func TestGPUStack_PartialIndexLeavesTheRestBare(t *testing.T) { + p := buildProfile(t, mapsFixture{maps: []procmap.Mapping{appMap}}) + got := names(p) + for i := 6; i < 13; i++ { + assert.True(t, strings.HasPrefix(got[i], "0x"), "frame %d = %q", i, got[i]) + } + require.Len(t, p.Mapping, 1) +} + +// The honesty checks downstream must not improve just because the names did. +// Seven of sixteen frame slots still have no symbol; the warning banner must +// say so with and without modules. +func TestGPUStack_SymbolizationGapStillReported(t *testing.T) { + idx := mapsFixture{maps: []procmap.Mapping{appMap, cudaMap}} + + for _, tc := range []struct { + name string + idx symbolize.ModuleIndex + }{{"before", nil}, {"after", idx}} { + t.Run(tc.name, func(t *testing.T) { + res, err := foldedstacks.Fold(buildProfile(t, tc.idx), foldedstacks.Options{}) + require.NoError(t, err) + assert.Equal(t, 16, res.Frames) + assert.Equal(t, 7, res.AddressOnlyFrames, + "the symbolization gap must read the same whether or not modules were recovered") + + var warned bool + for _, w := range res.Warnings { + if strings.Contains(w, "have no symbol") { + warned = true + } + } + assert.True(t, warned, "warnings: %v", res.Warnings) + }) + } +} + +// And the flame graph must still colour them as unsymbolized rather than +// promoting them to ordinary vendor frames now that the module is known. +func TestGPUStack_FlameGraphDomains(t *testing.T) { + idx := mapsFixture{maps: []procmap.Mapping{appMap, cudaMap}} + res, err := foldedstacks.Fold(buildProfile(t, idx), foldedstacks.Options{}) + require.NoError(t, err) + require.Len(t, res.Stacks, 1) + + st := res.Stacks[0] + require.Len(t, st.Frames, 16) + for i := 6; i < 13; i++ { + assert.Equal(t, flamegraph.DomainUnsymbolized, + flamegraph.Classify(st.Frames[i], st.Modules[i]), + "frame %d (%s) should read as unsymbolized", i, st.Frames[i]) + } + assert.Equal(t, flamegraph.DomainBoundary, flamegraph.Classify(st.Frames[14], st.Modules[14])) + assert.Equal(t, flamegraph.DomainGPUKernel, flamegraph.Classify(st.Frames[15], st.Modules[15])) +} diff --git a/internal/framename/framename.go b/internal/framename/framename.go new file mode 100644 index 00000000..8b4a5278 --- /dev/null +++ b/internal/framename/framename.go @@ -0,0 +1,134 @@ +// Package framename owns the one textual form perf-agent uses for a stack +// frame that was unwound correctly but could not be given a symbol name. +// +// There are exactly two such forms, and the difference between them is +// load-bearing: +// +// 0x7f2c945b2c2b no mapping covers this PC. Nothing is known about +// it beyond the address, and the address is an +// ASLR'd runtime VA that means nothing across runs. +// libcuda.so.1+0x1b71c6 a mapping covers this PC. The module is known and +// the offset is relative to that module's file, so +// it is stable across runs and can be fed to +// addr2line/objdump/nvdisasm later. +// +// The second form must never be synthesized from a guess: it is emitted only +// where a real /proc//maps entry produced the module and the offset. +// +// The form is centralized here because three packages have to agree on it. +// pprof/ writes it into Function.Name; internal/foldedstacks counts frames +// that carry no symbol and warns about the proportion; internal/flamegraph +// colours them as their own domain. If any one of those stopped recognizing +// the form, the profile would look better exactly when symbolization worked +// worst - which is the failure mode this package exists to prevent. +package framename + +import ( + "path" + "strings" +) + +// Format renders the module-relative form. modulePath is the mapping's file +// (the full path; only its base is shown), off is the offset of the PC +// within that file. +// +// Returns "" when modulePath is empty or is one of the bracketed sentinel +// "files" perf-agent uses for non-file mappings ("[kernel]", "[jit]"). +// Callers must treat "" as "keep the bare address": inventing a module is +// worse than admitting there is none. +func Format(modulePath string, off uint64) string { + if modulePath == "" || strings.HasPrefix(modulePath, "[") { + return "" + } + b := path.Base(modulePath) + if b == "" || b == "." || b == "/" { + return "" + } + return b + "+0x" + hexOf(off) +} + +// hexOf formats off as lowercase hex without the fmt package, which keeps +// this on the allocation budget the symbolize package asserts against. +func hexOf(off uint64) string { + if off == 0 { + return "0" + } + const digits = "0123456789abcdef" + var buf [16]byte + i := len(buf) + for off > 0 { + i-- + buf[i] = digits[off&0xf] + off >>= 4 + } + return string(buf[i:]) +} + +// IsAddressOnly reports whether name is one of the two unresolved forms: +// a bare "0x" address, or the "+0x" module-relative form. +// +// It does NOT recognize the "[unknown]" placeholder - that string belongs to +// the package that emits it, and callers already test for it separately. +// +// The module half is required to look like a file name (non-empty, and free +// of the characters that appear in demangled C++ signatures) so that a real +// symbol containing "+0x" - an expression baked into a template argument, a +// demangled operator+ overload - is not mistaken for an unresolved frame. +// +// What this cannot rule out: a symbol whose whole name is an identifier, a +// "+", and hex digits, with no parenthesis, colon, space or bracket anywhere. +// Once the profile is written the pprof.Frame.Unresolved bit is gone and +// there is nothing else to consult. Such a symbol would be drawn as +// unsymbolized and counted in the symbolization-gap warning - erring towards +// over-reporting the gap, never towards hiding it. +func IsAddressOnly(name string) bool { + if isHexAddr(name) { + return true + } + plus := strings.LastIndexByte(name, '+') + if plus <= 0 { + return false + } + return looksLikeModuleBase(name[:plus]) && isHexAddr(name[plus+1:]) +} + +// Module splits the module-relative form back into its parts. ok is false +// for anything that is not that form, including the bare-address form - +// which is the point: a caller asking "which module is this frame in?" must +// get "none" for an address with no mapping, not a plausible-looking guess. +func Module(name string) (module string, ok bool) { + plus := strings.LastIndexByte(name, '+') + if plus <= 0 { + return "", false + } + if !looksLikeModuleBase(name[:plus]) || !isHexAddr(name[plus+1:]) { + return "", false + } + return name[:plus], true +} + +func isHexAddr(s string) bool { + if !strings.HasPrefix(s, "0x") || len(s) < 3 { + return false + } + for i := 2; i < len(s); i++ { + if !isHexDigit(s[i]) { + return false + } + } + return true +} + +func isHexDigit(c byte) bool { + return (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F') +} + +// looksLikeModuleBase is deliberately strict. Format only ever produces +// path.Base of a mapping file, so anything with a separator, a space, or a +// character that only occurs in a demangled symbol is not one of ours. +func looksLikeModuleBase(s string) bool { + if s == "" || strings.HasPrefix(s, "[") { + return false + } + return !strings.ContainsAny(s, " \t/()<>,:*&[]") +} diff --git a/internal/framename/framename_test.go b/internal/framename/framename_test.go new file mode 100644 index 00000000..2a08c6ce --- /dev/null +++ b/internal/framename/framename_test.go @@ -0,0 +1,117 @@ +package framename + +import "testing" + +func TestFormat(t *testing.T) { + cases := []struct { + module string + off uint64 + want string + }{ + {"/usr/lib/x86_64-linux-gnu/libcuda.so.1", 0x1b71c6, "libcuda.so.1+0x1b71c6"}, + {"libcupti.so.12", 0, "libcupti.so.12+0x0"}, + {"/opt/app/bin/worker", 0xdeadbeef, "worker+0xdeadbeef"}, + + // Never invent a module: the bracketed sentinels perf-agent uses for + // non-file mappings are not files, and an empty module is not a name. + {"", 0x10, ""}, + {"[kernel]", 0x10, ""}, + {"[jit]", 0x10, ""}, + } + for _, c := range cases { + if got := Format(c.module, c.off); got != c.want { + t.Errorf("Format(%q, %#x) = %q, want %q", c.module, c.off, got, c.want) + } + } +} + +func TestIsAddressOnly(t *testing.T) { + yes := []string{ + "0x7f2c945b2c2b", + "0x0", + "0xDEADBEEF", + "libcuda.so.1+0x1b71c6", + "libcupti.so.12+0x0", + "worker+0xdeadbeef", + } + for _, n := range yes { + if !IsAddressOnly(n) { + t.Errorf("IsAddressOnly(%q) = false, want true", n) + } + } + + // The second group is the one that matters: a real symbol must never be + // mistaken for an unresolved frame, or the profile's symbolization-gap + // warning would under-report itself. + no := []string{ + "main.main", + "cuLaunchKernel", + "0x", + "0xNotHex", + "+0x10", + "std::operator+(int)+0x10", // demangled C++: parens and colons + "foo bar+0x10", // space + "/usr/lib/libc.so.6+0x10", // a path, not a base: Format never emits this + "[kernel]+0x10", // sentinel + "a+0x10", // template + "libcuda.so.1+0xzz", + "libcuda.so.1+", + } + for _, n := range no { + if IsAddressOnly(n) { + t.Errorf("IsAddressOnly(%q) = true, want false", n) + } + } +} + +// TestIsAddressOnlyAcceptsEverythingFormatEmits is the property that keeps the +// producer and the three consumers from drifting apart. +func TestIsAddressOnlyAcceptsEverythingFormatEmits(t *testing.T) { + mods := []string{ + "/usr/lib/x86_64-linux-gnu/libcuda.so.1", + "/usr/lib/libcupti.so.12.4.127", + "/opt/a-b_c.d/exe", + "ld-linux-x86-64.so.2", + } + for _, m := range mods { + for _, off := range []uint64{0, 1, 0x1b71c6, ^uint64(0)} { + n := Format(m, off) + if n == "" { + t.Fatalf("Format(%q, %#x) returned empty", m, off) + } + if !IsAddressOnly(n) { + t.Errorf("Format produced %q which IsAddressOnly rejects", n) + } + if mod, ok := Module(n); !ok || mod == "" { + t.Errorf("Module(%q) = %q, %v; want a non-empty module", n, mod, ok) + } + } + } +} + +// TestKnownAmbiguity states the one thing this test cannot rule out, so that +// nobody later reads TestIsAddressOnly as a proof it is impossible. +// +// A symbol whose demangled name is EXACTLY an identifier followed by "+0x" and +// hex digits - no parentheses, colons, spaces or template brackets anywhere - +// is indistinguishable from the module-relative form once the profile is +// written, because pprof has no unsymbolized bit to carry the difference. No +// such symbol has been observed; the consequence if one appeared would be a +// frame drawn in the unsymbolized domain and counted in the symbolization-gap +// warning. That direction is the safe one: it over-reports the gap rather than +// hiding it. +func TestKnownAmbiguity(t *testing.T) { + if !IsAddressOnly("operator+0x10") { + t.Skip("behaviour changed; update the doc above with the new limit") + } +} + +func TestModuleRefusesBareAddress(t *testing.T) { + // A bare address has no module, and the caller must not be handed one. + if mod, ok := Module("0x7f2c945b2c2b"); ok { + t.Errorf("Module(bare address) = %q, true; want ok=false", mod) + } + if mod, ok := Module("main.main"); ok { + t.Errorf("Module(symbol) = %q, true; want ok=false", mod) + } +} diff --git a/perfagent/agent.go b/perfagent/agent.go index 7a416b5e..19858b6a 100644 --- a/perfagent/agent.go +++ b/perfagent/agent.go @@ -200,7 +200,19 @@ func chooseSymbolizer(cfg *Config, res *procmap.Resolver, logger *slog.Logger) ( } } if len(urls) == 0 { - return symbolize.NewLocalSymbolizer() + // res names the module behind an address blazesym could not resolve + // to a symbol, so an unresolved frame reads "libcuda.so.1+0x1b71c6" + // instead of an ASLR'd address. It is the same Resolver the + // debuginfod branch below takes, so exactly one maps cache exists + // per agent either way. + // + // It is never invalidated, which it shares with the Resolvers + // profile/ and offcpu/ hand to the pprof builders: a PID that exits + // and is reused within one run would keep the first process's + // mappings. Adding module attribution here does not widen that + // window - the builders already attribute every user frame through + // the same kind of cache - but it does not narrow it either. + return symbolize.NewLocalSymbolizer(symbolize.WithModuleIndex(res)) } cacheDir := cmp.Or(cfg.SymbolCacheDir, "/tmp/perf-agent-debuginfod") cacheMax := cmp.Or(cfg.SymbolCacheMaxBytes, int64(2<<30)) diff --git a/pprof/pprof.go b/pprof/pprof.go index 48f33b99..72463a22 100644 --- a/pprof/pprof.go +++ b/pprof/pprof.go @@ -16,6 +16,7 @@ import ( "github.com/klauspost/compress/gzip" + "github.com/dpsoft/perf-agent/internal/framename" "github.com/dpsoft/perf-agent/unwind/procmap" ) @@ -75,6 +76,18 @@ type Frame struct { MapLimit uint64 MapOff uint64 IsKernel bool + + // Unresolved says this frame was unwound correctly but could not be + // given a symbol name, and that Name therefore holds a placeholder + // (perf-agent's symbolizers write the hex PC) rather than a function. + // + // It has to be carried rather than inferred: "0x4017c2" is a valid C + // identifier for a symbol and pprof has no unsymbolized bit to consult. + // Set by symbolize.ToProfFrames from symbolize.Frame.Reason. When it is + // set AND the frame lands in a real file-backed mapping, addLocationByAddr + // renames it to "+0x"; a resolved frame is never + // renamed. + Unresolved bool } // FrameFromName is a convenience constructor for callers that only know the @@ -355,10 +368,50 @@ func (p *ProfileBuilder) addLocation(frame Frame, pid uint32) *profile.Location } } - // 4. Fallback: name-based dedup on the default single mapping. + // 3b. Frame-carried mapping. The frame already knows which file it fell + // in because the symbolizer looked it up while the target was still + // alive (symbolize.attachModules). This is the only path that works when + // the profile is built after the process exited - the GPU tools do + // exactly that - and it is also the only mapping the GPU pipeline has at + // all, since it wires no Resolver into the builder. + // + // Deliberately below the Resolver: when both are available they agree, + // and preferring the live lookup keeps this a pure addition rather than + // a change to what the CPU profilers already produce. + if m, ok := frameMapping(frame); ok { + mapping := p.addMapping(m, frame) + return p.addLocationByAddr(mapping, frame) + } + + // 4. Fallback: name-based dedup on the default single mapping. Nothing + // is known about where this address lives, so an Unresolved frame keeps + // its bare "0x..." name here. That is not the same outcome as branch 3b + // and must never be made to look like it. return p.addLocationByFallback(p.Profile.Mapping[0], frame) } +// frameMapping reconstructs the procmap.Mapping a Frame carries, when it +// carries a usable one. Requires the address to fall inside the range: a +// frame whose MapStart/MapLimit do not actually contain its Address is a bug +// upstream, and building a mapping from it would put the location at a +// nonsense file offset under a confidently-named file. +func frameMapping(frame Frame) (procmap.Mapping, bool) { + if frame.Module == "" || frame.Address == 0 || frame.MapLimit <= frame.MapStart { + return procmap.Mapping{}, false + } + if frame.Address < frame.MapStart || frame.Address >= frame.MapLimit { + return procmap.Mapping{}, false + } + return procmap.Mapping{ + Path: frame.Module, + Start: frame.MapStart, + Limit: frame.MapLimit, + Offset: frame.MapOff, + BuildID: frame.BuildID, + IsExec: true, + }, true +} + func (p *ProfileBuilder) addMapping(m procmap.Mapping, frame Frame) *profile.Mapping { key := mappingKey{ Path: m.Path, Start: m.Start, Limit: m.Limit, Off: m.Offset, BuildID: m.BuildID, @@ -383,7 +436,11 @@ func (p *ProfileBuilder) addMapping(m procmap.Mapping, frame Frame) *profile.Map } func (p *ProfileBuilder) updateMappingFlags(m *profile.Mapping, f Frame) { - if f.Name != "" { + // An unresolved frame is not a function this mapping has. Setting + // HasFunctions for it would tell every downstream reader that the + // mapping's symbols are present when the reason we are here is that + // they are not. + if f.Name != "" && !f.Unresolved { m.HasFunctions = true } if f.File != "" { @@ -405,6 +462,20 @@ func (p *ProfileBuilder) addLocationByAddr(mapping *profile.Mapping, frame Frame if loc, ok := p.locations[key]; ok { return loc } + // An unresolved frame that landed in a real file gets named after that + // file and its module-relative offset. The absolute PC it arrived with + // is an ASLR'd runtime address, meaningless across runs; the offset is + // the mapping-relative one pprof already stores in Location.Address, so + // the name and the Location agree and either can be fed to addr2line. + // + // This goes into Function.Name rather than being left for the flame + // graph because the same .pb.gz is read by `go tool pprof`, which shows + // Function.Name and knows nothing about perf-agent's conventions. + if frame.Unresolved { + if n := framename.Format(mapping.File, offset); n != "" { + frame.Name = n + } + } id := uint64(len(p.Profile.Location) + 1) loc := &profile.Location{ ID: id, diff --git a/pprof/unresolved_test.go b/pprof/unresolved_test.go new file mode 100644 index 00000000..e4640109 --- /dev/null +++ b/pprof/unresolved_test.go @@ -0,0 +1,209 @@ +package pprof + +import ( + "path/filepath" + "testing" + + "github.com/google/pprof/profile" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dpsoft/perf-agent/unwind/procmap" +) + +// libcudaFrame is an unresolved frame carrying the mapping the symbolizer +// looked up while the target was alive: 0x7f2c958b71c6 inside libcuda.so.1, +// mapped at 0x7f2c94700000 with file offset 0x1000. +func libcudaFrame() Frame { + return Frame{ + Name: "0x7f2c958b71c6", + Address: 0x7f2c958b71c6, + Module: "/usr/lib/x86_64-linux-gnu/libcuda.so.1", + BuildID: "cafe", + MapStart: 0x7f2c94700000, + MapLimit: 0x7f2c96000000, + MapOff: 0x1000, + Unresolved: true, + } +} + +func onlyBuilder(t *testing.T, bs *ProfileBuilders) *ProfileBuilder { + t.Helper() + require.Len(t, bs.Builders, 1) + for _, b := range bs.Builders { + return b + } + return nil +} + +func leafLocation(t *testing.T, b *ProfileBuilder) *profile.Location { + t.Helper() + require.Len(t, b.Profile.Sample, 1) + require.NotEmpty(t, b.Profile.Sample[0].Location) + return b.Profile.Sample[0].Location[0] +} + +// The headline behaviour: a frame with no symbol but a known mapping renders +// as "+0x", not as an ASLR'd absolute address. +func TestUnresolvedFrameRendersAsModulePlusOffset(t *testing.T) { + bs := NewProfileBuilders(BuildersOptions{SampleRate: 1}) + bs.AddSample(&ProfileSample{ + Pid: 4242, SampleType: SampleTypeGpu, Value: 100, + Stack: []Frame{libcudaFrame()}, + }) + + b := onlyBuilder(t, bs) + loc := leafLocation(t, b) + + require.Len(t, loc.Line, 1) + assert.Equal(t, "libcuda.so.1+0x11b81c6", loc.Line[0].Function.Name) + + // The name and the Location agree: both are the same module-relative + // offset, so either can be fed to addr2line against libcuda.so.1. + assert.Equal(t, uint64(0x11b81c6), loc.Address) + + // And the profile now has a real mapping rather than the 0x0/0x0/0x0 + // default. This is what `go tool pprof -raw` reports. + require.NotNil(t, loc.Mapping) + assert.Equal(t, "/usr/lib/x86_64-linux-gnu/libcuda.so.1", loc.Mapping.File) + assert.Equal(t, uint64(0x7f2c94700000), loc.Mapping.Start) + assert.Equal(t, uint64(0x7f2c96000000), loc.Mapping.Limit) + assert.Equal(t, uint64(0x1000), loc.Mapping.Offset) + assert.Equal(t, "cafe", loc.Mapping.BuildID) + + // The mapping must not claim to have symbols. It is here precisely + // because it does not. + assert.False(t, loc.Mapping.HasFunctions) +} + +// The other half of the contract: no mapping, no module. The frame stays a +// bare address, on the default mapping, and is not quietly folded in with the +// frames that do know where they are. +func TestUnresolvedFrameWithNoMappingStaysBare(t *testing.T) { + bs := NewProfileBuilders(BuildersOptions{SampleRate: 1}) + bs.AddSample(&ProfileSample{ + Pid: 4242, SampleType: SampleTypeGpu, Value: 100, + Stack: []Frame{{Name: "0x7f2c945ace62", Address: 0x7f2c945ace62, Unresolved: true}}, + }) + + b := onlyBuilder(t, bs) + loc := leafLocation(t, b) + assert.Equal(t, "0x7f2c945ace62", loc.Line[0].Function.Name) + assert.Equal(t, uint64(1), loc.Mapping.ID, "should be the default mapping") + assert.Empty(t, loc.Mapping.File) +} + +// A frame whose carried range does not actually contain its address is a bug +// upstream. Building a mapping from it would put the location at a nonsense +// offset under a confidently-named file, so it is refused. +func TestFrameCarryingAnInconsistentMappingIsRefused(t *testing.T) { + f := libcudaFrame() + f.Address = 0x400000 // below MapStart + + bs := NewProfileBuilders(BuildersOptions{SampleRate: 1}) + bs.AddSample(&ProfileSample{ + Pid: 4242, SampleType: SampleTypeGpu, Value: 100, Stack: []Frame{f}, + }) + + b := onlyBuilder(t, bs) + loc := leafLocation(t, b) + assert.Equal(t, "0x7f2c958b71c6", loc.Line[0].Function.Name, "name must not be rewritten") + assert.Equal(t, uint64(1), loc.Mapping.ID) + assert.Len(t, b.Profile.Mapping, 1, "no mapping may be interned from an inconsistent frame") +} + +// A resolved frame renders exactly as it did before, mapping or no mapping. +func TestResolvedFrameIsNeverRenamed(t *testing.T) { + f := libcudaFrame() + f.Name = "cuLaunchKernel" + f.Unresolved = false + + bs := NewProfileBuilders(BuildersOptions{SampleRate: 1}) + bs.AddSample(&ProfileSample{ + Pid: 4242, SampleType: SampleTypeGpu, Value: 100, Stack: []Frame{f}, + }) + + b := onlyBuilder(t, bs) + loc := leafLocation(t, b) + assert.Equal(t, "cuLaunchKernel", loc.Line[0].Function.Name) + assert.True(t, loc.Mapping.HasFunctions) +} + +// Kernel frames route through the "[kernel]" sentinel, which is not a file. +// They must keep whatever name they arrived with - "[kernel]+0x..." would be +// a fabricated module. +func TestKernelSentinelIsNeverUsedAsAModuleName(t *testing.T) { + bs := NewProfileBuilders(BuildersOptions{SampleRate: 1}) + bs.AddSample(&ProfileSample{ + Pid: 4242, SampleType: SampleTypeCpu, Value: 1, + Stack: []Frame{{ + Name: "0xffffffffc0201234", Address: 0xffffffffc0201234, + IsKernel: true, Unresolved: true, + }}, + }) + + b := onlyBuilder(t, bs) + loc := leafLocation(t, b) + assert.Equal(t, "0xffffffffc0201234", loc.Line[0].Function.Name) + assert.Equal(t, "[kernel]", loc.Mapping.File) +} + +// The builder's own Resolver wins when it has an answer, so nothing about the +// CPU profilers' existing mapping attribution changes; the rename then runs on +// top of the mapping the Resolver produced. +func TestResolverStillWinsAndRenameAppliesOnTop(t *testing.T) { + resolver := procmap.NewResolver(procmap.WithProcRoot( + filepath.Join("..", "unwind", "procmap", "testdata", "proc"))) + defer resolver.Close() + + bs := NewProfileBuilders(BuildersOptions{SampleRate: 99, Resolver: resolver}) + bs.AddSample(&ProfileSample{ + Pid: 4242, SampleType: SampleTypeCpu, Value: 1, + Stack: []Frame{{Name: "0x401000", Address: 0x00401000, Unresolved: true}}, + }) + + b := onlyBuilder(t, bs) + loc := leafLocation(t, b) + require.NotNil(t, loc.Mapping) + assert.Equal(t, "/usr/bin/target", loc.Mapping.File) + assert.Equal(t, "target+0x"+hexs(loc.Address), loc.Line[0].Function.Name) + assert.False(t, loc.Mapping.HasFunctions) +} + +func hexs(v uint64) string { + const digits = "0123456789abcdef" + if v == 0 { + return "0" + } + var buf [16]byte + i := len(buf) + for v > 0 { + i-- + buf[i] = digits[v&0xf] + v >>= 4 + } + return string(buf[i:]) +} + +// Two unresolved frames at different offsets in the same library must stay +// two frames. Renaming must not collapse a call path into one box. +func TestTwoOffsetsInOneModuleStayDistinct(t *testing.T) { + f1 := libcudaFrame() + f2 := libcudaFrame() + f2.Address = 0x7f2c958b7200 + f2.Name = "0x7f2c958b7200" + + bs := NewProfileBuilders(BuildersOptions{SampleRate: 1}) + bs.AddSample(&ProfileSample{ + Pid: 4242, SampleType: SampleTypeGpu, Value: 100, Stack: []Frame{f1, f2}, + }) + + b := onlyBuilder(t, bs) + require.Len(t, b.Profile.Sample[0].Location, 2) + n0 := b.Profile.Sample[0].Location[0].Line[0].Function.Name + n1 := b.Profile.Sample[0].Location[1].Line[0].Function.Name + assert.NotEqual(t, n0, n1) + assert.Equal(t, "libcuda.so.1+0x11b81c6", n0) + assert.Equal(t, "libcuda.so.1+0x11b8200", n1) + assert.Len(t, b.Profile.Mapping, 2, "one default + one libcuda") +} diff --git a/symbolize/debuginfod/symbolizer.go b/symbolize/debuginfod/symbolizer.go index c27770f3..f9c18d50 100644 --- a/symbolize/debuginfod/symbolizer.go +++ b/symbolize/debuginfod/symbolizer.go @@ -73,13 +73,26 @@ func New(opts Options) (*Symbolizer, error) { // /proc//exe is restricted). Construct lazily-tolerant: // failure to build the fallback is non-fatal — the dispatcher // just runs without it. - if lf, lfErr := symbolize.NewLocalSymbolizer(); lfErr == nil { + // opts.Resolver doubles as the fallback's module index, so a frame the + // fallback cannot name still says which library it is in. It may be nil, + // in which case those frames stay bare addresses. + if lf, lfErr := symbolize.NewLocalSymbolizer(symbolize.WithModuleIndex(moduleIndex(opts.Resolver))); lfErr == nil { st.localFallback = lf } s.cgo = st return s, nil } +// moduleIndex adapts a possibly-nil *procmap.Resolver to symbolize.ModuleIndex +// without handing over a non-nil interface wrapping a nil pointer, which would +// panic on the first Lookup instead of being skipped. +func moduleIndex(r *procmap.Resolver) symbolize.ModuleIndex { + if r == nil { + return nil + } + return r +} + // SymbolizeProcess resolves abs IPs into Frames. Each address is routed // per-mapping: // diff --git a/symbolize/local.go b/symbolize/local.go index daeeaf1f..0eb38ce6 100644 --- a/symbolize/local.go +++ b/symbolize/local.go @@ -23,16 +23,41 @@ var ErrMapFilesUnavailable = errors.New("symbolize: cannot follow /proc//ma // preserves perf-agent's pre-debuginfod behavior. Used when no debuginfod // URL is configured. type LocalSymbolizer struct { - bz *blazesym.Symbolizer - closed atomic.Bool - stats localCounters + bz *blazesym.Symbolizer + modules ModuleIndex + closed atomic.Bool + stats localCounters +} + +// LocalOption configures a LocalSymbolizer. +type LocalOption func(*LocalSymbolizer) + +// WithModuleIndex supplies the /proc//maps index used to name the module +// behind an address blazesym could not resolve to a symbol. Without it, an +// unresolved frame stays a bare hex address - which is the honest result, not +// a degraded one, because there is then nothing to say about it. +// +// Pass the *procmap.Resolver the rest of the pipeline already owns rather +// than building a private one: a second cache doubles the /proc parsing and +// gives the caller two things to keep fresh instead of one. +// +// The index is consulted only for frames blazesym failed on, and only with +// Lookup - a miss leaves the frame bare rather than guessing at a neighbour. +// What it cannot defend against is a Resolver whose cache has gone stale +// because the PID exited and was reused; keeping the cache honest is the +// owner's job, exactly as it already is for the Resolver the pprof builder +// uses to attribute every user frame. +func WithModuleIndex(idx ModuleIndex) LocalOption { + return func(s *LocalSymbolizer) { s.modules = idx } } // localCounters are the process-side symbolization counters. Kept separate // from Counters, which is the kernel symbolizer's and is already wired into // the /metrics endpoint with a fixed field set. type localCounters struct { - rawAddrBatches atomic.Uint64 + rawAddrBatches atomic.Uint64 + modulesAttached atomic.Uint64 + modulesBare atomic.Uint64 } // LocalStats is a point-in-time view of what SymbolizeProcess could not @@ -42,11 +67,27 @@ type LocalStats struct { // cause being a pid that exited before its /proc entry could be read — // and where every frame therefore came back as a bare hex address. RawAddrBatches uint64 + // ModulesAttached counts frames that blazesym could not name but which + // a mapping lookup placed inside a known file: they render as + // "libcuda.so.1+0x1b71c6" rather than as a bare address. + ModulesAttached uint64 + // ModulesBare counts frames that blazesym could not name and for which + // no mapping was found either - no ModuleIndex configured, the process + // already gone, or a PC genuinely outside every file-backed executable + // range. These stay bare hex, and they are deliberately counted apart + // from ModulesAttached: recovering the module is a real improvement and + // a run where it never happens must not look like one where it always + // did. + ModulesBare uint64 } // Stats returns the current process-side symbolization counters. func (s *LocalSymbolizer) Stats() LocalStats { - return LocalStats{RawAddrBatches: s.stats.rawAddrBatches.Load()} + return LocalStats{ + RawAddrBatches: s.stats.rawAddrBatches.Load(), + ModulesAttached: s.stats.modulesAttached.Load(), + ModulesBare: s.stats.modulesBare.Load(), + } } // checkMapFilesAccess reports whether this process may follow a @@ -132,7 +173,7 @@ func checkMapFilesAccess() error { // profile.pb.gz full of hex. A profiler whose every user frame is "0x7f..." // is not degraded, it is useless, and this codebase refuses everywhere else // rather than hand back output that looks like a result. -func NewLocalSymbolizer() (*LocalSymbolizer, error) { +func NewLocalSymbolizer(opts ...LocalOption) (*LocalSymbolizer, error) { if err := checkMapFilesAccess(); err != nil { return nil, err } @@ -143,7 +184,11 @@ func NewLocalSymbolizer() (*LocalSymbolizer, error) { if err != nil { return nil, err } - return &LocalSymbolizer{bz: bz}, nil + s := &LocalSymbolizer{bz: bz} + for _, opt := range opts { + opt(s) + } + return s, nil } // SymbolizeProcess returns one Frame per IP. blazesym's Inlined chain is @@ -173,7 +218,7 @@ func (s *LocalSymbolizer) SymbolizeProcess(pid uint32, ips []uint64) ([]Frame, e ) if err != nil { s.stats.rawAddrBatches.Add(1) - return rawUserAddrFrames(ips), nil + return s.withModules(pid, rawUserAddrFrames(ips)), nil } out := make([]Frame, 0, len(syms)) for i, sym := range syms { @@ -183,7 +228,22 @@ func (s *LocalSymbolizer) SymbolizeProcess(pid uint32, ips []uint64) ([]Frame, e } out = append(out, fromBlazesymSym(sym, addr)) } - return out, nil + return s.withModules(pid, out), nil +} + +// withModules names the module behind every frame blazesym left unresolved, +// and counts what it could and could not place. Called on both return paths +// of SymbolizeProcess: the whole-batch failure produces exactly the frames +// that need this most. +func (s *LocalSymbolizer) withModules(pid uint32, frames []Frame) []Frame { + attached, bare := attachModules(s.modules, pid, frames) + if attached > 0 { + s.stats.modulesAttached.Add(uint64(attached)) + } + if bare > 0 { + s.stats.modulesBare.Add(uint64(bare)) + } + return frames } // Close releases the underlying blazesym Symbolizer. Idempotent. diff --git a/symbolize/module.go b/symbolize/module.go new file mode 100644 index 00000000..a92558ae --- /dev/null +++ b/symbolize/module.go @@ -0,0 +1,61 @@ +package symbolize + +import "github.com/dpsoft/perf-agent/unwind/procmap" + +// ModuleIndex answers "which file is this address in, in this process". +// *procmap.Resolver satisfies it; the interface exists so symbolize does not +// have to own a maps cache of its own, and so tests can supply a fixture. +// +// A ModuleIndex must return ok=false rather than a nearby or stale mapping: +// everything downstream treats a hit as a fact about the profile. +type ModuleIndex interface { + Lookup(pid uint32, addr uint64) (procmap.Mapping, bool) +} + +// attachModules fills Module/BuildID/MapStart/MapLimit/MapOff on the frames a +// symbolizer could not name, so that an unresolved frame can at least say +// which library it is in. +// +// It runs only over frames with Reason != FailureNone and Module == "", so a +// resolved frame is never touched and a symbolizer that already knew the +// module keeps its own answer. That also keeps the cost proportional to the +// failures, not to the stack: a fully symbolized stack does no lookups. +// +// The lookup must happen while the target process is alive - /proc//maps +// vanishes the moment it exits - which is why this lives in the symbolizer +// and not in the pprof builder. The GPU tools build their profile after the +// workload has exited; by then there is nothing left to ask. +// +// Returns how many frames gained a module and how many were left bare. Both +// numbers matter: "unresolved" and "unresolved, and we cannot even say where" +// are different failures and must not be reported as one. +func attachModules(idx ModuleIndex, pid uint32, frames []Frame) (attached, bare int) { + if idx == nil { + for i := range frames { + if frames[i].Reason != FailureNone && frames[i].Module == "" { + bare++ + } + } + return 0, bare + } + for i := range frames { + f := &frames[i] + if f.Reason == FailureNone || f.Module != "" { + continue + } + m, ok := idx.Lookup(pid, f.Address) + if !ok || m.Path == "" { + bare++ + continue + } + f.Module = m.Path + f.MapStart = m.Start + f.MapLimit = m.Limit + f.MapOff = m.Offset + if f.BuildID == "" { + f.BuildID = m.BuildID + } + attached++ + } + return attached, bare +} diff --git a/symbolize/module_test.go b/symbolize/module_test.go new file mode 100644 index 00000000..985a9a02 --- /dev/null +++ b/symbolize/module_test.go @@ -0,0 +1,191 @@ +package symbolize + +import ( + "testing" + + "github.com/dpsoft/perf-agent/unwind/procmap" +) + +// fakeIndex is a fixture ModuleIndex: a flat list of ranges, no /proc. +type fakeIndex struct { + pid uint32 + maps []procmap.Mapping + hits int +} + +func (f *fakeIndex) Lookup(pid uint32, addr uint64) (procmap.Mapping, bool) { + f.hits++ + if pid != f.pid { + return procmap.Mapping{}, false + } + for _, m := range f.maps { + if addr >= m.Start && addr < m.Limit { + return m, true + } + } + return procmap.Mapping{}, false +} + +func libcuda() procmap.Mapping { + return procmap.Mapping{ + Path: "/usr/lib/x86_64-linux-gnu/libcuda.so.1", + Start: 0x7f2c94700000, + Limit: 0x7f2c96000000, + Offset: 0x1000, + BuildID: "cafe", + IsExec: true, + } +} + +func TestAttachModules_NamesTheModuleOfAnUnresolvedFrame(t *testing.T) { + idx := &fakeIndex{pid: 77, maps: []procmap.Mapping{libcuda()}} + frames := []Frame{ + {Address: 0x7f2c958b71c6, Name: "0x7f2c958b71c6", Reason: FailureMissingSymbols}, + } + + attached, bare := attachModules(idx, 77, frames) + if attached != 1 || bare != 0 { + t.Fatalf("attached=%d bare=%d, want 1/0", attached, bare) + } + f := frames[0] + if f.Module != "/usr/lib/x86_64-linux-gnu/libcuda.so.1" { + t.Errorf("Module = %q", f.Module) + } + if f.BuildID != "cafe" { + t.Errorf("BuildID = %q", f.BuildID) + } + off, ok := f.ModuleOffset() + if !ok { + t.Fatal("ModuleOffset() not ok") + } + // 0x7f2c958b71c6 - 0x7f2c94700000 + 0x1000 + if want := uint64(0x11b81c6); off != want { + t.Errorf("ModuleOffset() = %#x, want %#x", off, want) + } +} + +func TestAttachModules_LeavesResolvedFramesAlone(t *testing.T) { + idx := &fakeIndex{pid: 77, maps: []procmap.Mapping{libcuda()}} + frames := []Frame{ + {Address: 0x7f2c958b71c6, Name: "cuLaunchKernel", Reason: FailureNone}, + } + + attached, bare := attachModules(idx, 77, frames) + if attached != 0 || bare != 0 { + t.Fatalf("attached=%d bare=%d, want 0/0", attached, bare) + } + if idx.hits != 0 { + t.Errorf("looked up %d addresses for a fully resolved stack; want 0", idx.hits) + } + if frames[0].Name != "cuLaunchKernel" || frames[0].Module != "" { + t.Errorf("resolved frame mutated: %+v", frames[0]) + } +} + +func TestAttachModules_KeepsAModuleTheSymbolizerAlreadyKnew(t *testing.T) { + idx := &fakeIndex{pid: 77, maps: []procmap.Mapping{libcuda()}} + frames := []Frame{{ + Address: 0x7f2c958b71c6, + Name: "0x7f2c958b71c6", + Module: "/somewhere/else.so", + Reason: FailureMissingSymbols, + }} + + attachModules(idx, 77, frames) + if frames[0].Module != "/somewhere/else.so" { + t.Errorf("overwrote the symbolizer's own module: %q", frames[0].Module) + } +} + +// An address in no mapping must stay a bare address. This is the case the +// whole design has to keep visible: naming a module we do not know would be +// worse than the hex. +func TestAttachModules_NoMappingStaysBare(t *testing.T) { + idx := &fakeIndex{pid: 77, maps: []procmap.Mapping{libcuda()}} + frames := []Frame{ + {Address: 0x400000, Name: "0x400000", Reason: FailureMissingSymbols}, + } + + attached, bare := attachModules(idx, 77, frames) + if attached != 0 || bare != 1 { + t.Fatalf("attached=%d bare=%d, want 0/1", attached, bare) + } + f := frames[0] + if f.Module != "" || f.MapStart != 0 || f.MapLimit != 0 || f.MapOff != 0 { + t.Errorf("invented a mapping for an unmapped address: %+v", f) + } + if _, ok := f.ModuleOffset(); ok { + t.Error("ModuleOffset() ok for a frame with no mapping") + } +} + +// No index configured is not the same as an index that found nothing, but +// both must leave the frame bare and both must be counted. +func TestAttachModules_NilIndexCountsBare(t *testing.T) { + frames := []Frame{ + {Address: 0x400000, Name: "0x400000", Reason: FailureMissingSymbols}, + {Address: 0x401000, Name: "main", Reason: FailureNone}, + } + attached, bare := attachModules(nil, 77, frames) + if attached != 0 || bare != 1 { + t.Fatalf("attached=%d bare=%d, want 0/1", attached, bare) + } + if frames[0].Module != "" { + t.Errorf("nil index produced a module: %q", frames[0].Module) + } +} + +func TestModuleOffset_RejectsAddressOutsideItsOwnMapping(t *testing.T) { + f := Frame{ + Address: 0x10, + Module: "/lib/x.so", + MapStart: 0x1000, + MapLimit: 0x2000, + } + if _, ok := f.ModuleOffset(); ok { + t.Error("ModuleOffset() ok for an address below its mapping") + } +} + +func TestToProfFrames_CarriesModuleAndUnresolvedBit(t *testing.T) { + frames := []Frame{ + { + Address: 0x7f2c958b71c6, Name: "0x7f2c958b71c6", Reason: FailureMissingSymbols, + Module: "/usr/lib/libcuda.so.1", BuildID: "cafe", + MapStart: 0x7f2c94700000, MapLimit: 0x7f2c96000000, MapOff: 0x1000, + }, + {Address: 0x401000, Name: "main", Reason: FailureNone}, + } + out := ToProfFrames(frames) + if len(out) != 2 { + t.Fatalf("got %d frames", len(out)) + } + if !out[0].Unresolved { + t.Error("unresolved frame did not carry Unresolved") + } + if out[0].MapStart != 0x7f2c94700000 || out[0].MapLimit != 0x7f2c96000000 || out[0].MapOff != 0x1000 { + t.Errorf("mapping not carried: %+v", out[0]) + } + if out[0].BuildID != "cafe" { + t.Errorf("build id not carried: %q", out[0].BuildID) + } + if out[1].Unresolved { + t.Error("resolved frame marked Unresolved") + } +} + +// An inline chain only exists where resolution succeeded, so no frame in one +// may be marked unresolved - that would drag real function names through the +// module+offset rename. +func TestToProfFrames_InlinedFramesAreNeverUnresolved(t *testing.T) { + frames := []Frame{{ + Address: 0x401000, Name: "outer", Reason: FailureNone, + Module: "/bin/app", + Inlined: []Frame{{Name: "inner"}}, + }} + for _, f := range ToProfFrames(frames) { + if f.Unresolved { + t.Errorf("frame %q marked Unresolved", f.Name) + } + } +} diff --git a/symbolize/symbolize.go b/symbolize/symbolize.go index a1b07c52..4099b781 100644 --- a/symbolize/symbolize.go +++ b/symbolize/symbolize.go @@ -24,6 +24,40 @@ type Frame struct { Offset uint64 Inlined []Frame Reason FailureReason + + // MapStart, MapLimit and MapOff describe the mapping Address fell in, + // when one is known: the mapping's start and (exclusive) end virtual + // addresses, and the file offset the mapping begins at. + // + // They exist because blazesym reports NOTHING but a failure reason for + // an address it cannot name - not even which file the address was in + // (capi/src/symbolize.rs zeroes the whole blaze_sym and sets only + // `reason`). For a stripped vendor library such as libcuda.so.1 the + // symbol is genuinely unrecoverable, but the module is not, and it is + // most of the diagnostic value: "seven frames deep inside libcuda" + // is an answer, "0x7f2c945b2c2b" is not. + // + // A Symbolizer fills these in from /proc//maps, via a ModuleIndex, + // only for frames it failed to name; see attachModules. All three are + // zero when no mapping is known, and that case must stay visibly + // distinct downstream rather than being filled with a plausible guess. + MapStart uint64 + MapLimit uint64 + MapOff uint64 +} + +// ModuleOffset returns Address relative to the start of the file backing its +// mapping - the number that is stable across runs, unlike the ASLR'd +// Address, and that addr2line/objdump/nvdisasm accept. ok is false when no +// mapping is known for this frame. +func (f Frame) ModuleOffset() (off uint64, ok bool) { + if f.Module == "" || f.MapLimit <= f.MapStart { + return 0, false + } + if f.Address < f.MapStart || f.Address >= f.MapLimit { + return 0, false + } + return f.Address - f.MapStart + f.MapOff, true } // FailureReason describes why a Frame's Name is empty. diff --git a/symbolize/toprof.go b/symbolize/toprof.go index 37441dea..4a460b97 100644 --- a/symbolize/toprof.go +++ b/symbolize/toprof.go @@ -30,6 +30,21 @@ func ToProfFrames(frames []Frame) []pprof.Frame { File: f.File, Line: uint32(f.Line), Address: f.Address, + // Carried, not re-derived. The mapping was read while the + // target process was alive; the pprof builder runs later - for + // the GPU tools, after the workload has exited - and its own + // Resolver would find nothing left in /proc to look at. + BuildID: f.BuildID, + MapStart: f.MapStart, + MapLimit: f.MapLimit, + MapOff: f.MapOff, + // The one bit pprof.Frame cannot recover for itself. After this + // conversion an address-shaped Name is indistinguishable from a + // function genuinely called "0x4017c2", so the failure has to be + // carried explicitly rather than sniffed out of the string. + // Inlined frames are never marked: an inline chain only exists + // where resolution succeeded. + Unresolved: f.Reason != FailureNone, }) } return out diff --git a/symbolize/user_fallback.go b/symbolize/user_fallback.go index 49f8ca95..3c032096 100644 --- a/symbolize/user_fallback.go +++ b/symbolize/user_fallback.go @@ -15,10 +15,11 @@ import "fmt" // flamegraph of perf-agent #1 was 100% [unknown] — the discovery // case that motivated this fix. // -// Module is left empty: the pprof builder routes user-side Locations -// through their /proc//maps-derived mapping (Bug 3 fix), so the -// mapping's filename still appears next to the raw-hex name in -// downstream tooling. +// Module is left empty here on purpose. LocalSymbolizer.withModules fills +// it in afterwards from /proc//maps when a ModuleIndex is configured, +// which is the only place that can tell an address with a mapping from one +// without. Frames that come back from here with Module still empty are +// exactly the ones nothing is known about. func rawUserAddrFrames(ips []uint64) []Frame { out := make([]Frame, len(ips)) for i, ip := range ips {