Re: r275417 - Diagnose taking address and reference binding of packed members

[Roger Ferrer Ibanez via cfe-commits]

Hi Richard,


may I suggest to handle the reference binding to unaligned fields in another 
change rather than trying to get it in the current change (which was originally 
intended for address taking of such fields)? After your message, it looks to me 
that references will require a different strategy.


Kind regards,

Roger


From: meta...@gmail.com <meta...@gmail.com> on behalf of Richard Smith 
<rich...@metafoo.co.uk>
Sent: 15 July 2016 01:28:57
To: Reid Kleckner
Cc: Roger Ferrer Ibanez; cfe-commits
Subject: Re: r275417 - Diagnose taking address and reference binding of packed 
members

OK, actually, GCC is doing something a lot more clever here.

Here's what seems to really be going on:

Packed members are modeled somewhat like bitfield members: a packed member 
lvalue is a different kind of lvalue to which a reference cannot be bound (like 
a bitfield). An attempt to bind a const / rvalue reference to such a member 
will create a temporary, copy the packed / bitfield member to the temporary, 
and then bind the reference to the temporary.

If we want to follow GCC here (and I think we do -- this seems like a very 
sensible model), we should handle this case as an ObjectKind and generally 
apply the same semantic rules that we use for bitfields.

On Thu, Jul 14, 2016 at 5:18 PM, Richard Smith 
<rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> wrote:
It appears that GCC accepts that case. More generally, it looks like GCC 
suppresses the warning when the reference is a function parameter (any 
function, not just a copy constructor or similar). I'm not sure if that's an 
intentional feature or a bug, but it should be pretty easy for us to be 
compatible with, at least...

On Thu, Jul 14, 2016 at 5:03 PM, Reid Kleckner 
<r...@google.com<mailto:r...@google.com>> wrote:
I wonder if GCC accepts this:

In file included from ../../net/tools/quic/quic_epoll_clock_test.cc:7:
In file included from ../../net/tools/quic/test_tools/mock_epoll_server.h:16:
In file included from ../../net/tools/epoll_server/epoll_server.h:41:
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/x86_64-linux-gnu/sys/epoll.h:89:8:
 error: binding reference to packed member 'data' of class or structure 
'epoll_event'
struct epoll_event
   ^~~
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_pair.h:267:14:
 note: in instantiation of function template specialization 'std::pair<long, 
epoll_event>::pair<long, epoll_event>' requested here
  return __pair_type(std::forward<_T1>(__x), std::forward<_T2>(__y));
 ^
../../net/tools/quic/test_tools/mock_epoll_server.h:69:30: note: in 
instantiation of function template specialization 'std::make_pair' requested here
event_queue_.insert(std::make_pair(time_in_usec, ee));

On Thu, Jul 14, 2016 at 4:54 PM, Richard Smith 
<rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> wrote:
On Thu, Jul 14, 2016 at 3:52 PM, Reid Kleckner via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Why did we upgrade the unaligned reference binding from a warning to an error? 
That will make it hard to roll this change out across many codebases.

GCC has given an error on this since version 4.7. If there are cases that GCC 
accepts and we reject, that sounds like a bug.

On Thu, Jul 14, 2016 at 7:10 AM, Roger Ferrer Ibanez via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Author: rogfer01
Date: Thu Jul 14 09:10:43 2016
New Revision: 275417

URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
Log:
Diagnose taking address and reference binding of packed members

This patch implements PR#22821.

Taking the address of a packed member is dangerous since the reduced
alignment of the pointee is lost. This can lead to memory alignment
faults in some architectures if the pointer value is dereferenced.

This change adds a new warning to clang emitted when taking the address
of a packed member. A packed member is either a field/data member
declared as attribute((packed)) or belonging to a struct/class
declared as such. The associated flag is -Waddress-of-packed-member.
Conversions (either implicit or via a valid casting) to pointer types
with lower or equal alignment requirements (e.g. void* or char*)
silence the warning.

This change also adds a new error diagnostic when the user attempts to
bind a reference to a packed member, regardless of the alignment.

Differential Revision: https://reviews.llvm.org/D20561



Added:
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
Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/include/clang/Sema/Sema

Re: r275417 - Diagnose taking address and reference binding of packed members

[Richard Smith via cfe-commits]

OK, actually, GCC is doing something a lot more clever here.

Here's what seems to really be going on:

Packed members are modeled somewhat like bitfield members: a packed member
lvalue is a different kind of lvalue to which a reference cannot be bound
(like a bitfield). An attempt to bind a const / rvalue reference to such a
member will create a temporary, copy the packed / bitfield member to the
temporary, and then bind the reference to the temporary.

If we want to follow GCC here (and I think we do -- this seems like a very
sensible model), we should handle this case as an ObjectKind and generally
apply the same semantic rules that we use for bitfields.

On Thu, Jul 14, 2016 at 5:18 PM, Richard Smith 
wrote:

