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

[Aaron Ballman via cfe-commits]

On Mon, Aug 15, 2016 at 6:20 PM, Matthias Braun  wrote:
> MatzeB added a subscriber: MatzeB.
> MatzeB added a comment.
>
> The sanitizer code triggers this warning for code that looks conceptually 
> like this:
>
>   typedef struct Bla { char bar; int foo; } __attribute__((packed));
>
>   uintptr_t getu(struct Bla *b) { return (uintptr_t)>foo; }
>
> Resulting in:
>
>   taking address of packed member 'foo' of class or structure
> 'Bla' may result in an unaligned pointer value
> [-Waddress-of-packed-member]
>
> Of course the warning can be silenced with `return 
> (uintptr_t)(char*)>foo;` still casting to an int type seems like a benign 
> case for this warning so maybe we should exclude that?

I agree, it seems sensible to silence the warning in that case, yes.

~Aaron

>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D20561
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Matthias Braun via cfe-commits]

MatzeB added a subscriber: MatzeB.
MatzeB added a comment.

The sanitizer code triggers this warning for code that looks conceptually like 
this:

  typedef struct Bla { char bar; int foo; } __attribute__((packed));
  
  uintptr_t getu(struct Bla *b) { return (uintptr_t)>foo; }

Resulting in:

  taking address of packed member 'foo' of class or structure
'Bla' may result in an unaligned pointer value
[-Waddress-of-packed-member]

Of course the warning can be silenced with `return (uintptr_t)(char*)>foo;` 
still casting to an int type seems like a benign case for this warning so maybe 
we should exclude that?


Repository:
  rL LLVM

https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL278483: This patch implements PR#22821. (authored by 
rogfer01).

Changed prior to commit:
  https://reviews.llvm.org/D20561?vs=64114=67807#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D20561

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaCast.cpp
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaInit.cpp

Index: cfe/trunk/lib/Sema/SemaCast.cpp
===
--- cfe/trunk/lib/Sema/SemaCast.cpp
+++ cfe/trunk/lib/Sema/SemaCast.cpp
@@ -256,6 +256,7 @@
   Op.CheckConstCast();
   if (Op.SrcExpr.isInvalid())
 return ExprError();
