Non-POD unions with bit-fields causes an assertion failure at clang/lib/AST/RecordLayoutBuilder.cpp:754

[eastig]

Summary

The following program:

union A {

int f1: 3;
A();

};

A::A() {}

causes the failure of the assertion "DataSize % Context.getCharWidth() == 0" at clang/lib/AST/RecordLayoutBuilder.cpp:754.

It happens because '2.4 Non-POD Class Types/II. Allocation of Members Other Than Virtual Bases/1. bit-fileds' of C++ ABI (http://mentorembedded.github.io/cxx-abi/abi.html#class-types) is implemented incorrectly: dsize(C) is not updated to include the last byte containing (part of) the bitfield.

[majnemer]

I don't think there is much need to run the test for both x86-64 and aarch64, I'd just run the x86-64 one.

I think it's unneccesary to have two different files here, one file for both tests is sufficient. Just rename one of the A classes to B.

Please match the existing convention in test/Layout, rename union_regular_bit_field.cpp to include 'itanium' somewhere in there and please use dashes instead of underscores. Perhaps itanium-x86-union-bitfield.

lib/AST/RecordLayoutBuilder.cpp

1350–1351

Functions start with a lower case character.

I would just call this roundUpSizeToCharAlignment. This name, while more generic, is less confusing if we chose to use it in other places of record layout.

const uint64_t looks abnormal compared to most of clang's code, I would just go with uint64_t here.

test/Layout/union_regular_bit_field.cpp

1–4 ↗

(On Diff #14870)

These triples are not specific enough. Make sure you specify the itanium ABI somehow.

13–14 ↗

(On Diff #14870)

Please include more CHECK lines so that more tests can easily be added to this file.

test/Layout/union_wide_bit_field.cpp

1–4 ↗

(On Diff #14870)

Likewise.

[ABataev]

test/Layout/union_regular_bit_field.cpp

1–4 ↗

(On Diff #14870)

You can use %itanium_abi_triple as a triple in tests

[eastig]

Updated the patch according to comments.

[majnemer]

LGTM

[eastig]

Committed to revisions: 220031, 220032

lib/AST/RecordLayoutBuilder.cpp

     LayoutField(Field);
 }
 
+// Rounds the specified size to have it a multiple of the char size.
+static uint64_t
+roundUpSizeToCharAlignment(uint64_t Size,
+                           const ASTContext &Context) {
+  uint64_t CharAlignment = Context.getTargetInfo().getCharAlign();
+  return llvm::RoundUpToAlignment(Size, CharAlignment);
+}
+
 void RecordLayoutBuilder::LayoutWideBitField(uint64_t FieldSize,
                                              uint64_t TypeSize,
                                              bool FieldPacked,

   uint64_t UnpaddedFieldOffset = getDataSizeInBits() - UnfilledBitsInLastUnit;
 
   if (IsUnion) {
-    setDataSize(std::max(getDataSizeInBits(), FieldSize));
+    uint64_t RoundedFieldSize = roundUpSizeToCharAlignment(FieldSize,
+                                                           Context);
+    setDataSize(std::max(getDataSizeInBits(), RoundedFieldSize));
     FieldOffset = 0;
   } else {
     // The bitfield is allocated starting at the next offset aligned 

 
   // For unions, this is just a max operation, as usual.
   if (IsUnion) {
-    // FIXME: I think FieldSize should be TypeSize here.
-    setDataSize(std::max(getDataSizeInBits(), FieldSize));
-
+    uint64_t RoundedFieldSize = roundUpSizeToCharAlignment(FieldSize,
+                                                           Context);
+    setDataSize(std::max(getDataSizeInBits(), RoundedFieldSize));
   // For non-zero-width bitfields in ms_struct structs, allocate a new
   // storage unit if necessary.
   } else if (IsMsStruct && FieldSize) {

test/Layout/itanium-union-bitfield.cpp

+// RUN: %clang_cc1 -emit-llvm-only -triple %itanium_abi_triple -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+
+union A {
+  int f1: 3;
+  A();
+};
+
+A::A() {}
+
+union B {
+  int f1: 69;
+  B();
+};
+
+B::B() {}
+
+// CHECK:*** Dumping AST Record Layout
+// CHECK-NEXT:   0 | union A
+// CHECK-NEXT:   0 |   int f1
+// CHECK-NEXT:     | [sizeof=4, dsize=1, align=4
+// CHECK-NEXT:     |  nvsize=1, nvalign=4]
+
+// CHECK:*** Dumping AST Record Layout
+// CHECK-NEXT:   0 | union B
+// CHECK-NEXT:   0 |   int f1
+// CHECK-NEXT:     | [sizeof=16, dsize=9, align=8
+// CHECK-NEXT:     |  nvsize=9, nvalign=8]
+