[clang] Fix to msvc::no_unique_address causing assert when used with __declspec(empty_bases) (PR #74776)

2023-12-11 Thread Amy Huang via cfe-commits 

https://github.com/amykhuang closed 
https://github.com/llvm/llvm-project/pull/74776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix to msvc::no_unique_address causing assert when used with __declspec(empty_bases) (PR #74776)

2023-12-11 Thread via cfe-commits 

https://github.com/zmodem approved this pull request.

lgtm, thanks for fixing!

https://github.com/llvm/llvm-project/pull/74776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix to msvc::no_unique_address causing assert when used with __declspec(empty_bases) (PR #74776)

2023-12-07 Thread via cfe-commits 

github-actions[bot] wrote:




:warning: C/C++ code formatter, clang-format found issues in your code. 
:warning:



You can test this locally with the following command:


``bash
git-clang-format --diff c6805ea44af3bfd57e6b46f2d65ec6b0d0d6c64a 
c3dedfd535f037fb54a8e04640f9332e655a9a7d -- 
clang/lib/AST/RecordLayoutBuilder.cpp 
clang/test/Layout/ms-x86-declspec-empty_bases.cpp
``





View the diff from clang-format here.


``diff
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp 
b/clang/lib/AST/RecordLayoutBuilder.cpp
index 73b248ffe5..4af8770db2 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2267,9 +2267,12 @@ 
ItaniumRecordLayoutBuilder::updateExternalFieldOffset(const FieldDecl *Field,
 /// \returns diagnostic %select index.
 static unsigned getPaddingDiagFromTagKind(TagTypeKind Tag) {
   switch (Tag) {
-  case TTK_Struct: return 0;
-  case TTK_Interface: return 1;
-  case TTK_Class: return 2;
+  case TTK_Struct:
+return 0;
+  case TTK_Interface:
+return 1;
+  case TTK_Class:
+return 2;
   default: llvm_unreachable("Invalid tag kind for field padding diagnostic!");
   }
 }
@@ -2300,15 +2303,13 @@ void ItaniumRecordLayoutBuilder::CheckFieldPadding(
 if (D->getIdentifier())
   Diag(D->getLocation(), diag::warn_padded_struct_field)
   << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
-  << Context.getTypeDeclType(D->getParent())
-  << PadSize
+  << Context.getTypeDeclType(D->getParent()) << PadSize
   << (InBits ? 1 : 0) // (byte|bit)
   << D->getIdentifier();
 else
   Diag(D->getLocation(), diag::warn_padded_struct_anon_field)
   << getPaddingDiagFromTagKind(D->getParent()->getTagKind())
-  << Context.getTypeDeclType(D->getParent())
-  << PadSize
+  << Context.getTypeDeclType(D->getParent()) << PadSize
   << (InBits ? 1 : 0); // (byte|bit)
  }
  if (isPacked && Offset != UnpackedOffset) {

``




https://github.com/llvm/llvm-project/pull/74776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix to msvc::no_unique_address causing assert when used with __declspec(empty_bases) (PR #74776)

2023-12-07 Thread via cfe-commits 

llvmbot wrote:




@llvm/pr-subscribers-clang

Author: Amy Huang (amykhuang)


Changes

no_unique_address makes it possible for a class to be empty and have non-zero 
virtual size, so just remove this assert.

Bug: https://github.com/llvm/llvm-project/issues/74442

---
Full diff: https://github.com/llvm/llvm-project/pull/74776.diff


2 Files Affected:

- (modified) clang/lib/AST/RecordLayoutBuilder.cpp (+2-2) 
- (modified) clang/test/Layout/ms-x86-declspec-empty_bases.cpp (+60) 


``diff
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp 
b/clang/lib/AST/RecordLayoutBuilder.cpp
index f1f2275da44dc..73b248ffe5899 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2936,8 +2936,8 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
   }
 
   if (!FoundBase) {
-if (MDCUsesEBO && BaseDecl->isEmpty()) {
-  assert(BaseLayout.getNonVirtualSize() == CharUnits::Zero());
+if (MDCUsesEBO && BaseDecl->isEmpty() &&
+(BaseLayout.getNonVirtualSize() == CharUnits::Zero())) {
   BaseOffset = CharUnits::Zero();
 } else {
   // Otherwise, lay the base out at the end of the MDC.
diff --git a/clang/test/Layout/ms-x86-declspec-empty_bases.cpp 
b/clang/test/Layout/ms-x86-declspec-empty_bases.cpp
index cc13a980cb5db..4738ce5720f75 100644
--- a/clang/test/Layout/ms-x86-declspec-empty_bases.cpp
+++ b/clang/test/Layout/ms-x86-declspec-empty_bases.cpp
@@ -264,3 +264,63 @@ int _ = sizeof(G);
 // CHECK-NEXT:| [sizeof=12, align=4,
 // CHECK-NEXT:|  nvsize=12, nvalign=4]
 }
+
+namespace test5 {
+
+struct A {
+  int a;
+};
+struct B {
+  int b;
+};
+struct C {};
+struct __declspec(align(16)) D {};
+struct E {
+  [[msvc::no_unique_address]] C c;
+};
+struct __declspec(empty_bases) X : A, D, B, C, E {
+};
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int a
+// CHECK-NEXT:| [sizeof=4, align=4,
+// CHECK-NEXT:|  nvsize=4, nvalign=4]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::D (empty)
+// CHECK-NEXT:| [sizeof=16, align=16,
+// CHECK-NEXT:|  nvsize=0, nvalign=16]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::B
+// CHECK-NEXT:  0 |   int b
+// CHECK-NEXT:| [sizeof=4, align=4,
+// CHECK-NEXT:|  nvsize=4, nvalign=4]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::C (empty)
+// CHECK-NEXT:| [sizeof=1, align=1,
+// CHECK-NEXT:|  nvsize=0, nvalign=1]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::E (empty)
+// CHECK-NEXT:  0 |   struct test5::C c (empty)
+// CHECK-NEXT:| [sizeof=1, align=1,
+// CHECK-NEXT:|  nvsize=1, nvalign=1]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::X
+// CHECK-NEXT:  0 |   struct test5::A (base)
+// CHECK-NEXT:  0 | int a
+// CHECK-NEXT:  0 |   struct test5::D (base) (empty)
+// CHECK-NEXT:  0 |   struct test5::C (base) (empty)
+// CHECK-NEXT:  4 |   struct test5::B (base)
+// CHECK-NEXT:  4 | int b
+// CHECK-NEXT:  8 |   struct test5::E (base) (empty)
+// CHECK-NEXT:  8 | struct test5::C c (empty)
+// CHECK-NEXT:| [sizeof=16, align=16,
+// CHECK-NEXT:|  nvsize=16, nvalign=16]
+
+int _ = sizeof(X);
+}

``




https://github.com/llvm/llvm-project/pull/74776
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Fix to msvc::no_unique_address causing assert when used with __declspec(empty_bases) (PR #74776)

2023-12-07 Thread Amy Huang via cfe-commits 

https://github.com/amykhuang created 
https://github.com/llvm/llvm-project/pull/74776

no_unique_address makes it possible for a class to be empty and have non-zero 
virtual size, so just remove this assert.

Bug: https://github.com/llvm/llvm-project/issues/74442

>From c3dedfd535f037fb54a8e04640f9332e655a9a7d Mon Sep 17 00:00:00 2001
From: Amy Huang 
Date: Thu, 7 Dec 2023 14:08:24 -0800
Subject: [PATCH] Fix to msvc::no_unique_address not interacting well with
 __declspec(empty_bases)

---
 clang/lib/AST/RecordLayoutBuilder.cpp |  4 +-
 .../Layout/ms-x86-declspec-empty_bases.cpp| 60 +++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp 
b/clang/lib/AST/RecordLayoutBuilder.cpp
index f1f2275da44dc..73b248ffe5899 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2936,8 +2936,8 @@ void MicrosoftRecordLayoutBuilder::layoutNonVirtualBase(
   }
 
   if (!FoundBase) {
-if (MDCUsesEBO && BaseDecl->isEmpty()) {
-  assert(BaseLayout.getNonVirtualSize() == CharUnits::Zero());
+if (MDCUsesEBO && BaseDecl->isEmpty() &&
+(BaseLayout.getNonVirtualSize() == CharUnits::Zero())) {
   BaseOffset = CharUnits::Zero();
 } else {
   // Otherwise, lay the base out at the end of the MDC.
diff --git a/clang/test/Layout/ms-x86-declspec-empty_bases.cpp 
b/clang/test/Layout/ms-x86-declspec-empty_bases.cpp
index cc13a980cb5db..4738ce5720f75 100644
--- a/clang/test/Layout/ms-x86-declspec-empty_bases.cpp
+++ b/clang/test/Layout/ms-x86-declspec-empty_bases.cpp
@@ -264,3 +264,63 @@ int _ = sizeof(G);
 // CHECK-NEXT:| [sizeof=12, align=4,
 // CHECK-NEXT:|  nvsize=12, nvalign=4]
 }
+
+namespace test5 {
+
+struct A {
+  int a;
+};
+struct B {
+  int b;
+};
+struct C {};
+struct __declspec(align(16)) D {};
+struct E {
+  [[msvc::no_unique_address]] C c;
+};
+struct __declspec(empty_bases) X : A, D, B, C, E {
+};
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::A
+// CHECK-NEXT:  0 |   int a
+// CHECK-NEXT:| [sizeof=4, align=4,
+// CHECK-NEXT:|  nvsize=4, nvalign=4]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::D (empty)
+// CHECK-NEXT:| [sizeof=16, align=16,
+// CHECK-NEXT:|  nvsize=0, nvalign=16]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::B
+// CHECK-NEXT:  0 |   int b
+// CHECK-NEXT:| [sizeof=4, align=4,
+// CHECK-NEXT:|  nvsize=4, nvalign=4]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::C (empty)
+// CHECK-NEXT:| [sizeof=1, align=1,
+// CHECK-NEXT:|  nvsize=0, nvalign=1]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::E (empty)
+// CHECK-NEXT:  0 |   struct test5::C c (empty)
+// CHECK-NEXT:| [sizeof=1, align=1,
+// CHECK-NEXT:|  nvsize=1, nvalign=1]
+
+// CHECK: *** Dumping AST Record Layout
+// CHECK-NEXT:  0 | struct test5::X
+// CHECK-NEXT:  0 |   struct test5::A (base)
+// CHECK-NEXT:  0 | int a
+// CHECK-NEXT:  0 |   struct test5::D (base) (empty)
+// CHECK-NEXT:  0 |   struct test5::C (base) (empty)
+// CHECK-NEXT:  4 |   struct test5::B (base)
+// CHECK-NEXT:  4 | int b
+// CHECK-NEXT:  8 |   struct test5::E (base) (empty)
+// CHECK-NEXT:  8 | struct test5::C c (empty)
+// CHECK-NEXT:| [sizeof=16, align=16,
+// CHECK-NEXT:|  nvsize=16, nvalign=16]
+
+int _ = sizeof(X);
+}

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