[cfe-dev] Implementation of MSRecordLayoutBuilder.
r4start
r4start at gmail.com
Tue Sep 20 01:17:59 PDT 2011
On 20/09/2011 00:06, Eli Friedman wrote:
> On Mon, Sep 19, 2011 at 3:30 AM, r4start<r4start at gmail.com> wrote:
>> On 15/09/2011 19:35, r4start wrote:
>>
>> On 15/09/2011 16:17, Don Williamson wrote:
>>
>> That sounds great!
>> ________________________________
>> From: r4start<r4start at gmail.com>
>> To: Don Williamson<don.williamson at yahoo.com>
>> Cc: cfe-dev at cs.uiuc.edu
>> Sent: Thursday, September 15, 2011 12:35 PM
>> Subject: Re: [cfe-dev] Implementation of MSRecordLayoutBuilder.
>>
>> On 15/09/2011 15:14, Don Williamson wrote:
>>
>> Sure, I started a previous thread about this:
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-September/016941.html
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-September/016942.html
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-September/016944.html
>> http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-September/016970.html
>>
>> It boils down to two problems I've had:
>> * If there is a double anywhere in a struct, MSVC pads the vftbl pointer.
>> * The IsPODForThePurposeOfLayout check is invalid for MS targets and so you
>> get a different layout depending upon whether your type has a struct or not.
>> I suggested a simple patch in my last post that fixes these. My test cases
>> so far are here:
>> https://bitbucket.org/dwilliamson/clreflect/src/tip/src/clReflectTest/TestOffsets.cpp
>>
>> my patch works for all of them but unfortunately I haven't had time to sync
>> to the lastest build of clang so I'm not in a position to submit a patch.
>> It would be nice if we could figure something out that solves the biggest
>> number of cases :)
>> Cheers,
>> - Don
>> ________________________________
>> From: r4start<r4start at gmail.com>
>> To: Don Williamson<don.williamson at yahoo.com>
>> Cc: cfe-dev at cs.uiuc.edu
>> Sent: Thursday, September 15, 2011 12:06 PM
>> Subject: Re: [cfe-dev] Implementation of MSRecordLayoutBuilder.
>>
>> On 15/09/2011 14:52, Don Williamson wrote:
>>
>> Hi Dmitry,
>> I'm really interested in the original test cases you have that failed with
>> the original implementation of the record layout builder - could you share
>> them?
>> How does your current patch work with the following test:
>> #pragma pack(push, 8)
>> class B {
>> public:
>> virtual void b(){}
>> int a;
>> double b;
>> };
>> #pragma pack(pop)
>> Have you also tried test cases with and without constructors?
>> Thanks,
>> - Don
>> ________________________________
>> From: r4start<r4start at gmail.com>
>> To: John McCall<rjmccall at apple.com>
>> Cc: cfe-dev at cs.uiuc.edu
>> Sent: Thursday, September 15, 2011 11:20 AM
>> Subject: Re: [cfe-dev] Implementation of MSRecordLayoutBuilder.
>>
>> On 15/09/2011 00:39, r4start wrote:
>>> On 15/09/2011 00:36, John McCall wrote:
>>>> On Sep 14, 2011, at 1:33 PM, r4start wrote:
>>>>
>>>>> On 15/09/2011 00:24, John McCall wrote:
>>>>>> On Sep 14, 2011, at 1:23 PM, r4start wrote:
>>>>>>> On 14/09/2011 21:53, John McCall wrote:
>>>>>>>> On Sep 14, 2011, at 4:05 AM, r4start wrote:
>>>>>>>>
>>>>>>>>> On 14/09/2011 00:42, r4start wrote:
>>>>>>>>>> On 14/09/2011 00:37, Eli Friedman wrote:
>>>>>>>>>>> On Mon, Sep 12, 2011 at 11:45 PM, r4start<r4start at gmail.com>
>>>>>>>>>>> wrote:
>>>>>>>>>>>> On 12/09/2011 21:57, r4start wrote:
>>>>>>>>>>>>> On 12/09/2011 21:37, Eli Friedman wrote:
>>>>>>>>>>>>>> On Mon, Sep 12, 2011 at 8:56 AM, r4start<r4start at gmail.com>
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>> I have some prototype code for MSRecordLayoutBuilder. I test
>>>>>>>>>>>>>>> this code
>>>>>>>>>>>>>>> with MSVS 2010 and \Zp8 compiler option. On simple examples it
>>>>>>>>>>>>>>> seems to
>>>>>>>>>>>>>>> work properly.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Is it interesting to the clang project?
>>>>>>>>>>>>>> We certainly want to fix any cases where clang does not do
>>>>>>>>>>>>>> struct
>>>>>>>>>>>>>> layout consistently with MSVC on Windows. It's hard to comment
>>>>>>>>>>>>>> on
>>>>>>>>>>>>>> your patch without seeing it.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> -Eli
>>>>>>>>>>>>> Now I'm not at work. When I come to work I'll send a patch.
>>>>>>>>>>>>>
>>>>>>>>>>>>> - Dmitry
>>>>>>>>>>>> Here is patch for MSRecordLayoutBuilder.
>>>>>>>>>>> I think it would be better to integrate the checks into the main
>>>>>>>>>>> RecordLayoutBuilder class rather than subclassing it.
>>>>>>>>>>>
>>>>>>>>>>> Please include tests in your patch. (See test/CodeGen/ms_struct.c
>>>>>>>>>>> for
>>>>>>>>>>> an example.)
>>>>>>>>>>>
>>>>>>>>>>> I would like someone more familiar with MSVC to review the actual
>>>>>>>>>>> logic here (ping me if nobody does within a few days, though, and
>>>>>>>>>>> I'll
>>>>>>>>>>> try to review anyway).
>>>>>>>>>>>
>>>>>>>>>>> -Eli
>>>>>>>>>> Ok, thanks for hint.
>>>>>>>>>>
>>>>>>>>>> -Dmitry.
>>>>>>>>> I have a problem when writing a test. Clang doesn't implement
>>>>>>>>> Microsoft
>>>>>>>>> ABI and he crashes at code generation stage.
>>>>>>>>> Clang have dump-record-layouts option but he prints AST layouts and
>>>>>>>>> CodeGen layouts.
>>>>>>>>>
>>>>>>>>> Is there a way to save only AST record layout to file and then check
>>>>>>>>> or maybe my patch must include CGRecordLayoutBuilder for Microsoft?
>>>>>>>> It would be easy enough to add a new testing switch if you'd like to
>>>>>>>> get this in.
>>>>>>> Can you give me some hint how can I do this?
>>>>>> Follow the code for -dump-record-layouts.
>>>>> You propose to duplicate the output to a file and then run FileCheck to
>>>>> check output, am I right?
>>>> It would probably be better to run FileCheck directly on the output, but
>>>> I think you get the general idea.
>>>>
>>>> Whenever you get CGRecordLayoutBuilder working adequately, you can rip
>>>> out the new option and just use -dump-record-layouts.
>>>>
>>>> John.
>>> Ok, thanks. Tomorrow I'll do it.
>>>
>> Here is a test and patch.
>>
>> - Dmitry.
>>
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>> Hi,
>> what I get from MSVS output :
>>
>> class B size(24):
>> 1> +---
>> 1> 0 | {vfptr}
>> 1> 8 | a
>> 1> |<alignment member> (size=4)
>> 1> 16 | c
>> 1> +---
>>
>> And this I get from clang with my patch:
>> 0 | class B
>> 0 | (B vtable pointer)
>> 4 | int a
>> 8 | double c
>> sizeof=16, dsize=16, align=8
>> nvsize=16, nvalign=8
>>
>> I'll try to fix this.
>>
>> What do you mean when you say "Have you also tried test cases with and
>> without constructors?", can you give me some examples and I check them.
>>
>> - Dmitry.
>>
>>
>> Thanks for examples. I'll add your patch to my code.
>> I think it will be great if we will have full support for Microsoft layout.
>>
>> - Dmitry.
>>
>>
>> Now it seems working fine.
>>
>> Hi Eli,
>> can you review my patch?
> Sure.
>
> In general, do not leave around code commented out with /**/ in
> patches. Also, please include any tests which should be committed in
> the diff itself (using svn add).
I`ve added test into test/AST, is that ok?
>
> +#ifndef NDEBUG
> + // Check that we have base offsets for all bases.
>
> Please don't copy-paste code; refactor instead. (This file actually
> could use a bunch more refactoring; a separate patch for that would be
> appreciated.)
Yes, I agree that the code isn't perfect. But this code is the first
step toward a Microsoft ABI :)
> +// r4start
> +// I don`t think that this approach is a good.
> +static bool AllBasesVirtual(const CXXRecordDecl* RD) {
> + for (CXXRecordDecl::base_class_const_iterator I = RD->bases_begin(),
> + E = RD->bases_end(); I != E; ++I) {
> + if (!I->isVirtual()) {
> + return false;
> + }
> + }
> + return true;
> +}
>
> I don't see anything particularly wrong with this... except I would
> make the method HasVBPtr and pull in the RD->getNumBases() check.
Done.
> Either way, please make the comment "I don't think that this approach
> is a good" more descriptive, and mark it with FIXME. On a side note,
> please try to avoid non-ASCII characters in comments.
>
> + bool HasVbptr = Layout.getVBPtrOffset() != CharUnits::fromQuantity(-1);
>
> What is the difference between this HasVbptr and HasVirtualBasePointer()?
>
> + /// HasVirtualBasePointer - Flag indicates that class has virtual base table.
> + /// This flag needs only for build MS layout.
> + bool IsVBPtrPersist;
Yes, this is unnecessary flag.
> Why is this member called IsVBPtrPersist?
>
> -Eli
Thank you for your review.
- Dmitry.
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: microsoft-layout-support.patch
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110920/e89e4c8c/attachment.ksh>
More information about the cfe-dev
mailing list