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

[Jorn Vernee]

On Thu, 1 Jun 2023 15:15:56 GMT, Maurizio Cimadamore <mcimadamore 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

I gave this a try here as well. Windows.h extracts successfully, nice work!

There is still one failing test due to https://github.com/openjdk/jextract/pull/106 missing from the panama branch. It is a really small fix, so maybe you could fold it into this PR?

src/main/java/org/openjdk/jextract/impl/StructLayoutComputer.java line 91:

> 89:         if (offset > expectedOffset) {
> 90:             // out-of-order field, skip
> 91:             return;

So, I think as a result of returning here, we will essentially just emit padding instead of the out of order field, right?

I think it would be nice to log a message as well to let the user know that something went wrong, and the resulting layout might not be usable (similar to how we do that for unsupported layouts)

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?

src/main/java/org/openjdk/jextract/impl/TypeImpl.java line 376:

> 374:         try {
> 375:             return Optional.of(getLayoutInternal(t));
> 376:         } catch (Throwable ex) {

This seems to be reverting the changes from: https://github.com/openjdk/jextract/pull/121/files#r1210157420

Not sure if that's intended?

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/LibUnsupportedTest.java
+++ b/test/jtreg/generator/test8257892/LibUnsupportedTest.java
@@ -28,6 +28,7 @@ import java.lang.foreign.MemoryLayout;
 import java.lang.foreign.MemoryLayout.PathElement;
 import java.lang.foreign.MemorySegment;
 import org.testng.annotations.Test;
+import org.testng.SkipException;

 import test.jextract.unsupported.unsupported_h;
 import static org.testng.Assert.assertEquals;
@@ -51,6 +52,8 @@ import test.jextract.unsupported.*;
  */

 public class LibUnsupportedTest {
+    private static final boolean IS_WINDOWS = System.getProperty("os.name").startsWith("Windows");
+
     @Test
     public void testAllocateFoo() {
         try (Arena arena = Arena.ofConfined()) {
@@ -86,6 +89,9 @@ public class LibUnsupportedTest {

     @Test
     public void testIgnoredMethods() {
+        if (IS_WINDOWS) {
+            throw new SkipException("long double works on Windows");
+        }
         assertNull(findMethod(unsupported_h.class, "func"));
         assertNull(findMethod(unsupported_h.class, "func2"));
         assertNull(findMethod(unsupported_h.class, "func3"));

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

PR Review: https://git.openjdk.org/jextract/pull/122#pullrequestreview-1467763096
PR Review Comment: https://git.openjdk.org/jextract/pull/122#discussion_r1221655651
PR Review Comment: https://git.openjdk.org/jextract/pull/122#discussion_r1221668473
PR Review Comment: https://git.openjdk.org/jextract/pull/122#discussion_r1221671068
PR Review Comment: https://git.openjdk.org/jextract/pull/122#discussion_r1221682039