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

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

13–14

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

Likewise.

[ABataev]

test/Layout/union_regular_bit_field.cpp

1–4

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 field up to have it multiple of the char size.
+static uint64_t
+RoundUpFieldSizeToCharAlignment(const uint64_t FieldSize,
+                                const ASTContext &Context) {
+  uint64_t CharAlignment = Context.getTargetInfo().getCharAlign();
+  return llvm::RoundUpToAlignment(FieldSize, 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 = RoundUpFieldSizeToCharAlignment(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 = RoundUpFieldSizeToCharAlignment(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/union_regular_bit_field.cpp

+// RUN: %clang_cc1 -emit-llvm-only -triple aarch64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+
+union A {
+  int f1: 3;
+  A();
+};
+
+A::A() {}
+
+// CHECK:      | [sizeof=4, dsize=1, align=4
+// CHECK-NEXT:     |  nvsize=1, nvalign=4]
+

test/Layout/union_wide_bit_field.cpp

+// RUN: %clang_cc1 -emit-llvm-only -triple aarch64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64 -fdump-record-layouts %s 2>/dev/null \
+// RUN:            | FileCheck %s
+
+union A {
+  int f1: 69;
+  A();
+};
+
+A::A() {}
+
+// CHECK:      | [sizeof=16, dsize=9, align=8
+// CHECK-NEXT:     |  nvsize=9, nvalign=8]
+