RFR: 7903485 Windows.h fails to extract on jextract/panama

[Maurizio Cimadamore]

On Wed, 7 Jun 2023 14:10:19 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> This patch fixes a number of issues I discovered while trying to extract Windows.h on a recent jextract:
>> 
>> * the size of `long double` layout is wrong. This means that if a struct with a field of that type is declared, StructLayoutComputer will fail (as it will see a size != expected size reported by clang)
>> * There are cases where, also for unsupported layouts, we end up creating function descriptors with things containing padding. This is caused by the fact that the check for unsupported layouts in functions is performed *after* the creation of the function descriptor.
>> * Sometimes libclang reports field cursors in an out-of-order fashion. I have not been able to pinpoint the exact cause, as all my attempts to create a reduced test case failed. When this behavior occurs, jextract ends up generating extra fields and padding. I believe this behavior has always been there, but now brought to the fore by the new eager checks.
>> * A change in behavior of recent libclang causes `cursor.spelling()` to return non-empty strings (see https://github.com/llvm/llvm-project/issues/58429)
>> * A quality of life fix, to generate "sensible" warnings when variadic callbacks are found
>
> src/main/java/org/openjdk/jextract/impl/TreeMaker.java line 217:
> 
>> 215:                 return null;
>> 216:             }
>> 217:             return Declaration.scoped(scopeKind, CursorPosition.of(c), c.spelling());
> 
> Could you explain this change? I guess we're assuming that there are no decls here since this is not a definition?

To be 100% fair, I don't understand which situation is the `return` in the `else` supposed to cover. Even the old code seems off. E.g. `RecordLayoutComputer` seems to filter out cases where `c.getDefinition().isInvalid()`. But for everything else there should be a layout. But the layout was thrown away in the old impl as well. So, my goal here was to have a cleaner separation between "supported" cases and "unsupported" ones. Note that if we create a scoped decl w/o a layout (as we do in the old code), that thing ends up being skipped anyway by `OutputFactory`:


if (d.layout().isEmpty() || structDefinitionSeen(d)) {
            //skip decl
            return null;
        }
 ```
 
Anyway, the main problem that was fixed here was to make sure that the _name_ of the type returned by `RecordLayoutComputer.compute` was reused, at least in the "supported" path. IMHO, we could just return `null` in the else, and we would probably not change semantics.

> src/main/java/org/openjdk/jextract/impl/UnsupportedLayouts.java line 44:
> 
>> 42:     public static final MemoryLayout __INT128 = makeUnsupportedLayout(16, "__int128");
>> 43: 
>> 44:     public static final MemoryLayout LONG_DOUBLE = makeUnsupportedLayout(IS_WINDOWS ? 8 : 16, "long double");
> 
> We could use the `ValueLayout.JAVA_DOUBLE` layout in `Type.java` for the `long double` type (we are already selecting the layout based on the platform for `long` there as well). If we do that `long double` would just work on Windows.
> 
> FWIW, I gave this a try, and it looks like we'd just have to skip `LibUnsupportedTest::testIgnoredMethods` on Windows in that case. Windows.h still extracts successfully as well:
> 
> 
> diff --git a/src/main/java/org/openjdk/jextract/Type.java b/src/main/java/org/openjdk/jextract/Type.java
> index 65e0fed..291ec0c 100644
> --- a/src/main/java/org/openjdk/jextract/Type.java
> +++ b/src/main/java/org/openjdk/jextract/Type.java
> @@ -137,7 +137,9 @@ public interface Type {
>              /**
>                * {@code long double} type.
>                */
> -            LongDouble("long double", UnsupportedLayouts.LONG_DOUBLE),
> +            LongDouble("long double", TypeImpl.IS_WINDOWS ?
> +                ValueLayout.JAVA_DOUBLE :
> +                UnsupportedLayouts.LONG_DOUBLE),
>              /**
>               * {@code float128} type.
>               */
> diff --git a/src/main/java/org/openjdk/jextract/impl/UnsupportedLayouts.java b/src/main/java/org/openjdk/jextract/impl/UnsupportedLayouts.java
> index a6b1aec..0797368 100644
> --- a/src/main/java/org/openjdk/jextract/impl/UnsupportedLayouts.java
> +++ b/src/main/java/org/openjdk/jextract/impl/UnsupportedLayouts.java
> @@ -37,11 +37,9 @@ import java.nio.ByteOrder;
>  public final class UnsupportedLayouts {
>      private UnsupportedLayouts() {}
> 
> -    private static final boolean IS_WINDOWS = System.getProperty("os.name").startsWith("Windows");
> -
>      public static final MemoryLayout __INT128 = makeUnsupportedLayout(16, "__int128");
> 
> -    public static final MemoryLayout LONG_DOUBLE = makeUnsupportedLayout(IS_WINDOWS ? 8 : 16, "long double");
> +    public static final MemoryLayout LONG_DOUBLE = makeUnsupportedLayout(16, "long double");
> 
>      public static final MemoryLayout _FLOAT128 = makeUnsupportedLayout(16, "_float128");
> 
> diff --git a/test/jtreg/generator/test8257892/LibUnsupportedTest.java b/test/jtreg/generator/test8257892/LibUnsupportedTest.java
> index 7605826..86f09d6 100644
> --- a/test/jtreg/generator/test8257892/LibUnsuppor...

I can do that if that's preferred. (it's mostly a choice on whether we want the set of supported types to be stable across platforms or not).

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

PR Review Comment: https://git.openjdk.org/jextract/pull/122#discussion_r1226398225
PR Review Comment: https://git.openjdk.org/jextract/pull/122#discussion_r1226399671