Repository navigation
Fix aarch64 stub unwinding in I2C/C2I adapters; fix StubUnwindCpuTest slowness on debug builds - #842
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…wness The stub analyzer truncated everything after the first mid-stub br to SU_UNSUPPORTED. The I2C/C2I adapters blob packs the i2c entry (ending in br) and the c2i entries back to back, so every c2i PC fell back to the legacy heuristics and failed with break_unwind_stub_failed. - Restart from the entry state after a mid-stub br; branch validation still degrades targets reached in a different state. - Model AdvSIMD structure loads/stores with sp writeback (push_CPU_state / pop_CPU_state), previously treated as neutral: exact for the multiple-structure immediate form and for the register form with a constant from mov (ORR/MOVZ/MOVN); otherwise sp becomes unknown. - Keep the frame when x29 is reloaded from its save slot; mov sp, x29 makes sp known again. - Drop the constant/save-slot facts at in-stub branch targets. - Logical immediates writing sp make sp unknown; move-wide/logical writes to x29 drop the frame. - Regression fixture from a real JDK 21 aarch64 adapter blob. StubUnwindCpuTest: the -O0 debug build needs ~160us per sample for the ~150-frame test stack, above the 100us wall interval, so attempts took minutes. Use wall=1ms for debug builds as for ASan, and lower @RetryTest from 10 to 2. Update the handover doc with the resolution. Environment: Datadog workspace Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
CI Test ResultsRun: #37774750489 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-10-08 12:28:46 UTC |
Contributor
Sampling at 1ms with the original 40 rounds was too sparse: a musl amd64 JDK 21 debug job saw no itable-stub sample in either attempt. When ASan or the debug build samples at 1ms, run 10x the workload rounds so the number of samples per stub matches the 100us configuration. Environment: Datadog workspace Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
rkennke
changed the title
Like StubUnwindCpuTest, the test sampled at wall=100us on the -O0 debug build, where walking the deep Gradle test-executor stack costs more than the interval. On JDK 25 aarch64 it took 138s locally (with or without the stub analyzer change) and stalled CI jobs until the 3h timeout. The test only asserts that samples exist, so sample at 1ms there as for ASan. Environment: Datadog workspace Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
rkennke
marked this pull request as ready for review
October 7, 2026 19:19
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96569af608
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Self-review follow-ups for the aarch64 stub analyzer:
- Code after a mid-stub br that is an ADR target ('adr lr, L; br xN; L:')
is reached through a register, not as a fresh entry: degrade it instead
of applying the restarted entry state. A stub with an ADRP into its own
pages keeps the old cut after br, since the built address is unknown.
- Writes to x29/x30 from instruction classes without a dedicated decoder
(unscaled, register-offset, literal, exclusive and atomic loads,
data-processing register, bitfield/extract, base writeback) now drop
the frame or the lr rule instead of being treated as neutral. The
mov sp, x29 recovery relies on fp tracking being sound.
- Drop the x29 save-slot fact on any load/store that is not an exact
sp-relative access, and when an sp-derived pointer reaches a GPR.
- With transitions frozen (>64 branches), only a change of the unwind
state truncates; changes of the auxiliary facts alone no longer do.
- Correct the stale comment claiming logical forms never write sp.
StubUnwindCpuTest: run 10x the rounds only on the debug build; ASan
already sampled at 1ms and keeps its workload. Share the debug-build
check as AbstractProfilerTest.isDebugBuild().
Environment: Datadog workspace
Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
An established fp points at a frame record, whose saved x30 is always at
fp + 8. The SU_FP_FRAME rule instead used the last tracked x30 spill, so
after a nested spill inside the frame was popped ('str x30, [sp, #-16]!'
... 'ldr x30, [sp], #16', or the pair form with an x29 reload) the rule
read the return address from below the live sp, where a signal may have
overwritten it. Always use the frame-record slot.
Add tests for the nested spill and for an STTR overwriting the x29 save
slot (already covered by dropping the slot on unmodeled accesses), and the
Datadog copyright header to the handover doc.
Environment: Datadog workspace
Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
kaahos
reviewed
Oct 8, 2026
kaahos
left a comment
kaahos
left a comment
Contributor
There was a problem hiding this comment.
thanks for the work! I left some comments. My main question is a design decision: whether the restart after br should apply to all stub blobs or only the adapters blob. Let me know what you think.
Only the adapters blob is known to pack several entry points behind a
mid-stub br. In any other blob the code after a br may be a jump-table
entry or a return point reached through a register, whose state the scan
cannot know, so it keeps falling back as before. analyzeStubUnwind() takes
a multi_entry flag, set by the DynamicCodeGenerated callback for the
"I2C/C2I adapters" blob name only.
Also, from review:
- Drop the x29 save-slot fact once the slot lies below the live sp (or sp
is unknown): a popped slot is unowned memory a signal may overwrite.
- Count the GPR base writeback of SIMD&FP loads/stores ('ldr q0, [x29],
#16') as a write, so it drops the frame.
Environment: Datadog workspace
Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
…ents "ARM ARM" reads like a typo to readers who do not know the abbreviation. Environment: Datadog workspace Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
jbachorik
approved these changes
Oct 8, 2026
jbachorik
left a comment
jbachorik
left a comment
Collaborator
There was a problem hiding this comment.
An impressive work! A few minor nits only from my side, I do not read the instructions for poetry, so the chances of me finding anything fundamental are pretty low ...
jbachorik
reviewed
Oct 8, 2026
An in-stub ADR target can be entered through a register jump in a state
the fall-through scan cannot know ('adr x9, L; ...; br x9', the computed
jumps in the *_fill and large_arrays_hashcode_* stubs). Previously such
targets only degraded after a restart; before the first mid-stub br they
kept the fall-through rule. Now every ADR target degrades unless it is
the return point of an in-stub call (set_last_Java_frame's last_Java_pc,
reached by the callee's ret in the fall-through state) or the ADR's own
address. A probe of every stub on JDK 21 and 25 found no other pre-br
ADR targets in analyzed stubs except SHA-1 constant data, so runtime-call
stubs keep their rules.
Drop the redundant 'combined == 0' guard in decodeLogicalImm64 and spell
out the arithmetic: len < 1 covers both unallocated inputs.
Remove the investigation handover doc; its open follow-ups are tracked
in Jira (PROF-16214, PROF-16215, PROF-16216).
Environment: Datadog workspace
Co-Authored-By: Claude Code Claude Opus 5.5 <noreply@anthropic.com>
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
3 participants
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 does this PR do?:
Fixes the two
StubUnwindCpuTestproblems on glibc aarch64 debug CI jobs: real but intermittentbreak_unwind_stub_failedframes hidden by@RetryTest(10), and attempts slow enough to push whole jobs into the 3h timeout.break_unwind_stub_failed). All of them came from theI2C/C2I adaptersblob. That blob packs the i2c entry, which ends inbr, and the c2i entries back to back.analyzeStubUnwind()truncated everything after the first mid-stubbrtoSU_UNSUPPORTED, so every c2i PC fell back to the legacy heuristics, which have no rule for adapters. Changes inhotspot/stubUnwindInfo.cpp:I2C/C2I adaptersblob only, restart from the entry state after a mid-stubbr. Existing branch validation still degrades targets that a branch reaches in a different state. Other blobs keep falling back after abr, since the code there may be a jump-table entry or a return point reached through a register.push_CPU_state/pop_CPU_state). These were treated as neutral, which was a latent soundness bug. The multiple-structure immediate form is now exact, and so is the register form when the offset register holds a constant frommov(ORR/MOVZ/MOVN). Otherwise sp becomes unknown.mov sp, x29makes sp known again.and sp, x8, #-16) make sp unknown. Move-wide and logical writes to x29 drop the frame.-O0 -DDEBUGbuild costs about 160 µs per sample, above the 100 µs wall interval, so the registered thread spends about 97% of its time in the signal handler (unwinding_ticks_async). The cost behaves like a cliff (extra depth 0: 243 ms, 50: 360 ms, 150: 35.6 s). The release build at depth 150 runs in 255 ms, so production profiling is not affected. Debug builds now usewall=1ms, as ASan already did, with 10x the workload rounds so that the samples per stub stay the same.@RetryTestdrops from 10 to 2.BoundMethodHandleProfilerTesthit the same cliff (138 s locally on JDK 25 with or without the analyzer change, and it stalled CI to the 3h timeout); it now samples at 1 ms on debug builds as well (4.4 s).Motivation:
Several PRs hit the 3h aarch64 job timeout, and
@RetryTest(10)was hiding real unwind failures.Additional Notes:
brrestart (adapters blob only): code after abrreached through an address loaded from memory, with a frame established, would get the entry rule. The adapters'brs are tail jumps that leave the blob.brdegrades; a stub with an ADRP into its own pages keeps the old cut; undecoded x29/x30 writes drop the frame or the lr rule; the x29 save slot is dropped on unmodeled memory accesses; auxiliary facts no longer truncate frozen tables. Each has a gtest that fails without it.b/retas well. That linear behavior predates this PR; changing it would alter the rules for every stub.last_Java_pcreturn points, self-references and SHA-1 constant data among pre-brADR targets).break_unwind_stub_failedin the interpreter'snative signature handlersblob (about 1 in 60k-850k samples, same before and after this PR).MegamorphicCallTest).ReferenceChainsLifecycleTestTSan flake seen infuzz-build-and-soak(unrelated to this PR).How to test the change?:
stubUnwindInfo_ut.cpp(87 pass). They include a regression fixture made from a real JDK 21 aarch64I2C/C2I adaptersblob, which fails without the fix.vmandvmx. Before the fix: 11 in 212k.StubUnwindCpuTestwith the final settings: 5/5 forced reruns pass without retries, 2.7 s (vm) and 1.9 s (vmx) per attempt, about 1,250 precomputed-info hits each.:ddprof-test:testDebugon JDK 21 (226 tests, 0 failures): all pass.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code