[Clang][Codegen] Truncate initializers of union bitfield members

[tmatheson]

Summary

If an initial value is given for a bitfield that does not fit in the
bitfield, the value should be truncated. Constant folding for
expressions did not account for this truncation in the case of union
member functions, despite a warning being emitted. In some contexts,
evaluation of expressions was not enabled unless C++11, ROPI or RWPI
was enabled.

[tmatheson]

Rebased

[tmatheson]

Ping

[joechrisellis]

clang/lib/AST/ExprConstant.cpp

9803–9804

nit: I would prefer this:

if (Field->isBitField() && truncateBitfieldValue(Info, InitExpr, Result.getUnionValue(), Field))
    return true;

return EvaluateInPlace(Result.getUnionValue(), Info, Subobject, InitExpr);

It feels more in-line with the rest of the function. But it is okay if you want to ignore this too. 😄

clang/test/CodeGenCXX/bitfield-layout.cpp

89–96

I'd like to see some more tests that check the truncation behaviour. My understanding is that this is trucating to -1 because of two's complement? How about something like:

int test_trunc() {
    union {
        int i : 4;
    } U = {80};
    return U.i;
    // CHECK: ret i32 0
}

Am I understanding the behaviour correctly?

Some comments about what is actually happening on the bit-level to get this result would also be nice.

[rsmith]

clang/lib/AST/ExprConstant.cpp

9801–9809

Shouldn't this be && not ||? These functions return true if they succeed (unlike the convention in Sema where true means an error diagnostic was produced).

[tmatheson]

Added more test cases and explanatory comments

[tmatheson]

Add test RUN line that checks C++11 behaviour

[tmatheson]

Make unions in test cases const, and clarify logic

[tmatheson]

clang/lib/AST/ExprConstant.cpp

9801–9809

You are correct. I have updated the logic to be simpler to follow as well.

clang/test/CodeGenCXX/bitfield-layout.cpp

89–96

Yes that is correct. I have added some new test cases and comments to make it clear what they are testing for.

[tmatheson]

Ping

[pratlucas]

The truncate conditions look a lot better and the test coverage seems reasonable now.
LGTM.

clang/lib/AST/ExprConstant.cpp

if (RD->isUnion()) {
     LValue Subobject = This;
     if (!HandleLValueMember(Info, InitExpr, Subobject, Field, &Layout))
       return false;
 
     // Temporarily override This, in case there's a CXXDefaultInitExpr in here.
     ThisOverrideRAII ThisOverride(*Info.CurrentCall, &This,
                                   isa<CXXDefaultInitExpr>(InitExpr));
 
-    return EvaluateInPlace(Result.getUnionValue(), Info, Subobject, InitExpr);
+    if (EvaluateInPlace(Result.getUnionValue(), Info, Subobject, InitExpr)) {
+      if (Field->isBitField())
+        return truncateBitfieldValue(Info, InitExpr, Result.getUnionValue(),
+                                     Field);
+      return true;
+    }
+
+    return false;
   }
 
   if (!Result.hasValue())
     Result = APValue(APValue::UninitStruct(), CXXRD ? CXXRD->getNumBases() : 0,
                      std::distance(RD->field_begin(), RD->field_end()));
   unsigned ElementNo = 0;
   bool Success = true;
 
   // Initialize base classes.

clang/test/CodeGenCXX/bitfield-layout.cpp

 // RUN: %clang_cc1 %s -triple=x86_64-apple-darwin10 -emit-llvm -o - -O3 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 %s
 // RUN: %clang_cc1 %s -triple=i386-apple-darwin10 -emit-llvm -o - -O3 | FileCheck %s
 // RUN: %clang_cc1 %s -triple=aarch64_be-none-eabi -emit-llvm -o - -O3 | FileCheck %s
 // RUN: %clang_cc1 %s -triple=thumbv7_be-none-eabi -emit-llvm -o - -O3 | FileCheck %s
+// RUN: %clang_cc1 %s -triple=x86_64-unknown-unknown -emit-llvm -o - -O3 -std=c++11 | FileCheck -check-prefix=CHECK -check-prefix=CHECK-LP64 %s
 
 // CHECK-LP64: %union.Test1 = type { i32, [4 x i8] }
 union Test1 {
   int a;
   int b: 39;
 };
 Test1 t1;
 
int test_init() {
 
   CHECK(u1.a == 1);
   CHECK(u1.b == 1);
   CHECK(u2.val == 42);
 
   // CHECK: ret i32 0
   return 0;
 }
+
+extern "C" {
+int test_trunc_int() {
+  union {
+    int i : 4; // truncated to 0b1111 == -1
+  } const U = {15};  // 0b00001111
+  return U.i;
+}
+// CHECK: define dso_local i32 @test_trunc_int()
+// CHECK: ret i32 -1
+
+int test_trunc_three_bits() {
+  union {
+    int i : 3; // truncated to 0b111 == -1
+  } const U = {15};  // 0b00001111
+  return U.i;
+}
+// CHECK: define dso_local i32 @test_trunc_three_bits()
+// CHECK: ret i32 -1
+
+int test_trunc_1() {
+  union {
+    int i : 1; // truncated to 0b1 == -1
+  } const U = {15};  // 0b00001111
+  return U.i;
+}
+// CHECK: define dso_local i32 @test_trunc_1()
+// CHECK: ret i32 -1
+
+int test_trunc_zero() {
+  union {
+    int i : 4; // truncated to 0b0000 == 0
+  } const U = {80};  // 0b01010000
+  return U.i;
+}
+// CHECK: define dso_local i32 @test_trunc_zero()
+// CHECK: ret i32 0
+
+int test_constexpr() {
+  union {
+    int i : 3;           // truncated to 0b111 == -1
+  } const U = {1 + 2 + 4 + 8}; // 0b00001111
+  return U.i;
+}
+// CHECK: define dso_local i32 @test_constexpr()
+// CHECK: ret i32 -1
+
+int test_notrunc() {
+  union {
+    int i : 12;          // not truncated
+  } const U = {1 + 2 + 4 + 8}; // 0b00001111
+  return U.i;
+}
+// CHECK: define dso_local i32 @test_notrunc()
+// CHECK: ret i32 15
+
+long long test_trunc_long_long() {
+  union {
+    long long i : 14; // truncated to 0b00111101001101 ==
+  } const U = {0b0100111101001101};
+  return U.i;
+}
+// CHECK: define dso_local i64 @test_trunc_long_long()
+// CHECK: ret i64 3917
+}