<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
pre
{mso-style-priority:99;
mso-style-link:"HTML Preformatted Char";
margin:0in;
margin-bottom:.0001pt;
font-size:10.0pt;
font-family:"Courier New";}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
span.HTMLPreformattedChar
{mso-style-name:"HTML Preformatted Char";
mso-style-priority:99;
mso-style-link:"HTML Preformatted";
font-family:"Courier New";}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">David, we’ve seen different paths to the same assertion. The PR18430 problem seems to be occurring while trying to emit the virtual function; it exists in
LLVM 3.3 but not LLVM 3.2, and we traced it to r169489. The other one seems to occur while trying to emit a global variable, and still occurs back in LLVM 3.2. Your example seems to be the other one; it crashes with LLVM 3.2, where Gao’s/Katya’s does not.
So I’m suspicious that there are two root causes here. Here’s my test case for the other crash. Note that yours uses pragma pack and mine uses attribute packed, but neither of those are used in the example from PR18430.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">--paulr<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">class Base<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">{<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New""> int foo;<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">};<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">class Derived_1 : virtual Base<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">{<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New""> double a;<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New""> int xyz;<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">};<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">struct Derived_2 : Derived_1<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">{ } __attribute__((packed))<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Courier New"">x;<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<div style="border:none;border-left:solid blue 1.5pt;padding:0in 0in 0in 4.0pt">
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> cfe-commits-bounces@cs.uiuc.edu [mailto:cfe-commits-bounces@cs.uiuc.edu]
<b>On Behalf Of </b>David Majnemer<br>
<b>Sent:</b> Thursday, January 16, 2014 1:16 PM<br>
<b>To:</b> reviews+D2560+public+207a35c3ab6593f1@llvm-reviews.chandlerc.com<br>
<b>Cc:</b> cfe-commits@cs.uiuc.edu<br>
<b>Subject:</b> Re: [PATCH] Fixing a struct layout bug with bitfields before virtual base<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<p class="MsoNormal">I tried your patch with the following test case and it still crashes with the same assertion.<o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">$ cat different.cpp<o:p></o:p></p>
</div>
<div>
<div>
<p class="MsoNormal">struct ClassName0 {<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> char : 3;<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"> virtual void ClassName0Method() {}<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">};<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">struct ClassName7 : public ClassName0 {};<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">#pragma pack(push, 2)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">struct ClassName9 : public ClassName7, public virtual ClassName0 {};<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">#pragma pack(pop)<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">ClassName9 x;<o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">$ ~/llvm/build/bin/clang++ -cc1 -x c++ different.cpp -emit-llvm -S -o -<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">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.<o:p></o:p></p>
</div>
</div>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
<div>
<p class="MsoNormal">On Thu, Jan 16, 2014 at 12:17 PM, Yunzhong Gao <<a href="mailto:Yunzhong_Gao@playstation.sony.com" target="_blank">Yunzhong_Gao@playstation.sony.com</a>> wrote:<o:p></o:p></p>
<p class="MsoNormal" style="margin-bottom:12.0pt">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><o:p></o:p></p>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</div>
</body>
</html>