On Fri, 5 Jun 2026 07:25:12 GMT, Aleksey Shipilev <[email protected]> wrote:
>> Andrew Haley has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 128 commits: >> >> - Merge https://github.com/openjdk/jdk into JDK-8134940 >> - Review comments >> - Review comments >> - Merge branch 'JDK-8134940' of https://github.com/theRealAph/jdk into >> JDK-8134940 >> - Merge remote-tracking branch 'refs/remotes/origin/JDK-8134940' into >> JDK-8134940 >> - Review comments >> - Review comments >> - Remove misleading comment >> - Merge remote-tracking branch 'refs/remotes/origin/JDK-8134940' into >> JDK-8134940 >> - Add macros >> - ... and 118 more: https://git.openjdk.org/jdk/compare/ca52afa3...f9440fbb > > src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 2253: > >> 2251: // Finally, update the counter >> 2252: bind(L_count_update); >> 2253: inc(this, Address(mdp, offset), DataLayout::counter_increment); > > You are going to love this. Here is an interesting sequence of bad things. > > 1. `offset` is `rscratch2` in this method. > 2. `inc` is `increment_mdo`, we call there; it does `increment(dst, src << > ratio_shift);` > 3. `increment` is now `MacroAssembler::increment(Address dst, int value)`. It > prepares for `MA::increment(Register, int)`: > > > void MacroAssembler::increment(Address dst, int value) > { > ... > ldr(rscratch1, dst); > increment(rscratch1, value); // <--- calling here > str(rscratch1, dst); > } > > > 4. When `value` is large, and AFAICS in can be large with large > `ProfileCaptureRation`, that increment has a branch that clobbers `rscratch2`: > > > void MacroAssembler::increment(Register reg, int value) > { > if (value < 0) { decrement(reg, -value); return; } > if (value == 0) { return; } > if (value < (1 << 12)) { add(reg, reg, value); return; } > /* else */ { > assert(reg != rscratch2, "invalid dst for register increment"); > movw(rscratch2, (unsigned)value); // <--- clobbers rscratch2, I am sure > it is fine... > add(reg, reg, rscratch2); > } > } > > > 5. We return to `MacroAssembler::increment(Address dst, int value)`: > > > void MacroAssembler::increment(Address dst, int value) > { > ... > ldr(rscratch1, dst); > increment(rscratch1, value); // <--- returned from here > str(rscratch1, dst); // dst is Address(mdo, rscratch2), but > rscratch2 IS CLOBBERED > // by increment() above, we do the store to > the WRONG ADDRESS, > // AWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWWW > } This is https://github.com/openjdk/jdk/pull/30605, which I thought I'd already integrated in April, but it needs re-reviewing. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3362812908
