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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
