[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGec273d3e3a8c: Add a warning for not packing non-POD members 
in packed structs (authored by dblaikie).

Changed prior to commit:
  https://reviews.llvm.org/D118511?vs=465161=470956#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp

Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,28 @@
   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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1890,12 +1890,6 @@
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- FieldClass->hasAttr() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver15 ||
- Target.isPS() || Target.isOSDarwin())) ||
- D->hasAttr();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
@@ -1976,6 +1970,13 @@
 }
   }
 
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+ FieldClass->hasAttr() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver15 ||
+ Target.isPS() || Target.isOSDarwin())) ||
+ D->hasAttr();
+
   // 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 @@
 
   // 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 @@
 << Context.getTypeDeclType(RD) << D->getName() << D->getType();
 }
   }
+
+  if (Packed && 

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
   !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 

dblaikie wrote:
> dblaikie wrote:
> > rsmith wrote:
> > > 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?
> > 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 `if`s 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.
> Ping on this discussion. @rsmith 
@rsmith ?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
   !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 

dblaikie wrote:
> rsmith wrote:
> > 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?
> 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 `if`s 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.
Ping on this discussion. @rsmith 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 465161.
dblaikie added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp

Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,28 @@
   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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1890,11 +1890,6 @@
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver14 ||
- Target.isPS() || Target.isOSDarwin())) ||
- D->hasAttr();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
@@ -1975,6 +1970,12 @@
 }
   }
 
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver14 ||
+ Target.isPS() || Target.isOSDarwin())) ||
+ D->hasAttr();
+
   // 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
@@ -2025,28 +2026,34 @@
 
   // 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.
@@ -2129,6 +2136,9 @@
 << 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) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ 

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 452023.
dblaikie added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp

Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,28 @@
   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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1890,11 +1890,6 @@
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver14 ||
- Target.isPS() || Target.isOSDarwin())) ||
- D->hasAttr();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
@@ -1975,6 +1970,12 @@
 }
   }
 
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver14 ||
+ Target.isPS() || Target.isOSDarwin())) ||
+ D->hasAttr();
+
   // 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
@@ -2025,28 +2026,34 @@
 
   // 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.
@@ -2129,6 +2136,9 @@
 << 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) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ 

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.



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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 431869.
dblaikie added a comment.

rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp

Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,28 @@
   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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1888,11 +1888,6 @@
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver14 ||
- Target.isPS4() || Target.isOSDarwin())) ||
- D->hasAttr();
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
@@ -1973,6 +1968,12 @@
 }
   }
 
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver14 ||
+ Target.isPS4() || Target.isOSDarwin())) ||
+ D->hasAttr();
+
   // 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
@@ -2023,28 +2024,34 @@
 
   // 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.
@@ -2127,6 +2134,9 @@
 << 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) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ 

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

@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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[Timm Bäder via Phabricator via cfe-commits]

tbaeder added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[Tom Stellard via Phabricator via cfe-commits]

tstellar added a comment.
Herald added a project: All.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie marked an inline comment as done.
dblaikie added inline comments.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
   !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 

rsmith wrote:
> 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?
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 `if`s 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.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2140-2141
+
+  if (Packed && !FieldPacked && PackedFieldAlign < FieldAlign)
+Diag(D->getLocation(), diag::warn_unpacked_field) << D;
 }

rsmith wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 406626.
dblaikie added a comment.

Refactor the max/min/preferred alignment calculations.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp

Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,28 @@
   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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1888,11 +1888,6 @@
   LastBitfieldStorageUnitSize = 0;
 
   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;
@@ -1973,6 +1968,12 @@
 }
   }
 
+  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
+ Context.getLangOpts().getClangABICompat() <=
+ LangOptions::ClangABI::Ver13 ||
+ Target.isPS4() || Target.isOSDarwin())) ||
+ D->hasAttr();
+
   // 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
@@ -2023,28 +2024,34 @@
 
   // 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.
@@ -2127,6 +2134,9 @@
 << 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) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- 

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2029-2036
   // The align if the field is not packed. This is to check if the attribute
   // was unnecessary (-Wpacked).
   CharUnits UnpackedFieldAlign =
   !DefaultsToAIXPowerAlignment ? FieldAlign : PreferredAlign;
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 

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?



Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2140-2141
+
+  if (Packed && !FieldPacked && PackedFieldAlign < FieldAlign)
+Diag(D->getLocation(), diag::warn_unpacked_field) << D;
 }

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.



Comment at: clang/test/CodeGenCXX/warn-padded-packed.cpp:157
+  char c2;
+  S28_non_pod p1; // expected-warning {{not packing field 'p1' as it is 
non-POD}}
+} __attribute__((packed));

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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie updated this revision to Diff 405468.
dblaikie added a comment.

- 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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118511/new/

