[cfe-commits] Building EFI with Clang on Mac OS X
Carl Norum
carl.norum at apple.com
Mon Jan 31 14:12:56 PST 2011
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
More information about the cfe-commits
mailing list