[cfe-commits] Building EFI with Clang on Mac OS X

[Carl Norum]

On Jan 31, 2011, at 11:20 AM, Douglas Gregor wrote:
> A couple of comments...

Hi Doug,

Thanks for going over these patches for me.  I've attached two new ones, split up as you suggested.  My comments follow:

> Instead of using %clang and llvm-dis in the RUN line, it'd be far better to use 
> 
> 	%clang_cc1 -triple <triple that makes sense for -mms-bitfields> -emit-llvm -o - 
> 
> 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.

Fixed!  When cc1 wouldn't take '-arch i386', I couldn't figure out what to do.  Passing a complete triple as in your example works great.

> This test seems like it's not really testing bit-field layout at all...

Nope - as I mentioned in my original patch review-request message, all I've done is a partial implementation of gcc's -mms-bitfields.  Just enough to get EFI working (we don't have many (any?) actual bitfields in the source base).

> Please update the AST reader and writer, so that the MSBitfields bit is properly saved/loaded/checked.

Done!

>> Index: lib/Basic/Targets.cpp
> 
> This change should be committed separately.

Attached as 'macho.patch'.  This change is actually more related to the patches I submitted for LLVM last week.

>> Index: lib/AST/RecordLayoutBuilder.cpp
> 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
> 
> 	if (const BuiltinType *BT = D->getType()->getAs<BuiltinType>())
> 	  if (FieldSize > FieldAlign)
> 		FieldAlign = FieldSize;
> 
> However, I think you also want to look through array types, first? ASTContext has a routine to do that.

That seems to work out just fine.  I've updated the test to have an array case as well.

> 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.

In our case, we only *really* need the alignment of 64-bit types on 64-bit boundaries, even for 32-bit code.  I was sent this link as a reference:

	http://lists.fedoraproject.org/pipermail/mingw/2008-November/000083.html

But as you noticed, these patches only deal with item #2 from that list.  #1 is required by the C standard, I think, so clang was already doing that, and item #3 and the zero-length bitfields handling I didn't investigate at all.

-- Carl