Repository navigation
runtime: preserve Windows caller identity after LTO inlining - #2773
Conversation
|
@fennoai review Please review this standalone Windows LTO runtime fix, extracted from #2748. Focus on correctness of PE entry normalization/deduplication on amd64 and arm64, leaf functions without unwind metadata, and regression coverage. The new cross-package scalar fixture fails on main |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Windows LTO funcinfo dedup fix
Targeted, well-reasoned fix for the Windows LTO/COFF case where an inlinee's entry-site anchor is carried into its caller and arbitrary duplicate removal would strip the caller's identity in CallersFrames. The core logic (normalize to the native PE entry, keep the first record per physical function, then feed the min-entry publication) is correct, and the explanatory comment at the dedup site matches the implementation. The unsafe PE-record dereference is correctly nil-guarded and reuses the pre-established runtimeFuncPCMayUseEntrySlack pattern — no memory-safety concerns.
Findings below are about maintainability and consistency; none are correctness blockers.
Security: No issues found.
| continue | ||
| } | ||
| pc := rtdebug.FunctionPC(unsafe.Pointer(site.pc)) | ||
| if GOOS == "windows" && (GOARCH == "amd64" || GOARCH == "arm64") { |
There was a problem hiding this comment.
[P2] Inline GOOS/GOARCH guard duplicates the build-tag platform split
This GOOS == "windows" && (GOARCH == "amd64" || GOARCH == "arm64") guard re-states the exact platform set already encoded by the build tags on funcinfo_entry_slack_windows_seh_llgo.go (//go:build windows && (amd64 || arm64)). runtimeFuncPCSiteEntry is already specialized per-platform (it returns pc unchanged everywhere except windows amd64/arm64), so the only thing this inline condition actually gates is the nativeEntries dedup map.
The amd64/arm64 knowledge now lives in two places that must stay in sync — adding a future Windows arch with PE unwind means remembering to update both the build tag and this GOARCH list. Consider pushing the dedup decision behind a single build-tagged predicate (e.g. a per-file const runtimeFuncPCDedupBySiteEntry paired with runtimeFuncPCSiteEntry), matching how runtimeFuncPCEntrySlack and runtimeFuncPCMayUseEntrySlack are already defined per file. That also keeps symtab.go platform-agnostic, consistent with the rest of the file.
| *value = arith.AddSeven(*value) | ||
| return runtime.Callers(1, pcs[:]) | ||
| } | ||
|
|
There was a problem hiding this comment.
[P3] Test exercises the real fix path only on Windows amd64/arm64
Good, deterministic test: empty expect.txt correctly means "empty stdout, clean exit; any panic fails," matching the harness. Two transparency notes:
- The dedup code path (
symtab.goguard body) only runs on windows amd64/arm64. On Linux/macOS CI,capturealready keeps its identity, sofound == 1passes without touching the fix — this is a fine regression guard but the fix itself is only validated when the suite runs on windows amd64/arm64. - The trigger depends on LTO actually inlining
arith.AddSevenintocapture. There's no assertion the inline copy was created, so a future optimizer change that stops inlining would let the test pass silently while no longer exercising its named scenario. Thevalue != 12check confirms the computation ran, not that it was inlined intocapture.
Relatedly, the comment "Cross-package LTO copies AddSeven's entry record into this function" reads as guaranteed behavior; "is expected to copy" would set accurate expectations. Consider a brief note acknowledging the LTO-inlining dependency.
| // loses the caller's identity. Keep the first record per physical | ||
| // PE function before sorting, and publish its true entry. | ||
| pc = runtimeFuncPCSiteEntry(pc) | ||
| if nativeEntries == nil { |
There was a problem hiding this comment.
[P2] nativeEntries map conflicts with loop's no-alloc discipline
This loop was deliberately rewritten to avoid Go-runtime machinery that keeps stack temporaries alive — see the comments at lines 876-878 ("Keep potentially large materialization loops free of append and checked slice indexing ... can exhaust a worker thread's fixed-size stack") and 969-971 (composite assignment "used to lower through a fresh stack temporary on every loop iteration, exhausting a fixed WebAssembly goroutine stack"). The frames array is pre-sized and written via raw unsafe.Add stores specifically to avoid append.
Introducing make(map[uintptr]bool) plus per-iteration mapassign (which can trigger incremental rehash/growth and heap allocation) reintroduces exactly the kind of per-iteration runtime calls this code works to avoid. The specific wasm failure mode doesn't directly apply here (this path is windows amd64/arm64 only) and the map is bounded by distinct PE entries, so this is not a bug — but it is an inconsistency worth a short justifying comment explaining why a map is acceptable here when append/composite literals were banned a few lines above.
Also note the records already arrive in COFF order and an existing sortRuntimeFuncPCFrames + uniqueRuntimeFuncPCFrames pass runs right after. The pcline path solves the identical "equal PC, keep emission order" problem with a sequence tiebreak field and an order-preserving sort (no map) — see runtimePCLineFrameLess. If runtimeFuncPCFrame gained a sequence/order field, this pre-sort dedup map could likely be eliminated entirely.
| if nativeEntries == nil { | ||
| nativeEntries = make(map[uintptr]bool) | ||
| } | ||
| if nativeEntries[pc] { |
There was a problem hiding this comment.
[P3] Dedup keys on un-normalized anchor for leaf functions
For a leaf function (no RUNTIME_FUNCTION record), runtimeFuncPCSiteEntry returns the raw pc unchanged, so the dedup key here is the un-normalized anchor rather than a true PE function begin. For non-leaf functions pc has been normalized, so "same PC => same physical function" holds and the dedup is sound; the leaf case relies on the pre-existing COFF-fold property. The slack file documents this ("A leaf without unwind metadata keeps its anchor"), but the dedup site itself does not restate it. A one-line comment here would stop a future reader from assuming the key is always a normalized function begin.
| // and must not resolve to the next densely packed function's anchor. | ||
| const runtimeFuncPCEntrySlack = 64 | ||
|
|
||
| // runtimeFuncPCSiteEntry groups LTO inline copies by the real PE function. |
There was a problem hiding this comment.
[P3] Doc comment overstates what runtimeFuncPCSiteEntry does
runtimeFuncPCSiteEntry only normalizes a single pc to the PE function begin address — it performs no grouping or deduplication itself. The verbs "groups LTO inline copies" and "equal-PC copies can still be deduplicated" describe behavior that actually lives in the caller (appendRuntimeFuncInfoEntryFrames's nativeEntries map). Consider rewording to make the responsibility boundary clear, e.g. "returns the real PE function entry for a pc, which the caller uses to group/dedup LTO inline copies."
Review: Windows LTO funcinfo fix (focused on correctness)Reviewed the standalone fix with the lens you asked for: PE entry normalization/dedup on amd64 + arm64, leaf functions without unwind metadata, and regression coverage. No changes made to the branch. Verdict: the fix is correct for its stated purpose. The normalization + "keep first record per physical PE function" strategy correctly preserves the caller's identity in Correctness — confirmed sound1. PE entry normalization (amd64 + arm64). 2. Dedup + ordering. Records are iterated in section (COFF) order via linear 3. Interaction with the sort/unique passes. This dedup runs before 4. Skipped 5. Leaf functions without unwind metadata. For a leaf (nil lookup) Regression coverageThe fixture is a valid, deterministic guard (empty
Optional cleanup (non-correctness)
These four match the inline comments posted on the earlier |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
Windows LTO can copy an inlined callee's function-entry record into its caller. The runtime then sorts these anchors and may resolve the caller's PC to the inlinee's name. A cross-package integer helper followed by
runtime.Callersreproduces the missing caller frame on current main, without SIMD or FMV.Normalize entry anchors to their physical PE function using the existing Windows unwind lookup and retain the first serialized record for each entry before sorting. For leaf functions without unwind metadata, retain the anchor and deduplicate equal PCs in record order. The change applies to Windows amd64/arm64; other targets retain their existing behavior.
The new
cl/_testlto/caller_inline_entryfixture checks both the computation and the captured caller's identity/source location, and runs in the existing Full LTO suite.Validation:
686181ad6, Windows amd64-O2 -lto=full: new fixture fails withcapture frame lost its physical function identity(exit 2).-O2 -lto=thinand-O2 -lto=full: fixture passes.go test ./cl -run '^TestRunAndTestFromTestlto$/caller_inline_entry$' -count=1passes.TestSIMDFMVInlineTraceback; restoring them makes the same Windows ThinLTO/GODEBUG test pass.Windows execution above used MinGW cross compilation and Wine; native MSVC/MinGW CI remains to be confirmed.
Extracted from #2748 so this general Windows runtime fix can be reviewed and merged separately. #2748 will drop its overlapping runtime diff when rebased after this PR merges.