On Fri, 5 Jun 2026 13:06:24 GMT, Andrew Haley <[email protected]> wrote:
>> 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.
> This is #30605, which I thought I'd already integrated in April, but it needs
> re-reviewing.
Pushed now.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28541#discussion_r3363611842
