[clang] Change the condition of unnecessary packed warning

[yawanng]

Summary

Change the condition of this unnecessary packed warning. The packed is unnecessary when

1. the alignment of the struct/class won't alter.
2. the size is unchanged.
3. the offset of each field is the same.

Remove all field-level warning.

[chh]

LGTM, leave it Argyrios's approval.

[yawanng]

Ping.

[srhines]

Richard, can you take a look at this, or suggest someone who would be a good reviewer for this improved diagnostic? Thanks.

[yawanng]

PING

[rsmith]

Some concrete suggestions throughout the patch, but I think we should take a step back and reconsider this warning approach: it seems bizarre for us to warn on any packed struct that happens to contain a char. It would make sense to warn if an __attribute__((packed)) written in the source has *no* effect (because it's applied to a struct where all fields already have alignment 1, or because it's applied to a field that has alignment 1) -- even then I'm not entirely convinced this is a valuable warning, but I assume there's some reason you want to warn on it :)

lib/AST/RecordLayoutBuilder.cpp

1894–1896

We intend for our diagnostics to be translatable, so sentence fragments like this that assume a certain phrasing of an English-language diagnostic should not be hard-coded.

Instead, change the diagnostic to pass the number of fields and use %plural to select between the different word suffixes. See http://clang.llvm.org/docs/InternalsManual.html#formatting-a-diagnostic-argument for more information.

1897–1900

Rather than building a potentially very long list here, consider passing each field name to the diagnostic as a separate argument, and only list the first few of them, for example:

packed attribute is unnecessary for field 'i'
packed attribute is unnecessary for fields 'i' and 'j'
packed attribute is unnecessary for fields 'i', 'j', and 'k'
packed attribute is unnecessary for fields 'i', 'j', 'k', ...

test/CodeGenCXX/warn-padded-packed.cpp

19

This looks backwards: the packed attribute *is* necessary for field i here (because it reduces i's alignment from 4 to 1), but not for field c.

49

Again this seems wrong.

75

Again, this looks exactly backwards.

[srhines]

Richard's points are really valid here. I missed the aspect that packed always implies alignment 1, which does have an effect on the code in most of the cases listed here. I agree that the value of this warning is low, since the possibility of false-positive is quite high. Would this make for a better clang-tidy check instead (although only for cases where there is no padding and/or cases where the struct is also declared to have a stricter alignment guarantee)?

test/CodeGenCXX/warn-padded-packed.cpp

19

Yeah, even if we wanted to suggest that aligned(1) is a better fit for these kinds of packed structs, it isn't sufficient. In this case, packed grants the 1-byte alignment, as well as ensures that arrays of this type are placed contiguously with no padding. Thus, the only place where this warning would make sense is for already packed structures that also require no padding at the end.

[chh]

These warnings are triggered by -Wpadded -Wpacked
or clang-tidy's clang-diagnostic-packed check.
I agree that they should be ignored or suppressed in many cases.
What I am not sure is the amount of real positive cases.

I found it too tedious to suppress one warning per struct field,
and hence liked this CL to consolidate those per-field warnings
into one per struct type. With this change we can use one NOLINT to
suppress all such warnings of a struct type, in the header file.

One alternative for users is to ignore these checks or warnings
for the whole compilation unit. That has the risk of masking
out valid warnings to other packed struct types in the same
compilation unit. Since the types are defined in header files,
these warnings would need to be suppressed in all compilation
units that include those header files.

I am not against removing these warnings completely.
If not removed, I hope that we can consolidate multiple
per-field warnings to one at the struct type level.

[yawanng]

Change the condition of this unnecessary packed warning to when the alignment of the class is one byte. Remove all field-level warning.

[yawanng]

In D34114#800253, @rsmith wrote:

Some concrete suggestions throughout the patch, but I think we should take a step back and reconsider this warning approach: it seems bizarre for us to warn on any packed struct that happens to contain a char. It would make sense to warn if an __attribute__((packed)) written in the source has *no* effect (because it's applied to a struct where all fields already have alignment 1, or because it's applied to a field that has alignment 1) -- even then I'm not entirely convinced this is a valuable warning, but I assume there's some reason you want to warn on it :)

Yes. I think the only case for this warning to be useful is when the alignment of the class or struct is 1 byte :-)

[chh]

lib/AST/RecordLayoutBuilder.cpp

1888

Why not keeping the (getSize() == UnpackedSize) condition?

[yawanng]

lib/AST/RecordLayoutBuilder.cpp

1888

It seems to me that when the alignment is 1 byte, the size won't change.

[yawanng]

Including cases 'attribute((packed, aligned(X)));'. When the alignment before packing is less or equal to the packed one, he 'packed' attribute not including 'aligned(X)' seems unnecessary. Because the alignment won't change as well as the size after removing 'packed'.

[yawanng]

Add more tests.

[srhines]

This looks better. It did lose context however, so some of the review is harder to read.

test/CodeGenCXX/warn-padded-packed.cpp

84

boundar -> boundary ?

93

boundary?

[chh]

lib/AST/RecordLayoutBuilder.cpp

1887

