Add a warning for not packing non-POD members in packed structs

Differential Revision: https://reviews.llvm.org/D118511
llvm · Oct 26, 2022 · ec273d3 · ec273d3
1 parent 76ebaf2
commit ec273d3
Show file tree

Hide file tree

Showing 4 changed files with 49 additions and 16 deletions.
diff --git a/clang/include/clang/Basic/DiagnosticASTKinds.td b/clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -934,6 +934,8 @@ def warn_padded_struct_size : Warning<
   InGroup<Padded>, DefaultIgnore;
 def warn_unnecessary_packed : Warning<
   "packed attribute is unnecessary for %0">, InGroup<Packed>, DefaultIgnore;
+def warn_unpacked_field : Warning<
+  "not packing field %0 as it is non-POD">, InGroup<PackedNonPod>, DefaultIgnore;
 
 // -Wunaligned-access
 def warn_unaligned_access : Warning<

diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td
@@ -562,7 +562,8 @@ def UnderalignedExceptionObject : DiagGroup<"underaligned-exception-object">;
 def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">;
 def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">;
 def OrderedCompareFunctionPointers : DiagGroup<"ordered-compare-function-pointers">;
-def Packed : DiagGroup<"packed">;
+def PackedNonPod : DiagGroup<"packed-non-pod">;
+def Packed : DiagGroup<"packed", [PackedNonPod]>;
 def Padded : DiagGroup<"padded">;
 def UnalignedAccess : DiagGroup<"unaligned-access">;
 

diff --git a/clang/lib/AST/RecordLayoutBuilder.cpp b/clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1890,12 +1890,6 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
-                                 FieldClass->hasAttr<PackedAttr>() ||
-                                 Context.getLangOpts().getClangABICompat() <=
-                                     LangOptions::ClangABI::Ver15 ||
-                                 Target.isPS() || Target.isOSDarwin())) ||
-                     D->hasAttr<PackedAttr>();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
@@ -1976,6 +1970,13 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
     }
   }
 
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+                                 FieldClass->hasAttr<PackedAttr>() ||
+                                 Context.getLangOpts().getClangABICompat() <=
+                                     LangOptions::ClangABI::Ver15 ||
+                                 Target.isPS() || Target.isOSDarwin())) ||
+                     D->hasAttr<PackedAttr>();
+
   // When used as part of a typedef, or together with a 'packed' attribute, the
   // 'aligned' attribute can be used to decrease alignment. In that case, it
   // overrides any computed alignment we have, and there is no need to upgrade
@@ -2026,28 +2027,34 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
 
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
-  CharUnits UnpackedFieldAlign =
-      !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
+  CharUnits UnpackedFieldAlign = FieldAlign; 
+  CharUnits PackedFieldAlign = CharUnits::One();
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 
-  if (FieldPacked) {
-    FieldAlign = CharUnits::One();
-    PreferredAlign = CharUnits::One();
-  }
   CharUnits MaxAlignmentInChars =
       Context.toCharUnitsFromBits(D->getMaxAlignment());
-  FieldAlign = std::max(FieldAlign, MaxAlignmentInChars);
+  PackedFieldAlign = std::max(PackedFieldAlign, MaxAlignmentInChars);
   PreferredAlign = std::max(PreferredAlign, MaxAlignmentInChars);
   UnpackedFieldAlign = std::max(UnpackedFieldAlign, MaxAlignmentInChars);
 
   // The maximum field alignment overrides the aligned attribute.
   if (!MaxFieldAlignment.isZero()) {
-    FieldAlign = std::min(FieldAlign, MaxFieldAlignment);
+    PackedFieldAlign = std::min(PackedFieldAlign, MaxFieldAlignment);
     PreferredAlign = std::min(PreferredAlign, MaxFieldAlignment);
     UnpackedFieldAlign = std::min(UnpackedFieldAlign, MaxFieldAlignment);
   }
 
+
+  if (!FieldPacked)
+    FieldAlign = UnpackedFieldAlign;
+  if (DefaultsToAIXPowerAlignment)
+    UnpackedFieldAlign = PreferredAlign;
+  if (FieldPacked) {
+    PreferredAlign = PackedFieldAlign;
+    FieldAlign = PackedFieldAlign;
+  }
+
   CharUnits AlignTo =
       !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   // Round up the current record size to the field's alignment boundary.
@@ -2130,6 +2137,9 @@ void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
                 << Context.getTypeDeclType(RD) << D->getName() << D->getType();
         }
   }
+
+  if (Packed && !FieldPacked && PackedFieldAlign < FieldAlign)
+    Diag(D->getLocation(), diag::warn_unpacked_field) << D;
 }
 
 void ItaniumRecordLayoutBuilder::FinishLayout(const NamedDecl *D) {

diff --git a/clang/test/CodeGenCXX/warn-padded-packed.cpp b/clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,28 @@ struct S27 { // expected-warning {{padding size of 'S27' with 7 bits to alignmen
   unsigned char b : 8;
 } __attribute__((packed));
 
+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}}
+} __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, "");
 
 // 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*){}
+       S26*, S27*, S28*, S29*){}