RFR: 8338379: Accesses to class init state should be properly synchronized [v2]

[Coleen Phillimore]

On Mon, 23 Sep 2024 07:17:50 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> See the bug for the discussion. We have not seen a clear evidence this is _the_ problem in the field, neither we were able to come up with a reproducer. We have found this gap by inspecting the code, while chasing a production bug.
>> 
>> In short, `InstanceKlass::_init_state` is used as the "witness" for initialized class state. When class initialization completes, it needs to publish the class state by writing `_init_state = _fully_initialized` with release semantics. Current patch makes a seqcst write, which is stronger than strictly necessary. I think it is okay to be extra paranoid on rarely-executed class initialization path.
>> 
>> Various accessors that poll `IK::_init_state`, looking for class initialization to complete, need to read the field with acquire semantics. This is where the change fans out, touching VM, interpreter and compiler paths that e.g. implement clinit barriers. In some cases in assembler code, we can rely on hardware memory model to do what we need (i.e. acquire barriers/fences are nops).
>> 
>> I made the best _guess_ what ARM32, S390X, PPC64, RISC-V code should look like, based on what related code does for volatile loads. It would be good if port maintainers could sanity-check those.
>> 
>> Additional testing:
>>  - [x] Linux x86_64 server fastdebug, `all`
>>  - [x] Linux AArch64 server fastdebug, `all`
>>  - [x] GHA to test platform buildability + adhoc platform cross-compilation
>
> Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Relax to just a release

I like this patch.

src/hotspot/share/oops/instanceKlass.hpp line 517:

> 515:   bool is_in_error_state() const           { return init_state() == initialization_error; }
> 516:   bool is_reentrant_initialization(Thread *thread)  { return thread == _init_thread; }
> 517:   ClassState  init_state() const           { return Atomic::load_acquire(&_init_state); }

This is the code that I want the most with this patch. If we're reading this field outside a lock, we need the acquire.  Let's not make it more complicated than that.

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

Marked as reviewed by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/21110#pullrequestreview-2322066095
PR Review Comment: https://git.openjdk.org/jdk/pull/21110#discussion_r1771328239