fix(java): avoid JDK 8 ARM Unsafe address miscompilation - #4049
Merged
Merged
Conversation
chaokunyang
requested review from
asf-gitbox-commits,
ayush00git,
pandalee99 and
urlyy
and removed request for
asf-gitbox-commits
September 15, 2026 04:29
ayush00git
approved these changes
Sep 15, 2026
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
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.
Why?
On Oracle JRE 8u503 ARM64, C2 can miscompile an indexed
Unsafeaccess when the address scale differs from the access width. For example, this reads eight bytes starting at a char index, so the byte offset must usecharIndex * 2:The captured machine code instead scales the index by eight (
x1is the array object andw2ischarIndex; the array base offset is 16 in this capture):For
charIndex = 4, this readschars[16..19]instead ofchars[4..7], which can cause the metadata-string failure in #4017. The same class of failure was reproduced for narrower reads and primitive stores. A singlegetIntis therefore not inherently a safe replacement.This PR works around the JDK compiler bug in Fory. The corresponding compiler-side mechanism is documented by JDK-8267689: its OpenJDK 8 patch checks that the address scale matches the memory access width before selecting a scaled addressing mode. This link identifies the matching failure mechanism; it does not establish which patches Oracle 8u503 contains.
What does this PR do?
JDK8_ARMgate and route indexed primitive reads/writes in buffers, string helpers, and generated codecs through the split access methods on JDK 8 ARM.MemoryBuffer,LittleEndian, andPlatformStringUtilsso hot callers reach the split methods without another forwarding layer. Other JVMs retain directUnsafeaccesses; generated codecs choose their path during code generation.How sharing the address avoids the bad instruction
For example, the long read uses the existing pair of native-order int loads:
Both loads consume the same indexed address expression. In the verified C2 output, their common address prefix is calculated once into a register, then the loads use fixed displacements. The captured string accessor becomes:
The
* 2now executes in the address calculation, while the memory instructions use constant offsets. This avoids the faulty indexed-load form that changed the scale to* 8. OpenJDK's shared-node matching and AArch64'sclone_shift_expressions = falseexplain this code shape.The split helper and its forwarding wrapper were both inlined in this capture. The workaround relies on the paired address uses, not on retaining a method-call boundary: forwarding the original single wide access still failed after inlining. Manually expanding the platform gate reduces inlining depth; the paired accesses are what avoid the observed miscompilation.
Validation
JDK8_ARMmade the same reproducer fail in all three runs. The inlined machine-code capture recorded zero mismatches in 20,000 reads.Related issues
Closes #4017
AI Contribution Checklist
yes/noyes, I included a completed AI Contribution Checklist in this PR description and the requiredAI Usage Disclosure.yes, my PR description includes the requiredai_reviewsummary and screenshot evidence or equivalent persisted links of the final clean AI review results from both fresh reviewers described inAI_POLICY.md, the Fory-guided reviewer and the independent general reviewer, on the current PR diff or current HEAD after the latest code changes.Does this PR introduce any user-facing change?
Benchmark
Apple M4 Pro, JMH 1.37,
UserTypeSerializeSuite.fory_serializeandUserTypeDeserializeSuite.fory_deserialize,objectType=STRUCT,bufferType=array,references=false,REGISTER_CLASS=true. The existing STRUCT factory produces 104 primitive fields. Both revisions round-trip it and produce identical 679-byte output.Compare baseline
677049dee9dcaebcfd528d4fb5db4fb879faf426with current75d9cc6bcf1c413ea62e206988e85249f0f4925e, using frozen core JARs and one common benchmark JAR. Each runtime/case has two adjacent baseline/current pairs, one process at a time:-f 1 -wi 3 -i 5 -t 1 -w 2s -r 2s,-Xms512m -Xmx512m, default JIT settings. Values are mean throughput in M ops/s.The active JDK 8 ARM workaround still has a material throughput cost. On JDK 11,
JDK8_ARM=falseand these two cases show no consistent regression: serialization pairs changed -1.09%/+1.52%, and deserialization pairs +4.73%/-4.50%. The two-fork means should not be interpreted as proof of exact zero overhead.