+  DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
 }
 return Op.complete(CXXConstCastExpr::Create(Context, Op.ResultType,
   Op.ValueKind, Op.SrcExpr.get(), DestTInfo,
@@ -279,6 +280,7 @@
   Op.CheckReinterpretCast();
   if (Op.SrcExpr.isInvalid())
 return ExprError();
+  DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
 }
 return Op.complete(CXXReinterpretCastExpr::Create(Context, Op.ResultType,
 Op.ValueKind, Op.Kind, Op.SrcExpr.get(),
@@ -291,6 +293,7 @@
   Op.CheckStaticCast();
   if (Op.SrcExpr.isInvalid())
 return ExprError();
+  DiscardMisalignedMemberAddress(DestType.getTypePtr(), E);
 }
 
 return Op.complete(CXXStaticCastExpr::Create(Context, Op.ResultType,
Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -6022,7 +6022,9 @@
   CheckTollFreeBridgeCast(castType, CastExpr);
   
   CheckObjCBridgeRelatedCast(castType, CastExpr);
-  
+
+  DiscardMisalignedMemberAddress(castType.getTypePtr(), CastExpr);
+
   return BuildCStyleCastExpr(LParenLoc, castTInfo, RParenLoc, CastExpr);
 }
 
@@ -10614,6 +10616,8 @@
   if (op->getType()->isObjCObjectType())
 return Context.getObjCObjectPointerType(op->getType());
 
+  CheckAddressOfPackedMember(op);
+
   return Context.getPointerType(op->getType());
 }
 
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -8383,6 +8383,8 @@
 
   DiagnoseNullConversion(S, E, T, CC);
 
+  S.DiscardMisalignedMemberAddress(Target, E);
+
   if (!Source->isIntegerType() || !Target->isIntegerType())
 return;
 
@@ -9453,6 +9455,7 @@
 CheckUnsequencedOperations(E);
   if (!IsConstexpr && !E->isValueDependent())
 CheckForIntOverflow(E);
+  DiagnoseMisalignedMembers();
 }
 
 void Sema::CheckBitFieldInitialization(SourceLocation InitLoc,
@@ -10998,3 +11001,67 @@
 << ArgumentExpr->getSourceRange()
 << TypeTagExpr->getSourceRange();
 }
+
+void Sema::AddPotentialMisalignedMembers(Expr *E, RecordDecl *RD, ValueDecl *MD,
+ CharUnits Alignment) {
+  MisalignedMembers.emplace_back(E, RD, MD, Alignment);
+}
+
+void Sema::DiagnoseMisalignedMembers() {
+  for (MisalignedMember  : MisalignedMembers) {
+Diag(m.E->getLocStart(), diag::warn_taking_address_of_packed_member)
+<< m.MD << m.RD << m.E->getSourceRange();
+  }
+  MisalignedMembers.clear();
+}
+
+void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
+  if (!T->isPointerType())
+return;
+  if (isa(E) &&
+  cast(E)->getOpcode() == UO_AddrOf) {
+auto *Op = cast(E)->getSubExpr()->IgnoreParens();
+if (isa(Op)) {
+  auto MA = std::find(MisalignedMembers.begin(), MisalignedMembers.end(),
+  MisalignedMember(Op));
+  if (MA != MisalignedMembers.end() &&
+  Context.getTypeAlignInChars(T->getPointeeType()) <= MA->Alignment)
+MisalignedMembers.erase(MA);
+}
+  }
+}
+
+void Sema::RefersToMemberWithReducedAlignment(
+Expr *E,
+std::function Action) {
+  const auto *ME = dyn_cast(E);
+  while (ME && isa(ME->getMemberDecl())) {
+QualType BaseType = ME->getBase()->getType();
+if (ME->isArrow())
+  BaseType = BaseType->getPointeeType();
+RecordDecl *RD = BaseType->getAs()->getDecl();
+
+ValueDecl *MD = ME->getMemberDecl();
+bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
+if (ByteAligned) // Attribute packed does not have any effect.
+  break;
+
+if (!ByteAligned &&
+(RD->hasAttr() || (MD->hasAttr( {
+  CharUnits Alignment = std::min(Context.getTypeAlignInChars(MD->getType()),
+ Context.getTypeAlignInChars(BaseType));
+  // Notify that this expression designates a member with reduced 

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Hi, friendly ping.

@rsmith any further comment on this?

Thank you very much.


https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

> Please wait until someone has had the chance to review before committing (the 
> fix does not appear trivial and the original code broke code). I'm on 
> vacation today (in theory), but perhaps @rsmith will have a chance to review.


Sure. Thanks.


https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 updated the summary for this revision.
rogfer01 removed rL LLVM as the repository for this revision.
rogfer01 updated this revision to Diff 64114.
rogfer01 added a comment.

Remove the diagnostic for the binding of references.


https://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  test/Sema/address-packed-member-memops.c
  test/Sema/address-packed.c
  test/SemaCXX/address-packed-member-memops.cpp
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,114 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void f4(int &);
+
+void to_void(void *);
+
+template 
+void sink(T...);
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+
+to_void(); // no-warning
+void *p1 =// no-warning
+void *p2 = static_cast();  // no-warning
+void *p3 = reinterpret_cast(); // no-warning
+void *p4 = (void *)   // no-warning
+sink(p1, p2, p3, p4);
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
+
+template 
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+return  // expected-warning {{packed member 'X' of class or structure 'S'}}
+   // expected-warning@-1 {{packed member 'X' of class or structure 'S'}}
+  }
+};
+
+template 
+void h(Ty *);
+
+void g1() {
+  S s1;
+  s1.get(); // expected-note {{in instantiation of member function 'S::get'}}
+
+  S s2;
+  s2.get();
+
+  S s3;
+  s3.get(); // expected-note {{in instantiation of member function 'S::get'}}
+}
Index: test/SemaCXX/address-packed-member-memops.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed-member-memops.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+struct B {
+  int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+  struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+extern "C" {
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+}
+
+int x;
+
+void foo() {
+  memcpy(, , sizeof(b));
+  memmove(, , sizeof(b));
+  memset(, 0, sizeof(b));
+  x = memcmp(, , sizeof(b));
+}
Index: test/Sema/address-packed.c
===
--- /dev/null
+++ test/Sema/address-packed.c
@@ -0,0 +1,160 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+};
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void to_void(void *);
+
+void g0(void) {
+  {
+struct Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+struct Arguable arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(); // no-warning
+
+f1((int *)(void *)); // no-warning
+to_void();   // no-warning
+void *p =   // no-warning;
+

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

I've opened https://llvm.org/bugs/show_bug.cgi?id=28571 to track the reference 
binding issue.


Repository:
  rL LLVM

https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

In https://reviews.llvm.org/D20561#484421, @jyknight wrote:

> This seems to trigger even for the implicitly generated copier of a packed 
> struct. E.g.
>
>   #include 
>  
>   void copyit(epoll_event, const epoll_event ) {
> out = in;
>   }
>  
>
>
> Is that as intended?


No, it wasn't. It seems to happen as well for the implicit copy constructor as 
well.

  #include 
  
  void copyit2(epoll_event foo);
  void copyit(epoll_event ) { copyit2(out); }

  clang++ -c test.cc
  In file included from test.cc:1:
  /usr/include/x86_64-linux-gnu/sys/epoll.h:87:8: error: binding reference to 
packed member 'data' of class or structure 'epoll_event'
  struct epoll_event
 ^~~
  test.cc:4:41: note: implicit copy constructor for 'epoll_event' first 
required here
  void copyit(epoll_event ) { copyit2(out); }
  ^
  1 error generated.


Repository:
  rL LLVM

https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 reopened this revision.
rogfer01 added a comment.
This revision is now accepted and ready to land.

I suggest that we handle the reference binding diagnostic in another change and 
leave this one only for the address taking diagnostic.


Repository:
  rL LLVM

https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Reverted.


Repository:
  rL LLVM

https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

Yes I will revert this ASAP.


Sorry for the fuss.


From: Aaron Ballman <aaron.ball...@gmail.com>
Sent: 14 July 2016 19:30:58
To: reviews+d20561+public+af1fea8a731d8...@reviews.llvm.org
Cc: Roger Ferrer Ibanez; Richard Smith; James Y Knight; Evgenii Stepanov; 
cfe-commits
Subject: Re: [PATCH] D20561: Warn when taking address of packed member

Roger, can you please revert? This seems to have caused some pain for
our users, and it would be good to unblock them while deciding what to
do about the issues.

~Aaron

On Thu, Jul 14, 2016 at 2:19 PM, James Y Knight <jykni...@google.com> wrote:
> jyknight added a comment.
>
> Regardless, I think this should not have added a new un-disableable error, 
> but instead only a default-on warning.
>
> The ""binding reference to packed member" error is firing on some of our 
> code, and even if it's not a false-positive, it should be possible to disable 
> it until the code is modified.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D20561
>
>
>

IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

Roger, can you please revert? This seems to have caused some pain for
our users, and it would be good to unblock them while deciding what to
do about the issues.

~Aaron

On Thu, Jul 14, 2016 at 2:19 PM, James Y Knight  wrote:
> jyknight added a comment.
>
> Regardless, I think this should not have added a new un-disableable error, 
> but instead only a default-on warning.
>
> The ""binding reference to packed member" error is firing on some of our 
> code, and even if it's not a false-positive, it should be possible to disable 
> it until the code is modified.
>
>
> Repository:
>   rL LLVM
>
> https://reviews.llvm.org/D20561
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via cfe-commits]

jyknight added a comment.

Regardless, I think this should not have added a new un-disableable error, but 
instead only a default-on warning.

The ""binding reference to packed member" error is firing on some of our code, 
and even if it's not a false-positive, it should be possible to disable it 
until the code is modified.


Repository:
  rL LLVM

https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[James Y Knight via cfe-commits]

jyknight added a subscriber: jyknight.
jyknight added a comment.

This seems to trigger even for the implicitly generated copier of a packed 
struct. E.g.

  #include 
  
  void copyit(epoll_event, const epoll_event ) {
out = in;
  }

Is that as intended?


Repository:
  rL LLVM

https://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL275417: Diagnose taking address and reference binding of 
packed members (authored by rogfer01).

Changed prior to commit:
  https://reviews.llvm.org/D20561?vs=61089=63970#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D20561

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/include/clang/Sema/Sema.h
  cfe/trunk/lib/Sema/SemaCast.cpp
  cfe/trunk/lib/Sema/SemaChecking.cpp
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/lib/Sema/SemaInit.cpp
  cfe/trunk/test/Sema/address-packed-member-memops.c
  cfe/trunk/test/Sema/address-packed.c
  cfe/trunk/test/SemaCXX/address-packed-member-memops.cpp
  cfe/trunk/test/SemaCXX/address-packed.cpp

Index: cfe/trunk/lib/Sema/SemaExpr.cpp
===
--- cfe/trunk/lib/Sema/SemaExpr.cpp
+++ cfe/trunk/lib/Sema/SemaExpr.cpp
@@ -6001,7 +6001,9 @@
   CheckTollFreeBridgeCast(castType, CastExpr);
   
   CheckObjCBridgeRelatedCast(castType, CastExpr);
-  
+
+  DiscardMisalignedMemberAddress(castType.getTypePtr(), CastExpr);
+
   return BuildCStyleCastExpr(LParenLoc, castTInfo, RParenLoc, CastExpr);
 }
 
@@ -10534,6 +10536,8 @@
   if (op->getType()->isObjCObjectType())
 return Context.getObjCObjectPointerType(op->getType());
 
+  CheckAddressOfPackedMember(op);
+
   return Context.getPointerType(op->getType());
 }
 
Index: cfe/trunk/lib/Sema/SemaChecking.cpp
===
--- cfe/trunk/lib/Sema/SemaChecking.cpp
+++ cfe/trunk/lib/Sema/SemaChecking.cpp
@@ -8302,6 +8302,8 @@
 
   DiagnoseNullConversion(S, E, T, CC);
 
+  S.DiscardMisalignedMemberAddress(Target, E);
+
   if (!Source->isIntegerType() || !Target->isIntegerType())
 return;
 
@@ -9371,6 +9373,7 @@
   CheckUnsequencedOperations(E);
   if (!IsConstexpr && !E->isValueDependent())
 CheckForIntOverflow(E);
+  DiagnoseMisalignedMembers();
 }
 
 void Sema::CheckBitFieldInitialization(SourceLocation InitLoc,
@@ -10916,3 +10919,67 @@
 << ArgumentExpr->getSourceRange()
 << TypeTagExpr->getSourceRange();
 }
+
+void Sema::AddPotentialMisalignedMembers(Expr *E, RecordDecl *RD, ValueDecl *MD,
+ CharUnits Alignment) {
+  MisalignedMembers.emplace_back(E, RD, MD, Alignment);
+}
+
+void Sema::DiagnoseMisalignedMembers() {
+  for (MisalignedMember  : MisalignedMembers) {
+Diag(m.E->getLocStart(), diag::warn_taking_address_of_packed_member)
+<< m.MD << m.RD << m.E->getSourceRange();
+  }
+  MisalignedMembers.clear();
+}
+
+void Sema::DiscardMisalignedMemberAddress(const Type *T, Expr *E) {
+  if (!T->isPointerType())
+return;
+  if (isa(E) &&
+  cast(E)->getOpcode() == UO_AddrOf) {
+auto *Op = cast(E)->getSubExpr()->IgnoreParens();
+if (isa(Op)) {
+  auto MA = std::find(MisalignedMembers.begin(), MisalignedMembers.end(),
+  MisalignedMember(Op));
+  if (MA != MisalignedMembers.end() &&
+  Context.getTypeAlignInChars(T->getPointeeType()) <= MA->Alignment)
+MisalignedMembers.erase(MA);
+}
+  }
+}
+
+void Sema::RefersToMemberWithReducedAlignment(
+Expr *E,
+std::function Action) {
+  const auto *ME = dyn_cast(E);
+  while (ME && isa(ME->getMemberDecl())) {
+QualType BaseType = ME->getBase()->getType();
+if (ME->isArrow())
+  BaseType = BaseType->getPointeeType();
+RecordDecl *RD = BaseType->getAs()->getDecl();
+
+ValueDecl *MD = ME->getMemberDecl();
+bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
+if (ByteAligned) // Attribute packed does not have any effect.
+  break;
+
+if (!ByteAligned &&
+(RD->hasAttr() || (MD->hasAttr( {
+  CharUnits Alignment = std::min(Context.getTypeAlignInChars(MD->getType()),
+ Context.getTypeAlignInChars(BaseType));
+  // Notify that this expression designates a member with reduced alignment
+  Action(E, RD, MD, Alignment);
+  break;
+}
+ME = dyn_cast(ME->getBase());
+  }
+}
+
+void Sema::CheckAddressOfPackedMember(Expr *rhs) {
+  using namespace std::placeholders;
+  RefersToMemberWithReducedAlignment(
+  rhs, std::bind(::AddPotentialMisalignedMembers, std::ref(*this), _1,
+ _2, _3, _4));
+}
+
Index: cfe/trunk/lib/Sema/SemaInit.cpp
===
--- cfe/trunk/lib/Sema/SemaInit.cpp
+++ cfe/trunk/lib/Sema/SemaInit.cpp
@@ -6457,6 +6457,15 @@
   ExtendingEntity->getDecl());
 
   CheckForNullPointerDereference(S, CurInit.get());
+
+  S.RefersToMemberWithReducedAlignment(CurInit.get(), [&](Expr *E,
+  RecordDecl *RD,
+  ValueDecl 

Re: [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#482963, @rogfer01 wrote:

> If there are not any objections I will commit this tomorrow.


If @rsmith doesn't comment by tomorrow, then I think any feedback can be 
handled post-commit instead.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

If there are not any objections I will commit this tomorrow.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Ping?

Looking forward any further comments before committing.

Thank you very much.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Ping?

Any comment on this?

Thank you very much.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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#466545, @rogfer01 wrote:

> Ping? Any further comments, thoughts?
>
> Thank you very much.


The usual practice is to ping after a week has gone by, btw. There were 
standards meetings this week, so that may be delaying some of the comments on 
this thread.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Ping? Any further comments, thoughts?

Thank you very much.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Ping?

Thank you very much.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 updated this revision to Diff 61089.
rogfer01 added a comment.

Following @rsmith suggestion, the diagnostic is silenced if the address is 
converted to a pointer with lower or equal alignment requirements.


http://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  test/Sema/address-packed-member-memops.c
  test/Sema/address-packed.c
  test/SemaCXX/address-packed-member-memops.cpp
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,118 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void f4(int &);
+
+void to_void(void *);
+
+template 
+void sink(T...);
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+
+int  = arguable.x; // expected-error {{binding reference to packed member 'x' of class or structure 'Foo::Arguable'}}
+sink(w);
+f4(arguable.x); // expected-error {{binding reference to packed member 'x' of class or structure 'Foo::Arguable'}}
+
+to_void(); // no-warning
+void *p1 =// no-warning
+void *p2 = static_cast();  // no-warning
+void *p3 = reinterpret_cast(); // no-warning
+void *p4 = (void *)   // no-warning
+sink(p1, p2, p3, p4);
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
+
+template 
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+return  // expected-warning {{packed member 'X' of class or structure 'S'}}
+   // expected-warning@-1 {{packed member 'X' of class or structure 'S'}}
+  }
+};
+
+template 
+void h(Ty *);
+
+void g1() {
+  S s1;
+  s1.get(); // expected-note {{in instantiation of member function 'S::get'}}
+
+  S s2;
+  s2.get();
+
+  S s3;
+  s3.get(); // expected-note {{in instantiation of member function 'S::get'}}
+}
Index: test/SemaCXX/address-packed-member-memops.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed-member-memops.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+struct B {
+  int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+  struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+extern "C" {
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+}
+
+int x;
+
+void foo() {
+  memcpy(, , sizeof(b));
+  memmove(, , sizeof(b));
+  memset(, 0, sizeof(b));
+  x = memcmp(, , sizeof(b));
+}
Index: test/Sema/address-packed.c
===
--- /dev/null
+++ test/Sema/address-packed.c
@@ -0,0 +1,160 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+};
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void to_void(void *);
+
+void g0(void) {
+  {
+struct Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+struct Arguable arguable;
+f2(); // no-warning
+ 

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked an inline comment as done.
rogfer01 added a comment.

http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Richard Smith via cfe-commits]

rsmith added a comment.

I agree, that looks like correct behavior for the warning. Thanks!



Comment at: lib/Sema/SemaCast.cpp:259-260
@@ -258,2 +258,4 @@
 return ExprError();
+  if (DestType->isVoidPointerType())
+DiscardMisalignedMemberAddress(E);
 }

More generally, I think we should discard it if the destination of the cast is 
a suitably-aligned pointer type. (For instance, casting to char* should be OK, 
and casting an __attribute__((packed,aligned(2))) to a 2-byte-aligned pointer 
type should be OK.)


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeniy Stepanov via cfe-commits]

eugenis added a comment.

This timeval thing looks like a legitimate warning to me.
I don't think the analysis should go beyond the function boundaries. If a 
callee expects timeval * as part of its signature it should get a properly 
aligned timeval *.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Firefox build shows a couple of warnings in the sctp library that already gave 
warnings with the earlier patches.

That code has the following structure.

  #define SCTP_PACKED __attribute__((packed))
  #define SCTP_IDENTIFICATION_SIZE 16
  // ...
  /* state cookie header */
  struct sctp_state_cookie {/* this is our definition... */
uint8_t identification[SCTP_IDENTIFICATION_SIZE];/* id of who we are */
struct timeval time_entered;/* the time I built cookie */
  // other fields
  } SCTP_PACKED;

The warning is triggered by the following code (the other occurence is almost 
exact).

  net->RTO = sctp_calculate_rto(stcb, asoc, net,
  >time_entered,   // ← warning 
here!
  sctp_align_unsafe_makecopy,
  SCTP_RTT_FROM_NON_DATA);

the called function being declared as

  uint32_t
  sctp_calculate_rto(struct sctp_tcb *stcb,
   struct sctp_association *asoc,
   struct sctp_nets *net,
   struct timeval *told,
   int safe, int rtt_from_sack)
  {
...

So I think that is a legitimate warning.

But the code is working (assuming this function is ever called that I didn't 
check). Checking the code, though, reveals a curious usage of `told`.

/* Copy it out for sparc64 */
if (safe == sctp_align_unsafe_makecopy) {
old = 
memcpy(, told, sizeof(struct timeval));
} else if (safe == sctp_align_safe_nocopy) {
old = told;
} else {
/* error */
SCTP_PRINTF("Huh, bad rto calc call\n");
return (0);
}

which suggests that the code somehow knows that the pointer is unaligned and 
cannot be copied straightforwardly.

We can cast to `void*` and back to the original pointer type.

  net->RTO = sctp_calculate_rto(stcb, asoc, net,
  (struct 
timeval*)(void*)>time_entered, // note: the cast is because this 
pointer is unaligned
  sctp_align_unsafe_makecopy,
  SCTP_RTT_FROM_NON_DATA);

Which looks a bit ugly to me but clearly pinpoints a problem and can be wrapped 
in a macro.

  #define UNALIGNED_ADDRESS(x) ((__typeof__(x))(void*)(x))
  
  ...
  
  net->RTO = sctp_calculate_rto(stcb, asoc, net,
  UNALIGNED_ADDRESS(>time_entered),
  sctp_align_unsafe_makecopy,
  SCTP_RTT_FROM_NON_DATA);

What do you think?


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 updated the summary for this revision.
rogfer01 updated this revision to Diff 60951.
rogfer01 added a comment.

Thanks @rsmith for the suggestions. I removed the whitelisting totally and 
changed the strategy. This patch implements your second suggestion which seemed 
more obvious to me. Since I expect the set of gathered misaligned member 
designations be small, I am using a plain SmallVector but maybe there is a more 
suitable structure.

I am now building Firefox and will post an update with the results.


http://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Sema/SemaCast.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  lib/Sema/SemaInit.cpp
  test/Sema/address-packed-member-memops.c
  test/Sema/address-packed.c
  test/SemaCXX/address-packed-member-memops.cpp
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,118 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void f4(int &);
+
+void to_void(void *);
+
+template 
+void sink(T...);
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+
+int  = arguable.x; // expected-error {{binding reference to packed member 'x' of class or structure 'Foo::Arguable'}}
+sink(w);
+f4(arguable.x); // expected-error {{binding reference to packed member 'x' of class or structure 'Foo::Arguable'}}
+
+to_void(); // no-warning
+void *p1 =// no-warning
+void *p2 = static_cast();  // no-warning
+void *p3 = reinterpret_cast(); // no-warning
+void *p4 = (void *)   // no-warning
+sink(p1, p2, p3, p4);
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
+
+template 
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+return  // expected-warning {{packed member 'X' of class or structure 'S'}}
+   // expected-warning@-1 {{packed member 'X' of class or structure 'S'}}
+  }
+};
+
+template 
+void h(Ty *);
+
+void g1() {
+  S s1;
+  s1.get(); // expected-note {{in instantiation of member function 'S::get'}}
+
+  S s2;
+  s2.get();
+
+  S s3;
+  s3.get(); // expected-note {{in instantiation of member function 'S::get'}}
+}
Index: test/SemaCXX/address-packed-member-memops.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed-member-memops.cpp
@@ -0,0 +1,28 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// expected-no-diagnostics
+
+struct B {
+  int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+  struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+extern "C" {
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+}
+
+int x;
+
+void foo() {
+  memcpy(, , sizeof(b));
+  memmove(, , sizeof(b));
+  memset(, 0, sizeof(b));
+  x = memcmp(, , sizeof(b));
+}
Index: test/Sema/address-packed.c
===
--- /dev/null
+++ test/Sema/address-packed.c
@@ -0,0 +1,143 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union 

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

[Richard Smith via cfe-commits]

rsmith added a comment.

This still looks like it will have has lots of false positives for cases like:

  struct __attribute__((packed)) A {
char c;
int n;
  } a;
  void *p = 

It also looks like it will now have false negatives for cases like:

  memcpy(x, y, *);

I think whitelisting specific functions is not a reasonable approach here; 
instead, how about deferring the check until you see how the misaligned pointer 
is used? A couple of approaches seem feasible:

- you could extend the conversion-checking code in SemaChecking to look for 
such misaligned operations that are not immediately converted to a pointer type 
with suitable (lower) alignment requirements
- you could build a list in Sema of the cases that are pending a diagnostic, 
produce diagnostics at the end of the full-expression, and remove items from 
the list when you see a suitable conversion applied to them

In any case, this diagnostic should apply to reference binding as well as 
pointers.

GCC appears to check this as a separate step, at least after it applies its 
fold; for example:

  struct __attribute__((packed, aligned(4))) S { char c[4]; int n; } s;
  int k;
  int  = true ? s.n : k; // gcc rejects, "cannot bind paced field 's.S::n' to 
'int&'
  int  = false ? s.n : k; // gcc accepts


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

A build of Firefox does not show any false positive now.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 removed rL LLVM as the repository for this revision.
rogfer01 updated this revision to Diff 60664.
rogfer01 added a comment.

This new patch adds a whitelist for memcpy, memmove, memset and memcmp.

I'm not entirely happy with the strategy that I'm using, where Parser 
communicates to Sema which is the LHS of a direct function call being parsed. 
Better approaches are of course welcome.


http://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  include/clang/Sema/Sema.h
  lib/Parse/ParseExpr.cpp
  lib/Sema/SemaChecking.cpp
  lib/Sema/SemaExpr.cpp
  test/Sema/address-packed-member-whitelist.c
  test/Sema/address-packed.c
  test/SemaCXX/address-packed-member-whitelist.cpp
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,100 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
+
+template 
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+return  // expected-warning {{packed member 'X' of class or structure 'S'}}
+   // expected-warning@-1 {{packed member 'X' of class or structure 'S'}}
+  }
+};
+
+template 
+void h(Ty *);
+
+void g1() {
+  S s1;
+  s1.get(); // expected-note {{in instantiation of member function 'S::get'}}
+
+  S s2;
+  s2.get();
+
+  S s3;
+  s3.get(); // expected-note {{in instantiation of member function 'S::get'}}
+}
Index: test/SemaCXX/address-packed-member-whitelist.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed-member-whitelist.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+struct B {
+  int x, y, z, w;
+} b;
+
+struct __attribute__((packed)) A {
+  struct B b;
+} a;
+
+typedef __typeof__(sizeof(int)) size_t;
+
+extern "C" {
+void *memcpy(void *dest, const void *src, size_t n);
+int memcmp(const void *s1, const void *s2, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+void *memset(void *s, int c, size_t n);
+}
+
+void *my_memcpy(void *dest, const void *src, size_t n);
+int my_memcmp(const void *s1, const void *s2, size_t n);
+void *my_memmove(void *dest, const void *src, size_t n);
+void *my_memset(void *s, int c, size_t n);
+
+int x;
+
+void foo() {
+  memcpy(, , sizeof(b)); // no-warning
+  memmove(, , sizeof(b));// no-warning
+  memset(, 0, sizeof(b));  // no-warning
+  x = memcmp(, , sizeof(b)); // no-warning
+
+  (memcpy)(, , sizeof(b)); // no-warning
+  (memmove)(, , sizeof(b));// no-warning
+  (memset)(, 0, sizeof(b));  // no-warning
+  x = (memcmp)(, , sizeof(b)); // no-warning
+
+  ::memcpy(, , sizeof(b)); // no-warning
+  ::memmove(, , sizeof(b));// no-warning
+  ::memset(, 0, sizeof(b));  // no-warning
+  x = ::memcmp(, , sizeof(b)); // no-warning
+
+  (::memcpy)(, , sizeof(b)); // no-warning
+  (::memmove)(, , sizeof(b));// no-warning
+  (::memset)(, 0, sizeof(b));  // no-warning
+  x = (::memcmp)(, , sizeof(b)); // no-warning
+
+  my_memcpy(, , sizeof(b)); // expected-warning {{packed member 'b' of class or structure 'A'}}
+  my_memmove(, , sizeof(b));// expected-warning {{packed member 'b' of class or structure 'A'}}
+  my_memset(, 0, sizeof(b));  // expected-warning {{packed member 'b' of class or structure 'A'}}
+  x = my_memcmp(, , sizeof(b)); // expected-warning 

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 reopened this revision.
rogfer01 added a comment.
This revision is now accepted and ready to land.

I've reverted the commit.

During implementation of the white list I felt the changes were too much for a 
follow up commit, so I prefer some feedback first.


Repository:
  rL LLVM

http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

Hi Aaron, Euvgeni,

I'll try to whitelist these functions.

Kind regards,
Roger

-Original Message-
From: Aaron Ballman [mailto:aaron.ball...@gmail.com] 
Sent: 14 June 2016 00:30
To: Evgenii Stepanov
Cc: reviews+d20561+public+af1fea8a731d8...@reviews.llvm.org; Roger Ferrer 
Ibanez; Richard Smith; cfe-commits
Subject: Re: [PATCH] D20561: Warn when taking address of packed member

On Mon, Jun 13, 2016 at 7:17 PM, Evgenii Stepanov <euge...@google.com> wrote:
> On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman <aaron.ball...@gmail.com> 
> wrote:
>> On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov <euge...@google.com> wrote:
>>> eugenis added a subscriber: eugenis.
>>> eugenis added a comment.
>>>
>>> In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
>>>
>>>> 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.
>>>
>>>
>>> This patch was committed without fixing the false positive case, why?
>>
>> Can you give some more code context, like perhaps a test case we can 
>> add to the suite? I believe the current behavior should be 
>> essentially false-positive-free because it only triggers the 
>> diagnostic when the alignments actually differ, so if there are other 
>> false-positives, I agree that we should determine a disposition for them.
>
> This is actually the same library the comment above is talking about:
> https://bugs.chromium.org/p/chromium/issues/detail?id=619640

The first example (gettimeofday()) looks to be a true-positive for sure. The 
remainder are tricky. They're not going to trigger UB in practice because the 
void * that memcpy() takes will get promptly typecast into a character pointer 
to avoid UB with the copy, and that's 1-byte aligned. There's a part of me that 
thinks it may be reasonable to assume that casting to void * always means 
"trust me, it'll be fine" because the pointer needs to be cast back out of a 
void
* to be useful, and so the diagnostic should be skipped in that case.
However, it's also just as likely to see code like (esp in C):

void func(void *ptr) {
  int *blah = ptr;
  *blah = 12;
}

void foo(void) {
  struct some_packed_struct s;
  func();
}

which has drifted into UB.

Roger, is there a way to whitelist memcpy (memmove, memset, et al) for this 
check?

>>> Could this warning be excluded from -Wall?
>>
>> Would you like the warning to be excluded from -Wall even if the 
>> false-positive is fixed?
>
> No, the warning seems useful if the false positive is fixed.

Ok, good to know.

~Aaron

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

On Mon, Jun 13, 2016 at 7:17 PM, Evgenii Stepanov  wrote:
> On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman  
> wrote:
>> On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov  wrote:
>>> eugenis added a subscriber: eugenis.
>>> eugenis added a comment.
>>>
>>> In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
>>>
 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.
>>>
>>>
>>> This patch was committed without fixing the false positive case, why?
>>
>> Can you give some more code context, like perhaps a test case we can
>> add to the suite? I believe the current behavior should be essentially
>> false-positive-free because it only triggers the diagnostic when the
>> alignments actually differ, so if there are other false-positives, I
>> agree that we should determine a disposition for them.
>
> This is actually the same library the comment above is talking about:
> https://bugs.chromium.org/p/chromium/issues/detail?id=619640

The first example (gettimeofday()) looks to be a true-positive for
sure. The remainder are tricky. They're not going to trigger UB in
practice because the void * that memcpy() takes will get promptly
typecast into a character pointer to avoid UB with the copy, and
that's 1-byte aligned. There's a part of me that thinks it may be
reasonable to assume that casting to void * always means "trust me,
it'll be fine" because the pointer needs to be cast back out of a void
* to be useful, and so the diagnostic should be skipped in that case.
However, it's also just as likely to see code like (esp in C):

void func(void *ptr) {
  int *blah = ptr;
  *blah = 12;
}

void foo(void) {
  struct some_packed_struct s;
  func();
}

which has drifted into UB.

Roger, is there a way to whitelist memcpy (memmove, memset, et al) for
this check?

>>> Could this warning be excluded from -Wall?
>>
>> Would you like the warning to be excluded from -Wall even if the
>> false-positive is fixed?
>
> No, the warning seems useful if the false positive is fixed.

Ok, good to know.

~Aaron
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgenii Stepanov via cfe-commits]

On Mon, Jun 13, 2016 at 4:12 PM, Aaron Ballman  wrote:
> On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov  wrote:
>> eugenis added a subscriber: eugenis.
>> eugenis added a comment.
>>
>> In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
>>
>>> 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.
>>
>>
>> This patch was committed without fixing the false positive case, why?
>
> Can you give some more code context, like perhaps a test case we can
> add to the suite? I believe the current behavior should be essentially
> false-positive-free because it only triggers the diagnostic when the
> alignments actually differ, so if there are other false-positives, I
> agree that we should determine a disposition for them.

This is actually the same library the comment above is talking about:
https://bugs.chromium.org/p/chromium/issues/detail?id=619640

>> Could this warning be excluded from -Wall?
>
> Would you like the warning to be excluded from -Wall even if the
> false-positive is fixed?

No, the warning seems useful if the false positive is fixed.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

On Mon, Jun 13, 2016 at 6:30 PM, Evgeniy Stepanov  wrote:
> eugenis added a subscriber: eugenis.
> eugenis added a comment.
>
> In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:
>
>> 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.
>
>
> This patch was committed without fixing the false positive case, why?

Can you give some more code context, like perhaps a test case we can
add to the suite? I believe the current behavior should be essentially
false-positive-free because it only triggers the diagnostic when the
alignments actually differ, so if there are other false-positives, I
agree that we should determine a disposition for them.

> Could this warning be excluded from -Wall?

Would you like the warning to be excluded from -Wall even if the
false-positive is fixed?

~Aaron
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Evgeniy Stepanov via cfe-commits]

eugenis added a subscriber: eugenis.
eugenis added a comment.

In http://reviews.llvm.org/D20561#446031, @aaron.ballman wrote:

> 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.


This patch was committed without fixing the false positive case, why?

Could this warning be excluded from -Wall?


Repository:
  rL LLVM

http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL272552: Warn when taking address of a packed member 
(authored by rogfer01).

Changed prior to commit:
  http://reviews.llvm.org/D20561?vs=59342=60537#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D20561

Files:
  cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
  cfe/trunk/lib/Sema/SemaExpr.cpp
  cfe/trunk/test/Sema/address-packed.c
  cfe/trunk/test/SemaCXX/address-packed.cpp

Index: cfe/trunk/test/SemaCXX/address-packed.cpp
===
--- cfe/trunk/test/SemaCXX/address-packed.cpp
+++ cfe/trunk/test/SemaCXX/address-packed.cpp
@@ -0,0 +1,100 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
+
+template 
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+return  // expected-warning {{packed member 'X' of class or structure 'S'}}
+   // expected-warning@-1 {{packed member 'X' of class or structure 'S'}}
+  }
+};
+
+template 
+void h(Ty *);
+
+void g1() {
+  S s1;
+  s1.get(); // expected-note {{in instantiation of member function 'S::get'}}
+
+  S s2;
+  s2.get();
+
+  S s3;
+  s3.get(); // expected-note {{in instantiation of member function 'S::get'}}
+}
Index: cfe/trunk/test/Sema/address-packed.c
===
--- cfe/trunk/test/Sema/address-packed.c
+++ cfe/trunk/test/Sema/address-packed.c
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+};
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void g0(void) {
+  {
+struct Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+struct Arguable arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(); // no-warning
+  }
+  {
+union UnionArguable arguable;
+f2(); // no-warning
+f1(); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
+  }
+  {
+ArguableT arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(); // no-warning
+  }
+  {
+struct Arguable *arguable = get_arguable();
+// These do not produce any warning because of the parentheses.
+f2(&(arguable->c0)); // no-warning
+f1(&(arguable->x));  // no-warning
+f2(&(arguable->c1)); // no-warning
+  }
+  {
+ArguableT *arguable = get_arguable();
+// These do not produce any warning because of the parentheses.
+f2(&(arguable->c0)); // no-warning
+f1(&(arguable->x));  // no-warning
+f2(&(arguable->c1)); // no-warning
+  }
+}
+
+struct S1 {
+  char c;
+  int i __attribute__((packed));
+};
+
+int *g1(struct S1 *s1) {
+  return >i; // expected-warning {{packed member 'i' of class or structure 'S1'}}
+}
+
+struct S2_i {
+  int i;
+};
+struct __attribute__((packed)) S2 {
+  char c;
+  struct S2_i inner;
+};
+
+int *g2(struct S2 *s2) {
+  return >inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2'}}
+}
+
+struct S2_a {
+  char c;
+  struct S2_i inner __attribute__((packed));
+};
+
+int *g2_a(struct S2_a 

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Thank you Aaron. I will commit ASAP.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

I think Richard has had enough time to comment. You can go ahead and commit, 
and if there are any issues, they can be handled post-commit. Thanks!


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Ping?

Thanks a lot.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked an inline comment as done.
rogfer01 added a comment.

http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

I think the patch LGTM (with a minor formatting nit). @rsmith, what are your 
thoughts?



Comment at: test/SemaCXX/address-packed.cpp:92
@@ +91,3 @@
+void g1()
+{
+S s1;

Formatting (may want to clang-format one last time before committing).


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 updated this revision to Diff 59342.
rogfer01 marked 3 inline comments as done.
rogfer01 added a comment.

Remove fix-it as it is not suitable for this diagnostic.


http://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/address-packed.c
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,101 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
+
+template 
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+return  // expected-warning {{packed member 'X' of class or structure 'S'}}
+   // expected-warning@-1 {{packed member 'X' of class or structure 'S'}}
+  }
+};
+
+template 
+void h(Ty*);
+
+void g1()
+{
+S s1;
+s1.get(); // expected-note {{in instantiation of member function 'S::get'}}
+
+S s2;
+s2.get();
+
+S s3;
+s3.get(); // expected-note {{in instantiation of member function 'S::get'}}
+}
Index: test/Sema/address-packed.c
===
--- /dev/null
+++ test/Sema/address-packed.c
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+};
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void g0(void) {
+  {
+struct Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+struct Arguable arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(); // no-warning
+  }
+  {
+union UnionArguable arguable;
+f2(); // no-warning
+f1(); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
+  }
+  {
+ArguableT arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(); // no-warning
+  }
+  {
+struct Arguable *arguable = get_arguable();
+// These do not produce any warning because of the parentheses.
+f2(&(arguable->c0)); // no-warning
+f1(&(arguable->x));  // no-warning
+f2(&(arguable->c1)); // no-warning
+  }
+  {
+ArguableT *arguable = get_arguable();
+// These do not produce any warning because of the parentheses.
+f2(&(arguable->c0)); // no-warning
+f1(&(arguable->x));  // no-warning
+f2(&(arguable->c1)); // no-warning
+  }
+}
+
+struct S1 {
+  char c;
+  int i __attribute__((packed));
+};
+
+int *g1(struct S1 *s1) {
+  return >i; // expected-warning {{packed member 'i' of class or structure 'S1'}}
+}
+
+struct S2_i {
+  int i;
+};
+struct __attribute__((packed)) S2 {
+  char c;
+  struct S2_i inner;
+};
+
+int *g2(struct S2 *s2) {
+  return >inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2'}}
+}
+
+struct S2_a {
+  char c;
+  struct S2_i inner __attribute__((packed));
+};
+
+int *g2_a(struct S2_a *s2_a) {
+  return _a->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2_a'}}
+}
+
+struct __attribute__((packed)) S3 {
+  char c;
+  struct {
+int i;
+  } inner;
+};
+
+int *g3(struct S3 *s3) {
+  

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked 3 inline comments as done.


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">;
 

aaron.ballman wrote:
> 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.
Ditto.


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;

aaron.ballman wrote:
> 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.
OK. I will remove it.


Comment at: test/SemaCXX/address-packed.cpp:92-93
@@ +91,4 @@
+   // expected-warning@-1 {{packed member 'X' of class or structure 
'S'}}
+   // expected-note@-2 {{place parentheses around the 'this->X'}}
+   // expected-note@-3 {{place parentheses around the 'this->X'}}
+  }

