From 277123376ce08c98b07c154bf83e4092a5d4d3c6 Mon Sep 17 00:00:00 2001 From: David Blaikie Date: Tue, 18 Jan 2022 14:28:14 -0800 Subject: [PATCH] GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs This matches GCC: https://godbolt.org/z/sM5q95PGY I realize this is an API break for clang+clang - so I'm totally open to discussing how we should deal with that. If Apple wants to keep the Clang layout indefinitely, if we want to put a flag on this so non-Apple folks can opt out of this fix/new behavior. Differential Revision: https://reviews.llvm.org/D117616 --- clang/docs/ReleaseNotes.rst | 6 ++++ clang/include/clang/Basic/LangOptions.h | 4 +++ clang/lib/AST/RecordLayoutBuilder.cpp | 7 ++++- clang/lib/Frontend/CompilerInvocation.cpp | 4 +++ clang/test/SemaCXX/class-layout.cpp | 37 +++++++++++++++++++++++ 5 files changed, 57 insertions(+), 1 deletion(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index 6a9b046a1427..45e80785e462 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -236,6 +236,12 @@ ABI Changes in Clang is still in the process of being stabilized, so this type should not yet be used in interfaces that require ABI stability. +- GCC doesn't pack non-POD members in packed structs unless the packed + attribute is also specified on the member. Clang historically did perform + such packing. Clang now matches the gcc behavior (except on Darwin and PS4). + You can switch back to the old ABI behavior with the flag: + ``-fclang-abi-compat=13.0``. + OpenMP Support in Clang ----------------------- diff --git a/clang/include/clang/Basic/LangOptions.h b/clang/include/clang/Basic/LangOptions.h index 09afa641acf9..50c7f038fc6b 100644 --- a/clang/include/clang/Basic/LangOptions.h +++ b/clang/include/clang/Basic/LangOptions.h @@ -181,6 +181,10 @@ public: /// global-scope inline variables incorrectly. Ver12, + /// Attempt to be ABI-compatible with code generated by Clang 13.0.x. + /// This causes clang to not pack non-POD members of packed structs. + Ver13, + /// Conform to the underlying platform's C and C++ ABIs as closely /// as we can. Latest diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp index 61a30ead165e..709e05716a56 100644 --- a/clang/lib/AST/RecordLayoutBuilder.cpp +++ b/clang/lib/AST/RecordLayoutBuilder.cpp @@ -1887,7 +1887,12 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D, UnfilledBitsInLastUnit = 0; LastBitfieldStorageUnitSize = 0; - bool FieldPacked = Packed || D->hasAttr(); + llvm::Triple Target = Context.getTargetInfo().getTriple(); + bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() || + Context.getLangOpts().getClangABICompat() <= + LangOptions::ClangABI::Ver13 || + Target.isPS4() || Target.isOSDarwin())) || + D->hasAttr(); AlignRequirementKind AlignRequirement = AlignRequirementKind::None; CharUnits FieldSize; diff --git a/clang/lib/Frontend/CompilerInvocation.cpp b/clang/lib/Frontend/CompilerInvocation.cpp index 7f1ce3da7e7e..553a0b31c0ab 100644 --- a/clang/lib/Frontend/CompilerInvocation.cpp +++ b/clang/lib/Frontend/CompilerInvocation.cpp @@ -3560,6 +3560,8 @@ void CompilerInvocation::GenerateLangArgs(const LangOptions &Opts, GenerateArg(Args, OPT_fclang_abi_compat_EQ, "11.0", SA); else if (Opts.getClangABICompat() == LangOptions::ClangABI::Ver12) GenerateArg(Args, OPT_fclang_abi_compat_EQ, "12.0", SA); + else if (Opts.getClangABICompat() == LangOptions::ClangABI::Ver13) + GenerateArg(Args, OPT_fclang_abi_compat_EQ, "13.0", SA); if (Opts.getSignReturnAddressScope() == LangOptions::SignReturnAddressScopeKind::All) @@ -4062,6 +4064,8 @@ bool CompilerInvocation::ParseLangArgs(LangOptions &Opts, ArgList &Args, Opts.setClangABICompat(LangOptions::ClangABI::Ver11); else if (Major <= 12) Opts.setClangABICompat(LangOptions::ClangABI::Ver12); + else if (Major <= 13) + Opts.setClangABICompat(LangOptions::ClangABI::Ver13); } else if (Ver != "latest") { Diags.Report(diag::err_drv_invalid_value) << A->getAsString(Args) << A->getValue(); diff --git a/clang/test/SemaCXX/class-layout.cpp b/clang/test/SemaCXX/class-layout.cpp index 5403bd6e6a6f..79fa67707110 100644 --- a/clang/test/SemaCXX/class-layout.cpp +++ b/clang/test/SemaCXX/class-layout.cpp @@ -1,6 +1,9 @@ // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++98 -Wno-inaccessible-base // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base +// RUN: %clang_cc1 -triple x86_64-apple-darwin %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=13 +// RUN: %clang_cc1 -triple x86_64-scei-ps4 %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -DCLANG_ABI_COMPAT=6 // RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=6 -DCLANG_ABI_COMPAT=6 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -fsyntax-only -verify -std=c++11 -Wno-inaccessible-base -fclang-abi-compat=13 -DCLANG_ABI_COMPAT=13 // expected-no-diagnostics #define SA(n, p) int a##n[(p) ? 1 : -1] @@ -604,3 +607,37 @@ namespace PR37275 { #endif #pragma pack(pop) } + +namespace non_pod { +struct t1 { +protected: + int a; +}; +// GCC prints warning: ignoring packed attribute because of unpacked non-POD field 't1 t2::v1'` +struct t2 { + char c1; + short s1; + char c2; + t1 v1; +} __attribute__((packed)); +#if defined(CLANG_ABI_COMPAT) && CLANG_ABI_COMPAT <= 13 +_Static_assert(_Alignof(t1) == 4, ""); +_Static_assert(_Alignof(t2) == 1, ""); +#else +_Static_assert(_Alignof(t1) == 4, ""); +_Static_assert(_Alignof(t2) == 4, ""); +#endif +_Static_assert(sizeof(t2) == 8, ""); // it's still packing the rest of the struct +} // namespace non_pod + +namespace non_pod_packed { +struct t1 { +protected: + int a; +} __attribute__((packed)); +struct t2 { + t1 v1; +} __attribute__((packed)); +_Static_assert(_Alignof(t1) == 1, ""); +_Static_assert(_Alignof(t2) == 1, ""); +} // namespace non_pod_packed