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

[dblaikie]

• Create a separate sub-warning flag of -Wpacked (-Wpacked-non-pod), in case you're just interested in the ABI breaks here
• Warn only when this produces a different alignment requirement for the field

[rsmith]

clang/lib/AST/RecordLayoutBuilder.cpp

2028–2035

It seems a little wasteful and error-prone that we're now computing the actual alignment, the alignment if the field were not packed, and the alignment if the field were packed. Is there any way we can reduce this down to computing just the alignment if the field were packed plus the alignment if the field were not packed, then picking one of those two as the actual field alignment? Or does that end up being messier?

2141–2142

Hm. Doing this from here will mean we only warn if we actually compute the layout of the class. But I suppose that's the same as what we do a few lines above, and for other -Wpacked warnings, and it seems necessary if we want to suppress the warning if packing wouldn't have made any difference anyway.

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

157

Maybe consider adding another test showing that we don't warn if packing would have made no difference anyway (for example if the alignment of the POD struct was already 1).

[dblaikie]

Refactor the max/min/preferred alignment calculations.

[dblaikie]

clang/lib/AST/RecordLayoutBuilder.cpp

2028–2035

I had a go at that refactor - we can't pull the FieldPacked computation lower (it'd be great if it could move down to after the packed/unpacked computation, so it was clear that those values were computed independently of the FieldPacked value, and that FieldPacked just determined which one to pick) because of the alignedAttrCanDecreaseAIXAlignment, by the looks of it.

And also the AIX alignment stuff seems to do some weird things around the preferred alignment that caused the carefully constructed 3 ifs below (!FieldPacked, DefaultsToAIXPowerAlignment, and FieldPacked) which I spent more time than I'd like to admit figuring out why anything else/less/more streamlined was inadequate.

