RFR: 8357782: JVM JIT Causes Static Initialization Order Issue

[Tobias Hartmann]

On Thu, 12 Jun 2025 06:45:23 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> # Issue Summary
>> 
>> When C1 compiles a method that allocates a new instance of a class that is not fully initialized at compile time, it does not take into account that the `<clinit>` might run a static initializer that might have side effects. Consider the following example:
>> 
>> class A {
>>     static class B {
>>         static String field;
>>         static void test() {
>>             String tmp = field;
>>             new C(field);
>>         }
>>     }
>> 
>>     static class C {
>>         static {
>>             B.field = "Hello";
>>         }
>> 
>>         C(String val) {
>>             if (val == null) {
>>                 throw new RuntimeException("Should not reach here");
>>             }
>>         }
>>     }
>> }
>> 
>> Here, `B.field` gets assigned in `C`'s static initializer. Since C1 believes that the `newinstance` does not have memory side effects, local value numbering eliminates the field access for the argument in `C.<init>` because it believes that `B.field` is still the same as `tmp`. Hence, the assignment in `C.<clinit>` gets effectively ignored and the code triggers the runtime exception. Because this only happens if `C` is not fully initialized when it is compiled, we need `-Xcomp` to reproduce this issue.
>> 
>> # Changes
>> 
>> To fix the illustrated issue, this PR ensures that `newinstance` kills the memory state in C1's LVN if the class might not be fully initialized. Since we can not reliably detect if a class has a static initializer, we kill memory whenever a class is not yet loaded or, if it has already been loaded, when it has not been fully initialized, which is conservative and might kill memory when it is not necessary for correctness and have an impact on performance in the form of some additional field accesses.
>> 
>> # Benchmark Results
>> 
>> Since this might have an effect on startup, I ran some benchmarks. The results mostly did not show effects outside the run-to-run variance.
>> 
>> # Testing
>> 
>> - [x] [GHA](https://github.com/mhaessig/jdk/actions/runs/15560262225)
>> - [x] tier1 through tier3 plus Oracle internal testing on Oracle supported platforms and OSes
>> 
>> # Acknowledgements
>> 
>> Shout out to @TobiHartmann who wrote the reproducer that became the regression test and helped me find my way around C1 and narrow down the problem.
>
> src/hotspot/share/ci/ciInstanceKlass.cpp line 553:
> 
>> 551: 
>> 552: bool ciInstanceKlass::has_class_initializer() {
>> 553:   VM_ENTRY_MARK;
> 
> Out of curiosity: do we strictly need to add `VM_ENTRY_MARK` here (since it is only called from the compiler) or it is mostly to ensure it is VM-safe if called from "outside'?

But it's needed exactly because we are calling from compiler, right? It will bring the compiler thread into the VM state which is required for accessing instance klass data.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/25725#discussion_r2141859139