8262198: Overhaul bitfield parsing logic

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/ConstantBuilder.java

@@ -251,21 +251,21 @@ private Constant emitLayoutField(String javaName, MemoryLayout layout) {
         incrAlign();
         indent();
         append(memberMods() + "MemoryLayout " + fieldName + " = ");
-        emitLayoutString(layout);
+        emitLayoutString(layout, false);
         append(";\n");
         decrAlign();
         return new Constant(className(), javaName, Constant.Kind.LAYOUT);
     }
 
-    private void emitLayoutString(MemoryLayout l) {
+    private void emitLayoutString(MemoryLayout l, boolean inBitfield) {
         if (l instanceof ValueLayout val) {
-            append(typeToLayoutName(val));
+            append(typeToLayoutName(val, inBitfield));
         } else if (l instanceof SequenceLayout seq) {
             append("MemoryLayout.ofSequence(");
             if (seq.elementCount().isPresent()) {
                 append(seq.elementCount().getAsLong() + ", ");
             }
-            emitLayoutString(seq.elementLayout());
+            emitLayoutString(seq.elementLayout(), false);
             append(")");
         } else if (l instanceof GroupLayout group) {
             if (group.isStruct()) {
@@ -275,10 +275,11 @@ private void emitLayoutString(MemoryLayout l) {
             }
             incrAlign();
             String delim = "";
+            boolean isBitfield = LayoutUtils.isBitfields(group);
             for (MemoryLayout e : group.memberLayouts()) {
                 append(delim);
                 indent();
-                emitLayoutString(e);
+                emitLayoutString(e, isBitfield);
                 delim = ",\n";
             }
             append("\n");
@@ -305,7 +306,7 @@ private Constant emitFunctionDescField(String javaName, FunctionDescriptor desc)
         append(" = ");
         if (desc.returnLayout().isPresent()) {
             append("FunctionDescriptor.of(");
-            emitLayoutString(desc.returnLayout().get());
+            emitLayoutString(desc.returnLayout().get(), false);
             if (!noArgs) {
                 append(",");
             }
@@ -319,7 +320,7 @@ private Constant emitFunctionDescField(String javaName, FunctionDescriptor desc)
             for (MemoryLayout e : desc.argumentLayouts()) {
                 append(delim);
                 indent();
-                emitLayoutString(e);
+                emitLayoutString(e, false);
                 delim = ",\n";
             }
             append("\n");
@@ -359,23 +360,25 @@ private Constant emitConstantAddress(String javaName, Object value) {
         return new Constant(className(), javaName, Constant.Kind.ADDRESS);
     }
 
-    private static String typeToLayoutName(ValueLayout vl) {
+    private static String typeToLayoutName(ValueLayout vl, boolean inBitfields) {
         if (UnsupportedLayouts.isUnsupported(vl)) {
             return "MemoryLayout.ofPaddingBits(" + vl.bitSize() + ")";
+        } else if (inBitfields) {
+            return "MemoryLayout.ofValueBits(" + vl.bitSize() + ", ByteOrder.nativeOrder())";
+        } else {
+            CLinker.TypeKind kind = (CLinker.TypeKind) vl.attribute(CLinker.TypeKind.ATTR_NAME).orElseThrow(
+                    () -> new IllegalStateException("Unexpected value layout: could not determine ABI class"));
+            return switch (kind) {
+                case CHAR -> "C_CHAR";
+                case SHORT -> "C_SHORT";
+                case INT -> "C_INT";
+                case LONG -> "C_LONG";
+                case LONG_LONG -> "C_LONG_LONG";
+                case FLOAT -> "C_FLOAT";
+                case DOUBLE -> "C_DOUBLE";
+                case POINTER -> "C_POINTER";
+            };
         }
-
-        CLinker.TypeKind kind = (CLinker.TypeKind)vl.attribute(CLinker.TypeKind.ATTR_NAME).orElseThrow(
-                () -> new IllegalStateException("Unexpected value layout: could not determine ABI class"));
-        return switch (kind) {
-            case CHAR -> "C_CHAR";
-            case SHORT -> "C_SHORT";
-            case INT -> "C_INT";
-            case LONG -> "C_LONG";
-            case LONG_LONG -> "C_LONG_LONG";
-            case FLOAT -> "C_FLOAT";
-            case DOUBLE -> "C_DOUBLE";
-            case POINTER -> "C_POINTER";
-        };
     }
 
     private Constant emitSegmentField(String javaName, String nativeName, MemoryLayout layout) {

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/JavaSourceBuilder.java

@@ -221,6 +221,7 @@ protected void emitPackagePrefix() {
     protected void emitImportSection() {
         append("import java.lang.invoke.MethodHandle;\n");
         append("import java.lang.invoke.VarHandle;\n");
+        append("import java.nio.ByteOrder;\n");
         append("import java.util.Objects;\n");
         append("import jdk.incubator.foreign.*;\n");
         append("import jdk.incubator.foreign.MemoryLayout.PathElement;\n");

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/LayoutUtils.java

@@ -27,6 +27,7 @@
 package jdk.internal.jextract.impl;
 
 import jdk.incubator.foreign.FunctionDescriptor;
+import jdk.incubator.foreign.GroupLayout;
 import jdk.incubator.foreign.MemoryLayout;
 import jdk.incubator.jextract.Type.Primitive;
 import jdk.internal.clang.Cursor;
@@ -43,6 +44,7 @@
 public final class LayoutUtils {
 
     public static final String JEXTRACT_ANONYMOUS = "jextract/anonymous";
+    public static final String JEXTRACT_BITFIELDS = "jextract/bitfields";
 
     private LayoutUtils() {}
 
@@ -216,4 +218,13 @@ public static Primitive.Kind valueLayoutForSize(long size) {
             default -> throw new IllegalStateException("Cannot infer container layout");
         };
     }
+
+    static boolean isBitfields(GroupLayout layout) {
+        return layout.attribute(JEXTRACT_BITFIELDS).isPresent();
+    }
+
+    @SuppressWarnings("unchecked")
+    static <Z extends MemoryLayout> Z setBitfields(Z layout) {
+        return (Z) layout.withAttribute(JEXTRACT_BITFIELDS, true);
+    }
 }

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/Parser.java

@@ -85,7 +85,7 @@ public Declaration.Scoped parse(Path path, Collection<String> args) {
                     } else {
                         Declaration decl = treeMaker.createTree(c);
                         if (decl != null) {
-                            decls.add(treeMaker.createTree(c));
+                            decls.add(decl);
                         }
                     }
                 } else if (isMacro(c) && src.path() != null) {

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/RecordLayoutComputer.java

@@ -26,9 +26,7 @@
 
 package jdk.internal.jextract.impl;
 
-import jdk.incubator.foreign.GroupLayout;
 import jdk.incubator.foreign.MemoryLayout;
-import jdk.incubator.foreign.ValueLayout;
 import jdk.internal.clang.Cursor;
 import jdk.internal.clang.CursorKind;
 import jdk.internal.clang.Type;
@@ -137,13 +135,8 @@ long fieldSize(Cursor c) {
         return c.isBitField() ? c.getBitFieldWidth() : c.type().size() * 8;
     }
 
-    ValueLayout bitfield(ValueLayout container, List<MemoryLayout> sublayouts) {
-        return Utils.addContents(container, MemoryLayout.ofStruct(sublayouts.toArray(new MemoryLayout[0])));
-    }
-
-    ValueLayout bitfield(long containerSize, List<MemoryLayout> sublayouts) {
-        return bitfield((ValueLayout)LayoutUtils.valueLayoutForSize(containerSize)
-                        .layout().orElseThrow(() -> new IllegalStateException("Unsupported size: " + containerSize)), sublayouts);
+    MemoryLayout bitfield(List<MemoryLayout> sublayouts) {
+        return LayoutUtils.setBitfields(MemoryLayout.ofStruct(sublayouts.toArray(new MemoryLayout[0])));
     }
 
     long offsetOf(Type parent, Cursor c) {

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/StructLayoutComputer.java

@@ -131,65 +131,8 @@ MemoryLayout finishLayout() {
     // process bitfields if any and clear bitfield layouts
     private void handleBitfields() {
         if (bitfieldLayouts != null) {
-            fieldLayouts.addAll(convertBitfields(bitfieldLayouts));
+            fieldLayouts.add(bitfield(bitfieldLayouts));
             bitfieldLayouts = null;
         }
     }
-
-    private List<MemoryLayout> convertBitfields(List<MemoryLayout> layouts) {
-        long offset = 0L;
-        List<MemoryLayout> newFields = new ArrayList<>();
-        List<MemoryLayout> pendingFields = new ArrayList<>();
-        for (MemoryLayout l : layouts) {
-            offset += l.bitSize();
-            if (offset > MAX_STORAGE_SIZE) {
-                throw new IllegalStateException("Crossing storage unit boundaries");
-            }
-            pendingFields.add(l);
-            long storageSize = storageSize(offset);
-            if (!pendingFields.isEmpty() && storageSize != -1) {
-                //emit new
-                newFields.add(bitfield(storageSize, pendingFields));
-                pendingFields.clear();
-                offset = 0L;
-            }
-        }
-        if (!pendingFields.isEmpty()) {
-            long storageSize = nextStorageSize(offset);
-            //emit new
-            newFields.add(bitfield(storageSize, pendingFields));
-            pendingFields.clear();
-        }
-        return newFields;
-    }
-
-    static int[] STORAGE_SIZES = { 64, 32, 16, 8 };
-    static int[] ALIGN_SIZES = { 8, 16, 32, 64 };
-    static int MAX_STORAGE_SIZE = 64;
-
-    private long storageSize(long size) {
-        // offset should be < MAX_STORAGE_SIZE
-        for (int s : STORAGE_SIZES) {
-            if (size == s) {
-                return s;
-            }
-        }
-        return -1;
-    }
-
-    private long nextStorageSize(long size) {
-        // offset should be < MAX_STORAGE_SIZE
-        for (int s : ALIGN_SIZES) {
-            long alignedSize = alignUp(size, s);
-            long storageSize = storageSize(alignedSize);
-            if (storageSize != -1) {
-                return storageSize;
-            }
-        }
-        return -1;
-    }
-
-    private static long alignUp(long n, long alignment) {
-        return (n + alignment - 1) & -alignment;
-    }
 }

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/TreeMaker.java

@@ -34,7 +34,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -299,10 +298,9 @@ private List<Declaration> collectBitfields(MemoryLayout layout, List<Declaration
                                                                        .collect(Collectors.toSet());
         List<Declaration> newDecls = new ArrayList<>();
         for (MemoryLayout e : ((GroupLayout)layout).memberLayouts()) {
-            Optional<GroupLayout> contents = Utils.getContents(e);
-            if (contents.isPresent()) {
+            if (e instanceof GroupLayout contents && LayoutUtils.isBitfields(contents)) {
                 List<Declaration.Variable> bfDecls = new ArrayList<>();
-                outer: for (MemoryLayout bitfield : contents.get().memberLayouts()) {
+                outer: for (MemoryLayout bitfield : contents.memberLayouts()) {
                     if (bitfield.name().isPresent() && !nestedBitfieldNames.contains(bitfield.name().get())) {
                         Iterator<Declaration> declIt = declarations.iterator();
                         while (declIt.hasNext()) {
@@ -317,7 +315,7 @@ private List<Declaration> collectBitfields(MemoryLayout layout, List<Declaration
                     }
                 }
                 if (!bfDecls.isEmpty()) {
-                    newDecls.add(Declaration.bitfields(bfDecls.get(0).pos(), "", contents.get(), bfDecls.toArray(new Declaration.Variable[0])));
+                    newDecls.add(Declaration.bitfields(bfDecls.get(0).pos(), "", contents, bfDecls.toArray(new Declaration.Variable[0])));
                 }
             }
         }

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/UnionLayoutComputer.java

@@ -33,6 +33,7 @@
 import jdk.internal.clang.Type;
 import jdk.internal.clang.TypeKind;
 
+import java.util.ArrayList;
 import java.util.List;
 
 /**
@@ -66,8 +67,7 @@ void startBitfield() {
     @Override
     MemoryLayout fieldLayout(Cursor c) {
         if (c.isBitField()) {
-            MemoryLayout layout = LayoutUtils.getLayout(c.type());
-            return bitfield((ValueLayout) layout, List.of(super.fieldLayout(c)));
+            return bitfield(List.of(super.fieldLayout(c)));
         } else {
             return super.fieldLayout(c);
         }
@@ -77,15 +77,19 @@ MemoryLayout fieldLayout(Cursor c) {
     long fieldSize(Cursor c) {
         if (c.type().kind() == TypeKind.IncompleteArray) {
             return 0;
+        } else if (c.isBitField()) {
+            return c.getBitFieldWidth();
+        } else {
+            return c.type().size() * 8;
         }
-        return c.type().size() * 8;
     }
 
     @Override
     MemoryLayout finishLayout() {
-        // size mismatch indicates anonymous bitfield used for padding
+        // size mismatch indicates use of bitfields in union
         long expectedSize = type.size() * 8;
         if (actualSize < expectedSize) {
+            // emit an extra padding of expected size to make sure union layout size is computed correctly
             addFieldLayout(MemoryLayout.ofPaddingBits(expectedSize));
         }
 

src/jdk.incubator.jextract/share/classes/jdk/internal/jextract/impl/Utils.java

@@ -26,28 +26,22 @@
 
 package jdk.internal.jextract.impl;
 
-import jdk.incubator.foreign.GroupLayout;
-import jdk.incubator.foreign.MemoryLayout;
 import jdk.internal.clang.Cursor;
 import jdk.internal.clang.CursorKind;
 import jdk.internal.clang.SourceLocation;
 import jdk.internal.clang.Type;
-import jdk.internal.clang.TypeKind;
 
 import javax.lang.model.SourceVersion;
 import javax.tools.JavaFileObject;
 import javax.tools.SimpleJavaFileObject;
 import java.io.IOException;
-import java.lang.reflect.Method;
 import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Optional;
-import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
 /**
@@ -322,13 +316,4 @@ static String quote(char ch) {
     private static boolean isPrintableAscii(char ch) {
         return ch >= ' ' && ch <= '~';
     }
-
-    static Optional<GroupLayout> getContents(MemoryLayout layout) {
-        return layout.attribute("contents").map(GroupLayout.class::cast);
-    }
-
-    @SuppressWarnings("unchecked")
-    static <Z extends MemoryLayout> Z addContents(Z layout, GroupLayout contents) {
-        return (Z) layout.withAttribute("contents", contents);
-    }
 }

test/jdk/tools/jextract/BadBitfieldTest.java

@@ -23,17 +23,13 @@
 
 /*
  * @test
- * @requires os.family != "windows"
  * @modules jdk.incubator.jextract
  * @library /test/lib
  * @build BadBitfieldTest
  * @run testng/othervm -Dforeign.restricted=permit BadBitfieldTest
  */
 
 /*
- * Not running on Windows since MSVC will not cross storage unit boundaries.
- * Resulting struct on SysV is 16 bytes, but 24 on MSx64
- *
  * MSVC: (/d1reportSingleClassLayoutFoo)
  * class Foo    size(24):
  *      +---
@@ -65,8 +61,6 @@ public class BadBitfieldTest extends JextractToolRunner {
     @Test
     public void testBadBitfield() {
         run("-d", getOutputFilePath("badBitfieldsGen").toString(),
-                getInputFilePath("badBitfields.h").toString())
-            .checkFailure()
-            .checkContainsOutput("Crossing storage unit boundaries");
+                getInputFilePath("badBitfields.h").toString()).checkSuccess();
     }
 }