From 342fd05054fed8569dbc1e9c7ce19ac1198c1d37 Mon Sep 17 00:00:00 2001 From: diego Date: Tue, 25 Aug 2026 16:27:00 -0300 Subject: [PATCH] symbolize: unresolved frames name their module, not just an address A GPU profile's libcuda/libcupti internals rendered as bare hex (0x7f2c945b2c2b) and the whole .pb.gz carried one mapping, 0x0/0x0/0x0. Those frames were missing not merely a symbol but the module - and the module is most of the diagnostic value, since NVIDIA ships no symbols for the internals but "seven frames deep inside libcuda" is an answer. Two independent losses: blazesym discards the mapping for an address it cannot name. Its Symbolized::Unknown variant carries a Reason and nothing else, and the C API zeroes the whole blaze_sym before setting it (capi/src/ symbolize.rs:1105), so Frame.Module arrives empty for exactly the frames that need it. The GPU tools build their pprof with no procmap.Resolver, so addLocation's only route to a real mapping was closed and every frame fell through to the unconditional default. Wiring a Resolver into the builder cannot fix it: those tools build the profile after the workload has exited, and /proc//maps is gone by then. So the lookup happens in the symbolizer, where the process is still alive, and the frame carries the mapping forward: attachModules (only for frames with Reason != FailureNone) -> ToProfFrames carries Map*/BuildID + an explicit Unresolved bit -> pprof branch 3b builds a mapping from the frame -> addLocationByAddr renames to "+0x" The offset is module-relative, so it survives ASLR and can be fed to addr2line; it is the same number pprof already stores in Location.Address. The name goes in Function.Name because go tool pprof reads the same file and knows none of perf-agent's conventions. An address in no mapping stays a bare address: attachModules writes nothing without a Lookup hit, frameMapping refuses a range that does not contain its own address, and framename.Format refuses the [kernel] and [jit] sentinels. The two cases are counted apart - LocalStats.ModulesAttached/ModulesBare and gpuprobe.Stats.StackFramesModuleOnly. Two honesty features needed defending against the improvement: flamegraph.Classify checked module rules before isUnsymbolized, which was safe only while GPU profiles had no modules. With them, an unnamed libcuda frame would have been painted as ordinary vendor code and DomainUnsymbolized - labelled "vendor, no symbols" - would have become unreachable for vendor libraries. isUnsymbolized now sits above the module rules, below the kernel rule. foldedstacks.isAddressOnly now recognizes the module form too, so AddressOnlyFrames and the "N of M frame slots have no symbol" warning report the same gap before and after. A test asserts that equality. For the CPU profilers this is additive - the builder's own Resolver still runs first, so resolved frames and existing mapping attribution are unchanged - except that an unresolved frame no longer sets Mapping.HasFunctions, and gains a module name of its own. Details, including what still needs a live RTX 3090 run to confirm, in .superpowers/sdd/unresolved-frame-modules-report.md --- .../sdd/unresolved-frame-modules-report.md | 311 +++++++++++++++++ cmd/gpu-cuda-profile/main.go | 12 +- cmd/gpu-stub-profile/main.go | 12 +- gpuprobe/consumer.go | 27 +- internal/flamegraph/domain.go | 40 ++- internal/flamegraph/domain_test.go | 27 ++ internal/foldedstacks/fold.go | 48 ++- internal/foldedstacks/gpu_stack_e2e_test.go | 318 ++++++++++++++++++ internal/framename/framename.go | 134 ++++++++ internal/framename/framename_test.go | 117 +++++++ perfagent/agent.go | 14 +- pprof/pprof.go | 75 ++++- pprof/unresolved_test.go | 209 ++++++++++++ symbolize/debuginfod/symbolizer.go | 15 +- symbolize/local.go | 78 ++++- symbolize/module.go | 61 ++++ symbolize/module_test.go | 191 +++++++++++ symbolize/symbolize.go | 34 ++ symbolize/toprof.go | 15 + symbolize/user_fallback.go | 9 +- 20 files changed, 1692 insertions(+), 55 deletions(-) create mode 100644 .superpowers/sdd/unresolved-frame-modules-report.md create mode 100644 internal/foldedstacks/gpu_stack_e2e_test.go create mode 100644 internal/framename/framename.go create mode 100644 internal/framename/framename_test.go create mode 100644 pprof/unresolved_test.go create mode 100644 symbolize/module.go create mode 100644 symbolize/module_test.go 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 {