[clang] Implement gcc_struct attribute on Itanium targets #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 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
erichkeane
left a comment
erichkeane
left a comment
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
erichkeane
left a comment
erichkeane
left a comment
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
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) |
AaronBallman
left a comment
AaronBallman
left a comment
There was a problem hiding this comment.
The attribute bits LGTM, but I added some more reviewers to double-check the driver changes.
|
I agree with the Sema/AST-level LGTM (but also don't feel comfortable approving the driver changes) |
There was a problem hiding this comment.
If we use BoolOption in Options.td, we won't need this custom code.
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, |
Sounds reasonable. I tested with GCC, and it doesn't error out though, but it gives the following warning: (And the same for Would you be ok with mirroring this behaviour, or do you explicitly want it to be an error by default? |
|
Oh, and also - for someone not very familiar with the Clang codebase at this level; where would the right place be to emit such a diagnostic? By looking at the diff of the PR, the main place I'd see where interacting with these attributes, is here: bool RecordDecl::isMsStruct(const ASTContext &C) const {
if (hasAttr<GCCStructAttr>())
return false;
if (hasAttr<MSStructAttr>())
return true;
auto LayoutCompatibility = C.getLangOpts().getLayoutCompatibility();
if (LayoutCompatibility == LangOptions::LayoutCompatibilityKind::Default)
return C.defaultsToMsStruct();
return LayoutCompatibility == LangOptions::LayoutCompatibilityKind::Microsoft;
}Should we add an |
|
I'd recommend adding something like this to Attr.td: and the rest should happen automagically. |
|
Thanks! That seems to work great! However - that uncovers a case in the existing tests in this PR, which starts failing: This now also hits the same error with incompatible attributes. This is a case I think should be supported; just like |
Oof. I didn't realize we had that as a pragma. :-/ Yeah, I think the expected priority is: local takes precedence over pragma takes precedence over command line. So you may have to do the exclusion the hard way (manually). |
Thanks! Yes, such an order of precedence seems reasonable. As the pragma seems to show up in the form of an attribute at the level of |
I haven't spent any time with that pragma, but best I can tell is that the pragma creates it (SemaAttr.cpp ~92) as 'implicit'. I DO wonder if we should have some sort of fix to the Otherwise, you'd need a custom Obviously the 'implicit' one is added in a different step, so that probably should check to make sure it isn't adding if the GCC one already is though. |
Thanks!
That sounds like a potential future improvement - but I'll skip trying to take on that at this point; your suggestions below were very doable!
Thanks - with these pointers I was able to make it work! I also added tests for covering cases like having an implicit MSStructAttr from a pragma, plus an explicit one - making sure that this does produce an error if there's a GCCStructAttr on that one, etc. |
|
Thanks for the assistance here! I'll go ahead and merge this one soon then, after the CI completes. |
|
Congrats for finally merging this! :) |
Fix the args in the mms-bitfields test file to be aligned with the same test in classical codegen (clang/test/CodeGen/mms-bitfields.c). After #71148 is merged
|
It turns out that this change did break the diff --git a/clang/test/Sema/struct-packed-align.c b/clang/test/Sema/struct-packed-align.c
index d6d0724da49f..c996a58c11e4 100644
--- a/clang/test/Sema/struct-packed-align.c
+++ b/clang/test/Sema/struct-packed-align.c
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 %s -fsyntax-only -verify
// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-coff -verify
+// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-windows-gnu -verify
// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-scei-ps4 -verify
// RUN: %clang_cc1 %s -fsyntax-only -triple=x86_64-sie-ps5 -verify
With this change, we're getting the following new failures: Among other, this part of the testcase now behaves differently: typedef long long __attribute__((aligned(1))) nt;
struct nS {
char buf_nr;
nt start_lba;
};
#if defined(_WIN32) && !defined(__declspec) // _MSC_VER is unavailable in cc1.
// Alignment doesn't affect packing in MS mode.
extern int n1[sizeof(struct nS) == 16 ? 1 : -1];
extern int n2[__alignof(struct nS) == 8 ? 1 : -1];
#else
extern int n1[sizeof(struct nS) == 9 ? 1 : -1];
extern int n2[__alignof(struct nS) == 1 ? 1 : -1];
#endif |
This commit implements gcc_struct attribute with the behavior similar to
one in GCC. Current behavior is as follows:
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 ASTContext, since it now non-trivially depends on other
supplied flags. This also, 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.
Work towards llvm#24757
---------
Co-authored-by: Martin Storsjö <[email protected]>
Fix the args in the mms-bitfields test file to be aligned with the same test in classical codegen (clang/test/CodeGen/mms-bitfields.c). After llvm#71148 is merged
That seems reasonably revert worrthy until we can re-apply this. Would you like to create the revert patch? |
|
Hmm, it looks like the previous behavior was a bug: with |
|
-> #172337 |
….c (#172337) Before #71148, providing only `-triple=x86_64-windows-gnu` to cc1 did not set `-mms-bitfields` (`-fms-layout-compatibility=microsoft`). Therefore, MS-compatible layout was only triggered in true MSVC targets. This is not the case now, so we should only check if we are compiling for Windows to test to determine if MS layout will be used. The change of behavior is harmless as it only affects direct invocations of cc1.
11 participants
This commit implements gcc_struct attribute with the behavior similar to one in GCC. Current behavior is as follows:
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 ASTContext, since it now non-trivially depends on other supplied flags. This also, 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-bitfieldsoption.Work towards #24757