<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Carl,<div><br><div><div>On Jan 27, 2011, at 12:02 PM, Carl Norum wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><br>Hi folks,<br><br>I've attached a patch for review; these are the changes we use to build EFI for Mac boot ROMs using clang/llvm on Mac OS X. The simplest explanation is that it's a partial implementation of gcc's "-mms-bitfields" flag for clang.<br><br>Let me know what you think!<br></div></blockquote><div><br></div><div>A couple of comments...</div><div><br></div><div><div>Index: test/Misc/mms-bitfields.c</div><div>===================================================================</div><div>--- test/Misc/mms-bitfields.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>+++ test/Misc/mms-bitfields.c<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>@@ -0,0 +1,9 @@</div><div>+// RUN: %clang -arch i386 -mms-bitfields -emit-llvm -c %s -o %t</div><div>+// RUN: llvm-dis %t -o - | FileCheck %s</div><div>+</div><div>+struct s {</div><div>+ int f32;</div><div>+ long long f64;</div><div>+} s;</div><div>+</div><div>+// CHECK: @s = common global %struct.s zeroinitializer, align 8</div><div><br></div><div>Instead of using %clang and llvm-dis in the RUN line, it'd be far better to use </div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>%clang_cc1 -triple <triple that makes sense for -mms-bitfields> -emit-llvm -o - </div><div><br></div><div>and pipe the results to FileCheck. The important part is the use of %clang_cc1 with -triple, so the test is completely independent of the host environment.</div><div><br></div><div>This test seems like it's not really testing bit-field layout at all...</div><div><br></div><div><div>Index: include/clang/Basic/LangOptions.h</div><div>===================================================================</div><div>--- include/clang/Basic/LangOptions.h<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 124313)</div><div>+++ include/clang/Basic/LangOptions.h<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -56,6 +56,7 @@</div><div> unsigned SjLjExceptions : 1; // Use setjmp-longjump exception handling.</div><div> unsigned RTTI : 1; // Support RTTI information.</div><div> </div><div>+ unsigned MSBitfields : 1; // MS-compatible structure layout</div><div> unsigned NeXTRuntime : 1; // Use NeXT runtime.</div><div> unsigned Freestanding : 1; // Freestanding implementation</div><div> unsigned NoBuiltin : 1; // Do not use builtin functions (-fno-builtin)</div><div>@@ -163,6 +164,7 @@</div><div> C99 = Microsoft = Borland = CPlusPlus = CPlusPlus0x = 0;</div><div> CXXOperatorNames = PascalStrings = WritableStrings = ConstStrings = 0;</div><div> Exceptions = SjLjExceptions = Freestanding = NoBuiltin = 0;</div><div>+ MSBitfields = 0;</div><div> NeXTRuntime = 1;</div><div> RTTI = 1;</div><div> LaxVectorConversions = 1;</div></div><div><br></div><div>Please update the AST reader and writer, so that the MSBitfields bit is properly saved/loaded/checked.</div><div><br></div><div><div>Index: lib/Basic/Targets.cpp</div><div>===================================================================</div><div>--- lib/Basic/Targets.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 124313)</div><div>+++ lib/Basic/Targets.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -2675,7 +2675,10 @@</div><div> case llvm::Triple::MinGW64:</div><div> return new MinGWX86_64TargetInfo(T);</div><div> case llvm::Triple::Win32: // This is what Triple.h supports now.</div><div>- return new VisualStudioWindowsX86_64TargetInfo(T);</div><div>+ if (Triple.getEnvironment() == llvm::Triple::MachO)</div><div>+ return new DarwinX86_64TargetInfo(T);</div><div>+ else</div><div>+ return new VisualStudioWindowsX86_64TargetInfo(T);</div><div> default:</div><div> return new X86_64TargetInfo(T);</div><div> }</div></div><div><br></div><div>This change should be committed separately.</div><div><br></div><div><div>Index: lib/AST/RecordLayoutBuilder.cpp</div><div>===================================================================</div><div>--- lib/AST/RecordLayoutBuilder.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 124313)</div><div>+++ lib/AST/RecordLayoutBuilder.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -1401,6 +1401,25 @@</div><div> std::pair<uint64_t, unsigned> FieldInfo = Context.getTypeInfo(D->getType());</div><div> FieldSize = FieldInfo.first;</div><div> FieldAlign = FieldInfo.second;</div><div>+</div><div>+ if (Context.getLangOptions().MSBitfields) {</div><div>+ // If MS bitfield layout is required, figure out what type is being</div><div>+ // laid out and align the field to the width of that type.</div><div>+ const Type *T = D->getType().getTypePtr();</div><div>+ Type::TypeClass TC = T->getTypeClass();</div><div>+</div><div>+ // Resolve all typedefs down to their base type</div><div>+ while (TC == Type::Typedef) {</div><div>+ const TypedefDecl *Typedef = cast<TypedefType>(T)->getDecl();</div><div>+ T = Typedef->getUnderlyingType().getTypePtr();</div><div>+ TC = T->getTypeClass();</div><div>+ }</div><div>+</div><div>+ // Round up the field alignment if necessary.</div><div>+ if ((TC == Type::Builtin) && (FieldSize > FieldAlign)) {</div><div>+ FieldAlign = FieldSize;</div><div>+ }</div><div>+ }</div><div> }</div><div><br></div></div><div>It's best not to try to walk through typedefs manually; Type's getAs member function template will do that for you. You can collapse most of that logic down to</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>if (const BuiltinType *BT = D->getType()->getAs<BuiltinType>())</div><div><span class="Apple-tab-span" style="white-space:pre"> </span> if (FieldSize > FieldAlign)</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>FieldAlign = FieldSize;</div><div><br></div><div>However, I think you also want to look through array types, first? ASTContext has a routine to do that.</div><div><br></div><div>Thanks for working on this! Do we have a good sense of what else is needed for -mms-bitfields? Documentation on this feature seems a bit scarce.</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div></div></div></div></body></html>