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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

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

> Only warn if the expression is of the form &a.x this way &(a.x) can be used to silence the warning.


I think this is a reasonable idea, but would still like to make sure we have a low false-positive rate by default.


================
Comment at: lib/Sema/SemaExpr.cpp:10519
@@ +10518,3 @@
+  // struct may be a problem if the pointer value is dereferenced.
+  Expr* rhs = OrigOp.get();
+  const auto *ME = dyn_cast<MemberExpr>(rhs);
----------------
The * binds to the identifier instead of the type; you should run the patch through clang-format (formatting issues are also in the test files).

================
Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+    if (RD->hasAttr<PackedAttr>() ||
+        ME->getMemberDecl()->hasAttr<PackedAttr>()) {
+      Diag(OpLoc, diag::warn_taking_address_of_packed_member)
----------------
Ah, I forgot that the attribute also affected the alignment of the struct itself. Amending my false-positive idea, the following should be fine, shouldn't it?:
```
struct __attribute__((packed)) S {
  char c;
  int i;
  char c2;
};

void f(S s) {
  char *cp = &s.c;
  char *cp2 = &s.c2;
}
```
I think perhaps we should not diagnose if the member's type also has a one-byte alignment (or, for instance, uses alignas(1) to achieve that). What do you think?

================
Comment at: test/SemaCXX/address-packed.cpp:1
@@ +1,2 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
----------------
This test should only really test the parts that are different from the C test (such as inheritance). The goal is to have the maximum test coverage with the minimum amount of testing code.


http://reviews.llvm.org/D20561