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

[Aaron Ballman via cfe-commits]

aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a comment.

Thank you for working on this diagnostic! A few questions and comments below.


================
Comment at: lib/Sema/SemaExpr.cpp:10518
@@ +10517,3 @@
+  // Taking the address of a data member/field of a packed
+  // struct may be a problem if the pointer value is dereferenced
+  MemberExpr *ME = dyn_cast<MemberExpr>(op);
----------------
Missing a full stop at the end of the sentence.

================
Comment at: lib/Sema/SemaExpr.cpp:10519
@@ +10518,3 @@
+  // struct may be a problem if the pointer value is dereferenced
+  MemberExpr *ME = dyn_cast<MemberExpr>(op);
+  while (ME && isa<FieldDecl>(ME->getMemberDecl())) {
----------------
Can use `const auto *` here instead of repeating the type name.

================
Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+        ME->getMemberDecl()->hasAttr<PackedAttr>()) {
+      Diag(OpLoc, diag::warn_taking_address_of_packed_member)
+          << ME->getMemberDecl() << RD;
----------------
I wonder if we should diagnose only when the resulting pointer is actually misaligned. For instance, the following code should be just fine:
```
struct __attribute__((packed)) S {
  int i;
};

void f(S s) {
  int *ip = &s.i; // Why should this warn?
}
```
Have you run this patch over any large code bases to see how high the false-positive rate is? How do you envision users silencing this diagnostic if they have a false-positive? Something like `&(s.x)` (since I you're not ignoring paren imp casts)?

================
Comment at: test/Sema/address-packed.c:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -xc++ -fsyntax-only -verify %s
----------------
You should run clang-format over this file (or indeed, the entire patch).

================
Comment at: test/Sema/address-packed.c:2
@@ +1,3 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -xc++ -fsyntax-only -verify %s
+extern void f1(int *);
----------------
I would split this test case into two, one in Sema for C and one in SemaCXX for C++.


http://reviews.llvm.org/D20561