[foreign-memaccess+abi] RFR: 8302098: Fix initialization order in NativeMemorySegmentImpl

[Jorn Vernee]

On Wed, 8 Feb 2023 18:58:52 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Hi,
> the recent fix for 8300201 [1] exposed a latent initialization bug in the FFM API implementation. That fix added a call to `UNSAFE.addressSize()` in the constructor of `NativeMemorySegmentImpl` (in order to perform some normalization of the incoming address on 32-bit platforms). After we integrated that change we have started to see some failures in our nightly builds. Most failures had to do with creating memory mapped segments and manifested themselves as NPEs when tryin to call `UNSAFE.addressSize()` (because the `UNSAFE` static field was `null`).
> 
> Yesterday I have integrated a PR in order to fix these issues [2] - the fix simply replaces `UNSAFE.addressSize()` with `UNSAFE.ADDRESS_SIZE` (e.g. replace instance method call with static field access).
> 
> Today I've spent more time investigating what's actually wrong with the code, given that I couldn't see any obvious looping logic. After staring at the code for some time, I realized that we indeed suffer from cyclic initialization. The cycle goes as follows:
> 
> * A client calls `FileChannel::map`
> * That method wants to create a new instance of `MappedMemorySegmentImpl`
> * To do that, we need to load `MappedMemorySegmentImpl` and run its static initializers
> * But, to do that, we need to load `NativeMemorySegmentImpl` and run its static initializers
> * But, but, to do that, we need to load `AbstractMemorySegmentImpl` and run its static initializers
> * But, but, but, to do that, we need to load `MemorySegment` and run its static initializers
> * Unfortunately, `MemorySegment` has a static final field, namely `NULL` which calls the static method `NativeMemorySegment.makeNativeSegmentUnchecked`
> * This method creates a new `NativeMemorySegmentImpl` (for `NULL`), but when evaluating the constructor, no static field in the class is set; hence we NPE.
> 
> In other words, while initializing a subclass, we initialize a superclass - but that initialization points back to the subclass. Hence the cycle.
> 
> To solve this w/o having to change the public API, I have added a dedicated, simple constructor which can be used to construct the memory segment instance for `NULL`. This constructor has some documentation which explains the initialization issues, and should be left alone (other than when initializing `NULL`).
> 
> [1] - https://git.openjdk.org/panama-foreign/pull/774
> [2] - https://git.openjdk.org/panama-foreign/pull/782

Marked as reviewed by jvernee (Committer).

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

PR: https://git.openjdk.org/panama-foreign/pull/783