double/virtual offsets for MSVC broken in 3.1

Hi,

last year I submitted a patch (commit by somebody else) to solve cases like this:

// The addition of a double anywhere in this struct forces the vtable ptr to occupy 4 bytes + 4 bytes padding in MSVC
struct DoubleInPolymorphicStruct
{
virtual void Empty() { }
int a;
double b;
};

The offset of a in MSVC is 8 bytes, but clang in MSVC offset calculation mode puts it at 4. The initial posts are here:

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-September/016944.html
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-September/016970.html

These changes did not make it into 3.0 but as far as I could tell, did make it into the trunk. However, I’ve just sync’ed up to the 3.1 release and this is still a problem.

I’m about to go back and take a look and what’s broken but it’ll take me a couple of hours to settle back into the code again.

Meanwhile, could anybody help with any ideas as to what may have changed?

Thanks!

  • Don

The change appears to stem from:

http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/RecordLayoutBuilder.cpp?r1=143800&r2=144072&diff_format=h
Revision 144072, Modified Mon Nov 7 22:01:03 2011 CST (9 months, 3 weeks ago) by rjmccall
Fix the layout of vb-tables and vf-tables in the MS C++ ABI. Based on work by Dmitry Sokolov!

Trying to figure out what’s different…

So, the problem is that the same approach needs to be applied to the vftable pointer, as well as the vbtable pointer.

I have a small diff of changes that pass all my tests… is this good enough to submit? (I’m working against 3.1 but the same is true of the trunk and I can sync to that to submit a patch)

Index: lib/AST/RecordLayoutBuilder.cpp

Can you add some new test cases testing this particular behaviour?

I’ve got to figure out how to write/run tests before I can do that but I’ll give it a shot.

Cheers,

  • Don

You should have Python installed and GnuWin32 in your path. After that
you just build clang-test project inside Visual Studio and it runs the
test suite.

Oh, assuming you're using CMake, make that Python 2.7 -- the CMake
generation has failed with subtle errors when I ran it with Python
3.x. Not sure how the testing platform fares with Python 3.

- Kim

Hi, all.

Can anybody tell me about patch status?
Today I download svn trunk and compile Don`s example. Here what I got:
*** Dumping AST Record Layout
    0 | struct d
    0 | (d vftable pointer)
    8 | int a
   16 | double dd
   sizeof=24, dsize=24, align=8
   nvsize=24, nvalign=8

But I didn`t see commits fixing this issue.

Thanks in advanse,
Dmitry Sokolov.

Sorry, I’ve been offline for the last few days. Is this x86 or x64?

Cheers,

  • Don

What is the status on this?

I just checked with trunk, and can verify the behavior Don is seeing.

*** Dumping AST Record Layout
0 | struct DoubleInPolymorphicStruct
0 | (DoubleInPolymorphicStruct vtable pointer)
0 | (DoubleInPolymorphicStruct vftable pointer)
4 | int a
8 | double b
sizeof=16, dsize=16, align=8
nvsize=16, nvalign=8

What test case did you use Dmitry?

Hi. Test case similar with Don example. I used -Xclang -cxx-abi -Xclang microsoft 1.cpp -Xclang -fdump-record-layouts, but the right way is -Xclang -cxx-abi -Xclang microsoft -Xclang 1.cpp -Xclang -fdump-record-layouts. So the issue is still alive. Anybody work on it? -Dmitry Sokolov.

In my earlier mail, I actually posted the wrong dump without MS ABI.

Here is the correct one:

*** Dumping AST Record Layout
0 | struct DoubleInPolymorphicStruct
0 | (DoubleInPolymorphicStruct vftable pointer)
4 | int a
8 | double b
sizeof=16, dsize=16, align=8
nvsize=16, nvalign=8

But the issue still exists.

I tried to apply the patch by Don but it’s not compatible with trunk. You seem to know a lot more about the layout code, so if you or Don could fix it it would be the best.