Remove -Wpacked false positive for non-pod types where the layout isn't directly changed

[dblaikie]

Summary

The packed attribute can still be useful in this case if the struct is
then placed inside another packed struct - the non-pod element type's
packed attribute declares that it's OK to misalign this element inside
the packed structure. (otherwise the non-pod element is not packed/its
alignment is preserved, as per D117616/2771233)

Fixes PR62353

[dblaikie]

ping

[aaron.ballman]

Precommit CI found a valid issue (at least on Debian, the Windows failure appears to be unrelated but it's really hard to tell from that output). Also, this should have a release note, right?

clang/lib/AST/RecordLayoutBuilder.cpp

2203–2208

You probably should update the comment to explain the new changes.

[dblaikie]

Release note
Restrict change to Clang ABI 16 and above (& test that condition)
Fix AIX test

[dblaikie]

Describe condition in comment

[dblaikie]

In D149182#4312202, @aaron.ballman wrote:

Precommit CI found a valid issue (at least on Debian, the Windows failure appears to be unrelated but it's really hard to tell from that output).

Hmm, right ,yeah, valid test failure - fixed.

Also, this should have a release note, right?

Happy to add one. (done)

[aaron.ballman]

LGTM aside from a nit with the release notes. Thank you for the fix!

clang/docs/ReleaseNotes.rst

114–118

I think this should move down to Improvements to Clang's diagnostics instead of adding a new category for it.

[dblaikie]

clang/docs/ReleaseNotes.rst

114–118

Ah, hadn't spotted that category - thanks!

[dblaikie]

clang/docs/ReleaseNotes.rst

114–118

Failed to include the fix there in the initial commit, but got it in 793c5b1

clang/docs/ReleaseNotes.rst

- Implemented partial support for `P2448R2: Relaxing some constexpr restrictions <https://wg21.link/p2448r2>`_
   functions. Which include allowing non-literal types as return values and paremeters, allow calling of
   non-constexpr functions and constructors.
 
 Resolutions to C++ Defect Reports
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 - Implemented `DR2397 <https://wg21.link/CWG2397>`_ which allows ``auto`` specifier for pointers
   and reference to arrays.
 
+Warnings
+^^^^^^^^
+- Address a false positive in ``-Wpacked`` when applied to a non-pod type using
+  Clang ABI >= 15 (fixes `#62353<https://github.com/llvm/llvm-project/issues/62353>`_,
+  fallout from the non-POD packing ABI fix in LLVM 15)
+
 C Language Changes
 ------------------
 - Support for outputs from asm goto statements along indirect edges has been
   added. (`#53562 <https://github.com/llvm/llvm-project/issues/53562>`_)
 - Fixed a bug that prevented initialization of an ``_Atomic``-qualified pointer
   from a null pointer constant.
 - Fixed a bug that prevented casting to an ``_Atomic``-qualified type.
   (`#39596 <https://github.com/llvm/llvm-project/issues/39596>`_)

clang/lib/AST/RecordLayoutBuilder.cpp

if (getSizeInBits() > UnpaddedSize) {
         InBits = false;
       }
       Diag(RD->getLocation(), diag::warn_padded_struct_size)
           << Context.getTypeDeclType(RD)
           << PadSize
           << (InBits ? 1 : 0); // (byte|bit)
     }
 
+    const auto *CXXRD = dyn_cast<CXXRecordDecl>(RD);
+
     // Warn if we packed it unnecessarily, when the unpacked alignment is not
     // greater than the one after packing, the size in bits doesn't change and
     // the offset of each field is identical.
+    // Unless the type is non-POD (for Clang ABI > 15), where the packed
+    // attribute on such a type does allow the type to be packed into other
+    // structures that use the packed attribute.
     if (Packed && UnpackedAlignment <= Alignment &&
-        UnpackedSizeInBits == getSizeInBits() && !HasPackedField)
+        UnpackedSizeInBits == getSizeInBits() && !HasPackedField &&
+        (!CXXRD || CXXRD->isPOD() ||
+         Context.getLangOpts().getClangABICompat() <=
+             LangOptions::ClangABI::Ver15))
       Diag(D->getLocation(), diag::warn_unnecessary_packed)
           << Context.getTypeDeclType(RD);
   }
 }
 
 void ItaniumRecordLayoutBuilder::UpdateAlignment(
     CharUnits NewAlignment, CharUnits UnpackedNewAlignment,
     CharUnits PreferredNewAlignment) {

clang/test/CodeGenCXX/warn-padded-packed.cpp

-// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,top %s -emit-llvm-only
+// RUN: %clang_cc1 -triple=x86_64-none-none -Wpadded -Wpacked -verify=expected,abi15 -fclang-abi-compat=15 %s -emit-llvm-only
 
 struct S1 {
   char c;
   short s; // expected-warning {{padding struct 'S1' with 1 byte to align 's'}}
   long l; // expected-warning {{padding struct 'S1' with 4 bytes to align 'l'}}
 };
 
 struct S2 { // expected-warning {{padding size of 'S2' with 3 bytes to alignment boundary}}

 struct S28_non_pod {
  protected:
   int i;
 };
 struct S28 {
   char c1;
   short s1;
   char c2;
-  S28_non_pod p1; // expected-warning {{not packing field 'p1' as it is non-POD for the purposes of layout}}
+  S28_non_pod p1; // top-warning {{not packing field 'p1' as it is non-POD for the purposes of layout}}
 } __attribute__((packed));
 
 struct S29_non_pod_align_1 {
  protected:
   char c;
 };
 struct S29 {
   S29_non_pod_align_1 p1;
   int i;
 } __attribute__((packed)); // no warning
 static_assert(alignof(S29) == 1, "");
 
+struct S30 {
+protected:
+ short s;
+} __attribute__((packed)); // no warning
+struct S30_use { // abi15-warning {{packed attribute is unnecessary for 'S30_use'}}
+  char c;
+  S30 u;
+} __attribute__((packed));
+static_assert(sizeof(S30_use) == 3, "");
+
 // The warnings are emitted when the layout of the structs is computed, so we have to use them.
 void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*,
        S14*, S15*, S16*, S17*, S18*, S19*, S20*, S21*, S22*, S23*, S24*, S25*,
        S26*, S27*, S28*, S29*){}

clang/test/Layout/aix-Wpacked-expecting-diagnostics.cpp

 // RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -Wpacked \
 // RUN:     -fdump-record-layouts -fsyntax-only -verify -x c++ < %s | \
 // RUN:   FileCheck %s
 
 // RUN: %clang_cc1 -triple powerpc64-ibm-aix-xcoff -Wpacked \
 // RUN:     -fdump-record-layouts -fsyntax-only -verify -x c++ < %s | \
 // RUN:   FileCheck %s
 
+// expected-no-diagnostics
+
 struct A {
   double d;
 };
 
 struct B {
   char x[8];
 };
 
-struct [[gnu::packed]] C : B, A { // expected-warning{{packed attribute is unnecessary for 'C'}}
+struct [[gnu::packed]] C : B, A {
   char x alignas(4)[8];
 };
 
 int b = sizeof(C);
 
 // CHECK:               0 | struct C
 // CHECK-NEXT:          0 |   struct B (base)
 // CHECK-NEXT:          0 |     char[8] x
 // CHECK-NEXT:          8 |   struct A (base)
 // CHECK-NEXT:          8 |     double d
 // CHECK-NEXT:         16 |   char[8] x
 // CHECK-NEXT:            | [sizeof=24, dsize=24, align=4, preferredalign=4,
 // CHECK-NEXT:            |  nvsize=24, nvalign=4, preferrednvalign=4]