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

[Jorn Vernee]

On Thu, 14 Dec 2023 13:22:23 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.
>
> Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Drop spurious option in tests

I think this also fixes: https://bugs.openjdk.org/browse/CODETOOLS-7903607 ?

src/main/java/org/openjdk/jextract/impl/HeaderFileBuilder.java line 361:

> 359:         if (declaration != null) {
> 360:             emitDocComment(declaration);
> 361:         }

Spurious change? (same below)

src/main/java/org/openjdk/jextract/impl/SourceFileBuilder.java line 95:

> 93: 
> 94:     public JavaFileObject toFile(String suffix, Function<String, String> finisher) {
> 95:         return Utils.fileFromString(packageName, STR."\{className}\{suffix}", finisher.apply(sb.toString()));

So, it looks like we adjust the class name at the last moment when generating the file. That's fine I think, but it also means that the `SourceFileBuilder::className` field is 'fake'/a place holder in some cases. That seems like it might be problematic if other code starts to naively use `SourceFileBuilder::className`.

Could we maybe make this more explicit? e.g. could we make the class name an `Optional<String>` ? and then require the name to be injected when calling `toFile`? Or maybe always require the class name to be specified to `toFile`, and just remove the `className` field?

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

PR Review: https://git.openjdk.org/jextract/pull/160#pullrequestreview-1781736923
PR Review Comment: https://git.openjdk.org/jextract/pull/160#discussion_r1426672110
PR Review Comment: https://git.openjdk.org/jextract/pull/160#discussion_r1426714795