<div dir="ltr">I tried your patch with the following test case and it still crashes with the same assertion.<div><br></div><div>$ cat different.cpp</div><div><div>struct ClassName0 {</div><div> char : 3;</div><div> virtual void ClassName0Method() {}</div>
<div>};</div><div>struct ClassName7 : public ClassName0 {};</div><div>#pragma pack(push, 2)</div><div>struct ClassName9 : public ClassName7, public virtual ClassName0 {};</div><div>#pragma pack(pop)</div><div>ClassName9 x;</div>
</div><div><br></div><div>$ ~/llvm/build/bin/clang++ -cc1 -x c++ different.cpp -emit-llvm -S -o -<br></div><div>clang++: ~/llvm/src/tools/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp:801: bool <anonymous namespace>::CGRecordLayoutBuilder::ComputeNonVirtualBaseType(const clang::CXXRecordDecl *): Assertion `!Packed && "cannot layout even as packed struct"' failed.<br>
</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 16, 2014 at 12:17 PM, Yunzhong Gao <span dir="ltr"><<a href="mailto:Yunzhong_Gao@playstation.sony.com" target="_blank">Yunzhong_Gao@playstation.sony.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">ygao added you to the CC list for the revision "Fixing a struct layout bug with bitfields before virtual base".<br>
<br>
Hi,<br>
The following patch fixes PR18430 by checking that the size of bitfields plus padding<br>
does not grow into the following virtual base. Without this fix, clang would assert on<br>
this test file:<br>
<br>
```<br>
/* test.cpp<br>
* clang++ -S -o - test.cpp<br>
*/<br>
struct A { virtual void vf2(); };<br>
<br>
struct B { virtual void vf1(); };<br>
<br>
struct C : A, virtual B {<br>
char fld0;<br>
long long fld1 : 48;<br>
virtual void vf1();<br>
};<br>
<br>
void C::vf1() { }<br>
/* end of test.cpp */<br>
```<br>
<br>
Codes in CGRecordLayoutBuilder.cpp are trying to grow fld1 from 48 bits to 64 bits. fld1 starts<br>
at 8 bits from the beginning of the struct and the virtual base starts at 64-bits, so if fld1 is<br>
expanded to take 64 bits, it would overlap with the storage space reserved for the virtual base.<br>
The proposed patch is attempting to check that the bitfields do not grow into the virtual base.<br>
<br>
I would also like to bring up a related discussion on some codes in the same function. It seems to<br>
me that growing the bitfields is intended for optimization rather than for correctness (?) If that is the<br>
case, growing fld1 from 48 bits to 64 bits seems a little odd because loading that 64 bits would be<br>
an unaligned data access on many targets, and my impression is that unaligned data access is<br>
slow. Maybe it makes more sense to grow fld1 from 48 bits to 56 bits only, so that fld1 will end at<br>
a 64-bit boundary. For example, with the current implementation, clang would generate a 64-bit load<br>
at -O2 on my x86-64 Ubuntu with some masks to get the second bitfield, although it appears that a<br>
32-bit load alone should suffice.<br>
<br>
```<br>
struct __attribute__((aligned(16))) s7 {<br>
char f0;<br>
long long f1:24;<br>
long long f2:32;<br>
};<br>
<br>
int foo(struct s7 *a0) { return a0->f2; }<br>
```<br>
<br>
I have a patch in mind that would do something like this:<br>
<br>
```<br>
Index: lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
===================================================================<br>
--- lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
+++ lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
@@ -353,8 +353,9 @@<br>
"End offset is not past the end of the known storage bits.");<br>
uint64_t SpaceBits = EndOffset - FirstFieldOffset;<br>
uint64_t LongBits = Types.getTarget().getLongWidth();<br>
- uint64_t WidenedBits = (StorageBits / LongBits) * LongBits +<br>
- llvm::NextPowerOf2(StorageBits % LongBits - 1);<br>
+ uint64_t WidenedBits = ((FirstFieldOffset + StorageBits) / LongBits) * LongBits +<br>
+ llvm::NextPowerOf2((FirstFieldOffset + StorageBits) % LongBits - 1) -<br>
+ FirstFieldOffset;<br>
assert(WidenedBits >= StorageBits && "Widening shrunk the bits!");<br>
if (WidenedBits <= SpaceBits) {<br>
StorageBits = WidenedBits;<br>
```<br>
<br>
This would change the backend code generation for some struct layouts, so I would like to<br>
have some discussion on whether this is beneficial.<br>
<br>
Could someone take a look?<br>
<br>
- Gao<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D2560" target="_blank">http://llvm-reviews.chandlerc.com/D2560</a><br>
<br>
Files:<br>
lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
test/CodeGenCXX/bitfield.cpp<br>
<br>
Index: lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
===================================================================<br>
--- lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
+++ lib/CodeGen/CGRecordLayoutBuilder.cpp<br>
@@ -345,6 +345,9 @@<br>
// these in conjunction with shifts and masks to narrower operations where<br>
// beneficial.<br>
uint64_t EndOffset = Types.getContext().toBits(Layout.getDataSize());<br>
+ if (Types.getContext().getLangOpts().CPlusPlus)<br>
+ // Do not grow the bitfield storage into the following virtual base.<br>
+ EndOffset = Types.getContext().toBits(Layout.getNonVirtualSize());<br>
if (BFE != FE)<br>
// If there are more fields to be laid out, the offset at the end of the<br>
// bitfield is the offset of the next field in the record.<br>
Index: test/CodeGenCXX/bitfield.cpp<br>
===================================================================<br>
--- test/CodeGenCXX/bitfield.cpp<br>
+++ test/CodeGenCXX/bitfield.cpp<br>
@@ -426,3 +426,55 @@<br>
s->b2 = x;<br>
}<br>
}<br>
+<br>
+namespace N7 {<br>
+ // Similar to N4 except that this adds a virtual base to the picture.<br>
+ // Do NOT widen loads and stores to bitfields into padding at the end of<br>
+ // a class which might end up with members inside of it when inside a derived<br>
+ // class.<br>
+ struct B1 {<br>
+ virtual void f();<br>
+ unsigned b1 : 24;<br>
+ };<br>
+ struct B2 : virtual B1 {<br>
+ virtual ~B2();<br>
+ unsigned b : 24;<br>
+ };<br>
+ // Imagine some other translation unit introduces:<br>
+#if 0<br>
+ struct Derived : public B2 {<br>
+ char c;<br>
+ };<br>
+#endif<br>
+ unsigned read(B2* s) {<br>
+ // FIXME: We should widen this load as long as the function isn't being<br>
+ // instrumented by thread-sanitizer.<br>
+ //<br>
+ // CHECK-X86-64-LABEL: define i32 @_ZN2N74read<br>
+ // CHECK-X86-64: %[[gep:.*]] = getelementptr inbounds {{.*}}* %{{.*}}, i32 0, i32 1<br>
+ // CHECK-X86-64: %[[ptr:.*]] = bitcast [3 x i8]* %[[gep]] to i24*<br>
+ // CHECK-X86-64: %[[val:.*]] = load i24* %[[ptr]]<br>
+ // CHECK-X86-64: %[[ext:.*]] = zext i24 %[[val]] to i32<br>
+ // CHECK-X86-64: ret i32 %[[ext]]<br>
+ // CHECK-PPC64-LABEL: define zeroext i32 @_ZN2N74read<br>
+ // CHECK-PPC64: %[[gep:.*]] = getelementptr inbounds {{.*}}* %{{.*}}, i32 0, i32 1<br>
+ // CHECK-PPC64: %[[ptr:.*]] = bitcast [3 x i8]* %[[gep]] to i24*<br>
+ // CHECK-PPC64: %[[val:.*]] = load i24* %[[ptr]]<br>
+ // CHECK-PPC64: %[[ext:.*]] = zext i24 %[[val]] to i32<br>
+ // CHECK-PPC64: ret i32 %[[ext]]<br>
+ return s->b;<br>
+ }<br>
+ void write(B2* s, unsigned x) {<br>
+ // CHECK-X86-64-LABEL: define void @_ZN2N75write<br>
+ // CHECK-X86-64: %[[gep:.*]] = getelementptr inbounds {{.*}}* %{{.*}}, i32 0, i32 1<br>
+ // CHECK-X86-64: %[[ptr:.*]] = bitcast [3 x i8]* %[[gep]] to i24*<br>
+ // CHECK-X86-64: %[[new:.*]] = trunc i32 %{{.*}} to i24<br>
+ // CHECK-X86-64: store i24 %[[new]], i24* %[[ptr]]<br>
+ // CHECK-PPC64-LABEL: define void @_ZN2N75write<br>
+ // CHECK-PPC64: %[[gep:.*]] = getelementptr inbounds {{.*}}* %{{.*}}, i32 0, i32 1<br>
+ // CHECK-PPC64: %[[ptr:.*]] = bitcast [3 x i8]* %[[gep]] to i24*<br>
+ // CHECK-PPC64: %[[new:.*]] = trunc i32 %{{.*}} to i24<br>
+ // CHECK-PPC64: store i24 %[[new]], i24* %[[ptr]]<br>
+ s->b = x;<br>
+ }<br>
+}<br>
<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>