> It appears that GCC accepts that case. More generally, it looks like GCC
> suppresses the warning when the reference is a function parameter (any
> function, not just a copy constructor or similar). I'm not sure if that's
> an intentional feature or a bug, but it should be pretty easy for us to be
> compatible with, at least...
>
> On Thu, Jul 14, 2016 at 5:03 PM, Reid Kleckner  wrote:
>
>> I wonder if GCC accepts this:
>>
>> In file included from ../../net/tools/quic/quic_epoll_clock_test.cc:7:
>> In file included from
>> ../../net/tools/quic/test_tools/mock_epoll_server.h:16:
>> In file included from ../../net/tools/epoll_server/epoll_server.h:41:
>> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/x86_64-linux-gnu/sys/epoll.h:89:8:
>> error: binding reference to packed member 'data' of class or structure
>> 'epoll_event'
>> struct epoll_event
>>^~~
>> ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_pair.h:267:14:
>> note: in instantiation of function template specialization 'std::pair> epoll_event>::pair' requested here
>>   return __pair_type(std::forward<_T1>(__x), std::forward<_T2>(__y));
>>  ^
>> ../../net/tools/quic/test_tools/mock_epoll_server.h:69:30: note: in
>> instantiation of function template specialization 'std::make_pair> const epoll_event &>' requested here
>> event_queue_.insert(std::make_pair(time_in_usec, ee));
>>
>> On Thu, Jul 14, 2016 at 4:54 PM, Richard Smith 
>> wrote:
>>
>>> On Thu, Jul 14, 2016 at 3:52 PM, Reid Kleckner via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Why did we upgrade the unaligned reference binding from a warning to an
 error? That will make it hard to roll this change out across many 
 codebases.

>>>
>>> GCC has given an error on this since version 4.7. If there are cases
>>> that GCC accepts and we reject, that sounds like a bug.
>>>
>>>
 On Thu, Jul 14, 2016 at 7:10 AM, Roger Ferrer Ibanez via cfe-commits <
 cfe-commits@lists.llvm.org> wrote:

> Author: rogfer01
> Date: Thu Jul 14 09:10:43 2016
> New Revision: 275417
>
> URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
> Log:
> Diagnose taking address and reference binding of packed members
>
> This patch implements PR#22821.
>
> Taking the address of a packed member is dangerous since the reduced
> alignment of the pointee is lost. This can lead to memory alignment
> faults in some architectures if the pointer value is dereferenced.
>
> This change adds a new warning to clang emitted when taking the address
> of a packed member. A packed member is either a field/data member
> declared as attribute((packed)) or belonging to a struct/class
> declared as such. The associated flag is -Waddress-of-packed-member.
> Conversions (either implicit or via a valid casting) to pointer types
> with lower or equal alignment requirements (e.g. void* or char*)
> silence the warning.
>
> This change also adds a new error diagnostic when the user attempts to
> bind a reference to a packed member, regardless of the alignment.
>
> Differential Revision: https://reviews.llvm.org/D20561
>
>
>
> Added:
> 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
> Modified:
> 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
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
>
> ==
> --- 

Re: r275417 - Diagnose taking address and reference binding of packed members

[Richard Smith via cfe-commits]

It appears that GCC accepts that case. More generally, it looks like GCC
suppresses the warning when the reference is a function parameter (any
function, not just a copy constructor or similar). I'm not sure if that's
an intentional feature or a bug, but it should be pretty easy for us to be
compatible with, at least...

On Thu, Jul 14, 2016 at 5:03 PM, Reid Kleckner  wrote:

> I wonder if GCC accepts this:
>
> In file included from ../../net/tools/quic/quic_epoll_clock_test.cc:7:
> In file included from
> ../../net/tools/quic/test_tools/mock_epoll_server.h:16:
> In file included from ../../net/tools/epoll_server/epoll_server.h:41:
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/include/x86_64-linux-gnu/sys/epoll.h:89:8:
> error: binding reference to packed member 'data' of class or structure
> 'epoll_event'
> struct epoll_event
>^~~
> ../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_pair.h:267:14:
> note: in instantiation of function template specialization 'std::pair epoll_event>::pair' requested here
>   return __pair_type(std::forward<_T1>(__x), std::forward<_T2>(__y));
>  ^
> ../../net/tools/quic/test_tools/mock_epoll_server.h:69:30: note: in
> instantiation of function template specialization 'std::make_pair const epoll_event &>' requested here
> event_queue_.insert(std::make_pair(time_in_usec, ee));
>
> On Thu, Jul 14, 2016 at 4:54 PM, Richard Smith 
> wrote:
>
>> On Thu, Jul 14, 2016 at 3:52 PM, Reid Kleckner via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Why did we upgrade the unaligned reference binding from a warning to an
>>> error? That will make it hard to roll this change out across many codebases.
>>>
>>
>> GCC has given an error on this since version 4.7. If there are cases that
>> GCC accepts and we reject, that sounds like a bug.
>>
>>
>>> On Thu, Jul 14, 2016 at 7:10 AM, Roger Ferrer Ibanez via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
 Author: rogfer01
 Date: Thu Jul 14 09:10:43 2016
 New Revision: 275417

 URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
 Log:
 Diagnose taking address and reference binding of packed members

 This patch implements PR#22821.

 Taking the address of a packed member is dangerous since the reduced
 alignment of the pointee is lost. This can lead to memory alignment
 faults in some architectures if the pointer value is dereferenced.

 This change adds a new warning to clang emitted when taking the address
 of a packed member. A packed member is either a field/data member
 declared as attribute((packed)) or belonging to a struct/class
 declared as such. The associated flag is -Waddress-of-packed-member.
 Conversions (either implicit or via a valid casting) to pointer types
 with lower or equal alignment requirements (e.g. void* or char*)
 silence the warning.

 This change also adds a new error diagnostic when the user attempts to
 bind a reference to a packed member, regardless of the alignment.

 Differential Revision: https://reviews.llvm.org/D20561



 Added:
 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
 Modified:
 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

 Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
 URL:
 http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff

 ==
 --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
 +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 14
 09:10:43 2016
 @@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incomp
