RFR: 8370405: C2: mismatched store from MergeStores wrongly scalarized in allocation elimination

[Emanuel Peter]

On Wed, 29 Oct 2025 08:25:18 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> Note: @oliviermattmann found this bug with his whitebox fuzzer. See also https://github.com/openjdk/jdk/pull/27991
>> 
>> **Analysis**
>> We run Escape Analysis, and see that a local array allocation could possibly be removed, we only have matching `StoreI` to the `int[]`. But there is one `StoreI` that is still in a loop, and so we wait with the actual allocation removal until later, hoping it may go away, or drop out of the loop.
>> During loop opts, the `StoreI` drops out of the loop, now there should be nothing in the way of allocation removal.
>> But now we run `MergeStores`, and merge two of the `StoreI` into a mismatched `StoreL`.
>> 
>> Then, we eventually remove the allocation, but don't check again if any new mismatched store has appeared.
>> Instead of a `ConI`, we receive a `ConL`, for the first of the two merged `StoreI`. The second merged `StoreI` instead captures the state before the `StoreL`, and that is wrong.
>> 
>> **Solution**
>> We should have some assert, that checks that the captured `field_val` corresponds to the expected `field_type`.
>> 
>> But the real fix was suggested by @merykitty : apparently he just had a similar issue in Valhalla:
>> https://github.com/openjdk/valhalla/blame/60af17ff5995cfa5de075332355f7f475c163865/src/hotspot/share/opto/macro.cpp#L709-L713
>> (the idea is to bail out of the elimination if any of the found stores are mismatched.)
>> 
>> **Details**
>> 
>> How the bad sequence develops, and which components are involved.
>> 
>> 1) The `SafePoint` contains a `ConL` and 3 `ConI`. (Correct would have been 4 `ConI`)
>> 
>>   6  ConI  === 23  [[ 4 ]]  #int:16777216
>>   7  ConI  === 23  [[ 4 ]]  #int:256
>>   8  ConI  === 23  [[ 4 ]]  #int:1048576
>>   9  ConL  === 23  [[ 4 ]]  #long:68719476737
>>  54  DefinitionSpillCopy  === _ 27  [[ 16 12 4 ]]
>>   4  CallStaticJavaDirect  === 47 29 30 26 32 33 0 34 0 54 9 8 7 6  [[ 5 3 52 ]] Static wrapper for: uncommon_trap(reason='unstable_if' action='reinterpret' debug_id='0') # void ( int ) C=0.000100 Test::test @ bci:38 (line 21) reexecute !jvms: Test::test @ bci:38 (line 21)
>> 
>> 
>> 2) This is then encoded into an `ObjectValue`. A `Type::Long` / `ConL` is converted into a `[int=0, long=ConL]` pair, see:
>> https://github.com/openjdk/jdk/blob/da7121aff9eccb046b82a75093034f1cdbd9b9e4/src/hotspot/share/opto/output.cpp#L920-L925
>> If I understand it right, there zero is just a placeholder.
>> 
>> And so we get:
>> 
>> (rr) p sv->print_fields_on(tty)
>>         Fields: 0, 68719476737, 1048576, 256, 167772...
>
> src/hotspot/share/opto/macro.cpp line 874:
> 
>> 872:       assert(false, "field_val does not fit field_type");
>> 873:     }
>> 874: #endif
> 
> I'm not yet happy with this assert. It is not super easy to get it right, but currently it is a bit weak.
> 
> Do reviewers have any good ideas here?

I can make the assert strong for primitive types, and that is where the bug happened.

I tried to make it work for pointers too, but I got this example:

value_type: java/lang/Object:NotNull *
field_type: java/lang/Object (java/util/Enumeration) *

It happens in `ClassLoader.getResources` (about line 1445).
We seem to get back the `value_type` from a nested call to `parent.getResources(name);`, but we don't seem to capture that this has the `Enumeration` interface.
But the `field_type` (store to `tmp[0]`) knows about that, and so it is a "narrower" type.
Is this expected?
- If yes: can I even write an assert here?
- If no: is this something we need/should fix?


  1436     public Enumeration<URL> getResources(String name) throws IOException {
  1437         Objects.requireNonNull(name);
  1438         @SuppressWarnings("unchecked")
  1439         Enumeration<URL>[] tmp = (Enumeration<URL>[]) new Enumeration<?>[2];
  1440         if (parent != null) {
  1441             tmp[0] = parent.getResources(name);   <------ look here
  1442         } else {    
  1443             tmp[0] = BootLoader.findResources(name);
  1444         }
  1445         tmp[1] = findResources(name);                                                                                                                                                                                                                                         
  1446 
  1447         return new CompoundEnumeration<>(tmp);
  1448     }

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/27997#discussion_r2472374332