fix(test): stop -O3 tail-calling away the return-address trampolines - #819
Merged
Merged
Conversation
Test3_UnwindRowSelectedAtReturnAddress fails in the release configuration on
x86_64. prof_ra_cfi_trampoline exists so that the caller of the boundary frame
is a specific, symbolizable function rather than the gtest body, but at -O3 the
compiler turns its call into a tail jump:
prof_ra_cfi_trampoline:
movl $0x1,-0x4(%rsp)
mov -0x4(%rsp),%eax
jmp prof_ra_cfi_caller
With the jmp the trampoline never establishes a frame, so at runtime the real
caller of prof_ra_cfi_caller is TestBody. The walker reports that correctly and
the fixture's expectation is what is wrong -- chain[kBoundaryIndex] still
resolves to prof_ra_cfi_caller, so the boundary traversal the test was written
to cover is unaffected.
The existing guard did not prevent this. noinline stops the trampoline being
inlined into its caller, and the volatile store completed before the call, so
neither blocked the tail jump. Moving the volatile store after the call makes
the call non-tail by construction, which is compiler-agnostic -- unlike
__attribute__((disable_tail_calls)), which is clang-only while
ConfigurationPresets still carries a gcc path.
prof_ra_plt_trampoline had the same defect; its test is currently gated off on
x86_64 by the alignment check, so it never surfaced. Both are fixed.
Release codegen after the change, for both trampolines:
push %rbp / mov %rsp,%rbp / ... / call <callee> / movl $0x2 / ret
Verified on linux-x64 in all four configurations -- release, debug, asan and
tsan -- each 15 passed, 3 arch-gated skips, 0 failures.
The failure dates to #786, which added the fixture; no later commit is
involved. It stayed invisible because PR CI builds its matrix from labels
(ci.yml, compute-configurations): debug always, release only behind a
test:release label. #786 carried only sphinx:critical, so every cell in its
run was debug, where the tail call does not occur. The asan and tsan presets
both pass -fno-optimize-sibling-calls, so they cannot exhibit it either.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
Contributor
Contributor
CI Test ResultsRun: #36051969614 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Failed Jobs
Summary: Total: 64 | Passed: 63 | Failed: 1 Updated: 2026-09-24 20:20:13 UTC |
The volatile store before the call never contributed anything. Both compilers emit it and then tear the frame down anyway -- that is the original defect, not a weaker version of the fix. Only the store after the call leaves work to do on return, which is what stops the call being turned into a tail jump. Keeping both halves left the ineffective one sitting directly under a comment claiming it worked, which is how the first version came to be believed. Codegen checked at -O3 with clang and gcc on x86_64 and with clang on aarch64: a store only before the call tail-jumps, a store only after does not, and the two together are no better than after alone. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
ReturnAddressAttributionTest.Test3_UnwindRowSelectedAtReturnAddressfails in the release configuration on x86_64 (returnAddressAttribution_ut.cpp:929).prof_ra_cfi_trampolineexists so the caller of the boundary frame is a specific, symbolizable function rather than the gtest body. At-O3the compiler turns its call into a tail jump:With the
jmpthe trampoline never establishes a frame, so at runtime the real caller ofprof_ra_cfi_callerisTestBody. The walker reports that correctly — this is a fixture bug, not a walker bug.chain[kBoundaryIndex]still resolves toprof_ra_cfi_caller, so the boundary traversal the test was written to cover is unaffected; only the frame-identity assertion at[2]encoded a call-shape assumption that-O3invalidates.Why the existing guard didn't hold
The fixture carried
noinlineplus avolatile int guard, commented "volatile to defeat tail-call/inlining folding this frame away." Neither does:noinlinestops the trampoline being inlined into its caller, and the volatile store completed before the call.Moving the volatile store after the call makes the call non-tail by construction. That is compiler-agnostic, unlike
__attribute__((disable_tail_calls)), which is clang-only whileConfigurationPresetsstill carries a gcc path.prof_ra_plt_trampolinehad the identical defect; its test (Test4) is currently gated off on x86_64 by the alignment check, so it never surfaced. Both are fixed.Verification
Release codegen after the change, for both trampolines:
Run on linux-x64 in all four configurations — release, debug, asan, tsan — each 15 passed, 3 arch-gated skips, 0 failures.
spotlessCheckgreen.Not verified from this box: aarch64 release, which is presumably the same failure (Test3 runs there and
-O3tail-calls there too).test:releaseis applied to this PR so the release matrix actually exercises it.Why CI stayed green on #786
The failure dates to #786, which added the fixture — no later commit is involved; it reproduces at
0d0653365itself.PR CI builds its config matrix from PR labels (
ci.yml,compute-configurations):debugalways,releaseonly behind atest:releaselabel. #786 carried onlysphinx:critical, so every cell in its run read(<jdk>, debug, regular)— no release cell existed. gtests did run there, in debug, and passed legitimately. The asan and tsan presets both pass-fno-optimize-sibling-callsand no-O, so they cannot exhibit it either.-O3is the only configuration that produces the tail call, and it is opt-in.Not quarantine-related: no matching entry in
ddprof-test/quarantine.txt.Worth deciding separately (not changed here): release gtests being label-gated means release-only breakage stays invisible on a normal PR. Also
nightly.yml:18's comment that gtests "run on every PR via native-sanitizer-tests in ci.yml" is stale —ci.ymlhas no sanitizer job.🤖 Generated with Claude Code