"dereference of type %1 that was reinterpret_cast from type %0 has
 undefined "
"behavior">,
InGroup, DefaultIgnore;
 +def warn_taking_address_of_packed_member : Warning<
 +  "taking address of packed member %0 of class or structure %q1 may
 result in an unaligned pointer value">,
 +  InGroup>;
 +def err_binding_reference_to_packed_member : Error<
 +  "binding reference to packed member %0 of class or structure %q1">;

  def err_objc_object_assignment : Error<
"cannot assign to class object (%0 invalid)">;

 Modified: 

Re: r275417 - Diagnose taking address and reference binding of packed members

[Reid Kleckner via cfe-commits]

I wonder if GCC accepts this:

In file included from ../../net/tools/quic/quic_epoll_clock_test.cc:7:
In file included from
../../net/tools/quic/test_tools/mock_epoll_server.h:16:
In file included from ../../net/tools/epoll_server/epoll_server.h:41:
../../build/linux/debian_wheezy_amd64-sysroot/usr/include/x86_64-linux-gnu/sys/epoll.h:89:8:
error: binding reference to packed member 'data' of class or structure
'epoll_event'
struct epoll_event
   ^~~
../../build/linux/debian_wheezy_amd64-sysroot/usr/lib/gcc/x86_64-linux-gnu/4.6/../../../../include/c++/4.6/bits/stl_pair.h:267:14:
note: in instantiation of function template specialization 'std::pair::pair' requested here
  return __pair_type(std::forward<_T1>(__x), std::forward<_T2>(__y));
 ^
../../net/tools/quic/test_tools/mock_epoll_server.h:69:30: note: in
instantiation of function template specialization 'std::make_pair' requested here
event_queue_.insert(std::make_pair(time_in_usec, ee));

On Thu, Jul 14, 2016 at 4:54 PM, Richard Smith 
wrote:

> On Thu, Jul 14, 2016 at 3:52 PM, Reid Kleckner via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Why did we upgrade the unaligned reference binding from a warning to an
>> error? That will make it hard to roll this change out across many codebases.
>>
>
> GCC has given an error on this since version 4.7. If there are cases that
> GCC accepts and we reject, that sounds like a bug.
>
>
>> On Thu, Jul 14, 2016 at 7:10 AM, Roger Ferrer Ibanez via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Author: rogfer01
>>> Date: Thu Jul 14 09:10:43 2016
>>> New Revision: 275417
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
>>> Log:
>>> Diagnose taking address and reference binding of packed members
>>>
>>> This patch implements PR#22821.
>>>
>>> Taking the address of a packed member is dangerous since the reduced
>>> alignment of the pointee is lost. This can lead to memory alignment
>>> faults in some architectures if the pointer value is dereferenced.
>>>
>>> This change adds a new warning to clang emitted when taking the address
>>> of a packed member. A packed member is either a field/data member
>>> declared as attribute((packed)) or belonging to a struct/class
>>> declared as such. The associated flag is -Waddress-of-packed-member.
>>> Conversions (either implicit or via a valid casting) to pointer types
>>> with lower or equal alignment requirements (e.g. void* or char*)
>>> silence the warning.
>>>
>>> This change also adds a new error diagnostic when the user attempts to
>>> bind a reference to a packed member, regardless of the alignment.
>>>
>>> Differential Revision: https://reviews.llvm.org/D20561
>>>
>>>
>>>
>>> Added:
>>> 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
>>> Modified:
>>> 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
>>>
>>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
>>>
>>> ==
>>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 14
>>> 09:10:43 2016
>>> @@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incomp
>>>"dereference of type %1 that was reinterpret_cast from type %0 has
>>> undefined "
>>>"behavior">,
>>>InGroup, DefaultIgnore;
>>> +def warn_taking_address_of_packed_member : Warning<
>>> +  "taking address of packed member %0 of class or structure %q1 may
>>> result in an unaligned pointer value">,
>>> +  InGroup>;
>>> +def err_binding_reference_to_packed_member : Error<
>>> +  "binding reference to packed member %0 of class or structure %q1">;
>>>
>>>  def err_objc_object_assignment : Error<
>>>"cannot assign to class object (%0 invalid)">;
>>>
>>> Modified: cfe/trunk/include/clang/Sema/Sema.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=275417=275416=275417=diff
>>>
>>> ==
>>> --- cfe/trunk/include/clang/Sema/Sema.h (original)
>>> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Jul 14 09:10:43 2016
>>> @@ -9518,6 +9518,10 @@ private:
>>>void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
>>>  const Expr * const *ExprArgs);
>>>
>>> +  /// 

