[PATCH] D43908: [RecordLayout] Only assert that fundamental type sizes are power of two on MSVC
This revision was automatically updated to reflect the committed changes.
Closed by commit rL326476: [RecordLayout] Only assert that fundamental type
sizes are power of two on MSVC (authored by mstorsjo, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D43908?vs=136471=136583#toc
Repository:
rL LLVM
https://reviews.llvm.org/D43908
Files:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
cfe/trunk/test/CodeGen/ms_struct-long-double.c
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -759,6 +759,10 @@
Warning<"ms_struct may not produce Microsoft-compatible layouts for classes "
"with base classes or virtual functions">,
DefaultError, InGroup;
+def warn_npot_ms_struct :
+ Warning<"ms_struct may not produce Microsoft-compatible layouts with
fundamental "
+ "data types with sizes that aren't a power of two">,
+ DefaultError, InGroup;
def err_section_conflict : Error<"%0 causes a section type conflict with %1">;
def err_no_base_classes : Error<"invalid use of '__super', %0 has no base
classes">;
def err_invalid_super_scope : Error<"invalid use of '__super', "
Index: cfe/trunk/test/CodeGen/ms_struct-long-double.c
===
--- cfe/trunk/test/CodeGen/ms_struct-long-double.c
+++ cfe/trunk/test/CodeGen/ms_struct-long-double.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-windows-gnu
-fdump-record-layouts %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-linux -fdump-record-layouts
-Wno-incompatible-ms-struct %s | FileCheck %s
+// RUN: not %clang_cc1 -emit-llvm-only -triple i686-linux
-fdump-record-layouts %s 2>&1 | FileCheck %s -check-prefix=ERROR
+
+struct ldb_struct {
+ char c;
+ long double ldb;
+} __attribute__((__ms_struct__));
+
+struct ldb_struct a;
+
+// CHECK: 0 | struct ldb_struct
+// CHECK-NEXT:0 | char c
+// CHECK-NEXT:4 | long double ldb
+// CHECK-NEXT: | [sizeof=16, align=4]
+
+// ERROR: error: ms_struct may not produce Microsoft-compatible layouts with
fundamental data types with sizes that aren't a power of two
Index: cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
===
--- cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
+++ cfe/trunk/lib/AST/RecordLayoutBuilder.cpp
@@ -1752,10 +1752,32 @@
QualType T = Context.getBaseElementType(D->getType());
if (const BuiltinType *BTy = T->getAs()) {
CharUnits TypeSize = Context.getTypeSizeInChars(BTy);
-assert(
-(llvm::isPowerOf2_64(TypeSize.getQuantity()) ||
- Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) &&
-"Non PowerOf2 size outside of GNU mode");
+
+if (!llvm::isPowerOf2_64(TypeSize.getQuantity())) {
+ assert(
+ !Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment()
&&
+ "Non PowerOf2 size in MSVC mode");
+ // Base types with sizes that aren't a power of two don't work
+ // with the layout rules for MS structs. This isn't an issue in
+ // MSVC itself since there are no such base data types there.
+ // On e.g. x86_32 mingw and linux, long double is 12 bytes though.
+ // Any structs involving that data type obviously can't be ABI
+ // compatible with MSVC regardless of how it is laid out.
+
+ // Since ms_struct can be mass enabled (via a pragma or via the
+ // -mms-bitfields command line parameter), this can trigger for
+ // structs that don't actually need MSVC compatibility, so we
+ // need to be able to sidestep the ms_struct layout for these types.
+
+ // Since the combination of -mms-bitfields together with structs
+ // like max_align_t (which contains a long double) for mingw is
+ // quite comon (and GCC handles it silently), just handle it
+ // silently there. For other targets that have ms_struct enabled
+ // (most probably via a pragma or attribute), trigger a diagnostic
+ // that defaults to an error.
+ if (!Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+Diag(D->getLocation(), diag::warn_npot_ms_struct);
+}
if (TypeSize > FieldAlign &&
llvm::isPowerOf2_64(TypeSize.getQuantity()))
FieldAlign = TypeSize;
Index: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
@@ -759,6 +759,10 @@
[PATCH] D43908: [RecordLayout] Only assert that fundamental type sizes are power of two on MSVC
compnerd accepted this revision. compnerd added a comment. Awesome, thanks, this makes me feel much more comfortable. https://reviews.llvm.org/D43908 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43908: [RecordLayout] Only assert that fundamental type sizes are power of two on MSVC
mstorsjo updated this revision to Diff 136471.
mstorsjo edited the summary of this revision.
mstorsjo added a comment.
Added an error-by-default diagnostic (just like the existing
warn_cxx_ms_struct) for this case, trigger it on all targets other than mingw
(where the situation is quite likely to occur, and GCC handles it silently).
@compnerd, do you like this better?
https://reviews.llvm.org/D43908
Files:
include/clang/Basic/DiagnosticSemaKinds.td
lib/AST/RecordLayoutBuilder.cpp
test/CodeGen/ms_struct-long-double.c
Index: test/CodeGen/ms_struct-long-double.c
===
--- /dev/null
+++ test/CodeGen/ms_struct-long-double.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-windows-gnu
-fdump-record-layouts %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-linux -fdump-record-layouts
-Wno-incompatible-ms-struct %s | FileCheck %s
+// RUN: not %clang_cc1 -emit-llvm-only -triple i686-linux
-fdump-record-layouts %s 2>&1 | FileCheck %s -check-prefix=ERROR
+
+struct ldb_struct {
+ char c;
+ long double ldb;
+} __attribute__((__ms_struct__));
+
+struct ldb_struct a;
+
+// CHECK: 0 | struct ldb_struct
+// CHECK-NEXT:0 | char c
+// CHECK-NEXT:4 | long double ldb
+// CHECK-NEXT: | [sizeof=16, align=4]
+
+// ERROR: error: ms_struct may not produce Microsoft-compatible layouts with
fundamental data types with sizes that aren't a power of two
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -1752,10 +1752,32 @@
QualType T = Context.getBaseElementType(D->getType());
if (const BuiltinType *BTy = T->getAs()) {
CharUnits TypeSize = Context.getTypeSizeInChars(BTy);
-assert(
-(llvm::isPowerOf2_64(TypeSize.getQuantity()) ||
- Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) &&
-"Non PowerOf2 size outside of GNU mode");
+
+if (!llvm::isPowerOf2_64(TypeSize.getQuantity())) {
+ assert(
+ !Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment()
&&
+ "Non PowerOf2 size in MSVC mode");
+ // Base types with sizes that aren't a power of two don't work
+ // with the layout rules for MS structs. This isn't an issue in
+ // MSVC itself since there are no such base data types there.
+ // On e.g. x86_32 mingw and linux, long double is 12 bytes though.
+ // Any structs involving that data type obviously can't be ABI
+ // compatible with MSVC regardless of how it is laid out.
+
+ // Since ms_struct can be mass enabled (via a pragma or via the
+ // -mms-bitfields command line parameter), this can trigger for
+ // structs that don't actually need MSVC compatibility, so we
+ // need to be able to sidestep the ms_struct layout for these types.
+
+ // Since the combination of -mms-bitfields together with structs
+ // like max_align_t (which contains a long double) for mingw is
+ // quite comon (and GCC handles it silently), just handle it
+ // silently there. For other targets that have ms_struct enabled
+ // (most probably via a pragma or attribute), trigger a diagnostic
+ // that defaults to an error.
+ if (!Context.getTargetInfo().getTriple().isWindowsGNUEnvironment())
+Diag(D->getLocation(), diag::warn_npot_ms_struct);
+}
if (TypeSize > FieldAlign &&
llvm::isPowerOf2_64(TypeSize.getQuantity()))
FieldAlign = TypeSize;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -759,6 +759,10 @@
Warning<"ms_struct may not produce Microsoft-compatible layouts for classes "
"with base classes or virtual functions">,
DefaultError, InGroup;
+def warn_npot_ms_struct :
+ Warning<"ms_struct may not produce Microsoft-compatible layouts with
fundamental "
+ "data types with sizes that aren't a power of two">,
+ DefaultError, InGroup;
def err_section_conflict : Error<"%0 causes a section type conflict with %1">;
def err_no_base_classes : Error<"invalid use of '__super', %0 has no base
classes">;
def err_invalid_super_scope : Error<"invalid use of '__super', "
Index: test/CodeGen/ms_struct-long-double.c
===
--- /dev/null
+++ test/CodeGen/ms_struct-long-double.c
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-windows-gnu -fdump-record-layouts %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-linux -fdump-record-layouts -Wno-incompatible-ms-struct %s | FileCheck
[PATCH] D43908: [RecordLayout] Only assert that fundamental type sizes are power of two on MSVC
compnerd accepted this revision. compnerd added a comment. Ugh, really not a fan of this change. Repository: rC Clang https://reviews.llvm.org/D43908 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43908: [RecordLayout] Only assert that fundamental type sizes are power of two on MSVC
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang https://reviews.llvm.org/D43908 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D43908: [RecordLayout] Only assert that fundamental type sizes are power of two on MSVC
mstorsjo created this revision.
mstorsjo added reviewers: efriedma, compnerd.
This fixes using the ms_struct attribute together with long double on e.g.
x86-32 linux.
Repository:
rC Clang
https://reviews.llvm.org/D43908
Files:
lib/AST/RecordLayoutBuilder.cpp
test/CodeGen/ms_struct-long-double.c
Index: test/CodeGen/ms_struct-long-double.c
===
--- /dev/null
+++ test/CodeGen/ms_struct-long-double.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-windows-gnu
-fdump-record-layouts %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-linux -fdump-record-layouts %s
| FileCheck %s
+
+struct ldb_struct {
+ char c;
+ long double ldb;
+} __attribute__((__ms_struct__));
+
+struct ldb_struct a;
+
+// CHECK: 0 | struct ldb_struct
+// CHECK-NEXT:0 | char c
+// CHECK-NEXT:4 | long double ldb
+// CHECK-NEXT: | [sizeof=16, align=4]
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -1754,8 +1754,8 @@
CharUnits TypeSize = Context.getTypeSizeInChars(BTy);
assert(
(llvm::isPowerOf2_64(TypeSize.getQuantity()) ||
- Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) &&
-"Non PowerOf2 size outside of GNU mode");
+ !Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment())
&&
+"Non PowerOf2 size in MSVC mode");
if (TypeSize > FieldAlign &&
llvm::isPowerOf2_64(TypeSize.getQuantity()))
FieldAlign = TypeSize;
Index: test/CodeGen/ms_struct-long-double.c
===
--- /dev/null
+++ test/CodeGen/ms_struct-long-double.c
@@ -0,0 +1,14 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-windows-gnu -fdump-record-layouts %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm-only -triple i686-linux -fdump-record-layouts %s | FileCheck %s
+
+struct ldb_struct {
+ char c;
+ long double ldb;
+} __attribute__((__ms_struct__));
+
+struct ldb_struct a;
+
+// CHECK: 0 | struct ldb_struct
+// CHECK-NEXT:0 | char c
+// CHECK-NEXT:4 | long double ldb
+// CHECK-NEXT: | [sizeof=16, align=4]
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -1754,8 +1754,8 @@
CharUnits TypeSize = Context.getTypeSizeInChars(BTy);
assert(
(llvm::isPowerOf2_64(TypeSize.getQuantity()) ||
- Context.getTargetInfo().getTriple().isWindowsGNUEnvironment()) &&
-"Non PowerOf2 size outside of GNU mode");
+ !Context.getTargetInfo().getTriple().isWindowsMSVCEnvironment()) &&
+"Non PowerOf2 size in MSVC mode");
if (TypeSize > FieldAlign &&
llvm::isPowerOf2_64(TypeSize.getQuantity()))
FieldAlign = TypeSize;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