But I also don't understand why DefaultsToAIXAlignment causes the AlignTo value to be the PreferredAlign, but the FieldAlign stays as it is? (like why doesn't DefaultsToAIXPowerAlignment cause FieldAlign to /be/ PreferredAlign - I think that'd simplify things, but tests (maybe the tests are incorrect/) seemed to break when I tried that) - I would've thought not doing that (as the code currently doesn't) would cause problems for the UnadjustedAlignment, UpdateAlignment, and warn_unaligned_access issues later on that depend on FieldAlign, but feel like they should probably depend on the alignment that actually got used (the PreferredAlign) instead? It's pretty confusing to me, so... yeah.

2141–2142

Yeah, the test has that workaround f function that references all the types for that reason - but could be nice to avoid that, though would mean moving all this layout stuff somewhere else/more reusable, I guess? Probably out of scope for this patch, but I'm open to ideas if it's worth addressing as a follow-up.

[tstellar]

Should we backport this to the release/14.x branch?

[tbaeder]

Hey @dblaikie, seems like this has never been pushed?

[dblaikie]

In D118511#3369114, @tbaeder wrote:

Hey @dblaikie, seems like this has never been pushed?

Yeah, was holding off on this because it looked like maybe there was still outstanding work on the nuance/precise nature of the ABI change - turned out to be a known/outstanding issue: https://reviews.llvm.org/D119051 though it does have some impact on the warning and the layout change: If we get that POD ABI fix in, then the layout change/warning will fire on fewer places. So it seems sort of unfortunate to add the warning & end up having folks cleaning up things that don't need cleaning up once the POD ABI change happens.

But equally, bad to ship the layout change without teh associated warning.

Thoughts? Revert the layout change for now/in the release until the broader POD ABI issue is addressed? Then apply the layout change+warning?

[tstellar]

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

[jyknight]

In D118511#3371432, @tstellar wrote:

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

[dblaikie]

In D118511#3372728, @jyknight wrote:

In D118511#3371432, @tstellar wrote:

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

[tstellar]

In D118511#3373082, @dblaikie wrote:

In D118511#3372728, @jyknight wrote:

In D118511#3371432, @tstellar wrote:

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

I can revert in the release branch. Is this the commit: https://reviews.llvm.org/rG277123376ce08c98b07c154bf83e4092a5d4d3c6?

It doesn't seem necessary to me to revert in the main branch, but I think that can be your call.

[dblaikie]

In D118511#3373173, @tstellar wrote:

In D118511#3373082, @dblaikie wrote:

In D118511#3372728, @jyknight wrote:

In D118511#3371432, @tstellar wrote:

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

I can revert in the release branch. Is this the commit: https://reviews.llvm.org/rG277123376ce08c98b07c154bf83e4092a5d4d3c6?

Yep, that's the one!

It doesn't seem necessary to me to revert in the main branch, but I think that can be your call.

Yeah, I'm leaning towards not reverting on main & hoping we can sort out the POD ABI issue.

[dblaikie]

@rsmith I had a few responses to your last round of review here - could you have a look through them/see if this is the right way to go, or if more refactoring would be suitable?

[dblaikie]

rebase

[dblaikie]

In D118511#3373181, @dblaikie wrote:

In D118511#3373173, @tstellar wrote:

In D118511#3373082, @dblaikie wrote:

In D118511#3372728, @jyknight wrote:

In D118511#3371432, @tstellar wrote:

I'm fine with reverting if you think this is the best solution. I just would like to conclude soon so I can make the final release candidate.

ISTM that reverting the ABI change in the 14.x branch makes sense, to avoid ping-ponging the ABI for packed structs which would become non-packed (breaking ABI) in 14.x and packed again (breaking ABI) in https://reviews.llvm.org/D119051.

Yeah - I think it'd be a pretty niche amount of code that'd churn like that, but doesn't seem super important to rush this either.

@tstellar - can/do you want to revert this on the release branch yourself? Is that something I should do? Should I revert this on trunk (would be a bit awkward/more churny for users - maybe not a full revert, but one that leaves the new ABI version flag available as a no-op so users opting out don't need to remove the flag only to add it back in later) so it can be integrated to the release?

I can revert in the release branch. Is this the commit: https://reviews.llvm.org/rG277123376ce08c98b07c154bf83e4092a5d4d3c6?

Yep, that's the one!

It doesn't seem necessary to me to revert in the main branch, but I think that can be your call.

Yeah, I'm leaning towards not reverting on main & hoping we can sort out the POD ABI issue.

@tstellar seems we didn't sort out the POD ABI issue in time for the 15 release branch - though we're making some progress on it now (see discussion here: D119051 ) - reckon we should again revert the packed ABI layout in 15, to try to put this off until the 3 issues (ignoring packed when packing non-pod (277123376ce08c98b07c154bf83e4092a5d4d3c6), pod with defaulted special members (D119051), warning for ignoring packed due to non-pod (this patch, D118511)) are put together in one release?

Sorry for the churn/slow-movement on these.

[dblaikie]

rebase

[tstellar]

@dblaikie Can you file a bug and add the 15.0.0 Release Milestone, so we don't forget to fix this in the release branch.

[dblaikie]

In D118511#3732200, @tstellar wrote:

@dblaikie Can you file a bug and add the 15.0.0 Release Milestone, so we don't forget to fix this in the release branch.

Yeah, sorry about the delay on this, last week was a lot on my side. Filed https://github.com/llvm/llvm-project/issues/57346 - happy to edit/change/have that changed in whatever way you were thinking.

[dblaikie]

rebase

[dblaikie]

clang/lib/AST/RecordLayoutBuilder.cpp

2028–2035

Ping on this discussion. @rsmith

[dblaikie]

clang/lib/AST/RecordLayoutBuilder.cpp

2028–2035

@rsmith ?

clang/include/clang/Basic/DiagnosticASTKinds.td

def warn_padded_struct_anon_field : Warning<
   "padding %select{struct|interface|class}0 %1 with %2 "
   "%select{byte|bit}3%s2 to align anonymous bit-field">,
   InGroup<Padded>, DefaultIgnore;
 def warn_padded_struct_size : Warning<
   "padding size of %0 with %1 %select{byte|bit}2%s1 to alignment boundary">,
   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<
   "field %1 within %0 is less aligned than %2 and is usually due to %0 being "
   "packed, which can lead to unaligned accesses">, InGroup<UnalignedAccess>, DefaultIgnore;
 }

clang/include/clang/Basic/DiagnosticGroups.td

 def ObjCBoxing : DiagGroup<"objc-boxing">;
 def CompletionHandler : DiagGroup<"completion-handler">;
 def CalledOnceParameter : DiagGroup<"called-once-parameter", [CompletionHandler]>;
 def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">;
 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">;
 
 def PessimizingMove : DiagGroup<"pessimizing-move">;
 def ReturnStdMove : DiagGroup<"return-std-move">;
 
 def GNUPointerArith : DiagGroup<"gnu-pointer-arith">;
 def PointerArith : DiagGroup<"pointer-arith", [GNUPointerArith]>;

clang/lib/AST/RecordLayoutBuilder.cpp

void ItaniumRecordLayoutBuilder::LayoutField(const FieldDecl *D,
   }
 
   uint64_t UnpaddedFieldOffset = getDataSizeInBits() - UnfilledBitsInLastUnit;
   // Reset the unfilled bits.
   UnfilledBitsInLastUnit = 0;
   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;
   CharUnits FieldAlign;
   // The amount of this class's dsize occupied by the field.
   // This is equal to FieldSize unless we're permitted to pack
   // into the field's tail padding.
   CharUnits EffectiveFieldSize;
if (IsMsStruct) {
         }
         if (TypeSize > FieldAlign &&
             llvm::isPowerOf2_64(TypeSize.getQuantity()))
           FieldAlign = TypeSize;
       }
     }
   }
 
