[PATCH] D49871: Improve support of PDB as an external layout source

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov created this revision.
aleksandr.urakov added reviewers: rsmith, zturner, rnk, mstorsjo, majnemer.
aleksandr.urakov added a project: clang.

This patch improves support of PDB as an external layout source in the next 
cases:

- Multiple non-virtual inheritance from packed base classes. When using 
external layout, there's no need to align `NonVirtualSize` of a base class. It 
may cause an overlapping when the next base classes will be layouted (but there 
is a slightly different case in the test because I can't find a way to specify 
a base offset);
- Support of nameless structs and unions. There is no info about nameless child 
structs and unions in Microsoft cl-emitted PDBs. Instead all its fields are 
just treated as outer structure's (union's) fields. This also causes a fields 
overlapping, and makes it possible for unions to have fields located at a 
non-zero offset.


Repository:
  rC Clang

https://reviews.llvm.org/D49871

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/CodeGenCXX/Inputs/override-layout-nameless-struct-union.layout
  test/CodeGenCXX/Inputs/override-layout-packed-base.layout
  test/CodeGenCXX/override-layout-nameless-struct-union.cpp
  test/CodeGenCXX/override-layout-packed-base.cpp

Index: test/CodeGenCXX/override-layout-packed-base.cpp
===
--- /dev/null
+++ test/CodeGenCXX/override-layout-packed-base.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-packed-base.layout %s | FileCheck %s
+
+// CHECK: Type: class B<0>
+// CHECK:   FieldOffsets: [0, 32]
+
+// CHECK: Type: class B<1>
+// CHECK:   FieldOffsets: [0, 32]
+
+//#pragma pack(push, 1)
+template
+class B {
+  int _b1;
+  char _b2;
+};
+//#pragma pack(pop)
+
+// CHECK: Type: class C
+// CHECK:   FieldOffsets: [80]
+
+class C : B<0>, B<1> {
+  char _c;
+};
+
+void use_structs() {
+  C cs[sizeof(C)];
+}
Index: test/CodeGenCXX/override-layout-nameless-struct-union.cpp
===
--- /dev/null
+++ test/CodeGenCXX/override-layout-nameless-struct-union.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-nameless-struct-union.layout %s | FileCheck %s
+
+// CHECK: Type: struct S
+// CHECK:   Size:64
+// CHECK:   Alignment:32
+// CHECK:   FieldOffsets: [0, 32, 32]
+struct S {
+  short _s;
+//union {
+int _su0;
+char _su1;
+//};
+};
+
+// CHECK: Type: union U
+// CHECK:   Size:96
+// CHECK:   Alignment:32
+// CHECK:   FieldOffsets: [0, 0, 32, 64, 68, 73]
+union U {
+  short _u;
+//struct {
+char _us0;
+int _us1;
+unsigned _us20 : 4;
+unsigned _us21 : 5;
+unsigned _us22 : 6;
+//};
+};
+
+void use_structs() {
+  S ss[sizeof(S)];
+  U us[sizeof(U)];
+}
Index: test/CodeGenCXX/Inputs/override-layout-packed-base.layout
===
--- /dev/null
+++ test/CodeGenCXX/Inputs/override-layout-packed-base.layout
@@ -0,0 +1,18 @@
+
+*** Dumping AST Record Layout
+Type: class B<0>
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: class B<1>
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: class C
+
+Layout: 
Index: test/CodeGenCXX/Inputs/override-layout-nameless-struct-union.layout
===
--- /dev/null
+++ test/CodeGenCXX/Inputs/override-layout-nameless-struct-union.layout
@@ -0,0 +1,16 @@
+
+*** Dumping AST Record Layout
+Type: struct S
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: union U
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2445,7 +2445,9 @@
   auto RoundingAlignment = Alignment;
   if (!MaxFieldAlignment.isZero())
 RoundingAlignment = std::min(RoundingAlignment, MaxFieldAlignment);
-  NonVirtualSize = Size = Size.alignTo(RoundingAlignment);
+  if (!UseExternalLayout)
+Size = Size.alignTo(RoundingAlignment);
+  NonVirtualSize = Size;
   RequiredAlignment = std::max(
   RequiredAlignment, Context.toCharUnitsFromBits(RD->getMaxAlignment()));
   layoutVirtualBases(RD);
@@ -2646,21 +2648,16 @@
   LastFieldIsNonZeroWidthBitfield = false;
   ElementInfo Info = getAdjustedElementInfo(FD);
   Alignment = std::max(Alignment, Info.Alignment);
