RFR: 7903608: Cyclic initialization leads to NPE in header class with global variable

[Maurizio Cimadamore]

On Thu, 14 Dec 2023 10:57:07 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> This PR fixes some new static initialization cycles that have been made possible by (a) the removal of constant classes and (b) the fact that we refer to struct layouts by name.
> 
> The fix consists in two parts:
> 
> * First, we make sure that whenever the header class needs to reference another layout, we do so behind a holder class idiom (this patch only touches globals, as all other cases are already using the holder idiom)
> 
> * Secondly, we invert the inheritance order of header classes (this is only relevant when using header file splitting). In the current order, symbols that are found "later" in the header files, are added in classes that are "higher up" in the inheritance chain. This leads to issues, as you can have e.g. a primitive typedef depending on a previous primitive typedef, which is in reality defined in a subclass (which leads to a cycle).
> 
> Since I was there, I decided to simplify the logic for global variables, and make it more uniform with the rest: now we only emit an accessor for the global segment and a layout. We no longer emit a var handle accessor - developers have all they need to get one. Global vars setters/getters have also been simplified to just use memory segment accessors.

src/main/java/org/openjdk/jextract/impl/ToplevelBuilder.java line 89:

> 87:         List<JavaFileObject> files = new ArrayList<>();
> 88: 
> 89:         if (headerBuilders.size() == 1) {

The changes here are the trickiest. Basically, we can't figure out in advance which name header classes should have. So we end up generating something like:


<headerName>_h#{SUFFIX} extends {PREV_SUFFIX}


And then leave it to the code here to fixup all the suffixes. For this reason, the name of the runtime helper class cannot just be the current "class name" as that would include the `#{SUFFIX}` bit - but we know that there is going to be _some_ `<headerName>_h` generated at some point, so we can just use that as a runtime helper name.

Overall, I'm not too proud of this code, but at the same time, after evaluating a couple of alternatives they all seemed too complicated. And, we were doing some magic name adaptation even before.

src/main/java/org/openjdk/jextract/impl/Utils.java line 233:

> 231:     }
> 232: 
> 233:     public static Class<?> layoutCarrierFor(Type t) {

I have generalized this, so that we emit the layout carrier type for any jextract type

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

PR Review Comment: https://git.openjdk.org/jextract/pull/160#discussion_r1426570323
PR Review Comment: https://git.openjdk.org/jextract/pull/160#discussion_r1426570855