aaron.ballman wrote:
> Why `this->X` in the diagnostic when that's not what the user wrote?
Probably clang introduces it internally when parsing an id-expression that 
happens to be a nonstatic data member because the diagnostic does include it.

That said this is going away as I'm removing the fix-it.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

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@-1 {{packed member 'X' of class or structure 
'S'}}
+   // expected-note@-2 {{place parentheses around the 'this->X'}}
+   // expected-note@-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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 updated this revision to Diff 59241.
rogfer01 added a comment.

This change adds a fix-it suggesting parentheses to silence the warning.


http://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/address-packed.c
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,110 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+   // expected-note@-1 {{place parentheses around the 'arguable.x'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+   // expected-note@-1 {{place parentheses around the 'arguable.x'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+// expected-note@-1 {{place parentheses around the 'arguable->x'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+ // expected-note@-1 {{place parentheses around the 'this->x'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+   // expected-note@-1 {{place parentheses around the 'this->x'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+ // expected-note@-1 {{place parentheses around the 'this->x'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+   // expected-note@-1 {{place parentheses around the 'this->x'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
+
+template 
+class __attribute__((packed)) S {
+  Ty X;
+
+public:
+  const Ty *get() const {
+return  // expected-warning {{packed member 'X' of class or structure 'S'}}
+   // expected-warning@-1 {{packed member 'X' of class or structure 'S'}}
+   // expected-note@-2 {{place parentheses around the 'this->X'}}
+   // expected-note@-3 {{place parentheses around the 'this->X'}}
+  }
+};
+
+template 
+void h(Ty*);
+
+void g1()
+{
+S s1;
+s1.get(); // expected-note {{in instantiation of member function 'S::get'}}
+
+S s2;
+s2.get();
+
+S s3;
+s3.get(); // expected-note {{in instantiation of member function 'S::get'}}
+}
Index: test/Sema/address-packed.c
===
--- /dev/null
+++ test/Sema/address-packed.c
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+};
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void g0(void) {
+  {
+struct Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+struct Arguable arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+  // expected-note@-1 {{place parentheses around the 'arguable.x'}}
+f2(); // no-warning
+  }
+  {
+union UnionArguable arguable;
+f2(); // no-warning
+f1(); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
+ // expected-note@-1 {{place parentheses around the 'arguable.x'}}
+  }
+  {
+ArguableT arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+  // expected-note@-1 {{place parentheses around the 'arguable.x'}}
+f2(); // no-warning
+  }
+  {
+struct Arguable *arguable = get_arguable();
+// These do not produce any warning because of the parentheses.
+f2(&(arguable->c0)); // no-warning
+f1(&(arguable->x));  // no-warning
+f2(&(arguable->c1)); // no-warning
+  }
+  {
+

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked 5 inline comments as done.
rogfer01 added a comment.

http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

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.

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.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked 2 inline comments as done.
rogfer01 added a comment.

Well, the assignment-vs-comparison warning does emit a fix-it in 
`Sema::DiagnoseAssignmentAsCondition(Expr *E)`

  Diag(Loc, diag::note_condition_assign_silence)
<< FixItHint::CreateInsertion(Open, "(")
<< FixItHint::CreateInsertion(Close, ")");



  def note_condition_assign_silence : Note<
"place parentheses around the assignment to silence this warning">;

so I thought it could be appropiate.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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#445730, @rogfer01 wrote:

> It came to my mind that might be good idea adding one of those "fix-it" 
> suggestions so the user knows it can silence the warning by using 
> parentheses. What do you think?


I don't think that would be appropriate here. We usually only use fix-it hints 
when we know the code is incorrect and the hint is the proper way to fix the 
problem. In this case, when we trigger the warning, we want users to fix the 
underlying problem of taking the address of something that will result in an 
unaligned pointer, but the fix-it you're thinking of will merely silence the 
warning without fixing the underlying problem.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

It came to my mind that might be good idea adding one of those "fix-it" 
suggestions so the user knows it can silence the warning by using parentheses. 
What do you think?


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

This is getting really close, I mostly only have nits left.



Comment at: lib/Sema/SemaExpr.cpp:10529
@@ +10528,3 @@
+bool ByteAligned = Context.getTypeAlignInChars(MD->getType()).isOne();
+if (ByteAligned) // packed has had not any effect on it
+  break;

Comments should be complete sentences including capitalization and punctuation, 
so perhaps instead: `// Packed does not have any effect.`


Comment at: test/Sema/address-packed.c:21
@@ +20,3 @@
+
+extern void f3(void());
+

This is unused and can be removed.


Comment at: test/Sema/address-packed.c:27
@@ +26,3 @@
+
+void g0() {
+  {

Function should take `(void)` since this is C.


Comment at: test/Sema/address-packed.c:47
@@ +46,3 @@
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or 
structure }}
+f2(); // no-warning

Extra space before `}}`.


Comment at: test/Sema/address-packed.c:51
@@ +50,3 @@
+  {
+struct Arguable *arguable = get_arguable();
+f2(&(arguable->c0)); // no-warning

Should put in a comment explaining that this produces no diagnostic because of 
the parens.


Comment at: test/SemaCXX/address-packed.cpp:13
@@ +12,3 @@
+
+typedef struct Arguable ArguableT;
+

This is unused and can be removed.


Comment at: test/SemaCXX/address-packed.cpp:77
@@ +76,2 @@
+  }
+};

Can we get one further test?
```
template 
class __attribute__((packed)) S {
  Ty X;
public:
  Ty *get() const {
return  // no warning?
  }
};
```
I am fine if this does not warn. I am also fine if it only warns when there are 
instantiations of S for which Ty should warn (e.g., do an explicit 
instantiation with `char` that does not warn and another one with `int` that 
does).


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked an inline comment as done.
rogfer01 added a comment.

http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 updated this revision to Diff 59178.
rogfer01 marked an inline comment as done.
rogfer01 added a comment.

Do not warn if the alignment of the type of the field is already 1 as packed 
does not have any effect on those fields.


http://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/address-packed.c
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,77 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct __attribute__((packed)) Arguable {
+  int x;
+  char c;
+  static void foo();
+};
+
+extern void f3(void());
+
+typedef struct Arguable ArguableT;
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable *get_arguable();
+
+void g0() {
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // no-warning
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // no-warning
+f3(>foo); // no-warning
+  }
+}
+
+struct __attribute__((packed)) A {
+  int x;
+  char c;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h0() {
+return  // no-warning
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  char *h1() {
+return  // no-warning
+  }
+};
Index: test/Sema/address-packed.c
===
--- /dev/null
+++ test/Sema/address-packed.c
@@ -0,0 +1,126 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c0;
+  int x;
+  char c1;
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+};
+
+extern void f3(void());
+
+typedef struct Arguable ArguableT;
+
+struct Arguable *get_arguable();
+
+void g0() {
+  {
+struct Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+struct Arguable arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(); // no-warning
+  }
+  {
+union UnionArguable arguable;
+f2(); // no-warning
+f1(); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
+  }
+  {
+ArguableT arguable;
+f2(); // no-warning
+f1();  // expected-warning {{packed member 'x' of class or structure }}
+f2(); // no-warning
+  }
+  {
+struct Arguable *arguable = get_arguable();
+f2(&(arguable->c0)); // no-warning
+f1(&(arguable->x));  // no-warning
+f2(&(arguable->c1)); // no-warning
+  }
+  {
+ArguableT *arguable = get_arguable();
+f2(&(arguable->c0)); // no-warning
+f1(&(arguable->x));  // no-warning
+f2(&(arguable->c1)); // no-warning
+  }
+}
+
+struct S1 {
+  char c;
+  int i __attribute__((packed));
+};
+
+int *g1(struct S1 *s1) {
+  return >i; // expected-warning {{packed member 'i' of class or structure 'S1'}}
+}
+
+struct S2_i {
+  int i;
+};
+struct __attribute__((packed)) S2 {
+  char c;
+  struct S2_i inner;
+};
+
+int *g2(struct S2 *s2) {
+  return >inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2'}}
+}
+
+struct S2_a {
+  char c;
+  struct S2_i inner __attribute__((packed));
+};
+
+int *g2_a(struct S2_a *s2_a) {
+  return _a->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2_a'}}
+}
+
+struct __attribute__((packed)) S3 {
+  char c;
+  struct {
+int i;
+  } inner;
+};
+
+int *g3(struct S3 *s3) {
+  return >inner.i; // expected-warning {{packed member 'inner' of class or structure 'S3'}}
+}
+
+struct S4 {
+  char c;
+  struct __attribute__((packed)) {
+int i;
+  } inner;
+};
+
+int *g4(struct S4 *s4) {
+  return >inner.i; // expected-warning {{packed member 'i' of class or structure 'S4::(anonymous)'}}
+}
+
+struct S5 {
+  char c;
+  struct {
+char c1;
+int i __attribute__((packed));
+  } inner;
+};
+
+int *g5(struct S5 *s5) {
+  return >inner.i; // expected-warning {{packed member 'i' of class or structure 'S5::(anonymous)'}}

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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.


Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+if (RD->hasAttr() ||
+ME->getMemberDecl()->hasAttr()) {
+  Diag(OpLoc, diag::warn_taking_address_of_packed_member)

rogfer01 wrote:
> aaron.ballman wrote:
> > 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 = 
> >   char *cp2 = 
> > }
> > ```
> > 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?
> Yes. I like the idea since the packed attribute has no effect on already 
> 1-aligned data (like char). But note that C++11 does not allow reducing the 
> alignment through `alignas`.
Hmm, okay, I was not remembering properly; I thought one of `alignas`, 
`__declspec(align)`, or `__attribute__((aligned))` allowed for decreasing the 
alignment, but documentation implies otherwise.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked 3 inline comments as done.


Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+if (RD->hasAttr() ||
+ME->getMemberDecl()->hasAttr()) {
+  Diag(OpLoc, diag::warn_taking_address_of_packed_member)

aaron.ballman wrote:
> 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 = 
>   char *cp2 = 
> }
> ```
> 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?
Yes. I like the idea since the packed attribute has no effect on already 
1-aligned data (like char). But note that C++11 does not allow reducing the 
alignment through `alignas`.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Ping?

Thank you very much


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 updated this revision to Diff 58760.
rogfer01 added a comment.

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


http://reviews.llvm.org/D20561

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaExpr.cpp
  test/Sema/address-packed.c
  test/SemaCXX/address-packed.cpp

Index: test/SemaCXX/address-packed.cpp
===
--- /dev/null
+++ test/SemaCXX/address-packed.cpp
@@ -0,0 +1,186 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+extern void f1(int *);
+extern void f2(char *);
+
+struct Ok {
+  char c;
+  int x;
+};
+
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+
+union __attribute__((packed)) UnionArguable {
+  char c;
+  int x;
+  static void foo();
+};
+
+extern void f3(void());
+
+typedef struct Arguable ArguableT;
+
+namespace Foo {
+struct __attribute__((packed)) Arguable {
+  char c;
+  int x;
+  static void foo();
+};
+}
+
+struct Arguable* get_arguable();
+
+void g0() {
+  {
+struct Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+struct Arguable arguable;
+f1(); // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(); // expected-warning {{packed member 'c' of class or structure 'Arguable'}}
+  }
+  {
+union UnionArguable arguable;
+f1(); // expected-warning {{packed member 'x' of class or structure 'UnionArguable'}}
+f2(); // expected-warning {{packed member 'c' of class or structure 'UnionArguable'}}
+  }
+  {
+ArguableT arguable;
+f1(); // expected-warning {{packed member 'x' of class or structure }}
+f2(); // expected-warning {{packed member 'c' of class or structure }}
+  }
+  {
+struct Arguable *arguable = get_arguable();
+f1(&(arguable->x)); // no-warning
+f2(&(arguable->c)); // no-warning
+  }
+  {
+ArguableT *arguable = get_arguable();
+f1(&(arguable->x)); // no-warning
+f2(&(arguable->c)); // no-warning
+  }
+
+  {
+Ok ok;
+f1(); // no-warning
+f2(); // no-warning
+  }
+  {
+Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // expected-warning {{packed member 'c' of class or structure 'Arguable'}}
+f3(); // no-warning
+  }
+  {
+Foo::Arguable arguable;
+f1();   // expected-warning {{packed member 'x' of class or structure 'Foo::Arguable'}}
+f2();   // expected-warning {{packed member 'c' of class or structure 'Foo::Arguable'}}
+f3(); // no-warning
+  }
+  {
+Arguable arguable1;
+Arguable (arguable1);
+f1();   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2();   // expected-warning {{packed member 'c' of class or structure 'Arguable'}}
+f3(); // no-warning
+  }
+  {
+Arguable *arguable1;
+Arguable *(arguable1);
+f1(>x);   // expected-warning {{packed member 'x' of class or structure 'Arguable'}}
+f2(>c);   // expected-warning {{packed member 'c' of class or structure 'Arguable'}}
+f3(>foo); // no-warning
+  }
+}
+
+struct S1 {
+  char c;
+  int i __attribute__((packed));
+};
+
+int *g1(struct S1 *s1) {
+  return >i; // expected-warning {{packed member 'i' of class or structure 'S1'}}
+}
+
+struct S2_i {
+  int i;
+};
+struct __attribute__((packed)) S2 {
+  char c;
+  struct S2_i inner;
+};
+
+int *g2(struct S2 *s2) {
+  return >inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2'}}
+}
+
+struct S2_a {
+  char c;
+  struct S2_i inner __attribute__((packed));
+};
+
+int *g2_a(struct S2_a *s2_a) {
+  return _a->inner.i; // expected-warning {{packed member 'inner' of class or structure 'S2_a'}}
+}
+
+struct __attribute__((packed)) S3 {
+  char c;
+  struct {
+int i;
+  } inner;
+};
+
+int *g3(struct S3 *s3) {
+  return >inner.i; // expected-warning {{packed member 'inner' of class or structure 'S3'}}
+}
+
+struct S4 {
+  char c;
+  struct __attribute__((packed)) {
+int i;
+  } inner;
+};
+
+int *g4(struct S4 *s4) {
+  return >inner.i; // expected-warning {{packed member 'i' of class or structure 'S4::(anonymous)'}}
+}
+
+struct S5 {
+  char c;
+  struct {
+char c1;
+int i __attribute__((packed));
+  } inner;
+};
+
+int *g5(struct S5 *s5) {
+  return >inner.i; // expected-warning {{packed member 'i' of class or structure 'S5::(anonymous)'}}
+}
+
+struct __attribute__((packed)) A {
+  char c;
+  int x;
+
+  int *f0() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g0() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+};
+
+struct B : A {
+  int *f1() {
+return >x; // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+
+  int *g1() {
+return  // expected-warning {{packed member 'x' of class or structure 'A'}}
+  }
+};
Index: test/Sema/address-packed.c

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Forget my wrong comment about packed fields that might actually be aligned. It 
does not make sense.

Regarding the way to selectively disable this, there is little syntax available 
at this point that we can leverage, so I propose to use parentheses as a way to 
disable the warning, e.g. `` may warn but `&(a.b)` will never warn. This 
may not be ideal but is in practice not that far from the classical `( (x = b) 
)` idiom used to silence the assignment-vs-equality warning.

Closer examination of Firefox code shows that `memcpy`/`memcmp` are used with 
packed members in the networking code. Being able to disable those safe uses by 
means of parentheses seems sensible to me.

I will upload a new patch with this change.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 added a comment.

Firefox build has ended and exposes 62 diagnostics, 44 unique ocurrences and 10 
different diagnostics (shown below) in networking code.

  taking address of packed member 'address' of class or structure 
'sctp_state_cookie' may result in an unaligned pointer value
  taking address of packed member 'addr' of class or structure 
'sctp_ipv4addr_param' may result in an unaligned pointer value
  taking address of packed member 'addr' of class or structure 
'sctp_ipv6addr_param' may result in an unaligned pointer value
  taking address of packed member 'aph' of class or structure 
'sctp_asconf_addr_param' may result in an unaligned pointer value
  taking address of packed member 'cookie' of class or structure 
'sctp_cookie_echo_chunk' may result in an unaligned pointer value
  taking address of packed member 'init' of class or structure 
'sctp_init_chunk' may result in an unaligned pointer value
  taking address of packed member 'laddress' of class or structure 
'sctp_state_cookie' may result in an unaligned pointer value
  taking address of packed member 'ph' of class or structure 
'sctp_ipv4addr_param' may result in an unaligned pointer value
  taking address of packed member 'ph' of class or structure 
'sctp_ipv6addr_param' may result in an unaligned pointer value
  taking address of packed member 'time_entered' of class or structure 
'sctp_state_cookie' may result in an unaligned pointer value

which leads me to think that there might be cases where the field of a packed 
struct happens to be aligned for the type of the field itself. I'll add a check 
for this. I'll give it a spin.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roger Ferrer Ibanez via cfe-commits]

rogfer01 marked 4 inline comments as done.


Comment at: lib/Sema/SemaExpr.cpp:10527
@@ +10526,3 @@
+ME->getMemberDecl()->hasAttr()) {
+  Diag(OpLoc, diag::warn_taking_address_of_packed_member)
+  << ME->getMemberDecl() << RD;

aaron.ballman wrote:
> 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 =  // 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)?
I think it should warn because in this case  ==  and the alignment of the 
whole struct is 1 (due to the packed attribute) so s.i is also aligned to 1 
which would not be the suitable alignment for an int.

I'm currently building Firefox, I'll add a comment once it ends.

Regarding silencing the diagnostic: -Wno-address-of-packed-member should do but 
I understand it might be too coarse in some cases.


http://reviews.llvm.org/D20561



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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(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(op);
+  while (ME && isa(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()) {
+  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 =  // 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits