7903481: Jextract doesn't enforce group layout alignment correctly in…

… some cases Reviewed-by: jvernee
openjdk · May 30, 2023 · c3deba2 · c3deba2
1 parent 3f41267
commit c3deba2
Show file tree

Hide file tree

Showing 9 changed files with 99 additions and 26 deletions.
diff --git a/src/main/java/org/openjdk/jextract/clang/Type.java b/src/main/java/org/openjdk/jextract/clang/Type.java
@@ -164,6 +164,10 @@ private long size0() {
         return Index_h.clang_Type_getSizeOf(segment);
     }
 
+    private long align0() {
+        return Index_h.clang_Type_getAlignOf(segment);
+    }
+
     public long size() {
         long res = size0();
         if(TypeLayoutError.isError(res)) {
@@ -172,6 +176,14 @@ public long size() {
         return res;
     }
 
+    public long align() {
+        long res = align0();
+        if(TypeLayoutError.isError(res)) {
+            throw new TypeLayoutError(res, String.format("segment: %s", this));
+        }
+        return res;
+    }
+
     public TypeKind kind() {
         int v = kind0();
         TypeKind rv = TypeKind.valueOf(v);

diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java b/src/main/java/org/openjdk/jextract/clang/libclang/Index_h.java
@@ -4990,6 +4990,22 @@ public static long clang_Type_getSizeOf(MemorySegment T) {
             throw new AssertionError("should not reach here", ex$);
         }
     }
+    public static MethodHandle clang_Type_getAlignOf$MH() {
+        return RuntimeHelper.requireNonNull(constants$12.clang_Type_getAlignOf$MH,"clang_Type_getAlignOf");
+    }
+    /**
+     * {@snippet :
+     * long long clang_Type_getAlignOf(CXType T);
+     * }
+     */
+    public static long clang_Type_getAlignOf(MemorySegment T) {
+        var mh$ = clang_Type_getAlignOf$MH();
+        try {
+            return (long)mh$.invokeExact(T);
+        } catch (Throwable ex$) {
+            throw new AssertionError("should not reach here", ex$);
+        }
+    }
     public static MethodHandle clang_Type_getOffsetOf$MH() {
         return RuntimeHelper.requireNonNull(constants$12.clang_Type_getOffsetOf$MH,"clang_Type_getOffsetOf");
     }

diff --git a/src/main/java/org/openjdk/jextract/clang/libclang/constants$12.java b/src/main/java/org/openjdk/jextract/clang/libclang/constants$12.java
@@ -84,6 +84,17 @@ final class constants$12 {
         "clang_Type_getSizeOf",
         constants$12.clang_Type_getSizeOf$FUNC
     );
+    static final FunctionDescriptor clang_Type_getAlignOf$FUNC = FunctionDescriptor.of(Constants$root.C_LONG_LONG$LAYOUT,
+            MemoryLayout.structLayout(
+                    Constants$root.C_INT$LAYOUT.withName("kind"),
+                    MemoryLayout.paddingLayout(4),
+                    MemoryLayout.sequenceLayout(2, Constants$root.C_POINTER$LAYOUT).withName("data")
+            )
+    );
+    static final MethodHandle clang_Type_getAlignOf$MH = RuntimeHelper.downcallHandle(
+            "clang_Type_getAlignOf",
+            constants$12.clang_Type_getAlignOf$FUNC
+    );
     static final FunctionDescriptor clang_Type_getOffsetOf$FUNC = FunctionDescriptor.of(Constants$root.C_LONG_LONG$LAYOUT,
         MemoryLayout.structLayout(
             Constants$root.C_INT$LAYOUT.withName("kind"),

diff --git a/src/main/java/org/openjdk/jextract/impl/RecordLayoutComputer.java b/src/main/java/org/openjdk/jextract/impl/RecordLayoutComputer.java
@@ -26,6 +26,7 @@
 
 package org.openjdk.jextract.impl;
 
+import java.lang.foreign.AddressLayout;
 import java.lang.foreign.GroupLayout;
 import java.lang.foreign.MemoryLayout;
 import org.openjdk.jextract.Declaration;
@@ -130,10 +131,6 @@ void addField(long offset, Declaration declaration) {
             layout = org.openjdk.jextract.Type.layoutFor(var.type()).orElse(null);
         }
         if (layout != null) {
-            if ((offset % (layout.byteAlignment() * 8) != 0)) {
-                long maxAlign = Long.lowestOneBit(offset) / 8;
-                layout = forceAlign(layout, maxAlign);
-            }
             fieldLayouts.add(declaration.name().isEmpty() ? layout : layout.withName(declaration.name()));
         }
     }
@@ -193,19 +190,46 @@ long offsetOf(Type parent, Cursor c) {
         }
     }
 
-    MemoryLayout forceAlign(MemoryLayout layout, long maxAlign) {
-        if (layout instanceof GroupLayout groupLayout) {
-            MemoryLayout[] newMembers = groupLayout.memberLayouts()
-                    .stream().map(l -> forceAlign(l, maxAlign)).toArray(MemoryLayout[]::new);
-            return groupLayout instanceof StructLayout ?
-                    MemoryLayout.structLayout(newMembers) :
-                    MemoryLayout.unionLayout(newMembers);
-        } else if (layout instanceof SequenceLayout sequenceLayout) {
-            return MemoryLayout.sequenceLayout(sequenceLayout.elementCount(),
-                    forceAlign(sequenceLayout.elementLayout(), maxAlign));
-        } else {
-            return layout.byteAlignment() > maxAlign ?
-                    layout.withByteAlignment(maxAlign) : layout;
+    void checkSize(GroupLayout layout) {
+        // sanity check
+        if (cursor.type().size() != layout.byteSize()) {
+            throw new AssertionError(
+                    String.format("Unexpected size for layout %s. Found %d ; expected %d",
+                            layout, layout.byteSize(), cursor.type().size()));
+        }
+    }
+
+    MemoryLayout[] alignFields() {
+        long align = cursor.type().align();
+        return fieldLayouts.stream()
+                .map(l -> forceAlign(l, align))
+                .toArray(MemoryLayout[]::new);
+    }
+
+    private static MemoryLayout forceAlign(MemoryLayout layout, long align) {
+        if (align >= layout.byteAlignment()) {
+            return layout; // fast-path
+        }
+        MemoryLayout res = switch (layout) {
+            case GroupLayout groupLayout -> {
+                MemoryLayout[] newMembers = groupLayout.memberLayouts()
+                        .stream().map(l -> forceAlign(l, align)).toArray(MemoryLayout[]::new);
+                yield groupLayout instanceof StructLayout ?
+                        MemoryLayout.structLayout(newMembers) :
+                        MemoryLayout.unionLayout(newMembers);
+            }
+            case SequenceLayout sequenceLayout ->
+                MemoryLayout.sequenceLayout(sequenceLayout.elementCount(),
+                        forceAlign(sequenceLayout.elementLayout(), align));
+            default -> layout.withByteAlignment(align);
+        };
+        // copy name and target layout, if present
+        if (layout.name().isPresent()) {
+            res = res.withName(layout.name().get());
+        }
+        if (layout instanceof AddressLayout addressLayout && addressLayout.targetLayout().isPresent()) {
+            ((AddressLayout)res).withTargetLayout(addressLayout.targetLayout().get());
         }
+        return res;
     }
 }
diff --git a/src/main/java/org/openjdk/jextract/impl/StructLayoutComputer.java b/src/main/java/org/openjdk/jextract/impl/StructLayoutComputer.java
@@ -133,8 +133,8 @@ Declaration.Scoped finishRecord(String anonName) {
          */
         handleBitfields();
 
-        MemoryLayout[] fields = fieldLayouts.toArray(new MemoryLayout[0]);
-        GroupLayout g = MemoryLayout.structLayout(fields);
+        GroupLayout g = MemoryLayout.structLayout(alignFields());
+        checkSize(g);
         if (!cursor.spelling().isEmpty()) {
             g = g.withName(cursor.spelling());
         } else if (anonName != null) {

diff --git a/src/main/java/org/openjdk/jextract/impl/TypeImpl.java b/src/main/java/org/openjdk/jextract/impl/TypeImpl.java
@@ -373,7 +373,7 @@ public String toString() {
     public static Optional<MemoryLayout> getLayout(org.openjdk.jextract.Type t) {
         try {
             return Optional.of(getLayoutInternal(t));
-        } catch (Throwable ex) {
+        } catch (UnsupportedOperationException ex) {
             return Optional.empty();
         }
     }
@@ -389,7 +389,7 @@ public static Optional<FunctionDescriptor> getDescriptor(Function t) {
             } else {
                 return Optional.of(FunctionDescriptor.of(getLayoutInternal(retType), args));
             }
-        } catch (Throwable ex) {
+        } catch (UnsupportedOperationException ex) {
             return Optional.empty();
         }
     }

diff --git a/src/main/java/org/openjdk/jextract/impl/UnionLayoutComputer.java b/src/main/java/org/openjdk/jextract/impl/UnionLayoutComputer.java
@@ -91,12 +91,10 @@ Declaration.Scoped finishRecord(String anonName) {
         if (actualSize < expectedSize) {
             // emit an extra padding of expected size to make sure union layout size is computed correctly
             addPadding(expectedSize);
-        } else if (actualSize > expectedSize) {
-            throw new AssertionError("Invalid union size - expected: " + expectedSize + "; found: " + actualSize);
         }
 
-        MemoryLayout[] fields = fieldLayouts.toArray(new MemoryLayout[0]);
-        GroupLayout g = MemoryLayout.unionLayout(fields);
+        GroupLayout g = MemoryLayout.unionLayout(alignFields());
+        checkSize(g);
         if (!cursor.spelling().isEmpty()) {
             g = g.withName(cursor.spelling());
         } else if (anonName != null) {

diff --git a/test/testng/org/openjdk/jextract/test/api/TestPackedStructs.java b/test/testng/org/openjdk/jextract/test/api/TestPackedStructs.java
@@ -34,7 +34,7 @@
 public class TestPackedStructs extends JextractApiTestBase {
 
     static final String[] NAMES = {
-            "S1", "S2", "S3", "S4", "S5", "S6"
+            "S1", "S2", "S3", "S4", "S5", "S6", "S7", "S8"
     };
 
     @Test

diff --git a/test/testng/org/openjdk/jextract/test/api/packedstructs.h b/test/testng/org/openjdk/jextract/test/api/packedstructs.h
@@ -56,3 +56,15 @@ struct S6 {
    char first;
    union { int x[2]; int y[2]; } second;
 };
+
+#pragma pack(1)
+struct S7 {
+   long long first;
+   int second;
+};
+
+#pragma pack(1)
+struct S8 {
+   struct S7 first[1];
+   struct S7 second[1];
+};