[clang] Stub out gcc_struct attribute #71148
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-codegen Author: Dan Klishch (DanShaders) ChangesThis implements gcc_struct attribute with the behavior similar to one in GCC. In particular, when C++ ABI is not "Microsoft" (i. e. when ItaniumRecordBuilder is used), [[gcc_struct]] will locally cancel the effect of -mms-bitfields on a record. If -mms-bitfields is not supplied and is not a default behavior on a target, [[gcc_struct]] will be a no-op. This hopefully should provide compatibility with MinGW binaries. On the other hand, if C++ ABI is "Microsoft" and a record is annotated with [[gcc_struct]], we do not have any compatibility concerns, so I decided not to complicate MicrosoftRecordLayoutBuilder and just to fully switch record layout to GenericItanium. Note that -mno-ms-bitfields is no longer always a no-op. On Changes related to ms_struct should not hopefully cause any observable differences, thus the patch itself is fully backwards-compatible. Fixes #24757 Full diff: https://github.com/llvm/llvm-project/pull/71148.diff 15 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index 60b549999c155e5..34ec6c22481837e 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -3635,6 +3635,13 @@ def MSStruct : InheritableAttr {
let SimpleHandler = 1;
}
+def GCCStruct : InheritableAttr {
+ let Spellings = [GCC<"gcc_struct">];
+ let Subjects = SubjectList<[Record]>;
+ let Documentation = [Undocumented];
+ let SimpleHandler = 1;
+}
+
def DLLExport : InheritableAttr, TargetSpecificAttr<TargetHasDLLImportExport> {
let Spellings = [Declspec<"dllexport">, GCC<"dllexport">];
let Subjects = SubjectList<[Function, Var, CXXRecord, ObjCInterface]>;
diff --git a/clang/include/clang/Basic/LangOptions.def b/clang/include/clang/Basic/LangOptions.def
index c0ea4ecb9806a5b..afbb3c724a8d193 100644
--- a/clang/include/clang/Basic/LangOptions.def
+++ b/clang/include/clang/Basic/LangOptions.def
@@ -148,7 +148,6 @@ LANGOPT(ExternCNoUnwind , 1, 0, "Assume extern C functions don't unwind")
LANGOPT(TraditionalCPP , 1, 0, "traditional CPP emulation")
LANGOPT(RTTI , 1, 1, "run-time type information")
LANGOPT(RTTIData , 1, 1, "emit run-time type information data")
-LANGOPT(MSBitfields , 1, 0, "Microsoft-compatible structure layout")
LANGOPT(MSVolatile , 1, 0, "Microsoft-compatible volatile loads and stores")
LANGOPT(Freestanding, 1, 0, "freestanding implementation")
LANGOPT(NoBuiltin , 1, 0, "disable builtin functions")
diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h
index 20a8ada60e0fe51..07eae8dad61813c 100644
--- a/clang/include/clang/Basic/LangOptions.h
+++ b/clang/include/clang/Basic/LangOptions.h
@@ -502,6 +502,8 @@ class LangOptions : public LangOptionsBase {
// received as a result of a standard operator new (-fcheck-new)
bool CheckNew = false;
+ std::optional<bool> MSBitfields;
+
LangOptions();
/// Set language defaults for the given input language and
diff --git a/clang/include/clang/Basic/TargetInfo.h b/clang/include/clang/Basic/TargetInfo.h
index b3c5cbfb319f01f..6865b6e23bc9628 100644
--- a/clang/include/clang/Basic/TargetInfo.h
+++ b/clang/include/clang/Basic/TargetInfo.h
@@ -1724,6 +1724,10 @@ class TargetInfo : public TransferrableTargetInfo,
/// Whether to support HIP image/texture API's.
virtual bool hasHIPImageSupport() const { return true; }
+ virtual bool defaultsToMsStruct() const {
+ return getCXXABI().isMicrosoft() || getTriple().isWindowsGNUEnvironment();
+ }
+
protected:
/// Copy type and layout related info.
void copyAuxTarget(const TargetInfo *Aux);
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index fcf6a4b2ccb2369..bde324a51c5f77f 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4373,8 +4373,7 @@ def : Joined<["-"], "mmacosx-version-min=">,
Group<m_Group>, Alias<mmacos_version_min_EQ>;
def mms_bitfields : Flag<["-"], "mms-bitfields">, Group<m_Group>,
Visibility<[ClangOption, CC1Option]>,
- HelpText<"Set the default structure layout to be compatible with the Microsoft compiler standard">,
- MarshallingInfoFlag<LangOpts<"MSBitfields">>;
+ HelpText<"Set the default structure layout to be compatible with the Microsoft compiler standard">;
def moutline : Flag<["-"], "moutline">, Group<f_clang_Group>,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Enable function outlining (AArch64 only)">;
@@ -4382,6 +4381,7 @@ def mno_outline : Flag<["-"], "mno-outline">, Group<f_clang_Group>,
Visibility<[ClangOption, CC1Option]>,
HelpText<"Disable function outlining (AArch64 only)">;
def mno_ms_bitfields : Flag<["-"], "mno-ms-bitfields">, Group<m_Group>,
+ Visibility<[ClangOption, CC1Option]>,
HelpText<"Do not set the default structure layout to be compatible with the Microsoft compiler standard">;
def mskip_rax_setup : Flag<["-"], "mskip-rax-setup">, Group<m_Group>,
Visibility<[ClangOption, CC1Option]>,
diff --git a/clang/lib/AST/Decl.cpp b/clang/lib/AST/Decl.cpp
index 6efc177d61c03ba..352af5899a0c02a 100644
--- a/clang/lib/AST/Decl.cpp
+++ b/clang/lib/AST/Decl.cpp
@@ -5009,7 +5009,7 @@ void RecordDecl::completeDefinition() {
ASTContext &Ctx = getASTContext();
// Layouts are dumped when computed, so if we are dumping for all complete
- // types, we need to force usage to get types that wouldn't be used elsewhere.
+ // types, we need to force usage to pet types that wouldn't be used elsewhere.
if (Ctx.getLangOpts().DumpRecordLayoutsComplete)
(void)Ctx.getASTRecordLayout(this);
}
@@ -5018,7 +5018,12 @@ void RecordDecl::completeDefinition() {
/// This which can be turned on with an attribute, pragma, or the
/// -mms-bitfields command-line option.
bool RecordDecl::isMsStruct(const ASTContext &C) const {
- return hasAttr<MSStructAttr>() || C.getLangOpts().MSBitfields == 1;
+ if (hasAttr<MSStructAttr>())
+ return true;
+ if (hasAttr<GCCStructAttr>())
+ return false;
+ return C.getLangOpts().MSBitfields.value_or(
+ C.getTargetInfo().defaultsToMsStruct());
}
void RecordDecl::reorderDecls(const SmallVectorImpl<Decl *> &Decls) {
diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
index 5d4f930fca50e2b..f38c6432a9992a6 100644
--- a/clang/lib/AST/RecordLayoutBuilder.cpp
+++ b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -2447,8 +2447,9 @@ static bool mustSkipTailPadding(TargetCXXABI ABI, const CXXRecordDecl *RD) {
llvm_unreachable("bad tail-padding use kind");
}
-static bool isMsLayout(const ASTContext &Context) {
- return Context.getTargetInfo().getCXXABI().isMicrosoft();
+static bool isMsLayout(const ASTContext &Context, const RecordDecl *RD) {
+ return Context.getTargetInfo().getCXXABI().isMicrosoft() &&
+ RD->isMsStruct(Context);
}
// This section contains an implementation of struct layout that is, up to the
@@ -3341,7 +3342,7 @@ ASTContext::getASTRecordLayout(const RecordDecl *D) const {
const ASTRecordLayout *NewEntry = nullptr;
- if (isMsLayout(*this)) {
+ if (isMsLayout(*this, D)) {
if (const auto *RD = dyn_cast<CXXRecordDecl>(D)) {
EmptySubobjectMap EmptySubobjects(*this, RD);
MicrosoftRecordLayoutBuilder Builder(*this, &EmptySubobjects);
@@ -3617,7 +3618,7 @@ static void DumpRecordLayout(raw_ostream &OS, const RecordDecl *RD,
bool HasOwnVBPtr = Layout.hasOwnVBPtr();
// Vtable pointer.
- if (CXXRD->isDynamicClass() && !PrimaryBase && !isMsLayout(C)) {
+ if (CXXRD->isDynamicClass() && !PrimaryBase && !isMsLayout(C, RD)) {
PrintOffset(OS, Offset, IndentLevel);
OS << '(' << *RD << " vtable pointer)\n";
} else if (HasOwnVFPtr) {
@@ -3717,7 +3718,7 @@ static void DumpRecordLayout(raw_ostream &OS, const RecordDecl *RD,
PrintIndentNoOffset(OS, IndentLevel - 1);
OS << "[sizeof=" << Layout.getSize().getQuantity();
- if (CXXRD && !isMsLayout(C))
+ if (CXXRD && !isMsLayout(C, RD))
OS << ", dsize=" << Layout.getDataSize().getQuantity();
OS << ", align=" << Layout.getAlignment().getQuantity();
if (C.getTargetInfo().defaultsToAIXPowerAlignment())
@@ -3756,7 +3757,7 @@ void ASTContext::DumpRecordLayout(const RecordDecl *RD, raw_ostream &OS,
OS << "\nLayout: ";
OS << "<ASTRecordLayout\n";
OS << " Size:" << toBits(Info.getSize()) << "\n";
- if (!isMsLayout(*this))
+ if (!isMsLayout(*this, RD))
OS << " DataSize:" << toBits(Info.getDataSize()) << "\n";
OS << " Alignment:" << toBits(Info.getAlignment()) << "\n";
if (Target->defaultsToAIXPowerAlignment())
diff --git a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
index cbfa79e10bfefcc..88a766adfe82a1c 100644
--- a/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
+++ b/clang/lib/CodeGen/CGRecordLayoutBuilder.cpp
@@ -104,10 +104,7 @@ struct CGRecordLowering {
/// fields of the same formal type. We want to emit a layout with
/// these discrete storage units instead of combining them into a
/// continuous run.
- bool isDiscreteBitFieldABI() {
- return Context.getTargetInfo().getCXXABI().isMicrosoft() ||
- D->isMsStruct(Context);
- }
+ bool isDiscreteBitFieldABI() { return D->isMsStruct(Context); }
/// Helper function to check if we are targeting AAPCS.
bool isAAPCS() const {
@@ -122,7 +119,8 @@ struct CGRecordLowering {
///
/// Note specifically that the ms_struct attribute doesn't change this.
bool isOverlappingVBaseABI() {
- return !Context.getTargetInfo().getCXXABI().isMicrosoft();
+ return !Context.getTargetInfo().getCXXABI().isMicrosoft() ||
+ !D->isMsStruct(Context);
}
/// Wraps llvm::Type::getIntNTy with some implicit arguments.
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 79f7fba22570746..2602d1f6bedee9f 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -5645,9 +5645,13 @@ void Clang::ConstructJob(Compilation &C, const JobAction &JA,
if (KernelOrKext && RawTriple.isOSDarwin())
CmdArgs.push_back("-fforbid-guard-variables");
- if (Args.hasFlag(options::OPT_mms_bitfields, options::OPT_mno_ms_bitfields,
- Triple.isWindowsGNUEnvironment())) {
- CmdArgs.push_back("-mms-bitfields");
+ if (Args.hasArg(options::OPT_mms_bitfields) ||
+ Args.hasArg(options::OPT_mno_ms_bitfields)) {
+ if (Args.hasFlag(options::OPT_mms_bitfields, options::OPT_mno_ms_bitfields,
+ false))
+ CmdArgs.push_back("-mms-bitfields");
+ else
+ CmdArgs.push_back("-mno-ms-bitfields");
}
if (Triple.isWindowsGNUEnvironment()) {
diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp
index 637c6a35af6532b..a0576441c5d24a7 100644
--- a/clang/lib/Frontend/CompilerInvocation.cpp
+++ b/clang/lib/Frontend/CompilerInvocation.cpp
@@ -3619,6 +3619,13 @@ void CompilerInvocationBase::GenerateLangArgs(const LangOptions &Opts,
GenerateArg(Consumer, OPT_fcxx_abi_EQ,
TargetCXXABI::getSpelling(*Opts.CXXABI));
+ if (Opts.MSBitfields.has_value()) {
+ if (Opts.MSBitfields.value())
+ GenerateArg(Consumer, OPT_mms_bitfields);
+ else
+ GenerateArg(Consumer, OPT_mno_ms_bitfields);
+ }
+
if (Opts.RelativeCXXABIVTables)
GenerateArg(Consumer, OPT_fexperimental_relative_cxx_abi_vtables);
else
@@ -4153,6 +4160,11 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args,
options::OPT_fno_experimental_relative_cxx_abi_vtables,
TargetCXXABI::usesRelativeVTables(T));
+ if (Args.hasArg(options::OPT_mms_bitfields) ||
+ Args.hasArg(options::OPT_mno_ms_bitfields))
+ Opts.MSBitfields = Args.hasFlag(options::OPT_mms_bitfields,
+ options::OPT_mno_ms_bitfields, false);
+
// RTTI is on by default.
bool HasRTTI = Args.hasFlag(options::OPT_frtti, options::OPT_fno_rtti, true);
Opts.OmitVTableRTTI =
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index a8bad12b670fc75..f925972e7d3d104 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -18192,9 +18192,7 @@ ExprResult Sema::VerifyBitField(SourceLocation FieldLoc,
// ABI.
bool CStdConstraintViolation =
BitfieldIsOverwide && !getLangOpts().CPlusPlus;
- bool MSBitfieldViolation =
- Value.ugt(TypeStorageSize) &&
- (IsMsStruct || Context.getTargetInfo().getCXXABI().isMicrosoft());
+ bool MSBitfieldViolation = Value.ugt(TypeStorageSize) && IsMsStruct;
if (CStdConstraintViolation || MSBitfieldViolation) {
unsigned DiagWidth =
CStdConstraintViolation ? TypeWidth : TypeStorageSize;
diff --git a/clang/lib/Sema/SemaDeclCXX.cpp b/clang/lib/Sema/SemaDeclCXX.cpp
index be79defbbfac6f1..9d42157534eda47 100644
--- a/clang/lib/Sema/SemaDeclCXX.cpp
+++ b/clang/lib/Sema/SemaDeclCXX.cpp
@@ -7265,7 +7265,14 @@ void Sema::CheckCompletedCXXClass(Scope *S, CXXRecordDecl *Record) {
// language option (as opposed to via a pragma or attribute), as
// the option -mms-bitfields otherwise essentially makes it impossible
// to build C++ code, unless this diagnostic is turned off.
- if (Record->isMsStruct(Context) && !Context.getLangOpts().MSBitfields &&
+ // We do not support ABI compatibility mode in the other direction,
+ // however. Whenever we encounter type with gcc_struct attribute on a
+ // target with Microsoft C++ ABI, we switch layout completely to
+ // Itanium. Therefore, record ABI would be fully compatible and there
+ // is no point in emitting the diagnostic.
+ if (!Context.getLangOpts().MSBitfields.has_value() &&
+ Record->isMsStruct(Context) &&
+ !Context.getTargetInfo().defaultsToMsStruct() &&
(Record->isPolymorphic() || Record->getNumBases())) {
Diag(Record->getLocation(), diag::warn_cxx_ms_struct);
}
diff --git a/clang/test/CodeGenCXX/ms-bitfield-class-layout.cpp b/clang/test/CodeGenCXX/ms-bitfield-class-layout.cpp
new file mode 100644
index 000000000000000..6fefd3f608bd051
--- /dev/null
+++ b/clang/test/CodeGenCXX/ms-bitfield-class-layout.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-none %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=ITANIUM %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-windows-msvc %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=MSVC %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-windows-gnu %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=ITANIUM %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-none -mms-bitfields %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=ITANIUM %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-windows-msvc -mms-bitfields %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=MSVC %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-windows-gnu -mms-bitfields %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=ITANIUM %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-none -mno-ms-bitfields %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=ITANIUM %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-windows-msvc -mno-ms-bitfields %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=ITANIUM-EXCEPT-FOR-S3 %s < %t
+// RUN: %clang_cc1 -fdump-record-layouts -Wno-incompatible-ms-struct -emit-llvm-only -triple x86_64-none-windows-gnu -mno-ms-bitfields %s -o %t.ll > %t
+// RUN: FileCheck --check-prefix=ITANIUM %s < %t
+
+// ITANIUM:(S1 vtable pointer)
+// ITANIUM-EXCEPT-FOR-S3:(S1 vtable pointer)
+// MSVC:(S1 vftable pointer)
+struct S1 {
+ virtual void f() {}
+};
+
+// ITANIUM:(S2 vtable pointer)
+// ITANIUM-EXCEPT-FOR-S3:(S2 vtable pointer)
+// MSVC:(S2 vtable pointer)
+struct [[gnu::gcc_struct]] S2 {
+ virtual void f() {}
+};
+
+// ITANIUM:(S3 vtable pointer)
+// ITANIUM-EXCEPT-FOR-S3:(S3 vftable pointer)
+// MSVC:(S3 vftable pointer)
+struct [[gnu::ms_struct]] S3 {
+ virtual void f() {}
+};
+
+void use(S1 s1, S2 s2, S3 s3) { s1.f(); s2.f(); s3.f(); }
diff --git a/clang/test/CodeGenCXX/ms_struct-gcc_struct.cpp b/clang/test/CodeGenCXX/ms_struct-gcc_struct.cpp
new file mode 100644
index 000000000000000..803ca59ffca584b
--- /dev/null
+++ b/clang/test/CodeGenCXX/ms_struct-gcc_struct.cpp
@@ -0,0 +1,29 @@
+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64-none-none -verify=default-itanium %s
+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64-none-windows-msvc -verify=default-msvc %s
+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64-none-windows-gnu -verify=default-msvc %s
+// RUN: %clang_cc1 -mms-bitfields -emit-llvm-only -triple x86_64-none-none -verify=default-msvc %s
+// RUN: %clang_cc1 -mms-bitfields -emit-llvm-only -triple x86_64-none-windows-msvc -verify=default-msvc %s
+// RUN: %clang_cc1 -mms-bitfields -emit-llvm-only -triple x86_64-none-windows-gnu -verify=default-msvc %s
+// RUN: %clang_cc1 -mno-ms-bitfields -emit-llvm-only -triple x86_64-none-none -verify=default-itanium %s
+// RUN: %clang_cc1 -mno-ms-bitfields -emit-llvm-only -triple x86_64-none-windows-msvc -verify=default-itanium %s
+// RUN: %clang_cc1 -mno-ms-bitfields -emit-llvm-only -triple x86_64-none-windows-gnu -verify=default-itanium %s
+
+struct [[gnu::gcc_struct]] S1 {
+ unsigned a : 1;
+ unsigned long long b : 1;
+};
+
+struct S2 {
+ unsigned a : 1;
+ unsigned long long b : 1;
+};
+
+struct [[gnu::ms_struct]] S3 {
+ unsigned a : 1;
+ unsigned long long b : 1;
+};
+
+int a1[sizeof(S1) == 8 ? 1 : -1];
+int a2[sizeof(S2) == 8 ? 1 : -1]; // default-msvc-error {{'a2' declared as an array with a negative size}}
+int a3[sizeof(S2) == 16 ? 1 : -1]; // default-itanium-error {{'a3' declared as an array with a negative size}}
+int a4[sizeof(S3) == 16 ? 1 : -1];
diff --git a/clang/test/Driver/ms-bitfields.c b/clang/test/Driver/ms-bitfields.c
index 031ed41e2aad678..4cd3f1380e41acf 100644
--- a/clang/test/Driver/ms-bitfields.c
+++ b/clang/test/Driver/ms-bitfields.c
@@ -1,8 +1,13 @@
-// RUN: %clang -### -target x86_64-linux-gnu %s 2>&1 | FileCheck %s -check-prefix=NO-MSBITFIELDS
-// RUN: %clang -### -target x86_64-windows-gnu %s 2>&1 | FileCheck %s -check-prefix=MSBITFIELDS
+// RUN: %clang -### -target x86_64-linux-gnu %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-MSBITFIELDS
+// RUN: %clang -### -target x86_64-windows-gnu %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-MSBITFIELDS
+// RUN: %clang -### -target x86_64-windows-msvc %s 2>&1 | FileCheck %s -check-prefix=DEFAULT-MSBITFIELDS
+// RUN: %clang -### -mms-bitfields %s 2>&1 | FileCheck %s -check-prefix=MSBITFIELDS
+// RUN: %clang -### -mno-ms-bitfields %s 2>&1 | FileCheck %s -check-prefix=NO-MSBITFIELDS
// RUN: %clang -### -mno-ms-bitfields -mms-bitfields %s 2>&1 | FileCheck %s -check-prefix=MSBITFIELDS
// RUN: %clang -### -mms-bitfields -mno-ms-bitfields %s 2>&1 | FileCheck %s -check-prefix=NO-MSBITFIELDS
// MSBITFIELDS: -mms-bitfields
-// NO-MSBITFIELDS-NOT: -mms-bitfields
+// NO-MSBITFIELDS: -mno-ms-bitfields
+// DEFAULT-MSBITFIELDS-NOT: -mms-bitfields
+// DEFAULT-MSBITFIELDS-NOT: -mno-ms-bitfields
|
DanShaders
force-pushed
the
gcc-struct
branch
from
1912ae4
to
ea8a7ea
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
And, I guess, CC @erichkeane |
DanShaders
force-pushed
the
gcc-struct
branch
3 times, most recently
from
7e268c2
to
0d6728f
Compare
DanShaders
changed the title
DanShaders
force-pushed
the
gcc-struct
branch
from
0d6728f
to
1912422
Compare
There was a problem hiding this comment.
This needs attribute documentation and a release note.
Additionally, I think we should prohibit using this in Microsoft mode (that is, diagnose that) rather than having it be a no-op. We can enable it in the future, but if we permit it, folks will assume it works.
DanShaders
force-pushed
the
gcc-struct
branch
from
1912422
to
1bfe264
Compare
There was a problem hiding this comment.
Docs need work, code is fine from what I can tell, but @efriedma-quic or @rjmccall need to take a look at the ABI components.
clang/lib/AST/Decl.cpp
Outdated
| return true; | ||
| if (hasAttr<GCCStructAttr>()) | ||
| return false; | ||
| return C.getLangOpts().MSBitfields.value_or( |
There was a problem hiding this comment.
Is there a reason we don't just set the lang opt correctly for the target? I'm pretty sure language option defaults depending on target settings is well-established.
There was a problem hiding this comment.
But there is no TargetInfo object when I set MSBitfields in CompilerInvocation::ParseLangArgs.
There was a problem hiding this comment.
And, for example, for C++ ABI we do the same:
llvm-project/clang/lib/AST/ASTContext.cpp
Lines 813 to 816 in d19616f
There was a problem hiding this comment.
You have a target triple, and you have the base CXXABI. What information are you looking to get from the TargetInfo?
There was a problem hiding this comment.
I still don't quite understand what you want.
Yes, I can compute default value while parsing arguments, but I will duplicate logic of ASTContext::getCXXABIKind. And on top of that, I won't be able to remove TargetInfo::defaultsToMsStruct either, since I use it in SemaDeclCxx.
There was a problem hiding this comment.
I always prefer to have less complicated logic in the options types, because I've seen it get out of hand. I don't feel too strongly about this, though, and I do see how you have uses of the components feeding into it.
|
@rjmccall Would you mind merging this then? (I don't have write access) |
|
I agree with the Sema/AST-level LGTM (but also don't feel comfortable approving the driver changes) |
| @@ -3618,6 +3618,13 @@ void CompilerInvocationBase::GenerateLangArgs(const LangOptions &Opts, | |||
| GenerateArg(Consumer, OPT_fcxx_abi_EQ, | |||
| TargetCXXABI::getSpelling(*Opts.CXXABI)); | |||
|
|
|||
| if (Opts.MSBitfields.has_value()) { | |||
| if (Opts.MSBitfields.value()) | |||
| GenerateArg(Consumer, OPT_mms_bitfields); | |||
There was a problem hiding this comment.
If we use BoolOption in Options.td, we won't need this custom code.
| @@ -4393,15 +4393,15 @@ def : Joined<["-"], "mmacosx-version-min=">, | |||
| Group<m_Group>, Alias<mmacos_version_min_EQ>; | |||
| def mms_bitfields : Flag<["-"], "mms-bitfields">, Group<m_Group>, | |||
| Visibility<[ClangOption, CC1Option]>, | |||
| HelpText<"Set the default structure layout to be compatible with the Microsoft compiler standard">, | |||
| MarshallingInfoFlag<LangOpts<"MSBitfields">>; | |||
| HelpText<"Set the default structure layout to be compatible with the Microsoft compiler standard">; | |||
There was a problem hiding this comment.
The Driver and Frontend changes seem unneeded? -mno-ms-bitfields can remain a driver-only option that is not in CC1.
It's a convention that Driver supports both -mxxx and -mno-xxx while only one is supported by CC1.
There was a problem hiding this comment.
Once again, yes, I can theoretically use BoolOption and leave everything as it is. However, this requires computing default value for -mms-bitfields in driver, which is possible but will result in code duplication and is in general less clean that the current solution. I provided some more details in the reply to rjmccall above.
In case you are strongly against breaking the convention, I can alteranatively make both -mms-bitfields and -mno-ms-bitfields driver-only options and invent a new enum option for cc1 along the lines of -flayout-compatiblity-type={default,microsoft,itanium}. I should note that this would only slightly change the way I pass arguments from the driver to cc1.
DanShaders
force-pushed
the
gcc-struct
branch
2 times, most recently
from
7baf4c5
to
fc6a512
Compare
|
@MaskRay, I figured the alternative solution with the completely decoupled driver/frontend flags might actually be better than my initial approach. The only issue I see with it is that it requires Would you mind taking a look at this again? P.S.: Probably, |
|
This patch changes For the |
Let me share a bit of background here. While porting SerenityOS's libraries to Windows and, specifically, to While we probably won't enable
This won't change anything observable at all. When
As I said, supporting
It's not clear to me how to implement this in a simpler way. The requirements were the following:
|
|
I am now confused by the subject "[clang] Stub out gcc_struct attribute". SerenityOS is an OS, independent from Windows. What does porting it to Windows mean? Build some SerenityOS components on Windows targeting the PE object file format? I am sure there are many other differences between ELF/PE struct layouts other than those controlled by https://gitlab.com/qemu-project/qemu/-/issues/1782#note_1495842591 seems like ignored
|
In this case, Dan is referring to future plans to port the Ladybird browser to x86_64-windows-msvc. The browser and its ports use the system libraries from SerenityOS in the implementation. |
The user-facing change is
It's not like supporting As Andrew said, I indeed was referring to code used by cross-platform Ladybird browser. Microsoft bit-field layout didn't break an overly-specific regression test but rendered unusable double to string conversion. The culprit was the following snippet: union Extractor {
double value;
struct {
bool sign : 1;
u32 exponent : 11;
u64 mantissa : 52;
};
};According to MSVC ABI, there should be padding between fields. I hope you agree that this is not an intuitive and expected behavior. Additionally, I later found that, in Wasi implementation, we have more bit-fields that rely on the absence of padding which are susceptible to the same problem (they look like |
While it is implemented as an x86 specific option in GCC right now, that doesn't mean that it only is supposed to have an effect for x86. Upstream GCC only supports Windows on x86, and my recollection is that lots of the Windows specific logic is located in x86 specific files, even if the same logic also should apply for Windows on any other architecture - it's just not implemented (yet). As implemented in Clang, the option works for any MinGW target. As an example: struct field {
unsigned char a : 4;
unsigned int b : 4;
};
int size = sizeof(struct field);$ clang -target x86_64-windows-gnu -S -o - bitfields.c | grep -A1 size
.globl size # @size
.p2align 2, 0x0
size:
.long 8 # 0x8
$ clang -target x86_64-windows-gnu -S -o - bitfields.c -mno-ms-bitfields | grep -A1 size
.globl size # @size
.p2align 2, 0x0
size:
.long 4 # 0x4
$ clang -target aarch64-windows-gnu -S -o - bitfields.c | grep -A1 size
.globl size // @size
.p2align 2, 0x0
size:
.word 8 // 0x8
$ clang -target aarch64-windows-gnu -S -o - bitfields.c -mno-ms-bitfields | grep -A1 size
.globl size // @size
.p2align 2, 0x0
size:
.word 4 // 0x4
Yes, this would clearly be good to have. For orthogonality, it would be good to have both As noted somewhere (I don't see the comment to quote right now), the MS bitfield packing logic (as enabled by I don't know and can't speculate about what the implications would be for being able to switch to GCC struct layout in the MSVC C++ ABI, though. For individual structs, I guess it should be doable (I'm only thinking for trivial-ish structs like the one in my example above, right now though). For anything relating to C++ classes and the mechanisms behind them, I'm pretty sure one doesn't want to change how anything of that works. Therefore I don't think it's too relevant to implement |
It is indeed an unexpected thing. However it is possible to work around it for all these cases; if you declare the inner structure like this: Then there will be no extra padding between the fields. |
|
Yes, I know by now :) But this requires using the same type for all the bit-fields which might lead to unnecessary casts in the algorithm itself. And the other case is not as easy to fix. |
I want to only alter the layout of explicit fields in the class with the future MSVC I should stress that regardless of original motivation the additional complexity of Also, do we want to support |
|
One more thing. Re binary compatibility concerns: |
I guess this is for historical reasons. Originally the MinGW target had this as an opt-in, and this was indeed a silent ABI mismatch. Users who needed to use affected structs and interact with Microsoft APIs had to remember to build their code with |
|
Okay, @mstorsjo @MaskRay, what is the way forward? Am I right that, as for the user-facing changes, |
|
I'm totally not authoritative for these things, but...
Sounds quite fine for me
Silently ignoring options is clearly not good IMO, so either we warn about them or implement them
FWIW I wasn't even aware that it was possible to pick a nondefault C++ ABI, so I don't have a strong opinion on this matter. If it works and doesn't create inconsistencies, then I don't mind, but I guess the regular Clang maintainers have more of a final say on that.
I would be totally fine with that. |
|
It sounds like we're talking about extending the behavior of these attributes, and I'd like to make sure that whatever we do here is acceptable to GCC. That is, if we're going to start accepting these attributes on new targets which GCC doesn't currently support, we should make sure GCC agrees that these attributes should have that effect on those targets, in case GCC ever supports those targets. I agree that it would make the most sense to stage this as two separate patches, one to just add |
|
@rjmccall At the same time, I agree that inventing behavior for attributes in gnu:: namespace feels wrong. So, what do think about putting gcc_struct into @@ -3672,7 +3672,7 @@ def MSStruct : InheritableAttr {
}
def GCCStruct : InheritableAttr {
- let Spellings = [GCC<"gcc_struct">];
+ let Spellings = [CXX11<"clang", "gcc_struct">, C23<"clang", "gcc_struct">];
let Subjects = SubjectList<[Record]>;
let Documentation = [MSStructDocs]; // Covers this attribute too.
let SimpleHandler = 1; |
My question is actually more about supporting |
|
Hmm, but |
|
Right, I'd just like to make sure that we're not deepening a divergence here. It would be good to get agreement from the GCC devs that they think |
I'm not a GCC developer, but I would not think that GCC would consider this an x86-only feature. It just so happens that (upstream) GCC only supports Windows on x86. But MSVC does their own funky bitfield packing on all architectures - so it seems reasonable to want to be able to match it on all architectures. |
DanShaders
force-pushed
the
gcc-struct
branch
from
f2ca9e7
to
b26dd88
Compare
|
@MaskRay Bump |
This commit implements gcc_struct attribute with the behavior similar to
one in GCC. Current behavior is as follows:
When C++ ABI is not "Microsoft" (i. e. when
ItaniumRecordLayoutBuilder is used), [[gcc_struct]] will locally cancel
the effect of -mms-bitfields on a record. If -mms-bitfields is not
supplied and is not a default behavior on a target, [[gcc_struct]] will
be a no-op. This should provide enough compatibility with GCC.
If C++ ABI is "Microsoft", [[gcc_struct]] will currently always produce
a diagnostic, since support for it is not yet implemented in
MicrosoftRecordLayoutBuilder. Note, however, that all the infrastructure
is ready for the future implementation.
In particular, check for default value of -mms-bitfields is moved from a
driver to TargetInfo, since it now non-trivially depends on other
supplied flags. Also this, unfortunately, makes it impossible to use
usual argument parsing for `-m{no-,}ms-bitfields`.
The patch doesn't introduce any backwards-incompatible changes, except
for situations when cc1 is called directly with `-mms-bitfields` option.
|
@rjmccall Re asking GCC developers about gcc_struct on non-x86: https://gcc.gnu.org/pipermail/gcc/2024-January/243154.html. Either GCC devs aren't really worried about this or I can't properly write emails (what's totally possible). @MaskRay Any chance you can look at this PR again? Will you be more happy if instead of |
Alright, well, we tried. I think rolling forward with what your proposal on non-x86 platforms is reasonable. |
Yes, I don't foresee any issue with it. There's a proposed patchset to GCC for adding support for AArch64 on Windows, but it's all very early. So that shouldn't be taken as reference for how finer details should work in the end anyway. Even if they don't support these attributes from the start, the probably should/will in the end. |
This commit implements gcc_struct attribute with the behavior similar to one in GCC. Current behavior is as follows:
When C++ ABI is not "Microsoft" (i. e. when ItaniumRecordLayoutBuilder is used), [[gcc_struct]] will locally cancel the effect of -mms-bitfields on a record. If -mms-bitfields is not supplied and is not a default behavior on a target, [[gcc_struct]] will be a no-op. This should provide enough compatibility with GCC.
If C++ ABI is "Microsoft", [[gcc_struct]] will currently always be a no-op, since support for it is not yet implemented in MicrosoftRecordLayoutBuilder. Note, however, that all the infrastructure is ready for the future implementation.
In particular, check for default value of -mms-bitfield is moved from a driver to TargetInfo, since it now non-trivially depends on other supplied flags. Also, unfortunately, this makes it impossible to use usual argument parsing for
-m{no-,}ms-bitfield.The patch doesn't introduce any not backwards-compatible changes, except for situations when cc1 is called directly on a target with -mms-bitfields being a default option.
Work towards #24757
In case someone has already read the original description: I figured that randomly changing layout ABI is not a particularly bright idea, since we will then have problems when [[ms_struct]] inherits from [[gcc_struct]], for example. So then [[gcc_struct]] implementation in MicrosoftRecordLayoutBuilder is not trivial and I decided to separate it from this PR which is mostly about proper
-m{no-,}ms-bitfieldshandling.