-  if (IsUnion) {
-placeFieldAtOffset(CharUnits::Zero());
-Size = std::max(Size, Info.Size);
-  } else {
-CharUnits FieldOffset;
-if (UseExternalLayout) {
-  FieldOffset =
-  Context.toCharUnitsFromBits(External.getExternalFieldOffset(FD));
-  assert(FieldOffset >= Size && "field offset already allocated");
-} else {
-  FieldOffset = Size.alignTo(Info.Alignment);
-}
-placeFieldAtOffset(FieldOffset);
-Size = FieldOffset + Info.Size;
-  }
+  CharUnits FieldOffset;
+  if (Use

[PATCH] D49871: Improve support of PDB as an external layout source

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov added a comment.

Thank you!

Can you, please, commit this for me? I have no commit access.


Repository:
  rC Clang

https://reviews.llvm.org/D49871



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D49871: Improve support of PDB as an external layout source

[Aleksandr Urakov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL338353: Improve support of PDB as an external layout source 
(authored by aleksandr.urakov, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D49871?vs=157531&id=158185#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D49871

Files:
  cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
  cfe/trunk/test/CodeGenCXX/Inputs/override-layout-nameless-struct-union.layout
  cfe/trunk/test/CodeGenCXX/Inputs/override-layout-packed-base.layout
  cfe/trunk/test/CodeGenCXX/override-layout-nameless-struct-union.cpp
  cfe/trunk/test/CodeGenCXX/override-layout-packed-base.cpp

Index: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
===
--- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
+++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
@@ -2452,7 +2452,9 @@
   auto RoundingAlignment = Alignment;
   if (!MaxFieldAlignment.isZero())
 RoundingAlignment = std::min(RoundingAlignment, MaxFieldAlignment);
-  NonVirtualSize = Size = Size.alignTo(RoundingAlignment);
+  if (!UseExternalLayout)
+Size = Size.alignTo(RoundingAlignment);
+  NonVirtualSize = Size;
   RequiredAlignment = std::max(
   RequiredAlignment, Context.toCharUnitsFromBits(RD->getMaxAlignment()));
   layoutVirtualBases(RD);
@@ -2653,21 +2655,16 @@
   LastFieldIsNonZeroWidthBitfield = false;
   ElementInfo Info = getAdjustedElementInfo(FD);
   Alignment = std::max(Alignment, Info.Alignment);
-  if (IsUnion) {
-placeFieldAtOffset(CharUnits::Zero());
-Size = std::max(Size, Info.Size);
-  } else {
-CharUnits FieldOffset;
-if (UseExternalLayout) {
-  FieldOffset =
-  Context.toCharUnitsFromBits(External.getExternalFieldOffset(FD));
-  assert(FieldOffset >= Size && "field offset already allocated");
-} else {
-  FieldOffset = Size.alignTo(Info.Alignment);
-}
-placeFieldAtOffset(FieldOffset);
-Size = FieldOffset + Info.Size;
-  }
+  CharUnits FieldOffset;
+  if (UseExternalLayout)
+FieldOffset =
+Context.toCharUnitsFromBits(External.getExternalFieldOffset(FD));
+  else if (IsUnion)
+FieldOffset = CharUnits::Zero();
+  else
+FieldOffset = Size.alignTo(Info.Alignment);
+  placeFieldAtOffset(FieldOffset);
+  Size = std::max(Size, FieldOffset + Info.Size);
 }
 
 void MicrosoftRecordLayoutBuilder::layoutBitField(const FieldDecl *FD) {
@@ -2692,18 +2689,17 @@
   }
   LastFieldIsNonZeroWidthBitfield = true;
   CurrentBitfieldSize = Info.Size;
-  if (IsUnion) {
-placeFieldAtOffset(CharUnits::Zero());
-Size = std::max(Size, Info.Size);
-// TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
-  } else if (UseExternalLayout) {
+  if (UseExternalLayout) {
 auto FieldBitOffset = External.getExternalFieldOffset(FD);
 placeFieldAtBitOffset(FieldBitOffset);
 auto NewSize = Context.toCharUnitsFromBits(
 llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
-assert(NewSize >= Size && "bit field offset already allocated");
-Size = NewSize;
+Size = std::max(Size, NewSize);
 Alignment = std::max(Alignment, Info.Alignment);
+  } else if (IsUnion) {
+placeFieldAtOffset(CharUnits::Zero());
+Size = std::max(Size, Info.Size);
+// TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
   } else {
 // Allocate a new block of memory and place the bitfield in it.
 CharUnits FieldOffset = Size.alignTo(Info.Alignment);
Index: cfe/trunk/test/CodeGenCXX/Inputs/override-layout-packed-base.layout
===
--- cfe/trunk/test/CodeGenCXX/Inputs/override-layout-packed-base.layout
+++ cfe/trunk/test/CodeGenCXX/Inputs/override-layout-packed-base.layout
@@ -0,0 +1,18 @@
+
+*** Dumping AST Record Layout
+Type: class B<0>
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: class B<1>
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: class C
+
+Layout: 
Index: cfe/trunk/test/CodeGenCXX/Inputs/override-layout-nameless-struct-union.layout
===
--- cfe/trunk/test/CodeGenCXX/Inputs/override-layout-nameless-struct-union.layout
+++ cfe/trunk/test/CodeGenCXX/Inputs/override-layout-nameless-struct-union.layout
@@ -0,0 +1,16 @@
+
+*** Dumping AST Record Layout
+Type: struct S
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: union U
+
+Layout: 
Index: cfe/trunk/test/CodeGenCXX/override-layout-nameless-struct-union.cpp
===
--- cfe/trunk/test/CodeGenCXX/override-layout-nameless-struct-union.cpp
+++ cfe/trunk/test/CodeGenCXX/override-layout-nameless-struct-union.cpp
@@ -0,0 +1,33 @@
+// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-nameless-struct-union.layout %s | FileCheck %s
+
+// CHECK: Type: struct S

[PATCH] D53497: [AST] Do not align virtual bases in `MicrosoftRecordLayoutBuilder` when an external layout is used

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov created this revision.
aleksandr.urakov added reviewers: rnk, rsmith, zturner, mstorsjo, majnemer.
aleksandr.urakov added a project: clang.
Herald added a subscriber: cfe-commits.

The patch removes alignment of virtual bases when an external layout is used. 
We have two cases:

- the external layout source has an information about virtual bases offsets, so 
we just use them;
- the external source has no information about virtual bases offsets. In this 
case we can't predict where the base will be located. If we will align it but 
there will be something like `#pragma pack(push, 1)` really, then likely our 
layout will not fit into the real structure size, and then some asserts will 
hit. The asserts look reasonable, so I don't think that we need to remove them. 
May be it would be better instead don't align fields / bases etc. (so treat it 
always as `#pragma pack(push, 1)`) when an external layout source is used but 
no info about a field location is presented.

This one is related to https://reviews.llvm.org/D49871

Also it seems that `MicrosoftRecordLayoutBuilder` with an external source and 
without one differ considerably, may be it is worth to split this and to create 
two different builders? I think that it would make the logic simpler. What do 
you think about it?


Repository:
  rC Clang

https://reviews.llvm.org/D53497

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/CodeGenCXX/Inputs/override-layout-packed-base.layout
  test/CodeGenCXX/override-layout-packed-base.cpp

Index: test/CodeGenCXX/override-layout-packed-base.cpp
===
--- test/CodeGenCXX/override-layout-packed-base.cpp
+++ test/CodeGenCXX/override-layout-packed-base.cpp
@@ -1,26 +1,40 @@
-// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-packed-base.layout %s | FileCheck %s
+// RUN: %clang_cc1 -triple i686-windows-msvc -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-packed-base.layout %s | FileCheck %s
+
+//#pragma pack(push, 1)
 
 // CHECK: Type: class B<0>
+// CHECK:   Size:40
 // CHECK:   FieldOffsets: [0, 32]
 
 // CHECK: Type: class B<1>
+// CHECK:   Size:40
 // CHECK:   FieldOffsets: [0, 32]
 
-//#pragma pack(push, 1)
 template
 class B {
   int _b1;
   char _b2;
 };
-//#pragma pack(pop)
 
 // CHECK: Type: class C
+// CHECK:   Size:88
 // CHECK:   FieldOffsets: [80]
 
 class C : B<0>, B<1> {
   char _c;
 };
 
+// CHECK: Type: class D
+// CHECK:   Size:120
+// CHECK:   FieldOffsets: [32]
+
+class D : virtual B<0>, virtual B<1> {
+  char _d;
+};
+
+//#pragma pack(pop)
+
 void use_structs() {
   C cs[sizeof(C)];
+  D ds[sizeof(D)];
 }
Index: test/CodeGenCXX/Inputs/override-layout-packed-base.layout
===
--- test/CodeGenCXX/Inputs/override-layout-packed-base.layout
+++ test/CodeGenCXX/Inputs/override-layout-packed-base.layout
@@ -3,16 +3,26 @@
 Type: class B<0>
 
 Layout: 
 
 *** Dumping AST Record Layout
 Type: class B<1>
 
 Layout: 
 
 *** Dumping AST Record Layout
 Type: class C
 
 Layout: 
+
+*** Dumping AST Record Layout
+Type: class D
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2829,15 +2829,14 @@
 CharUnits BaseOffset;
 
 // Respect the external AST source base offset, if present.
-bool FoundBase = false;
 if (UseExternalLayout) {
-  FoundBase = External.getExternalVBaseOffset(BaseDecl, BaseOffset);
-  if (FoundBase)
-assert(BaseOffset >= Size && "base offset already allocated");
-}
-if (!FoundBase)
+  if (!External.getExternalVBaseOffset(BaseDecl, BaseOffset))
+BaseOffset = Size;
+} else
   BaseOffset = Size.alignTo(Info.Alignment);
 
+assert(BaseOffset >= Size && "base offset already allocated");
+
 VBases.insert(std::make_pair(BaseDecl,
 ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp)));
 Size = BaseOffset + BaseLayout.getNonVirtualSize();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53497: [AST] Do not align virtual bases in `MicrosoftRecordLayoutBuilder` when an external layout is used

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov added a comment.

Thank you!


Repository:
  rC Clang

https://reviews.llvm.org/D53497



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53497: [AST] Do not align virtual bases in `MicrosoftRecordLayoutBuilder` when an external layout is used

[Aleksandr Urakov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC345012: [AST] Do not align virtual bases in 
`MicrosoftRecordLayoutBuilder` when (authored by aleksandr.urakov, committed by 
).

Repository:
  rC Clang

https://reviews.llvm.org/D53497

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/CodeGenCXX/Inputs/override-layout-packed-base.layout
  test/CodeGenCXX/override-layout-packed-base.cpp

Index: test/CodeGenCXX/override-layout-packed-base.cpp
===
--- test/CodeGenCXX/override-layout-packed-base.cpp
+++ test/CodeGenCXX/override-layout-packed-base.cpp
@@ -1,26 +1,40 @@
-// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-packed-base.layout %s | FileCheck %s
+// RUN: %clang_cc1 -triple i686-windows-msvc -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-packed-base.layout %s | FileCheck %s
+
+//#pragma pack(push, 1)
 
 // CHECK: Type: class B<0>
+// CHECK:   Size:40
 // CHECK:   FieldOffsets: [0, 32]
 
 // CHECK: Type: class B<1>
+// CHECK:   Size:40
 // CHECK:   FieldOffsets: [0, 32]
 
-//#pragma pack(push, 1)
 template
 class B {
   int _b1;
   char _b2;
 };
-//#pragma pack(pop)
 
 // CHECK: Type: class C
+// CHECK:   Size:88
 // CHECK:   FieldOffsets: [80]
 
 class C : B<0>, B<1> {
   char _c;
 };
 
+// CHECK: Type: class D
+// CHECK:   Size:120
+// CHECK:   FieldOffsets: [32]
+
+class D : virtual B<0>, virtual B<1> {
+  char _d;
+};
+
+//#pragma pack(pop)
+
 void use_structs() {
   C cs[sizeof(C)];
+  D ds[sizeof(D)];
 }
Index: test/CodeGenCXX/Inputs/override-layout-packed-base.layout
===
--- test/CodeGenCXX/Inputs/override-layout-packed-base.layout
+++ test/CodeGenCXX/Inputs/override-layout-packed-base.layout
@@ -3,16 +3,26 @@
 Type: class B<0>
 
 Layout: 
 
 *** Dumping AST Record Layout
 Type: class B<1>
 
 Layout: 
 
 *** Dumping AST Record Layout
 Type: class C
 
 Layout: 
+
+*** Dumping AST Record Layout
+Type: class D
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2829,15 +2829,14 @@
 CharUnits BaseOffset;
 
 // Respect the external AST source base offset, if present.
-bool FoundBase = false;
 if (UseExternalLayout) {
-  FoundBase = External.getExternalVBaseOffset(BaseDecl, BaseOffset);
-  if (FoundBase)
-assert(BaseOffset >= Size && "base offset already allocated");
-}
-if (!FoundBase)
+  if (!External.getExternalVBaseOffset(BaseDecl, BaseOffset))
+BaseOffset = Size;
+} else
   BaseOffset = Size.alignTo(Info.Alignment);
 
+assert(BaseOffset >= Size && "base offset already allocated");
+
 VBases.insert(std::make_pair(BaseDecl,
 ASTRecordLayout::VBaseInfo(BaseOffset, HasVtordisp)));
 Size = BaseOffset + BaseLayout.getNonVirtualSize();
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov added a comment.

Ping! Can you take a look, please?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58544/new/

https://reviews.llvm.org/D58544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Aleksandr Urakov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL356047: [AST] Improve support of external layouts in 
`MicrosoftRecordLayoutBuilder` (authored by aleksandr.urakov, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58544?vs=187922&id=190403#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58544/new/

https://reviews.llvm.org/D58544

Files:
  cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
  cfe/trunk/test/CodeGenCXX/Inputs/override-bit-field-layout.layout
  cfe/trunk/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
  cfe/trunk/test/CodeGenCXX/override-bit-field-layout.cpp
  cfe/trunk/test/CodeGenCXX/override-layout-virtual-base.cpp
  cfe/trunk/test/CodeGenCXX/override-layout.cpp

Index: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
===
--- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
+++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
@@ -2692,7 +2692,8 @@
 auto FieldBitOffset = External.getExternalFieldOffset(FD);
 placeFieldAtBitOffset(FieldBitOffset);
 auto NewSize = Context.toCharUnitsFromBits(
-llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
+llvm::alignDown(FieldBitOffset, Context.toBits(Info.Alignment)) +
+Context.toBits(Info.Size));
 Size = std::max(Size, NewSize);
 Alignment = std::max(Alignment, Info.Alignment);
   } else if (IsUnion) {
@@ -2741,12 +2742,17 @@
   CharUnits InjectionSite = VBPtrOffset;
   // But before we do, make sure it's properly aligned.
   VBPtrOffset = VBPtrOffset.alignTo(PointerInfo.Alignment);
+  // Determine where the first field should be laid out after the vbptr.
+  CharUnits FieldStart = VBPtrOffset + PointerInfo.Size;
   // Shift everything after the vbptr down, unless we're using an external
   // layout.
-  if (UseExternalLayout)
+  if (UseExternalLayout) {
+// It is possible that there were no fields or bases located after vbptr,
+// so the size was not adjusted before.
+if (Size < FieldStart)
+  Size = FieldStart;
 return;
-  // Determine where the first field should be laid out after the vbptr.
-  CharUnits FieldStart = VBPtrOffset + PointerInfo.Size;
+  }
   // Make sure that the amount we push the fields back by is a multiple of the
   // alignment.
   CharUnits Offset = (FieldStart - InjectionSite)
@@ -2771,8 +2777,14 @@
   if (HasVBPtr)
 VBPtrOffset += Offset;
 
-  if (UseExternalLayout)
+  if (UseExternalLayout) {
+// The class may have no bases or fields, but still have a vfptr
+// (e.g. it's an interface class). The size was not correctly set before
+// in this case.
+if (FieldOffsets.empty() && Bases.empty())
+  Size += Offset;
 return;
+  }
 
   Size += Offset;
 
Index: cfe/trunk/test/CodeGenCXX/override-layout-virtual-base.cpp
===
--- cfe/trunk/test/CodeGenCXX/override-layout-virtual-base.cpp
+++ cfe/trunk/test/CodeGenCXX/override-layout-virtual-base.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-virtual-base.layout %s | FileCheck %s
+
+struct S1 {
+  int a;
+};
+
+struct S2 : virtual S1 {
+  virtual void foo() {}
+};
+
+// CHECK: Type: struct S3
+// CHECK:   FieldOffsets: [128]
+struct S3 : S2 {
+  char b;
+};
+
+void use_structs() {
+  S1 s1s[sizeof(S1)];
+  S2 s2s[sizeof(S2)];
+  S3 s3s[sizeof(S3)];
+}
Index: cfe/trunk/test/CodeGenCXX/Inputs/override-bit-field-layout.layout
===
--- cfe/trunk/test/CodeGenCXX/Inputs/override-bit-field-layout.layout
+++ cfe/trunk/test/CodeGenCXX/Inputs/override-bit-field-layout.layout
@@ -14,3 +14,11 @@
   Size:128
   Alignment:64
   FieldOffsets: [64]>
+
+*** Dumping AST Record Layout
+Type: struct S3
+
+Layout: 
Index: cfe/trunk/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
===
--- cfe/trunk/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
+++ cfe/trunk/test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
@@ -0,0 +1,8 @@
+
+*** Dumping AST Record Layout
+Type: struct S2
+
+Layout: 
Index: cfe/trunk/test/CodeGenCXX/override-bit-field-layout.cpp
===
--- cfe/trunk/test/CodeGenCXX/override-bit-field-layout.cpp
+++ cfe/trunk/test/CodeGenCXX/override-bit-field-layout.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
 

[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov added a comment.

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


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58544/new/

https://reviews.llvm.org/D58544



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D58544: [AST] Improve support of external layouts in `MicrosoftRecordLayoutBuilder`

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov created this revision.
aleksandr.urakov added reviewers: rnk, zturner, rsmith.
aleksandr.urakov added a project: clang.
Herald added subscribers: cfe-commits, jdoerfert.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D58544

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/CodeGenCXX/Inputs/override-bit-field-layout.layout
  test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
  test/CodeGenCXX/override-bit-field-layout.cpp
  test/CodeGenCXX/override-layout-virtual-base.cpp

Index: test/CodeGenCXX/override-layout-virtual-base.cpp
===
--- /dev/null
+++ test/CodeGenCXX/override-layout-virtual-base.cpp
@@ -0,0 +1,21 @@
+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-layout-virtual-base.layout %s | FileCheck %s
+
+struct S1 {
+  int a;
+};
+
+struct S2 : virtual S1 {
+  virtual void foo() {}
+};
+
+// CHECK: Type: struct S3
+// CHECK:   FieldOffsets: [128]
+struct S3 : S2 {
+  char b;
+};
+
+void use_structs() {
+  S1 s1s[sizeof(S1)];
+  S2 s2s[sizeof(S2)];
+  S3 s3s[sizeof(S3)];
+}
Index: test/CodeGenCXX/override-bit-field-layout.cpp
===
--- test/CodeGenCXX/override-bit-field-layout.cpp
+++ test/CodeGenCXX/override-bit-field-layout.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
+// RUN: %clang_cc1 -w -triple=x86_64-pc-win32 -fms-compatibility -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
 
 // CHECK: Type: struct S1
 // CHECK:   FieldOffsets: [0, 11]
@@ -14,7 +14,23 @@
   short a : 3;
 };
 
+// CHECK: Type: struct S3
+// CHECK:   Size:32
+// CHECK:   FieldOffsets: [0, 1]
+struct S3 {
+  int a : 1;
+  int b : 2;
+};
+
+// CHECK: Type: struct S4
+// CHECK:   FieldOffsets: [32]
+struct S4 : S3 {
+  char c;
+};
+
 void use_structs() {
   S1 s1s[sizeof(S1)];
   S2 s2s[sizeof(S2)];
+  S3 s3s[sizeof(S3)];
+  S4 s4s[sizeof(S4)];
 }
Index: test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
===
--- /dev/null
+++ test/CodeGenCXX/Inputs/override-layout-virtual-base.layout
@@ -0,0 +1,8 @@
+
+*** Dumping AST Record Layout
+Type: struct S2
+
+Layout: 
Index: test/CodeGenCXX/Inputs/override-bit-field-layout.layout
===
--- test/CodeGenCXX/Inputs/override-bit-field-layout.layout
+++ test/CodeGenCXX/Inputs/override-bit-field-layout.layout
@@ -14,3 +14,11 @@
   Size:128
   Alignment:64
   FieldOffsets: [64]>
+
+*** Dumping AST Record Layout
+Type: struct S3
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2692,7 +2692,8 @@
 auto FieldBitOffset = External.getExternalFieldOffset(FD);
 placeFieldAtBitOffset(FieldBitOffset);
 auto NewSize = Context.toCharUnitsFromBits(
-llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
+llvm::alignDown(FieldBitOffset, Context.toBits(Info.Alignment)) +
+Context.toBits(Info.Size));
 Size = std::max(Size, NewSize);
 Alignment = std::max(Alignment, Info.Alignment);
   } else if (IsUnion) {
@@ -2741,12 +2742,17 @@
   CharUnits InjectionSite = VBPtrOffset;
   // But before we do, make sure it's properly aligned.
   VBPtrOffset = VBPtrOffset.alignTo(PointerInfo.Alignment);
+  // Determine where the first field should be laid out after the vbptr.
+  CharUnits FieldStart = VBPtrOffset + PointerInfo.Size;
   // Shift everything after the vbptr down, unless we're using an external
   // layout.
-  if (UseExternalLayout)
+  if (UseExternalLayout) {
+// It is possible that there were no fields or bases located after vbptr,
+// so the size was not adjusted before.
+if (Size < FieldStart)
+  Size = FieldStart;
 return;
-  // Determine where the first field should be laid out after the vbptr.
-  CharUnits FieldStart = VBPtrOffset + PointerInfo.Size;
+  }
   // Make sure that the amount we push the fields back by is a m

[PATCH] D49227: Override a bit fields layout from an external source

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov created this revision.
aleksandr.urakov added reviewers: rsmith, mstorsjo, arphaman, whunt, majnemer.
aleksandr.urakov added a project: clang.

This patch adds a possibility to use an external layout for bit fields.

The problem occurred while debugging a Windows application with PDB symbols. 
The structure was as follows:

  struct S {
short a : 3;
short : 8;
short b : 5;
  }

The problem is that PDB doesn't have information about the unnamed bit field, 
so the field `b` was located just behind the field `a`. But PDB has a valid 
information about fields offsets, so we can use it to solve the problem.


Repository:
  rC Clang

https://reviews.llvm.org/D49227

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/CodeGenCXX/Inputs/override-bit-field-layout.layout
  test/CodeGenCXX/override-bit-field-layout.cpp


Index: test/CodeGenCXX/override-bit-field-layout.cpp
===
--- /dev/null
+++ test/CodeGenCXX/override-bit-field-layout.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -w -fdump-record-layouts-simple 
-foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | 
FileCheck %s
+
+// CHECK: Type: struct S
+// CHECK:   FieldOffsets: [0, 11]
+struct S {
+  short a : 3;
+  short b : 5;
+};
+
+void use_structs() {
+  S ss[sizeof(S)];
+}
Index: test/CodeGenCXX/Inputs/override-bit-field-layout.layout
===
--- /dev/null
+++ test/CodeGenCXX/Inputs/override-bit-field-layout.layout
@@ -0,0 +1,8 @@
+
+*** Dumping AST Record Layout
+Type: struct S
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2677,7 +2677,7 @@
   // Check to see if this bitfield fits into an existing allocation.  Note:
   // MSVC refuses to pack bitfields of formal types with different sizes
   // into the same allocation.
-  if (!IsUnion && LastFieldIsNonZeroWidthBitfield &&
+  if (!UseExternalLayout && !IsUnion && LastFieldIsNonZeroWidthBitfield &&
   CurrentBitfieldSize == Info.Size && Width <= RemainingBitsInField) {
 placeFieldAtBitOffset(Context.toBits(Size) - RemainingBitsInField);
 RemainingBitsInField -= Width;
@@ -2689,6 +2689,14 @@
 placeFieldAtOffset(CharUnits::Zero());
 Size = std::max(Size, Info.Size);
 // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
+  } else if (UseExternalLayout) {
+auto FieldBitOffset = External.getExternalFieldOffset(FD);
+placeFieldAtBitOffset(FieldBitOffset);
+auto NewSize = Context.toCharUnitsFromBits(
+llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
+assert(NewSize >= Size && "bit field offset already allocated");
+Size = NewSize;
+Alignment = std::max(Alignment, Info.Alignment);
   } else {
 // Allocate a new block of memory and place the bitfield in it.
 CharUnits FieldOffset = Size.alignTo(Info.Alignment);


Index: test/CodeGenCXX/override-bit-field-layout.cpp
===
--- /dev/null
+++ test/CodeGenCXX/override-bit-field-layout.cpp
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
+
+// CHECK: Type: struct S
+// CHECK:   FieldOffsets: [0, 11]
+struct S {
+  short a : 3;
+  short b : 5;
+};
+
+void use_structs() {
+  S ss[sizeof(S)];
+}
Index: test/CodeGenCXX/Inputs/override-bit-field-layout.layout
===
--- /dev/null
+++ test/CodeGenCXX/Inputs/override-bit-field-layout.layout
@@ -0,0 +1,8 @@
+
+*** Dumping AST Record Layout
+Type: struct S
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2677,7 +2677,7 @@
   // Check to see if this bitfield fits into an existing allocation.  Note:
   // MSVC refuses to pack bitfields of formal types with different sizes
   // into the same allocation.
-  if (!IsUnion && LastFieldIsNonZeroWidthBitfield &&
+  if (!UseExternalLayout && !IsUnion && LastFieldIsNonZeroWidthBitfield &&
   CurrentBitfieldSize == Info.Size && Width <= RemainingBitsInField) {
 placeFieldAtBitOffset(Context.toBits(Size) - RemainingBitsInField);
 RemainingBitsInField -= Width;
@@ -2689,6 +2689,14 @@
 placeFieldAtOffset(CharUnits::Zero());
 Size = std::max(Size, Info.Size);
 // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
+  } else if (UseExternalLayout) {
+auto FieldBitOffset = External.getExternalFieldOffset(FD);
+placeFieldAtBitOffset(FieldBitOffset);
+auto NewSize = Context.toCharUnitsFromBits(
+llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
+

[PATCH] D49227: Override a bit fields layout from an external source

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov updated this revision to Diff 155324.
aleksandr.urakov added a comment.

Thank you!

Yes, I have added a test case for that.

If it's OK, can you commit this for me, please? I have no commit access.


https://reviews.llvm.org/D49227

Files:
  lib/AST/RecordLayoutBuilder.cpp
  test/CodeGenCXX/Inputs/override-bit-field-layout.layout
  test/CodeGenCXX/override-bit-field-layout.cpp


Index: test/CodeGenCXX/override-bit-field-layout.cpp
===
--- /dev/null
+++ test/CodeGenCXX/override-bit-field-layout.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -w -fdump-record-layouts-simple 
-foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | 
FileCheck %s
+
+// CHECK: Type: struct S1
+// CHECK:   FieldOffsets: [0, 11]
+struct S1 {
+  short a : 3;
+  short b : 5;
+};
+
+// CHECK: Type: struct S2
+// CHECK:   FieldOffsets: [64]
+struct S2 {
+  virtual ~S2() = default;
+  short a : 3;
+};
+
+void use_structs() {
+  S1 s1s[sizeof(S1)];
+  S2 s2s[sizeof(S2)];
+}
Index: test/CodeGenCXX/Inputs/override-bit-field-layout.layout
===
--- /dev/null
+++ test/CodeGenCXX/Inputs/override-bit-field-layout.layout
@@ -0,0 +1,16 @@
+
+*** Dumping AST Record Layout
+Type: struct S1
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: struct S2
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2677,7 +2677,7 @@
   // Check to see if this bitfield fits into an existing allocation.  Note:
   // MSVC refuses to pack bitfields of formal types with different sizes
   // into the same allocation.
-  if (!IsUnion && LastFieldIsNonZeroWidthBitfield &&
+  if (!UseExternalLayout && !IsUnion && LastFieldIsNonZeroWidthBitfield &&
   CurrentBitfieldSize == Info.Size && Width <= RemainingBitsInField) {
 placeFieldAtBitOffset(Context.toBits(Size) - RemainingBitsInField);
 RemainingBitsInField -= Width;
@@ -2689,6 +2689,14 @@
 placeFieldAtOffset(CharUnits::Zero());
 Size = std::max(Size, Info.Size);
 // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
+  } else if (UseExternalLayout) {
+auto FieldBitOffset = External.getExternalFieldOffset(FD);
+placeFieldAtBitOffset(FieldBitOffset);
+auto NewSize = Context.toCharUnitsFromBits(
+llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
+assert(NewSize >= Size && "bit field offset already allocated");
+Size = NewSize;
+Alignment = std::max(Alignment, Info.Alignment);
   } else {
 // Allocate a new block of memory and place the bitfield in it.
 CharUnits FieldOffset = Size.alignTo(Info.Alignment);


Index: test/CodeGenCXX/override-bit-field-layout.cpp
===
--- /dev/null
+++ test/CodeGenCXX/override-bit-field-layout.cpp
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -w -fdump-record-layouts-simple -foverride-record-layout=%S/Inputs/override-bit-field-layout.layout %s | FileCheck %s
+
+// CHECK: Type: struct S1
+// CHECK:   FieldOffsets: [0, 11]
+struct S1 {
+  short a : 3;
+  short b : 5;
+};
+
+// CHECK: Type: struct S2
+// CHECK:   FieldOffsets: [64]
+struct S2 {
+  virtual ~S2() = default;
+  short a : 3;
+};
+
+void use_structs() {
+  S1 s1s[sizeof(S1)];
+  S2 s2s[sizeof(S2)];
+}
Index: test/CodeGenCXX/Inputs/override-bit-field-layout.layout
===
--- /dev/null
+++ test/CodeGenCXX/Inputs/override-bit-field-layout.layout
@@ -0,0 +1,16 @@
+
+*** Dumping AST Record Layout
+Type: struct S1
+
+Layout: 
+
+*** Dumping AST Record Layout
+Type: struct S2
+
+Layout: 
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -2677,7 +2677,7 @@
   // Check to see if this bitfield fits into an existing allocation.  Note:
   // MSVC refuses to pack bitfields of formal types with different sizes
   // into the same allocation.
-  if (!IsUnion && LastFieldIsNonZeroWidthBitfield &&
+  if (!UseExternalLayout && !IsUnion && LastFieldIsNonZeroWidthBitfield &&
   CurrentBitfieldSize == Info.Size && Width <= RemainingBitsInField) {
 placeFieldAtBitOffset(Context.toBits(Size) - RemainingBitsInField);
 RemainingBitsInField -= Width;
@@ -2689,6 +2689,14 @@
 placeFieldAtOffset(CharUnits::Zero());
 Size = std::max(Size, Info.Size);
 // TODO: Add a Sema warning that MS ignores bitfield alignment in unions.
+  } else if (UseExternalLayout) {
+auto FieldBitOffset = External.getExternalFieldOffset(FD);
+placeFieldAtBitOffset(FieldBitOffset);
+auto NewSize = Context.toCharUnitsFromBits(
+llvm::alignTo(FieldBitOffset + Width, Context.getCharWidth()));
+assert(NewSize >= S

[PATCH] D49227: Override a bit fields layout from an external source

[Aleksandr Urakov via Phabricator via cfe-commits]

aleksandr.urakov added a comment.

Thank you!


Repository:
  rL LLVM

https://reviews.llvm.org/D49227



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits