Page MenuHomePhabricator

[clang] Change the condition of unnecessary packed warning
ClosedPublic

Authored by yawanng on Jun 12 2017, 12:24 PM.

Details

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.

Diff Detail

Event Timeline

yawanng created this revision.Jun 12 2017, 12:24 PM
chh edited edge metadata.Jun 12 2017, 1:27 PM

LGTM, leave it Argyrios's approval.

Eugene.Zelenko retitled this revision from [clang-tidy] A better format for unnecessary packed warning. to [clang] A better format for unnecessary packed warning..Jun 12 2017, 2:23 PM
srhines added a subscriber: srhines.

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

rsmith added a subscriber: rsmith.Jul 5 2017, 4:36 PM

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.

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 added a comment.Jul 6 2017, 12:56 AM

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 updated this revision to Diff 108380.Jul 26 2017, 3:59 PM
yawanng edited the summary of this revision. (Show Details)

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

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

yawanng retitled this revision from [clang] A better format for unnecessary packed warning. to [clang] Change the condition of unnecessary packed warning.Jul 26 2017, 4:03 PM
chh added inline comments.Jul 26 2017, 4:24 PM
lib/AST/RecordLayoutBuilder.cpp
1888

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

yawanng added inline comments.Jul 26 2017, 4:26 PM
lib/AST/RecordLayoutBuilder.cpp
1888

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

yawanng updated this revision to Diff 108395.Jul 26 2017, 6:02 PM
yawanng edited the summary of this revision. (Show Details)

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 updated this revision to Diff 108397.Jul 26 2017, 6:08 PM
yawanng updated this revision to Diff 108401.Jul 26 2017, 6:29 PM

Add more tests.

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?

yawanng updated this revision to Diff 108513.Jul 27 2017, 12:20 PM
yawanng marked 2 inline comments as done.
yawanng edited the summary of this revision. (Show Details)
chh added inline comments.Jul 27 2017, 2:45 PM
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 updated this revision to Diff 108555.Jul 27 2017, 5:16 PM
yawanng marked an inline comment as done.
yawanng edited the summary of this revision. (Show Details)

Add more tests and restrict the conditions.

chh added inline comments.Jul 28 2017, 9:45 AM
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
152

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

yawanng updated this revision to Diff 108665.Jul 28 2017, 10:01 AM
yawanng marked 3 inline comments as done.
chh added a comment.Jul 28 2017, 10:33 AM

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

chh accepted this revision.Jul 31 2017, 3:27 PM
This revision is now accepted and ready to land.Jul 31 2017, 3:27 PM
yawanng closed this revision.Aug 1 2017, 2:42 PM
yawanng marked an inline comment as done.