Re: r275417 - Diagnose taking address and reference binding of packed members

[Richard Smith via cfe-commits]

On Thu, Jul 14, 2016 at 3:52 PM, Reid Kleckner via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Why did we upgrade the unaligned reference binding from a warning to an
> error? That will make it hard to roll this change out across many codebases.
>

GCC has given an error on this since version 4.7. If there are cases that
GCC accepts and we reject, that sounds like a bug.


> On Thu, Jul 14, 2016 at 7:10 AM, Roger Ferrer Ibanez via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: rogfer01
>> Date: Thu Jul 14 09:10:43 2016
>> New Revision: 275417
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
>> Log:
>> Diagnose taking address and reference binding of packed members
>>
>> This patch implements PR#22821.
>>
>> Taking the address of a packed member is dangerous since the reduced
>> alignment of the pointee is lost. This can lead to memory alignment
>> faults in some architectures if the pointer value is dereferenced.
>>
>> This change adds a new warning to clang emitted when taking the address
>> of a packed member. A packed member is either a field/data member
>> declared as attribute((packed)) or belonging to a struct/class
>> declared as such. The associated flag is -Waddress-of-packed-member.
>> Conversions (either implicit or via a valid casting) to pointer types
>> with lower or equal alignment requirements (e.g. void* or char*)
>> silence the warning.
>>
>> This change also adds a new error diagnostic when the user attempts to
>> bind a reference to a packed member, regardless of the alignment.
>>
>> Differential Revision: https://reviews.llvm.org/D20561
>>
>>
>>
>> Added:
>> 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
>> Modified:
>> 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
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 14
>> 09:10:43 2016
>> @@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incomp
>>"dereference of type %1 that was reinterpret_cast from type %0 has
>> undefined "
>>"behavior">,
>>InGroup, DefaultIgnore;
>> +def warn_taking_address_of_packed_member : Warning<
>> +  "taking address of packed member %0 of class or structure %q1 may
>> result in an unaligned pointer value">,
>> +  InGroup>;
>> +def err_binding_reference_to_packed_member : Error<
>> +  "binding reference to packed member %0 of class or structure %q1">;
>>
>>  def err_objc_object_assignment : Error<
>>"cannot assign to class object (%0 invalid)">;
>>
>> Modified: cfe/trunk/include/clang/Sema/Sema.h
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=275417=275416=275417=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Sema/Sema.h (original)
>> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Jul 14 09:10:43 2016
>> @@ -9518,6 +9518,10 @@ private:
>>void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
>>  const Expr * const *ExprArgs);
>>
>> +  /// \brief Check if we are taking the address of a packed field
>> +  /// as this may be a problem if the pointer value is dereferenced.
>> +  void CheckAddressOfPackedMember(Expr *rhs);
>> +
>>/// \brief The parser's current scope.
>>///
>>/// The parser maintains this state here.
>> @@ -9596,6 +9600,51 @@ public:
>>// Emitting members of dllexported classes is delayed until the class
>>// (including field initializers) is fully parsed.
>>SmallVector DelayedDllExportClasses;
>> +
>> +private:
>> +  /// \brief Helper class that collects misaligned member designations
>> and
>> +  /// their location info for delayed diagnostics.
>> +  struct MisalignedMember {
>> +Expr *E;
>> +RecordDecl *RD;
>> +ValueDecl *MD;
>> +CharUnits Alignment;
>> +
>> +MisalignedMember() : E(), RD(), MD(), Alignment() {}
>> +MisalignedMember(Expr *E, RecordDecl *RD, ValueDecl *MD,
>> + CharUnits Alignment)
>> +: E(E), RD(RD), MD(MD), Alignment(Alignment) {}
>> +explicit MisalignedMember(Expr *E)
>> +: MisalignedMember(E, nullptr, nullptr, CharUnits()) {}
>> +
>> +bool operator==(const MisalignedMember ) { 

Re: r275417 - Diagnose taking address and reference binding of packed members

[Reid Kleckner via cfe-commits]

Why did we upgrade the unaligned reference binding from a warning to an
error? That will make it hard to roll this change out across many codebases.

On Thu, Jul 14, 2016 at 7:10 AM, Roger Ferrer Ibanez via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rogfer01
> Date: Thu Jul 14 09:10:43 2016
> New Revision: 275417
>
> URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
> Log:
> Diagnose taking address and reference binding of packed members
>
> This patch implements PR#22821.
>
> Taking the address of a packed member is dangerous since the reduced
> alignment of the pointee is lost. This can lead to memory alignment
> faults in some architectures if the pointer value is dereferenced.
>
> This change adds a new warning to clang emitted when taking the address
> of a packed member. A packed member is either a field/data member
> declared as attribute((packed)) or belonging to a struct/class
> declared as such. The associated flag is -Waddress-of-packed-member.
> Conversions (either implicit or via a valid casting) to pointer types
> with lower or equal alignment requirements (e.g. void* or char*)
> silence the warning.
>
> This change also adds a new error diagnostic when the user attempts to
> bind a reference to a packed member, regardless of the alignment.
>
> Differential Revision: https://reviews.llvm.org/D20561
>
>
>
> Added:
> 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
> Modified:
> 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
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 14
> 09:10:43 2016
> @@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incomp
>"dereference of type %1 that was reinterpret_cast from type %0 has
> undefined "
>"behavior">,
>InGroup, DefaultIgnore;
> +def warn_taking_address_of_packed_member : Warning<
> +  "taking address of packed member %0 of class or structure %q1 may
> result in an unaligned pointer value">,
> +  InGroup>;
> +def err_binding_reference_to_packed_member : Error<
> +  "binding reference to packed member %0 of class or structure %q1">;
>
>  def err_objc_object_assignment : Error<
>"cannot assign to class object (%0 invalid)">;
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=275417=275416=275417=diff
>
> ==
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Jul 14 09:10:43 2016
> @@ -9518,6 +9518,10 @@ private:
>void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
>  const Expr * const *ExprArgs);
>
> +  /// \brief Check if we are taking the address of a packed field
> +  /// as this may be a problem if the pointer value is dereferenced.
> +  void CheckAddressOfPackedMember(Expr *rhs);
> +
>/// \brief The parser's current scope.
>///
>/// The parser maintains this state here.
> @@ -9596,6 +9600,51 @@ public:
>// Emitting members of dllexported classes is delayed until the class
>// (including field initializers) is fully parsed.
>SmallVector DelayedDllExportClasses;
> +
> +private:
> +  /// \brief Helper class that collects misaligned member designations and
> +  /// their location info for delayed diagnostics.
> +  struct MisalignedMember {
> +Expr *E;
> +RecordDecl *RD;
> +ValueDecl *MD;
> +CharUnits Alignment;
> +
> +MisalignedMember() : E(), RD(), MD(), Alignment() {}
> +MisalignedMember(Expr *E, RecordDecl *RD, ValueDecl *MD,
> + CharUnits Alignment)
> +: E(E), RD(RD), MD(MD), Alignment(Alignment) {}
> +explicit MisalignedMember(Expr *E)
> +: MisalignedMember(E, nullptr, nullptr, CharUnits()) {}
> +
> +bool operator==(const MisalignedMember ) { return this->E == m.E; }
> +  };
> +  /// \brief Small set of gathered accesses to potentially misaligned
> members
> +  /// due to the packed attribute.
> +  SmallVector MisalignedMembers;
> +
> +  /// \brief Adds an expression to the set of gathered misaligned members.
> +  void AddPotentialMisalignedMembers(Expr *E, 

Re: r275417 - Diagnose taking address and reference binding of packed members

[Nico Weber via cfe-commits]

I mean clang's diagnostic could say this too. For example, it could have a
note pointing at the attribute(packed) saying "struct alignment set to 1
here" or some such.

On Thu, Jul 14, 2016 at 5:25 PM, Roger Ferrer Ibanez <
roger.ferreriba...@arm.com> wrote:

> Hi Nico,
>
>
> yes, it should be clearly stated in the documentation. I will try to find
> some time to contribute a change in the docs.
>
>
> I forgot that fact in my first answer to your original question. So, I
> think that the warning should fire in this case. Actually, I analysed
> precisely this case in phabricator and there was some consensus in that the
> warning was sensible.
>
>
> Kind regards,
>
> Roger
>
>
> --
> *From:* tha...@google.com <tha...@google.com> on behalf of Nico Weber <
> tha...@chromium.org>
> *Sent:* 14 July 2016 22:14:10
> *To:* Richard Smith
> *Cc:* Roger Ferrer Ibanez; cfe-commits
> *Subject:* Re: r275417 - Diagnose taking address and reference binding of
> packed members
>
> On Thu, Jul 14, 2016 at 5:07 PM, Richard Smith <rich...@metafoo.co.uk>
> wrote:
>
>> On Thu, Jul 14, 2016 at 10:15 AM, Nico Weber via cfe-commits <
>> cfe-commits@lists.llvm.org> wrote:
>>
>>> Hi,
>>>
>>> this fires on (at least) usrsctplib [1]:
>>>
>>> FAILED: obj/third_party/usrsctp/usrsctp/sctp_input.o
>>> ../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:1708:15:
>>> error: taking address of packed member 'time_entered' of class or structure
>>> 'sctp_state_cookie' may result in an unaligned pointer value
>>> [-Werror,-Waddress-of-packed-member]
>>>
>>> >time_entered,
>>>
>>>  ^~~~
>>> ../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:2458:10:
>>> error: taking address of packed member 'time_entered' of class or structure
>>> 'sctp_state_cookie' may result in an unaligned pointer value
>>> [-Werror,-Waddress-of-packed-member]
>>>   >time_entered,
>>> sctp_align_unsafe_makecopy,
>>>^~~~
>>> 2 errors generated.
>>>
>>> The struct looks like so [2]:
>>>
>>> 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 */
>>>   ...
>>>
>>> The _SIZE is 16, so as long as sctp_state_cookie is aligned, that field
>>> should be aligned as well, right? And while the struct is packed, its
>>> alignment isn't overwritten (unless the packed attribute does that too, but
>>> the docs at least don't mention that).
>>>
>>
>> The packed attribute does that too.
>>
>> struct __attribute__((packed)) X {
>>   unsigned long long a;
>> };
>> static_assert(alignof(X) == 1, ""); // does not fire
>>
>> What __attribute__((packed)) really means is that the minimum alignment
>> is reduced to 1.
>>
>
> Wow, that's pretty surprising! Maybe the warning text could be more
> explicit about this.
>
>
>>
>>
>>> Should the warning really fire here?
>>>
>>> 1:
>>> https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5748/steps/compile/logs/stdio
>>> 2:
>>> https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h?sq=package:chromium=C=1468495044=190
>>>
>>> On Thu, Jul 14, 2016 at 10:10 AM, Roger Ferrer Ibanez via cfe-commits <
>>> cfe-commits@lists.llvm.org> wrote:
>>>
>>>> Author: rogfer01
>>>> Date: Thu Jul 14 09:10:43 2016
>>>> New Revision: 275417
>>>>
>>>> URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
>>>> Log:
>>>> Diagnose taking address and reference binding of packed members
>>>>
>>>> This patch implements PR#22821.
>>>>
>>>> Taking the address of a packed member is dangerous since the reduced
>>>> alignment of the pointee is lost. This can lead to memory alignment
>>>> faults in some architectures if the pointer value is dereferenced.
>>>>
>>>> This change adds a new warning to clang emitted when taking the address
>>>> of a packed member. A packed member is either a field/data member
>>>> declared as attribute((packed)) 

Re: r275417 - Diagnose taking address and reference binding of packed members

[Roger Ferrer Ibanez via cfe-commits]

Hi Nico,


yes, it should be clearly stated in the documentation. I will try to find some 
time to contribute a change in the docs.


I forgot that fact in my first answer to your original question. So, I think 
that the warning should fire in this case. Actually, I analysed precisely this 
case in phabricator and there was some consensus in that the warning was 
sensible.


Kind regards,

Roger



From: tha...@google.com <tha...@google.com> on behalf of Nico Weber 
<tha...@chromium.org>
Sent: 14 July 2016 22:14:10
To: Richard Smith
Cc: Roger Ferrer Ibanez; cfe-commits
Subject: Re: r275417 - Diagnose taking address and reference binding of packed 
members

On Thu, Jul 14, 2016 at 5:07 PM, Richard Smith 
<rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>> wrote:
On Thu, Jul 14, 2016 at 10:15 AM, Nico Weber via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Hi,

this fires on (at least) usrsctplib [1]:

FAILED: obj/third_party/usrsctp/usrsctp/sctp_input.o
../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:1708:15: 
error: taking address of packed member 'time_entered' of class or structure 
'sctp_state_cookie' may result in an unaligned pointer value 
[-Werror,-Waddress-of-packed-member]
  
>time_entered,
   
^~~~
../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:2458:10: 
error: taking address of packed member 'time_entered' of class or structure 
'sctp_state_cookie' may result in an unaligned pointer value 
[-Werror,-Waddress-of-packed-member]
  >time_entered, 
sctp_align_unsafe_makecopy,
   ^~~~
2 errors generated.

The struct looks like so [2]:

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

The _SIZE is 16, so as long as sctp_state_cookie is aligned, that field should 
be aligned as well, right? And while the struct is packed, its alignment isn't 
overwritten (unless the packed attribute does that too, but the docs at least 
don't mention that).

The packed attribute does that too.

struct __attribute__((packed)) X {
  unsigned long long a;
};
static_assert(alignof(X) == 1, ""); // does not fire

What __attribute__((packed)) really means is that the minimum alignment is 
reduced to 1.

Wow, that's pretty surprising! Maybe the warning text could be more explicit 
about this.


Should the warning really fire here?

1: 
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5748/steps/compile/logs/stdio
2: 
https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h?sq=package:chromium=C=1468495044=190

On Thu, Jul 14, 2016 at 10:10 AM, Roger Ferrer Ibanez via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Author: rogfer01
Date: Thu Jul 14 09:10:43 2016
New Revision: 275417

URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
Log:
Diagnose taking address and reference binding of packed members

This patch implements PR#22821.

Taking the address of a packed member is dangerous since the reduced
alignment of the pointee is lost. This can lead to memory alignment
faults in some architectures if the pointer value is dereferenced.

This change adds a new warning to clang emitted when taking the address
of a packed member. A packed member is either a field/data member
declared as attribute((packed)) or belonging to a struct/class
declared as such. The associated flag is -Waddress-of-packed-member.
Conversions (either implicit or via a valid casting) to pointer types
with lower or equal alignment requirements (e.g. void* or char*)
silence the warning.

This change also adds a new error diagnostic when the user attempts to
bind a reference to a packed member, regardless of the alignment.

Differential Revision: https://reviews.llvm.org/D20561



Added:
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
Modified:
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

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
===

Re: r275417 - Diagnose taking address and reference binding of packed members

[Richard Smith via cfe-commits]

On Thu, Jul 14, 2016 at 10:15 AM, Nico Weber via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Hi,
>
> this fires on (at least) usrsctplib [1]:
>
> FAILED: obj/third_party/usrsctp/usrsctp/sctp_input.o
> ../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:1708:15:
> error: taking address of packed member 'time_entered' of class or structure
> 'sctp_state_cookie' may result in an unaligned pointer value
> [-Werror,-Waddress-of-packed-member]
>
> >time_entered,
>
>  ^~~~
> ../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:2458:10:
> error: taking address of packed member 'time_entered' of class or structure
> 'sctp_state_cookie' may result in an unaligned pointer value
> [-Werror,-Waddress-of-packed-member]
>   >time_entered,
> sctp_align_unsafe_makecopy,
>^~~~
> 2 errors generated.
>
> The struct looks like so [2]:
>
> 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 */
>   ...
>
> The _SIZE is 16, so as long as sctp_state_cookie is aligned, that field
> should be aligned as well, right? And while the struct is packed, its
> alignment isn't overwritten (unless the packed attribute does that too, but
> the docs at least don't mention that).
>

The packed attribute does that too.

struct __attribute__((packed)) X {
  unsigned long long a;
};
static_assert(alignof(X) == 1, ""); // does not fire

What __attribute__((packed)) really means is that the minimum alignment is
reduced to 1.


> Should the warning really fire here?
>
> 1:
> https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5748/steps/compile/logs/stdio
> 2:
> https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h?sq=package:chromium=C=1468495044=190
>
> On Thu, Jul 14, 2016 at 10:10 AM, Roger Ferrer Ibanez via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: rogfer01
>> Date: Thu Jul 14 09:10:43 2016
>> New Revision: 275417
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
>> Log:
>> Diagnose taking address and reference binding of packed members
>>
>> This patch implements PR#22821.
>>
>> Taking the address of a packed member is dangerous since the reduced
>> alignment of the pointee is lost. This can lead to memory alignment
>> faults in some architectures if the pointer value is dereferenced.
>>
>> This change adds a new warning to clang emitted when taking the address
>> of a packed member. A packed member is either a field/data member
>> declared as attribute((packed)) or belonging to a struct/class
>> declared as such. The associated flag is -Waddress-of-packed-member.
>> Conversions (either implicit or via a valid casting) to pointer types
>> with lower or equal alignment requirements (e.g. void* or char*)
>> silence the warning.
>>
>> This change also adds a new error diagnostic when the user attempts to
>> bind a reference to a packed member, regardless of the alignment.
>>
>> Differential Revision: https://reviews.llvm.org/D20561
>>
>>
>>
>> Added:
>> 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
>> Modified:
>> 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
>>
>> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>> URL:
>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
>>
>> ==
>> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
>> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 14
>> 09:10:43 2016
>> @@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incomp
>>"dereference of type %1 that was reinterpret_cast from type %0 has
>> undefined "
>>"behavior">,
>>InGroup, DefaultIgnore;
>> +def warn_taking_address_of_packed_member : Warning<
>> +  "taking address of packed member %0 of class or structure %q1 may
>> result in an unaligned pointer value">,
>> +  InGroup>;
>> +def err_binding_reference_to_packed_member : Error<
>> +  "binding reference to packed member %0 of class or structure %q1">;
>>
>>  def err_objc_object_assignment : Error<
>>"cannot assign to class object (%0 invalid)">;
>>
>> Modified: cfe/trunk/include/clang/Sema/Sema.h
>> URL:
>> 

Re: r275417 - Diagnose taking address and reference binding of packed members

[Roger Ferrer Ibanez via cfe-commits]

Hi Nico,


it seems it may be necessary to take into account the actual offset of the 
field in such cases to avoid these kind of false positives.


I reverted the change until I get a better solution.


Thanks,

Roger


From: tha...@google.com <tha...@google.com> on behalf of Nico Weber 
<tha...@chromium.org>
Sent: 14 July 2016 18:15:42
To: Roger Ferrer Ibanez
Cc: cfe-commits
Subject: Re: r275417 - Diagnose taking address and reference binding of packed 
members

Hi,

this fires on (at least) usrsctplib [1]:

FAILED: obj/third_party/usrsctp/usrsctp/sctp_input.o
../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:1708:15: 
error: taking address of packed member 'time_entered' of class or structure 
'sctp_state_cookie' may result in an unaligned pointer value 
[-Werror,-Waddress-of-packed-member]
  
>time_entered,
   
^~~~
../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:2458:10: 
error: taking address of packed member 'time_entered' of class or structure 
'sctp_state_cookie' may result in an unaligned pointer value 
[-Werror,-Waddress-of-packed-member]
  >time_entered, 
sctp_align_unsafe_makecopy,
   ^~~~
2 errors generated.

The struct looks like so [2]:

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

The _SIZE is 16, so as long as sctp_state_cookie is aligned, that field should 
be aligned as well, right? And while the struct is packed, its alignment isn't 
overwritten (unless the packed attribute does that too, but the docs at least 
don't mention that). Should the warning really fire here?

1: 
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5748/steps/compile/logs/stdio
2: 
https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h?sq=package:chromium=C=1468495044=190

On Thu, Jul 14, 2016 at 10:10 AM, Roger Ferrer Ibanez via cfe-commits 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Author: rogfer01
Date: Thu Jul 14 09:10:43 2016
New Revision: 275417

URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
Log:
Diagnose taking address and reference binding of packed members

This patch implements PR#22821.

Taking the address of a packed member is dangerous since the reduced
alignment of the pointee is lost. This can lead to memory alignment
faults in some architectures if the pointer value is dereferenced.

This change adds a new warning to clang emitted when taking the address
of a packed member. A packed member is either a field/data member
declared as attribute((packed)) or belonging to a struct/class
declared as such. The associated flag is -Waddress-of-packed-member.
Conversions (either implicit or via a valid casting) to pointer types
with lower or equal alignment requirements (e.g. void* or char*)
silence the warning.

This change also adds a new error diagnostic when the user attempts to
bind a reference to a packed member, regardless of the alignment.

Differential Revision: https://reviews.llvm.org/D20561



Added:
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
Modified:
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

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 14 09:10:43 
2016
@@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incomp
   "dereference of type %1 that was reinterpret_cast from type %0 has undefined 
"
   "behavior">,
   InGroup, DefaultIgnore;
+def warn_taking_address_of_packed_member : Warning<
+  "taking address of packed member %0 of class or structure %q1 may result in 
an unaligned pointer value">,
+  InGroup<DiagGroup<"address-of-packed-member">>;
+def err_binding_reference_to_packed_member : Error<
+  "binding reference to packed member %0 of class or structure %q1">;

 def err_objc_object_assignment : Error&l

Re: r275417 - Diagnose taking address and reference binding of packed members

[Nico Weber via cfe-commits]

Hi,

this fires on (at least) usrsctplib [1]:

FAILED: obj/third_party/usrsctp/usrsctp/sctp_input.o
../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:1708:15:
error: taking address of packed member 'time_entered' of class or structure
'sctp_state_cookie' may result in an unaligned pointer value
[-Werror,-Waddress-of-packed-member]

>time_entered,

 ^~~~
../../third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_input.c:2458:10:
error: taking address of packed member 'time_entered' of class or structure
'sctp_state_cookie' may result in an unaligned pointer value
[-Werror,-Waddress-of-packed-member]
  >time_entered,
sctp_align_unsafe_makecopy,
   ^~~~
2 errors generated.

The struct looks like so [2]:

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

The _SIZE is 16, so as long as sctp_state_cookie is aligned, that field
should be aligned as well, right? And while the struct is packed, its
alignment isn't overwritten (unless the packed attribute does that too, but
the docs at least don't mention that). Should the warning really fire here?

1:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/5748/steps/compile/logs/stdio
2:
https://cs.chromium.org/chromium/src/third_party/usrsctp/usrsctplib/usrsctplib/netinet/sctp_header.h?sq=package:chromium=C=1468495044=190

On Thu, Jul 14, 2016 at 10:10 AM, Roger Ferrer Ibanez via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: rogfer01
> Date: Thu Jul 14 09:10:43 2016
> New Revision: 275417
>
> URL: http://llvm.org/viewvc/llvm-project?rev=275417=rev
> Log:
> Diagnose taking address and reference binding of packed members
>
> This patch implements PR#22821.
>
> Taking the address of a packed member is dangerous since the reduced
> alignment of the pointee is lost. This can lead to memory alignment
> faults in some architectures if the pointer value is dereferenced.
>
> This change adds a new warning to clang emitted when taking the address
> of a packed member. A packed member is either a field/data member
> declared as attribute((packed)) or belonging to a struct/class
> declared as such. The associated flag is -Waddress-of-packed-member.
> Conversions (either implicit or via a valid casting) to pointer types
> with lower or equal alignment requirements (e.g. void* or char*)
> silence the warning.
>
> This change also adds a new error diagnostic when the user attempts to
> bind a reference to a packed member, regardless of the alignment.
>
> Differential Revision: https://reviews.llvm.org/D20561
>
>
>
> Added:
> 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
> Modified:
> 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
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=275417=275416=275417=diff
>
> ==
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Thu Jul 14
> 09:10:43 2016
> @@ -5425,6 +5425,11 @@ def warn_pointer_indirection_from_incomp
>"dereference of type %1 that was reinterpret_cast from type %0 has
> undefined "
>"behavior">,
>InGroup, DefaultIgnore;
> +def warn_taking_address_of_packed_member : Warning<
> +  "taking address of packed member %0 of class or structure %q1 may
> result in an unaligned pointer value">,
> +  InGroup>;
> +def err_binding_reference_to_packed_member : Error<
> +  "binding reference to packed member %0 of class or structure %q1">;
>
>  def err_objc_object_assignment : Error<
>"cannot assign to class object (%0 invalid)">;
>
> Modified: cfe/trunk/include/clang/Sema/Sema.h
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=275417=275416=275417=diff
>
> ==
> --- cfe/trunk/include/clang/Sema/Sema.h (original)
> +++ cfe/trunk/include/clang/Sema/Sema.h Thu Jul 14 09:10:43 2016
> @@ -9518,6 +9518,10 @@ private:
>void CheckArgumentWithTypeTag(const ArgumentWithTypeTagAttr *Attr,
>  const Expr * const *ExprArgs);
>
> +  /// \brief Check if we are taking the address of a packed field
> +  /// as