[PATCH] D34114: [clang] A better format for unnecessary packed warning.
yawanng added a comment. In https://reviews.llvm.org/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 :-) https://reviews.llvm.org/D34114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34114: [clang] A better format for unnecessary packed warning.
yawanng updated this revision to Diff 108380.
yawanng edited the summary of this revision.
yawanng added a comment.
Change the condition of this unnecessary packed warning to when the alignment
of the class is one byte. Remove all field-level warning.
https://reviews.llvm.org/D34114
Files:
lib/AST/RecordLayoutBuilder.cpp
test/CodeGenCXX/warn-padded-packed.cpp
Index: test/CodeGenCXX/warn-padded-packed.cpp
===
--- test/CodeGenCXX/warn-padded-packed.cpp
+++ test/CodeGenCXX/warn-padded-packed.cpp
@@ -17,7 +17,7 @@
} __attribute__((packed));
struct S4 {
- int i; // expected-warning {{packed attribute is unnecessary for 'i'}}
+ int i;
char c;
} __attribute__((packed));
@@ -46,18 +46,18 @@
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));
@@ -72,5 +72,14 @@
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));
+
// 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*) { }
Index: lib/AST/RecordLayoutBuilder.cpp
===
--- lib/AST/RecordLayoutBuilder.cpp
+++ lib/AST/RecordLayoutBuilder.cpp
@@ -1882,10 +1882,8 @@
<< (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 alignment is 1 byte
already.
+if (Packed && UnpackedAlignment == CharUnits::One())
Diag(D->getLocation(), diag::warn_unnecessary_packed)
<< Context.getTypeDeclType(RD);
}
@@ -1978,12 +1976,6 @@
<< 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 ,
Index: test/CodeGenCXX/warn-padded-packed.cpp
===
--- test/CodeGenCXX/warn-padded-packed.cpp
+++ test/CodeGenCXX/warn-padded-packed.cpp
@@ -17,7 +17,7 @@
} __attribute__((packed));
struct S4 {
- int i; // expected-warning {{packed attribute is unnecessary for 'i'}}
+ int i;
char c;
} __attribute__((packed));
@@ -46,18 +46,18 @@
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));
@@ -72,5 +72,14 @@
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));
+
// 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*,
[PATCH] D34114: [clang] A better format for unnecessary packed warning.
chh added a comment. 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. Repository: rL LLVM https://reviews.llvm.org/D34114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34114: [clang] A better format for unnecessary packed warning.
srhines added a comment.
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)?
Comment at: test/CodeGenCXX/warn-padded-packed.cpp:19
-struct S4 {
- int i; // expected-warning {{packed attribute is unnecessary for 'i'}}
+struct S4 { // expected-warning {{packed attribute is unnecessary for field:
'i'}}
+ int i;
rsmith wrote:
> 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`.
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.
Repository:
rL LLVM
https://reviews.llvm.org/D34114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34114: [clang] A better format for unnecessary packed warning.
rsmith added a comment.
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 :)
Comment at: lib/AST/RecordLayoutBuilder.cpp:1894-1896
+ llvm::SmallString<128> WarningMsg = (UnnecessaryPackedFields.size() == 1)
+ ? StringRef("field:")
+ : StringRef("fields:");
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.
Comment at: lib/AST/RecordLayoutBuilder.cpp:1897-1900
+ for (const auto identifier : UnnecessaryPackedFields) {
+WarningMsg += " '";
+WarningMsg += identifier->getName();
+WarningMsg += "',";
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', ...
```
Comment at: test/CodeGenCXX/warn-padded-packed.cpp:19
-struct S4 {
- int i; // expected-warning {{packed attribute is unnecessary for 'i'}}
+struct S4 { // expected-warning {{packed attribute is unnecessary for field:
'i'}}
+ int i;
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`.
Comment at: test/CodeGenCXX/warn-padded-packed.cpp:49
struct S9 { // expected-warning {{packed attribute is unnecessary for 'S9'}}
+ int x;
Again this seems wrong.
Comment at: test/CodeGenCXX/warn-padded-packed.cpp:75
+struct S14 { // expected-warning {{packed attribute is unnecessary for fields:
'i', 'j'}}
+ int i;
Again, this looks exactly backwards.
Repository:
rL LLVM
https://reviews.llvm.org/D34114
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34114: [clang] A better format for unnecessary packed warning.
yawanng added a comment. PING Repository: rL LLVM https://reviews.llvm.org/D34114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34114: [clang] A better format for unnecessary packed warning.
srhines added a comment. Richard, can you take a look at this, or suggest someone who would be a good reviewer for this improved diagnostic? Thanks. Repository: rL LLVM https://reviews.llvm.org/D34114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D34114: [clang] A better format for unnecessary packed warning.
yawanng added a comment. Ping. Repository: rL LLVM https://reviews.llvm.org/D34114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
