Page MenuHomePhabricator

[AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`
ClosedPublic

Authored by aleksandr.urakov on Feb 22 2019, 4:29 AM.

Details

Summary

This patch fixes several small problems with external layouts support in MicrosoftRecordLayoutBuilder:

  • aligns properly the size of a struct that ends with a bit field. It was aligned on byte before, not on the size of the field, so the struct size was smaller than it should be;
  • adjusts the struct size when injecting a vbptr in the case when there were no bases or fields allocated after the vbptr. Similarly, without the adjustment the struct was smaller than it should be;
  • the same fix as above for the vfptr.

All these fixes affect the non-virtual size of a struct, so they are tested through non-virtual inheritance.

Diff Detail

Repository
rL LLVM

Event Timeline

Ping! Can you take a look, please?

Probably @rnk needs to look at this, i'll ping him today.

rnk accepted this revision.Mar 12 2019, 2:08 PM

lgtm

lib/AST/RecordLayoutBuilder.cpp
2750–2753 ↗(On Diff #187922)

I think this may be an interesting test case:

struct __declspec(align(16)) NVBase {
  int x, y;
  virtual ~NVBase();
};
struct VBase { int z; };
struct Foo : NVBase, virtual VBase {
};

On reading the code more, I don't think this test uncovers any bugs, but it seems worth including to make sure we do the right thing w.r.t. alignment.

This revision is now accepted and ready to land.Mar 12 2019, 2:08 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 13 2019, 6:37 AM

Thank you! I've added a slightly reworked test too.