https://reviews.llvm.org/D118511

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp

Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,18 @@
   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));
 
 // 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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1888,11 +1888,16 @@
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver13 ||
- Target.isPS4() || Target.isOSDarwin())) ||
- D->hasAttr();
+  bool FieldPacked = Packed;
+  if (FieldPacked && FieldClass && !FieldClass->isPOD() &&
+  Context.getLangOpts().getClangABICompat() >
+  LangOptions::ClangABI::Ver13 &&
+  !Target.isPS4() && !Target.isOSDarwin()) {
+FieldPacked = false;
+  }
+
+  if (!FieldPacked && D->hasAttr())
+FieldPacked = true;
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
@@ -2028,6 +2033,8 @@
   CharUnits UnpackedFieldOffset = FieldOffset;
   CharUnits OriginalFieldAlign = UnpackedFieldAlign;
 
+  CharUnits PackedFieldAlign = CharUnits::One();
+
   if (FieldPacked) {
 FieldAlign = CharUnits::One();
 PreferredAlign = CharUnits::One();
@@ -2035,12 +2042,14 @@
   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);
   }
@@ -2127,6 +2136,9 @@
 << 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) {
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -541,7 +541,8 @@
 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">;
 
Index: clang/include/clang/Basic/DiagnosticASTKinds.td
===
--- clang/include/clang/Basic/DiagnosticASTKinds.td
+++ clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -590,6 +590,8 @@
   InGroup, DefaultIgnore;
 def warn_unnecessary_packed : Warning<
   "packed attribute is unnecessary for %0">, InGroup, DefaultIgnore;
+def warn_unpacked_field : Warning<
+  "not 

[PATCH] D118511: Add a warning for not packing non-POD members in packed structs

[David Blaikie via Phabricator via cfe-commits]

dblaikie created this revision.
dblaikie added a reviewer: rsmith.
dblaikie requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D118511

Files:
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/test/CodeGenCXX/warn-padded-packed.cpp


Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,18 @@
   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));
 
 // 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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1888,11 +1888,17 @@
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver13 ||
- Target.isPS4() || Target.isOSDarwin())) ||
- D->hasAttr();
+  bool FieldPacked = Packed;
+  if (FieldPacked && FieldClass && !FieldClass->isPOD() &&
+  Context.getLangOpts().getClangABICompat() >
+  LangOptions::ClangABI::Ver13 &&
+  !Target.isPS4() && !Target.isOSDarwin()) {
+Diag(D->getLocation(), diag::warn_unpacked_field) << D;
+FieldPacked = false;
+  }
+
+  if (!FieldPacked && D->hasAttr())
+FieldPacked = true;
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
Index: clang/include/clang/Basic/DiagnosticASTKinds.td
===
--- clang/include/clang/Basic/DiagnosticASTKinds.td
+++ clang/include/clang/Basic/DiagnosticASTKinds.td
@@ -590,6 +590,8 @@
   InGroup, DefaultIgnore;
 def warn_unnecessary_packed : Warning<
   "packed attribute is unnecessary for %0">, InGroup, DefaultIgnore;
+def warn_unpacked_field : Warning<
+  "not packing field %0 as it is non-POD">, InGroup, DefaultIgnore;
 
 // -Wunaligned-access
 def warn_unaligned_access : Warning<


Index: clang/test/CodeGenCXX/warn-padded-packed.cpp
===
--- clang/test/CodeGenCXX/warn-padded-packed.cpp
+++ clang/test/CodeGenCXX/warn-padded-packed.cpp
@@ -146,8 +146,18 @@
   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));
 
 // 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*){}
Index: clang/lib/AST/RecordLayoutBuilder.cpp
===
--- clang/lib/AST/RecordLayoutBuilder.cpp
+++ clang/lib/AST/RecordLayoutBuilder.cpp
@@ -1888,11 +1888,17 @@
   LastBitfieldStorageUnitSize = 0;
 
   llvm::Triple Target = Context.getTargetInfo().getTriple();
-  bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||
- Context.getLangOpts().getClangABICompat() <=
- LangOptions::ClangABI::Ver13 ||
- Target.isPS4() || Target.isOSDarwin())) ||
- D->hasAttr();
+  bool FieldPacked = Packed;
+  if (FieldPacked && FieldClass && !FieldClass->isPOD() &&
+  Context.getLangOpts().getClangABICompat() >
+  LangOptions::ClangABI::Ver13 &&
+  !Target.isPS4() && !Target.isOSDarwin()) {
+Diag(D->getLocation(), diag::warn_unpacked_field) << D;
+FieldPacked = false;
+  }
+
+  if (!FieldPacked && D->hasAttr())
+FieldPacked = true;
 
   AlignRequirementKind AlignRequirement = AlignRequirementKind::None;
   CharUnits FieldSize;
Index: clang/include/clang/Basic/DiagnosticASTKinds.td
===
---