Implementation of MSRecordLayoutBuilder.

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?

- Dmitry.

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.

  - Dmitry.

microsoft-layout-support.patch (11 KB)

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?

I`m working on CGRecordLayoutBuilder for Microsoft but I have problems with vbtables.
Itanium ABI doesn't have vbtables and Microsoft ABI doesn't have VTT. It is quite heavy task.

  -Dmitry.

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.

I`m working on CGRecordLayoutBuilder for Microsoft but I have problems
with vbtables.
Itanium ABI doesn't have vbtables and Microsoft ABI doesn't have VTT. It
is quite heavy task.

Yeah, there's a lot of abstraction work to be done here.

John.

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?

I`m working on CGRecordLayoutBuilder for Microsoft but I have problems
with vbtables.
Itanium ABI doesn't have vbtables and Microsoft ABI doesn't have VTT. It
is quite heavy task.

Yeah, there's a lot of abstraction work to be done here.

In simple tests my code already work but if class has many bases and they have other bases it crashes.

John.

  - Dmitry.

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.

I`m working on CGRecordLayoutBuilder for Microsoft but I have problems
with vbtables.
Itanium ABI doesn't have vbtables and Microsoft ABI doesn't have VTT. It
is quite heavy task.

Yeah, there's a lot of abstraction work to be done here.

In simple tests my code already work but if class has many bases and they have other bases it crashes.

If it crashes unconditionally without your patch, then this is incremental progress. :slight_smile:

John

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?

I`m working on CGRecordLayoutBuilder for Microsoft but I have problems
with vbtables.
Itanium ABI doesn't have vbtables and Microsoft ABI doesn't have VTT. It
is quite heavy task.

Yeah, there's a lot of abstraction work to be done here.

In simple tests my code already work but if class has many bases and they have other bases it crashes.

If it crashes unconditionally without your patch, then this is incremental progress. :slight_smile:

John

  - Dmitry.

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.

- Dmitry.

Here is a test and patch.

- Dmitry.

microsoft-layout-support.patch (14.2 KB)

ms_class_layout.cpp (1.46 KB)

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

Hi,
what I get from MSVS output :

class B size(24):
1> ±–
1> 0 | {vfptr}
1> 8 | a
1> | (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.

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 :slight_smile:

Cheers,

  • Don

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.

That sounds great!

Now it seems working fine.

microsoft-layout-support.patch (17.4 KB)

ms_class_layout.cpp (4.51 KB)