[cfe-dev] Implementation of MSRecordLayoutBuilder.

Eli Friedman eli.friedman at gmail.com
Mon Sep 19 13:06:11 PDT 2011


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

+#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.)

+// 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.
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;

Why is this member called IsVBPtrPersist?

-Eli




More information about the cfe-dev mailing list