With this change, UnpackedSize is unused and caused a warning about unused-but-set-variable. Please use it or remove the variable.

I think UnpackedSizeInBits should be used somehow in the condition, because bit fields can be affected by the packed attribute. Please add a few unit test cases with bit fields. For example, the following cases showed that S21 is affected by packed, but got a wrong "unnecessary" warning after this change.

struct S21 {
  unsigned char a:6;
  unsigned char b:6;
} __attribute__((packed, aligned(1)));
struct S22 {
  unsigned char a:6;
  unsigned char b:6;
} __attribute__((aligned(1)));
struct S23 {
  unsigned char a:6;
  unsigned char b:6;
};

Warnings:

padding size of 'S21' with 4 bits to alignment boundary
packed attribute is unnecessary for 'S21'
padding struct 'S22' with 2 bits to align 'b'
padding size of 'S22' with 2 bits to alignment boundary
padding struct 'S23' with 2 bits to align 'b'
padding size of 'S23' with 2 bits to alignment boundary

[yawanng]

Add more tests and restrict the conditions.

[chh]

lib/AST/RecordLayoutBuilder.cpp

636

s/IsFieldOffsetChangedWithPacked/HasPackedField/

1887

I am still getting warning about unused-but-set-variable UnpackedSize.

1985

"then packed is not necessary" is incorrect.
Maybe this comment can be removed since the code is clear enough.

test/CodeGenCXX/warn-padded-packed.cpp

100

Please wrap line 150, 151, 152 to less than 80 chars per line.
The only exceptions in this file are the 'expected-warning' lines.

[chh]

LGTM.
rsmith, srhines, akyrtzi, rtrieu, do you have any comment?

lib/AST/RecordLayoutBuilder.cpp

           << (InBits ? 1 : 0); // (byte|bit)
     }
 
-    // Warn if we packed it unnecessarily. If the alignment is 1 byte don't
-    // bother since there won't be alignment issues.
-    if (Packed && UnpackedAlignment > CharUnits::One() && 
-        getSize() == UnpackedSize)
+    // Warn if we packed it unnecessarily, when the unpacked alignment is not
+    // greater than the one after packing.
+    if (Packed && UnpackedAlignment <= Alignment || )
       Diag(D->getLocation(), diag::warn_unnecessary_packed)
           << Context.getTypeDeclType(RD);
   }

           << PadSize
           << (InBits ? 1 : 0); // (byte|bit)
   }
-
-  // Warn if we packed it unnecessarily. If the alignment is 1 byte don't
-  // bother since there won't be alignment issues.
-  if (isPacked && UnpackedAlign > CharBitNum && Offset == UnpackedOffset)
-    Diag(D->getLocation(), diag::warn_unnecessary_packed)
-        << D->getIdentifier();
 }
 
 static const CXXMethodDecl *computeKeyFunction(ASTContext &Context,

test/CodeGenCXX/warn-padded-packed.cpp

 } __attribute__((packed));
 
 struct S4 {
-  int i; // expected-warning {{packed attribute is unnecessary for 'i'}}
+  int i;
   char c;
 } __attribute__((packed));
 

   int i; // expected-warning {{padding struct 'S8' with 3 bytes to align 'i'}}
 };
 
-struct S9 { // expected-warning {{packed attribute is unnecessary for 'S9'}}
-  int x; // expected-warning {{packed attribute is unnecessary for 'x'}}
-  int y; // expected-warning {{packed attribute is unnecessary for 'y'}}
+struct S9 {
+  int x;
+  int y;
 } __attribute__((packed));
 
-struct S10 { // expected-warning {{packed attribute is unnecessary for 'S10'}}
-  int x; // expected-warning {{packed attribute is unnecessary for 'x'}}
+struct S10 {
+  int x;
   char a,b,c,d;
 } __attribute__((packed));
 
 
-struct S11 {
+struct S11 { // expected-warning {{packed attribute is unnecessary for 'S11'}}
   bool x;
   char a,b,c,d;
 } __attribute__((packed));

   bool b : 10;
 };
 
+struct S14 { // expected-warning {{packed attribute is unnecessary for 'S14'}}
+  char a,b,c,d;
+} __attribute__((packed));
+
+struct S15 { // expected-warning {{packed attribute is unnecessary for 'S15'}}
+  struct S14 s;
+  char a;
+} __attribute__((packed));
+
+struct S16 { // expected-warning {{padding size of 'S16' with 2 bytes to alignment boundar}} expected-warning {{packed attribute is unnecessary for 'S16'}}
+  char a,b;
+} __attribute__((packed, aligned(4)));
+
+struct S17 {
+  struct S16 s;
+  char a,b;
+} __attribute__((packed, aligned(2)));
+
+struct S18 { // expected-warning {{padding size of 'S18' with 2 bytes to alignment boundar}} expected-warning {{packed attribute is unnecessary for 'S18'}}
+  struct S16 s;
+  char a,b;
+} __attribute__((packed, aligned(4)));
+
 // 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*) { }
+void f(S1*, S2*, S3*, S4*, S5*, S6*, S7*, S8*, S9*, S10*, S11*, S12*, S13*, S14*, S15*, S16*, S17*, S18*) { }