+  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
   // the alignment.
   auto alignedAttrCanDecreaseAIXAlignment = [AlignRequirement, FieldPacked] {
     // Enum alignment sources can be safely ignored here, because this only
     // helps decide whether we need the AIX alignment upgrade, which only
     // applies to floating-point types.
if (DefaultsToAIXPowerAlignment && !alignedAttrCanDecreaseAIXAlignment() &&
     } else if (const RecordType *RT = BaseTy->getAs<RecordType>()) {
       const RecordDecl *RD = RT->getDecl();
       assert(RD && "Expected non-null RecordDecl.");
       const ASTRecordLayout &FieldRecord = Context.getASTRecordLayout(RD);
       PreferredAlign = FieldRecord.getPreferredAlignment();
     }
   }
 
   // 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.
   FieldOffset = FieldOffset.alignTo(AlignTo);
   UnpackedFieldOffset = UnpackedFieldOffset.alignTo(UnpackedFieldAlign);
 
   if (UseExternalLayout) {
     FieldOffset = Context.toCharUnitsFromBits(
if (RD->hasAttr<PackedAttr>() || !MaxFieldAlignment.isZero())
           // If the offset is a multiple of the alignment of
           // the type, raise the warning.
           // TODO: Takes no account the alignment of the outer struct
           if (FieldOffset % OriginalFieldAlign != 0)
             Diag(D->getLocation(), diag::warn_unaligned_access)
                 << 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) {
   // In C++, records cannot be of size 0.
   if (Context.getLangOpts().CPlusPlus && getSizeInBits() == 0) {
     if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(D)) {
       // Compatibility with gcc requires a class (pod or non-pod)
       // which is not empty but of size 0; such as having fields of

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

struct S26 { // expected-warning {{packed attribute is unnecessary for 'S26'}}
   unsigned char b; //expected-warning {{padding struct 'S26' with 7 bits to align 'b'}}
 } __attribute__((packed));
 
 struct S27 { // expected-warning {{padding size of 'S27' with 7 bits to alignment boundary}}
   unsigned char a : 1;
   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*){}