Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8338379

Accesses to class init state should be properly synchronized

XMLWordPrintable

    • b19

      Volker and me were trying to understand what guarantees that initializations that happen in `<clinit>` methods are visible to the all readers of initialized static fields. We have seen weird Scala NPEs that look as if we see `null`-s in `static` field, even though it was initialized in `<clinit>`; and it manifests more readily on Gravitons.

      JVMS 5.5 outlines how class initialization is protected by the initialization lock, and then states:

      ```
      A Java Virtual Machine implementation may optimize this procedure by eliding the lock acquisition in step 1 (and release in step 4/5) when it can determine that the initialization of the class has already completed, provided that, in terms of the Java memory model, all happens-before orderings (JLS §17.4.5) that would exist if the lock were acquired, still exist when the optimization is performed.
      ```

      Reading the Hotspot code, I think we have a path that avoids initialization lock, when checking for `InstanceKlass::_init_state` via various accessors. Some thread can come just barely after class initialization completed, check `InstanceKlass::is_initialized()` without acquiring any locks, and thus break whatever happens-before between <clinit>-end and subsequent operations. Meaning, there is a possibility for a thread to see the static fields *not* in the state <clinit> left them in.

      I believe the whole thing got worse with JDK-8223213 and other platform-specific follow-ups, which read `InstanceKlass::_init_state` without synchronization at all. See e.g.: https://github.com/openjdk/jdk/blob/38bd8a36704a962f0ad1052fd2ec150a61663256/src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp#L1841

      There are still some barriers in `<clinit>`, mostly by accident? The template interpreters usually emit `StoreStore` at `return` bytecode, which implicitly puts a barrier at the end of `<clinit>`, e.g.: https://github.com/openjdk/jdk/blob/0e4d4a0c3150c01d927bd69cc578cea053cf16b3/src/hotspot/cpu/aarch64/templateTable_aarch64.cpp#L2203-L2207 But I am not sure this is sufficient for memory safety, and might just work by accident of ordering the stores to static fields and the `IK::_init_state`, thus eliminating part of the race on writer side. There is still a race on reader side, which I don't think is benign even after `StoreStore` on writer side (= there might be no data dependency to static field that might save us, like it does for Java instance final fields). Anyhow, C1 never had barriers for `<clinit>`-s: https://github.com/openjdk/jdk/blob/29ba4b7d1e62a834c1693fe6ad383c19467afc81/src/hotspot/share/c1/c1_GraphBuilder.cpp#L1566, and we relaxed the `StoreStore` check on `<clinit>` exits in C2 with JDK-8336466.

      I think we should strengthen the class initialization path by making `InstanceKlass::_init_state` a proper acquire/release witness. I.e. write to it with `Atomic::release_store_fence` and read from it with `Atomic::load_acquire`, plus do atomic ops in `MacroAssembler` and compiler code. This would give us a publication safety outside the initialization lock. E.g. like this: https://github.com/openjdk/jdk/compare/master...shipilev:wip-class-init-check

            shade Aleksey Shipilev
            shade Aleksey Shipilev
            Votes:
            0 Vote for this issue
            Watchers:
            9 Start watching this issue

              Created:
              Updated:
              Resolved: