[PATCH] D20561: Warn when taking address of packed member

Aaron Ballman via cfe-commits cfe-commits at lists.llvm.org
Wed Jun 1 12:06:52 PDT 2016


aaron.ballman requested changes to this revision.
aaron.ballman added a comment.
This revision now requires changes to proceed.

In http://reviews.llvm.org/D20561#445824, @rogfer01 wrote:

> I think I wasn't clear with the purpose of the fix-it: there are a few cases where getting the address of an unaligned pointer is safe (i.e. false positives).
>
> For instance, when I checked Firefox and Chromium there are cases where getting the address of an unaligned pointer is fine. For the particular case of these two browsers, they both use a library (usrsctp) that represents protocol data as packed structs. That library passes addresses of packed fields to `memcpy` and `memmove` which is OK.


I think this is a false-positive that should be fixed.

> The fix-it can help silencing the warning for those cases once deemed safe. This is the reason why I keep comparing the new warning to the assignment-vs-comparison warning: there may not be an error, just extra syntax (parentheses) can be used to silence the warning.


Assignment vs comparison is not a valuable comparison because that is a case relying on programmer intent (could be assignment, could be comparison, but there's no way to tell what's more likely, so use parens to disambiguate). In this case, there is code that could be dangerous (more likely) or benign (less likely), but adding parens does not disambiguate between two choices, it simply disables the diagnostic while leaving the semantics the same.

Fix-its are meant to be used when we can reasonably fix the user's code. Adding parens does *not* fix the user's code for the true positive case and is actually a dangerous code transformation (because it hides a true positive). I do not think this check should have a fix-it simply to silence false positives. If the false positive rate is that high, this should become a clang-tidy check instead (and I would still be opposed to the fix-it hint there as well).


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5392
@@ -5388,1 +5391,3 @@
+def note_address_of_packed_member_silence : Note<
+  "place parentheses around the '%0' expression to silence this warning">;
 
----------------
I don't think this note adds value. Placing parens around the expression does silence the warning for false-positives, but that seems more like a documentation issue than a diagnostic.

================
Comment at: lib/Sema/SemaExpr.cpp:10537-10538
@@ +10536,4 @@
+      Diag(rhs->getLocStart(), diag::note_address_of_packed_member_silence)
+          << rhs << FixItHint::CreateInsertion(rhs->getLocStart(), "(")
+          << FixItHint::CreateInsertion(rhs->getLocEnd(), ")");
+      break;
----------------
I do not think this diagnostic should have a FixIt hint. This isn't actually *fixing* the problem, it is simply a way to silence the diagnostic while retaining the same behavior.

================
Comment at: test/SemaCXX/address-packed.cpp:92-93
@@ +91,4 @@
+   // expected-warning at -1 {{packed member 'X' of class or structure 'S<float>'}}
+   // expected-note at -2 {{place parentheses around the 'this->X'}}
+   // expected-note at -3 {{place parentheses around the 'this->X'}}
+  }
----------------
Why `this->X` in the diagnostic when that's not what the user wrote?


http://reviews.llvm.org/D20561





More information about the cfe-commits mailing list