Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 07/05/2017 02:58 PM, Andrew Pinski wrote:

On Sun, Apr 30, 2017 at 1:02 PM, Pedro Alves  wrote:

Hi Martin,

Thanks much for doing this.  A few comments below, in light of my
experience doing the equivalent checks in the gdb patch linked below,
using standard C++11.

On 04/29/2017 09:09 PM, Martin Sebor wrote:

Calling memset, memcpy, or similar to write to an object of
a non-trivial type (such as one that defines a ctor or dtor,
or has such a member) can break the invariants otherwise
maintained by the class and cause undefined behavior.

The motivating example that prompted this work was a review of
a change that added to a plain old struct a new member with a ctor
and dtor (in this instance the member was of type std::vector).

To help catch problems of this sort some projects (such as GDB)
have apparently even devised their own clever solutions to detect
them: https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html.

The attached patch adds a new warning, -Wnon-trivial-memaccess,
that has GCC detect these mistakes.  The patch also fixes up
a handful of instances of the problem in GCC.  These instances
correspond to the two patterns below:

  struct A
  {
void *p;
void foo (int n) { p = malloc (n); }
~A () { free (p); }
  };

  void init (A *a)
  {
memset (a, 0, sizeof *a);
  }

and

  struct B
  {
int i;
~A ();
  };


(typo: "~B ();")



  void copy (B *p, const B *q)
  {
memcpy (p, q, sizeof *p);
...
   }



IMO the check should be relaxed from "type is trivial" to "type is
trivially copyable" (which is what the gdb detection at
https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
uses for memcpy/memmove).  Checking that the destination is trivial is
going to generate false positives -- specifically, [basic-types]/3
specifies that it's fine to memcpy trivially _copyable_ types, not
trivial types.  A type can be both non-trivial and trivially copyable
at the same time.  For example, this compiles, but triggers
your new warning:

#include 
#include 
#include 

struct NonTrivialButTriviallyCopyable
{
  NonTrivialButTriviallyCopyable () : i (0) {}
  int i;
};

static_assert (!std::is_trivial::value, "");
static_assert (std::is_trivially_copyable::value, 
"");

void copy (NonTrivialButTriviallyCopyable *dst, NonTrivialButTriviallyCopyable 
*src)
{
  memcpy (dst, src, sizeof (*src));
}

$ /opt/gcc/bin/g++ -std=gnu++11 trivial-warn.cc -o trivial-warn -g3 -O0 -Wall 
-Wextra -c
trivial-warn.cc: In function ‘void copy(NonTrivialButTriviallyCopyable*, 
NonTrivialButTriviallyCopyable*)’:
trivial-warn.cc:16:34: warning: calling ‘void* memcpy(void*, const void*, 
size_t)’ with a pointer to a non-trivial type ‘struct 
NonTrivialButTriviallyCopyable’ [-Wnon-trivial-memaccess]
   memcpy (dst, src, sizeof (*src));
  ^
$

Implementations of vector-like classes can very well (and are
encouraged) to make use of std::is_trivially_copyable to know whether
they can copy a range of elements to new storage
using memcpy/memmove/mempcpy.

Running your patch against GDB trips on such a case:

src/gdb/btrace.h: In function ‘btrace_insn_s* 
VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const 
btrace_insn_s*, const char*, unsigned int)’:
src/gdb/common/vec.h:948:62: error: calling ‘void* memmove(void*, const void*, 
size_t)’ with a pointer to a non-trivial type ‘btrace_insn_s {aka struct 
btrace_insn}’ [-Werror=non-trivial-memaccess]
   memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));\
  ^

There is nothing wrong with the code being warned here.
While "struct btrace_insn" is trivial (has a user-provided default
ctor), it is still trivially copyable.



Any news on getting a "fix" for this issue.  Right now it blocks my
testing of GCC/gdb because I am building the trunk of both in a CI
loop and my build is broken due to this warning.  Should I just add
--disable-werror to my gdb build instead?


I'm not aware of any serious bugs in the warning that need fixing.
The warning points out raw memory accesses to objects of non-trivial
types (among other things), or those with user-defined default or
copy ctors, dtor, or copy assignment operator.  Objects of such
types should be manipulated using these special member functions
rather than by raw memory functions.  In many (though not all(*))
cases the raw memory calls can put.leave such objects in an invalid
state and make using them undefined.

In the instance of the warning above, btrace_insn_s is a non-trivial
type because it has a user-defined default ctor, as a result of
defining a member of such a type (flags, which is of type
enum_flags).  To avoid the warning either
the memcpy/memmove calls should be replaced with a loop that makes
use of the special function(s), or in C++ 11 and later, the class
made trivial by defaulting the ctors and copy assignment operators.
In the GDB case, this can be 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Andrew Pinski]

On Sun, Apr 30, 2017 at 1:02 PM, Pedro Alves  wrote:
> Hi Martin,
>
> Thanks much for doing this.  A few comments below, in light of my
> experience doing the equivalent checks in the gdb patch linked below,
> using standard C++11.
>
> On 04/29/2017 09:09 PM, Martin Sebor wrote:
>> Calling memset, memcpy, or similar to write to an object of
>> a non-trivial type (such as one that defines a ctor or dtor,
>> or has such a member) can break the invariants otherwise
>> maintained by the class and cause undefined behavior.
>>
>> The motivating example that prompted this work was a review of
>> a change that added to a plain old struct a new member with a ctor
>> and dtor (in this instance the member was of type std::vector).
>>
>> To help catch problems of this sort some projects (such as GDB)
>> have apparently even devised their own clever solutions to detect
>> them: https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html.
>>
>> The attached patch adds a new warning, -Wnon-trivial-memaccess,
>> that has GCC detect these mistakes.  The patch also fixes up
>> a handful of instances of the problem in GCC.  These instances
>> correspond to the two patterns below:
>>
>>   struct A
>>   {
>> void *p;
>> void foo (int n) { p = malloc (n); }
>> ~A () { free (p); }
>>   };
>>
>>   void init (A *a)
>>   {
>> memset (a, 0, sizeof *a);
>>   }
>>
>> and
>>
>>   struct B
>>   {
>> int i;
>> ~A ();
>>   };
>
> (typo: "~B ();")
>
>>
>>   void copy (B *p, const B *q)
>>   {
>> memcpy (p, q, sizeof *p);
>> ...
>>}
>>
>
> IMO the check should be relaxed from "type is trivial" to "type is
> trivially copyable" (which is what the gdb detection at
> https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
> uses for memcpy/memmove).  Checking that the destination is trivial is
> going to generate false positives -- specifically, [basic-types]/3
> specifies that it's fine to memcpy trivially _copyable_ types, not
> trivial types.  A type can be both non-trivial and trivially copyable
> at the same time.  For example, this compiles, but triggers
> your new warning:
>
> #include 
> #include 
> #include 
>
> struct NonTrivialButTriviallyCopyable
> {
>   NonTrivialButTriviallyCopyable () : i (0) {}
>   int i;
> };
>
> static_assert (!std::is_trivial::value, "");
> static_assert 
> (std::is_trivially_copyable::value, "");
>
> void copy (NonTrivialButTriviallyCopyable *dst, 
> NonTrivialButTriviallyCopyable *src)
> {
>   memcpy (dst, src, sizeof (*src));
> }
>
> $ /opt/gcc/bin/g++ -std=gnu++11 trivial-warn.cc -o trivial-warn -g3 -O0 -Wall 
> -Wextra -c
> trivial-warn.cc: In function ‘void copy(NonTrivialButTriviallyCopyable*, 
> NonTrivialButTriviallyCopyable*)’:
> trivial-warn.cc:16:34: warning: calling ‘void* memcpy(void*, const void*, 
> size_t)’ with a pointer to a non-trivial type ‘struct 
> NonTrivialButTriviallyCopyable’ [-Wnon-trivial-memaccess]
>memcpy (dst, src, sizeof (*src));
>   ^
> $
>
> Implementations of vector-like classes can very well (and are
> encouraged) to make use of std::is_trivially_copyable to know whether
> they can copy a range of elements to new storage
> using memcpy/memmove/mempcpy.
>
> Running your patch against GDB trips on such a case:
>
> src/gdb/btrace.h: In function ‘btrace_insn_s* 
> VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const 
> btrace_insn_s*, const char*, unsigned int)’:
> src/gdb/common/vec.h:948:62: error: calling ‘void* memmove(void*, const 
> void*, size_t)’ with a pointer to a non-trivial type ‘btrace_insn_s {aka 
> struct btrace_insn}’ [-Werror=non-trivial-memaccess]
>memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));\
>   ^
>
> There is nothing wrong with the code being warned here.
> While "struct btrace_insn" is trivial (has a user-provided default
> ctor), it is still trivially copyable.


Any news on getting a "fix" for this issue.  Right now it blocks my
testing of GCC/gdb because I am building the trunk of both in a CI
loop and my build is broken due to this warning.  Should I just add
--disable-werror to my gdb build instead?

Thanks,
Andrew Pinski

>
> Now, this gdb code is using the old VEC (originated from
> gcc's C days, it's not the current C++fied VEC implementation),
> but the point is that any other random vector-like container out there
> is free to optimize copy of a range of non-trivial but trivially
> copyable types using memcpy/memmove.
>
> Note that libstdc++ does not actually do that optimization, but
> that's just a missed optimization, see PR libstdc++/68350 [1]
> "std::uninitialized_copy overly restrictive for
> trivially_copyable types".  (libstdc++'s std::vector defers
> copy to std::unitialized_copy.)
>
> [1] - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350
>
>> These aren't undefined and the patch could be tweaked to allow
>> them.
>
> I think they're 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Richard Earnshaw (lists)]

On 29/06/17 17:15, Jan Hubicka wrote:
> Hello,
>> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
>> index 0f7e21a..443d16c 100644
>> --- a/gcc/hash-table.h
>> +++ b/gcc/hash-table.h
>> @@ -803,7 +803,10 @@ hash_table::empty_slow ()
>>m_size_prime_index = nindex;
>>  }
>>else
>> -memset (entries, 0, size * sizeof (value_type));
>> +{
>> +  for ( ; size; ++entries, --size)
>> +*entries = value_type ();
>> +}
>>m_n_deleted = 0;
>>m_n_elements = 0;
>>  }
> 
> This change sends our periodic testers into an infinite loop.  It is fault of 
> gcc 4.2 being used
> as bootstrap compiler, but perhaps that can be worked around?
> 
> Honza
> 

Same is also true on RHE5 (gcc-4.1).

R.

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/30/2017 02:34 AM, Richard Biener wrote:

On Thu, Jun 29, 2017 at 10:23 PM, Martin Sebor  wrote:

On 06/29/2017 10:15 AM, Jan Hubicka wrote:


Hello,


diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index 0f7e21a..443d16c 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -803,7 +803,10 @@ hash_table::empty_slow ()
   m_size_prime_index = nindex;
 }
   else
-memset (entries, 0, size * sizeof (value_type));
+{
+  for ( ; size; ++entries, --size)
+   *entries = value_type ();
+}
   m_n_deleted = 0;
   m_n_elements = 0;
 }



This change sends our periodic testers into an infinite loop.  It is fault
of gcc 4.2 being used
as bootstrap compiler, but perhaps that can be worked around?



The warning in the original code could have been suppressed (by
casting the pointer to char*), but it was valid so I opted not
to.  I'd expect it to be possible to work around the bug but
I don't have easy access to GCC 4.2 to reproduce it or verify
the fix.

FWIW, after looking at the function again, I wondered if zeroing
out the elements (either way) was the right thing to do and if
they shouldn't be cleared by calling Descriptor::mark_empty()
instead, like in alloc_entries(), but making that change broke
a bunch of ipa/ipa-pta-*.c tests.  It's not really clear to me
what this code is supposed to do.

Martin

PS Does this help at all?

@@ -804,8 +804,8 @@ hash_table::empty_slow ()
 }
   else
 {
-  for ( ; size; ++entries, --size)
-   *entries = value_type ();
+  for (size_t i = 0; i != size; ++i)
+   entries[i] = value_type ();

 }
   m_n_deleted = 0;
   m_n_elements = 0;


alloc_entries uses mark_empty.  untested:


Right.  That was my initial thought as well but it broke a number
of ipa/ipa-pta-*.c tests.  I didn't have time to investigate why.

Martin



Index: gcc/hash-table.h
===
--- gcc/hash-table.h(revision 249780)
+++ gcc/hash-table.h(working copy)
@@ -804,8 +804,8 @@ hash_table::empty
 }
   else
 {
-  for ( ; size; ++entries, --size)
-   *entries = value_type ();
+  for (size_t i = 0; i < size; ++i)
+   mark_empty (entries[i]);
 }
   m_n_deleted = 0;
   m_n_elements = 0;

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Richard Biener]

On Thu, Jun 29, 2017 at 10:23 PM, Martin Sebor  wrote:
> On 06/29/2017 10:15 AM, Jan Hubicka wrote:
>>
>> Hello,
>>>
>>> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
>>> index 0f7e21a..443d16c 100644
>>> --- a/gcc/hash-table.h
>>> +++ b/gcc/hash-table.h
>>> @@ -803,7 +803,10 @@ hash_table::empty_slow ()
>>>m_size_prime_index = nindex;
>>>  }
>>>else
>>> -memset (entries, 0, size * sizeof (value_type));
>>> +{
>>> +  for ( ; size; ++entries, --size)
>>> +   *entries = value_type ();
>>> +}
>>>m_n_deleted = 0;
>>>m_n_elements = 0;
>>>  }
>>
>>
>> This change sends our periodic testers into an infinite loop.  It is fault
>> of gcc 4.2 being used
>> as bootstrap compiler, but perhaps that can be worked around?
>
>
> The warning in the original code could have been suppressed (by
> casting the pointer to char*), but it was valid so I opted not
> to.  I'd expect it to be possible to work around the bug but
> I don't have easy access to GCC 4.2 to reproduce it or verify
> the fix.
>
> FWIW, after looking at the function again, I wondered if zeroing
> out the elements (either way) was the right thing to do and if
> they shouldn't be cleared by calling Descriptor::mark_empty()
> instead, like in alloc_entries(), but making that change broke
> a bunch of ipa/ipa-pta-*.c tests.  It's not really clear to me
> what this code is supposed to do.
>
> Martin
>
> PS Does this help at all?
>
> @@ -804,8 +804,8 @@ hash_table::empty_slow ()
>  }
>else
>  {
> -  for ( ; size; ++entries, --size)
> -   *entries = value_type ();
> +  for (size_t i = 0; i != size; ++i)
> +   entries[i] = value_type ();
>
>  }
>m_n_deleted = 0;
>m_n_elements = 0;

alloc_entries uses mark_empty.  untested:

Index: gcc/hash-table.h
===
--- gcc/hash-table.h(revision 249780)
+++ gcc/hash-table.h(working copy)
@@ -804,8 +804,8 @@ hash_table::empty
 }
   else
 {
-  for ( ; size; ++entries, --size)
-   *entries = value_type ();
+  for (size_t i = 0; i < size; ++i)
+   mark_empty (entries[i]);
 }
   m_n_deleted = 0;
   m_n_elements = 0;

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/29/2017 04:34 PM, Jan Hubicka wrote:


The warning in the original code could have been suppressed (by
casting the pointer to char*), but it was valid so I opted not
to.  I'd expect it to be possible to work around the bug but
I don't have easy access to GCC 4.2 to reproduce it or verify
the fix.

FWIW, after looking at the function again, I wondered if zeroing
out the elements (either way) was the right thing to do and if
they shouldn't be cleared by calling Descriptor::mark_empty()
instead, like in alloc_entries(), but making that change broke
a bunch of ipa/ipa-pta-*.c tests.  It's not really clear to me
what this code is supposed to do.


Well, it is a standard hash table.


What I mean is that while the true branch of the if (nsize != size)
statement in hash_table::empty_slow () frees the table and then
allocates a new one which marks each element in the newly allocated
storage as empty by calling mark_empty() (i.e., Descriptor::
mark_empty()), while the false branch bypasses mark_empty() and
calls memset(entries, 0, ...) instead.  The two are equivalent
only when Descriptor::mark_empty(value_type ) sets e to all
zeros.  They are not equivalent when Descriptor::mark_empty()
does something else.  When value_type is a pointer they should
be the equivalent (so long as a null pointer is all zeros), but
it's not obvious to me what value_type is in your case (tree?)

In any case, calling memset bypasses the (undocumented)
customization point (Descriptor::mark_empty).  Calling it
conditionally depending on the size of the hash table seems
suspicious (as does setting *entry = value_type();).  If there
is a hash table instance where mark_empty is not equivalent to
memset(entries, 0, ...) it could mean a bug.

Looking at some other definitions of mark_empty I see int_hash
defines it this way:

  template 
  inline void
  int_hash ::mark_empty (Type )
  {
x = Empty;
  }

So for hash_tables with int value_types the memset is only
equivalent to calling mark_empty when Empty is zero.  There
are instances of hash tables in GCC like alias_set_hash and
uid_hash where Empty is non-zero.  I wonder what happens when
hash_table::empty_slow() is called on one of these instances
with a small number of elements and that instance is used
again.


The problem I hit was
lookup_with_hash walking infinitly around the hash table because
all elements seemed used.
is_empty is defined as:
template 
inline bool
pointer_hash ::is_empty (Type *e)
{
  return e == NULL;
}

and mark_empty as
template 
inline void
pointer_hash ::mark_empty (Type *)
{
  e = NULL;
}


It sounds like value_type is a pointer in your case and
the assignment '*entry = value_type();' isn't clearing the *entry.
I vaguely remember coming across a bug like that years ago but I
don't know if that was GCC or some other compiler.  I tried
compiling a small test case with a few GCC revisions close to
4.2 (r117926 and some others) but couldn't reproduce anything
unexpected.


I guess they are supposed to be definable to other implementations
but then the former memset code would break.


Right, that's my concern.

Martin

PS the below is just a shot in the dark. I'd be surprised if it
actually did something for you, but with little else to go on
it's worth a try.


Martin

PS Does this help at all?

@@ -804,8 +804,8 @@ hash_table::empty_slow ()
 }
   else
 {
-  for ( ; size; ++entries, --size)
-   *entries = value_type ();
+  for (size_t i = 0; i != size; ++i)
+   entries[i] = value_type ();


I can give it a try tomorrow.  Still wonder what goes wrong with ctors with 4.2 
(and 4.3 as well apparently)

Honza

 }
   m_n_deleted = 0;
   m_n_elements = 0;

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jan Hubicka]

> 
> The warning in the original code could have been suppressed (by
> casting the pointer to char*), but it was valid so I opted not
> to.  I'd expect it to be possible to work around the bug but
> I don't have easy access to GCC 4.2 to reproduce it or verify
> the fix.
> 
> FWIW, after looking at the function again, I wondered if zeroing
> out the elements (either way) was the right thing to do and if
> they shouldn't be cleared by calling Descriptor::mark_empty()
> instead, like in alloc_entries(), but making that change broke
> a bunch of ipa/ipa-pta-*.c tests.  It's not really clear to me
> what this code is supposed to do.

Well, it is a standard hash table.  The problem I hit was
lookup_with_hash walking infinitly around the hash table because
all elements seemed used.
is_empty is defined as:
template 
inline bool
pointer_hash ::is_empty (Type *e)
{
  return e == NULL;
}

and mark_empty as
template 
inline void
pointer_hash ::mark_empty (Type *)
{
  e = NULL;
}

I guess they are supposed to be definable to other implementations
but then the former memset code would break.
> Martin
> 
> PS Does this help at all?
> 
> @@ -804,8 +804,8 @@ hash_table::empty_slow ()
>  }
>else
>  {
> -  for ( ; size; ++entries, --size)
> -   *entries = value_type ();
> +  for (size_t i = 0; i != size; ++i)
> +   entries[i] = value_type ();

I can give it a try tomorrow.  Still wonder what goes wrong with ctors with 4.2 
(and 4.3 as well apparently)

Honza
>  }
>m_n_deleted = 0;
>m_n_elements = 0;

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/29/2017 10:15 AM, Jan Hubicka wrote:

Hello,

diff --git a/gcc/hash-table.h b/gcc/hash-table.h
index 0f7e21a..443d16c 100644
--- a/gcc/hash-table.h
+++ b/gcc/hash-table.h
@@ -803,7 +803,10 @@ hash_table::empty_slow ()
   m_size_prime_index = nindex;
 }
   else
-memset (entries, 0, size * sizeof (value_type));
+{
+  for ( ; size; ++entries, --size)
+   *entries = value_type ();
+}
   m_n_deleted = 0;
   m_n_elements = 0;
 }


This change sends our periodic testers into an infinite loop.  It is fault of 
gcc 4.2 being used
as bootstrap compiler, but perhaps that can be worked around?


The warning in the original code could have been suppressed (by
casting the pointer to char*), but it was valid so I opted not
to.  I'd expect it to be possible to work around the bug but
I don't have easy access to GCC 4.2 to reproduce it or verify
the fix.

FWIW, after looking at the function again, I wondered if zeroing
out the elements (either way) was the right thing to do and if
they shouldn't be cleared by calling Descriptor::mark_empty()
instead, like in alloc_entries(), but making that change broke
a bunch of ipa/ipa-pta-*.c tests.  It's not really clear to me
what this code is supposed to do.

Martin

PS Does this help at all?

@@ -804,8 +804,8 @@ hash_table::empty_slow ()
 }
   else
 {
-  for ( ; size; ++entries, --size)
-   *entries = value_type ();
+  for (size_t i = 0; i != size; ++i)
+   entries[i] = value_type ();
 }
   m_n_deleted = 0;
   m_n_elements = 0;

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jan Hubicka]

Hello,
> diff --git a/gcc/hash-table.h b/gcc/hash-table.h
> index 0f7e21a..443d16c 100644
> --- a/gcc/hash-table.h
> +++ b/gcc/hash-table.h
> @@ -803,7 +803,10 @@ hash_table::empty_slow ()
>m_size_prime_index = nindex;
>  }
>else
> -memset (entries, 0, size * sizeof (value_type));
> +{
> +  for ( ; size; ++entries, --size)
> + *entries = value_type ();
> +}
>m_n_deleted = 0;
>m_n_elements = 0;
>  }

This change sends our periodic testers into an infinite loop.  It is fault of 
gcc 4.2 being used
as bootstrap compiler, but perhaps that can be worked around?

Honza

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Richard Biener]

On Fri, Jun 16, 2017 at 9:38 AM, Richard Biener
 wrote:
> On Thu, Jun 15, 2017 at 11:31 PM, Jason Merrill  wrote:
>> On Thu, Jun 15, 2017 at 12:26 PM, Martin Sebor  wrote:
>>> On 06/12/2017 03:36 PM, Jason Merrill wrote:

 On 06/08/2017 01:25 PM, Martin Sebor wrote:
>
> +  if (TREE_CHAIN (DECL_ARGUMENTS (f)))
> +{
> +  /* Skip constructors that aren't copy or move ctors.  */
> +  if (!copy_fn_p (f))
> +continue;
> +
> +  cpy_or_move_ctor_p = true;
> +}
> +  else
> +{
> +  /* Constructor is a default ctor.  */
> +  cpy_or_move_ctor_p = false;
> +}

 A default constructor can have parameters, so long as they have default
 arguments.  You can use default_ctor_p to test for a default constructor.
>>>
>>> Thank you for the suggestion.  Attached is an incremental diff
>>> with this tweak plus a test for it.
>>>
>>> The code above has been there in the last three revisions of
>>> the patch
>>
>> Yeah, I don't always notice everything :)
>>
>>> are there any other changes you'd like me to make?
>>
>> No, the patch is OK with this change.
>
> This broke build with GCC 4.8 as host compiler:
>
> g++ -fno-PIE -c   -g  -DIN_GCC -fno-exceptions -fno-rtti
> -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings
> -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic
> -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings
> -fno-common  -DHAVE_CONFIG_H -I. -I.
> -I/space/rguenther/src/svn/early-lto-debug/gcc
> -I/space/rguenther/src/svn/early-lto-debug/gcc/.
> -I/space/rguenther/src/svn/early-lto-debug/gcc/../include
> -I/space/rguenther/src/svn/early-lto-debug/gcc/../libcpp/include
> -I/space/rguenther/src/svn/early-lto-debug/gcc/../libdecnumber
> -I/space/rguenther/src/svn/early-lto-debug/gcc/../libdecnumber/bid
> -I../libdecnumber
> -I/space/rguenther/src/svn/early-lto-debug/gcc/../libbacktrace   -o
> tree-switch-conversion.o -MT tree-switch-conversion.o -MMD -MP -MF
> ./.deps/tree-switch-conversion.TPo
> /space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c
> /space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c:
> In function ‘void emit_case_bit_tests(gswitch*, tree, tree, tree,
> tree)’:
> /space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c:271:53:
> warning: missing initializer for member ‘case_bit_test::mask’
> [-Wmissing-field-initializers]
>struct case_bit_test test[MAX_CASE_BIT_TESTS] = { };
>  ^
> ...
> /space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c:271:53:
> internal compiler error: in gimplify_init_constructor, at
> gimplify.c:4271
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See  for instructions.
> Makefile:1102: recipe for target 'tree-switch-conversion.o' failed
> make: *** [tree-switch-conversion.o] Error 1
>
> Please fix.

Using

  struct case_bit_test test[MAX_CASE_BIT_TESTS] = { {} };

avoids the ICE, the warning persists (not sure if correctly or not).
I'll commit this
workaround if it survives stage2/3 -Werror.

Richard.

> Richard.
>
>> Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Richard Biener]

On Thu, Jun 15, 2017 at 11:31 PM, Jason Merrill  wrote:
> On Thu, Jun 15, 2017 at 12:26 PM, Martin Sebor  wrote:
>> On 06/12/2017 03:36 PM, Jason Merrill wrote:
>>>
>>> On 06/08/2017 01:25 PM, Martin Sebor wrote:

 +  if (TREE_CHAIN (DECL_ARGUMENTS (f)))
 +{
 +  /* Skip constructors that aren't copy or move ctors.  */
 +  if (!copy_fn_p (f))
 +continue;
 +
 +  cpy_or_move_ctor_p = true;
 +}
 +  else
 +{
 +  /* Constructor is a default ctor.  */
 +  cpy_or_move_ctor_p = false;
 +}
>>>
>>> A default constructor can have parameters, so long as they have default
>>> arguments.  You can use default_ctor_p to test for a default constructor.
>>
>> Thank you for the suggestion.  Attached is an incremental diff
>> with this tweak plus a test for it.
>>
>> The code above has been there in the last three revisions of
>> the patch
>
> Yeah, I don't always notice everything :)
>
>> are there any other changes you'd like me to make?
>
> No, the patch is OK with this change.

This broke build with GCC 4.8 as host compiler:

g++ -fno-PIE -c   -g  -DIN_GCC -fno-exceptions -fno-rtti
-fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings
-Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic
-Wno-long-long -Wno-variadic-macros -Wno-overlength-strings
-fno-common  -DHAVE_CONFIG_H -I. -I.
-I/space/rguenther/src/svn/early-lto-debug/gcc
-I/space/rguenther/src/svn/early-lto-debug/gcc/.
-I/space/rguenther/src/svn/early-lto-debug/gcc/../include
-I/space/rguenther/src/svn/early-lto-debug/gcc/../libcpp/include
-I/space/rguenther/src/svn/early-lto-debug/gcc/../libdecnumber
-I/space/rguenther/src/svn/early-lto-debug/gcc/../libdecnumber/bid
-I../libdecnumber
-I/space/rguenther/src/svn/early-lto-debug/gcc/../libbacktrace   -o
tree-switch-conversion.o -MT tree-switch-conversion.o -MMD -MP -MF
./.deps/tree-switch-conversion.TPo
/space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c
/space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c:
In function ‘void emit_case_bit_tests(gswitch*, tree, tree, tree,
tree)’:
/space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c:271:53:
warning: missing initializer for member ‘case_bit_test::mask’
[-Wmissing-field-initializers]
   struct case_bit_test test[MAX_CASE_BIT_TESTS] = { };
 ^
...
/space/rguenther/src/svn/early-lto-debug/gcc/tree-switch-conversion.c:271:53:
internal compiler error: in gimplify_init_constructor, at
gimplify.c:4271
Please submit a full bug report,
with preprocessed source if appropriate.
See  for instructions.
Makefile:1102: recipe for target 'tree-switch-conversion.o' failed
make: *** [tree-switch-conversion.o] Error 1

Please fix.

Richard.

> Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On Thu, Jun 15, 2017 at 12:26 PM, Martin Sebor  wrote:
> On 06/12/2017 03:36 PM, Jason Merrill wrote:
>>
>> On 06/08/2017 01:25 PM, Martin Sebor wrote:
>>>
>>> +  if (TREE_CHAIN (DECL_ARGUMENTS (f)))
>>> +{
>>> +  /* Skip constructors that aren't copy or move ctors.  */
>>> +  if (!copy_fn_p (f))
>>> +continue;
>>> +
>>> +  cpy_or_move_ctor_p = true;
>>> +}
>>> +  else
>>> +{
>>> +  /* Constructor is a default ctor.  */
>>> +  cpy_or_move_ctor_p = false;
>>> +}
>>
>> A default constructor can have parameters, so long as they have default
>> arguments.  You can use default_ctor_p to test for a default constructor.
>
> Thank you for the suggestion.  Attached is an incremental diff
> with this tweak plus a test for it.
>
> The code above has been there in the last three revisions of
> the patch

Yeah, I don't always notice everything :)

> are there any other changes you'd like me to make?

No, the patch is OK with this change.

Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/12/2017 03:36 PM, Jason Merrill wrote:

On 06/08/2017 01:25 PM, Martin Sebor wrote:

+  if (TREE_CHAIN (DECL_ARGUMENTS (f)))
+{
+  /* Skip constructors that aren't copy or move ctors.  */
+  if (!copy_fn_p (f))
+continue;
+
+  cpy_or_move_ctor_p = true;
+}
+  else
+{
+  /* Constructor is a default ctor.  */
+  cpy_or_move_ctor_p = false;
+}


A default constructor can have parameters, so long as they have default
arguments.  You can use default_ctor_p to test for a default constructor.


Thank you for the suggestion.  Attached is an incremental diff
with this tweak plus a test for it.

The code above has been there in the last three revisions of
the patch, so just to streamline the review/redo/retest process:
are there any other changes you'd like me to make?

Martin
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 6e0951e..50c26bf 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8251,20 +8251,11 @@ has_trivial_copy_p (tree type, bool access, bool hasctor[2])
   if (TREE_CODE (f) != FUNCTION_DECL)
 	continue;
 
-  bool cpy_or_move_ctor_p;
-  if (TREE_CHAIN (DECL_ARGUMENTS (f)))
-	{
-	  /* Skip constructors that aren't copy or move ctors.  */
-	  if (!copy_fn_p (f))
-	continue;
+  bool cpy_or_move_ctor_p = copy_fn_p (f);
 
-	  cpy_or_move_ctor_p = true;
-	}
-  else
-	{
-	  /* Constructor is a default ctor.  */
-	  cpy_or_move_ctor_p = false;
-	}
+  /* Skip ctors other than default, copy, and move.  */
+  if (!cpy_or_move_ctor_p && !default_ctor_p (f))
+	continue;
 
   if (DECL_DELETED_FN (f))
 	continue;
@@ -8346,7 +8337,7 @@ maybe_warn_class_memaccess (location_t loc, tree fndecl, tree *args)
  accessible.  */
   bool trivassign = has_trivial_copy_assign_p (desttype, true, );
 
-  /* Set to true if DESTTYPE has an accessible defqault and copy ctor,
+  /* Set to true if DESTTYPE has an accessible default and copy ctor,
  respectively.  */
   bool hasctors[2] = { false, false };
 
diff --git a/gcc/testsuite/g++.dg/Wclass-memaccess.C b/gcc/testsuite/g++.dg/Wclass-memaccess.C
index d7ba44f..4783438 100644
--- a/gcc/testsuite/g++.dg/Wclass-memaccess.C
+++ b/gcc/testsuite/g++.dg/Wclass-memaccess.C
@@ -314,9 +314,11 @@ void test (HasCopy *p, const HasCopy ,
   const int i = *ia;
   const size_t n = *ia;
 
-  // Zeroing out is diagnosed because value initialization is
-  // invalid (the copy ctor makes no default ctor unavailable).
-  T (bzero, (p, sizeof *p));// { dg-warning "bzero" }
+  // Zeroing out is diagnosed because value initialization is invalid
+  // (the copy ctor makes no default ctor unavailable).  Since the type
+  // has no default ctor verify that the suggested alternative does not
+  // include value-initialization.
+  T (bzero, (p, sizeof *p));// { dg-warning "clearing an object of non-trivial type .struct HasCopy.; use assignment instead" }
   T (memset, (p, 0, sizeof *p));// { dg-warning "memset" }
   T (memset, (p, 1, sizeof *p));// { dg-warning "memset" }
   T (memset, (p, i, sizeof *p));// { dg-warning "memset" }
@@ -350,6 +352,27 @@ void test (HasCopy *p, const HasCopy ,
 
 #endif
 
+#if !defined TEST || TEST == TEST_HAS_DEFAULT_AND_COPY
+
+/* HasDefaultAndCopy is like HasCopy above but its default ctor takes
+   a default argument to verify that the suggested alternative offered
+   by the warning includes the default ctor (i.e., the test verifies
+   that the default ctor is recognized as such despite taking an argument.  */
+
+struct HasDefaultAndCopy
+{
+  HasDefaultAndCopy (int = 0);   // default ctor
+  HasDefaultAndCopy (const HasDefaultAndCopy&);
+};
+
+void test (HasDefaultAndCopy *p, const HasDefaultAndCopy )
+{
+  T (bzero, (p, sizeof *p));// { dg-warning "clearing an object of non-trivial type .struct HasDefaultAndCopy.; use assignment or value-initialization instead" }
+  T (memset, (p, 0, sizeof *p));// { dg-warning "clearing an object of non-trivial type .struct HasDefaultAndCopy.; use assignment or value-initialization instead" }
+}
+
+#endif
+
 #if !defined TEST || TEST == TEST_HAS_PRIVATE_COPY
 
 /* HasPrivateCopy cannot be copied using memcpy or memmove.  Since it's

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On 06/08/2017 01:25 PM, Martin Sebor wrote:

+  if (TREE_CHAIN (DECL_ARGUMENTS (f)))
+   {
+ /* Skip constructors that aren't copy or move ctors.  */
+ if (!copy_fn_p (f))
+   continue;
+
+ cpy_or_move_ctor_p = true;
+   }
+  else
+   {
+ /* Constructor is a default ctor.  */
+ cpy_or_move_ctor_p = false;
+   }


A default constructor can have parameters, so long as they have default 
arguments.  You can use default_ctor_p to test for a default constructor.


Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/07/2017 07:09 PM, Jason Merrill wrote:

On 06/06/2017 03:24 PM, Martin Sebor wrote:

+  /* Iterate over copy and move assignment overloads.  */
+
+  for (ovl_iterator oi (fns); oi; ++oi)
+{
+  tree f = *oi;
+
+  bool accessible = !access || !(TREE_PRIVATE (f) ||
TREE_PROTECTED (f));
+
+  /* Skip template assignment operators and deleted functions.  */
+  if (TREE_CODE (f) != FUNCTION_DECL || DECL_DELETED_FN (f))
+continue;
+
+  if (accessible)
+*hasassign = true;
+
+  if (!accessible || !trivial_fn_p (f))
+all_trivial = false;
+
+  /* Break early when both properties have been determined.  */
+  if (*hasassign && !all_trivial)
+break;
+}


This is iterating over all assignment operators, not just copy/move.  I
think you want copy_fn_p, here and in has_trivial_copy_p.


I assumed the cp_ in cp_assignment_operator_id meant copy.  Clearly,
even a 1,500 line test wasn't enough to catch this mistake.  I've
added more test cases.

FWIW, giving the macro a more descriptive name and/or adding
a comment above it explaining that's any assignment would help.

I also expected to see a similar thing for ctor but the closest
I could find was the three xxx_ctor_identifiers (or, conversely,
I expected to see an assignment_identifier).  I picked one based
on some of its uses, but I'm sure that it's the best choice
(ctor_identifier seems to work too).


It seems redundant to check access here and also check is
is_trivially_xible, which takes access into account.


The access check is done separately so that the right suggestion
can be made.  If the class is (non-trivially) copy assignable but
none of the copy assignment operators is public then the warning
doesn't suggest to use the copy assignment if it's not accessible.


And if we're going
to check access at all, it should consider the access of the current
scope, not just whether the function is private or protected.


Okay, I've added this.  Please see the attached update.

Martin
PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wclass-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field, maybe_warn_class_memaccess): New
	functions.
	(has_trivial_copy_assign_p, has_trivial_copy_p): Ditto.
	(build_cxx_call): Call maybe_warn_class_memaccess.

gcc/ChangeLog:

	PR c++/80560
	* dumpfile.c (dump_register): Avoid calling memset to initialize
	a class with a default ctor.
	* gcc.c (struct compiler): Remove const qualification.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* omp-low.c (lower_omp_ordered_clauses): Replace memset with
	default ctor.
	* params.h (struct param_info): Make struct members non-const.
	* tree-switch-conversion.c (emit_case_bit_tests): Replace memset
	with default initialization.
	* vec.h (vec_copy_construct, vec_default_construct): New helper
	functions.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	(vect::quick_grow_cleared): Replace memset with default ctor.
	(vect::vec_safe_grow_cleared, vec_safe_grow_cleared): Same.
	* doc/invoke.texi (-Wclass-memaccess): Document.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.
	(GTM::gtm_transaction_cp::commit): Use assignment instead of memcpy
	to copy an object.
	* method-ml.cc (orec_iterator::reinit): Avoid -Wclass-memaccess.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wclass-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 37bb236..363d104 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -804,6 +804,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wclass-memaccess
+C++ ObjC++ Var(warn_class_memaccess) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn for unsafe raw memory writes to objects of class types.
+
 Wnon-virtual-dtor
 C++ ObjC++ Var(warn_nonvdtor) Warning LangEnabledBy(C++ ObjC++,Weffc++)
 Warn about non-virtual destructors.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 51260f0..6e0951e 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8146,6 +8146,402 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
   return call;
 }
 
+/* Return the DECL of the first non-public data member of class TYPE
+   or null if none can be found.  */
+
+static tree
+first_non_public_field (tree type)
+{
+  if (!CLASS_TYPE_P (type))
+return 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On 06/06/2017 03:24 PM, Martin Sebor wrote:

+  /* Iterate over copy and move assignment overloads.  */
+
+  for (ovl_iterator oi (fns); oi; ++oi)
+{
+  tree f = *oi;
+
+  bool accessible = !access || !(TREE_PRIVATE (f) || TREE_PROTECTED (f));
+
+  /* Skip template assignment operators and deleted functions.  */
+  if (TREE_CODE (f) != FUNCTION_DECL || DECL_DELETED_FN (f))
+   continue;
+
+  if (accessible)
+   *hasassign = true;
+
+  if (!accessible || !trivial_fn_p (f))
+   all_trivial = false;
+
+  /* Break early when both properties have been determined.  */
+  if (*hasassign && !all_trivial)
+   break;
+}


This is iterating over all assignment operators, not just copy/move.  I 
think you want copy_fn_p, here and in has_trivial_copy_p.


It seems redundant to check access here and also check is 
is_trivially_xible, which takes access into account.  And if we're going 
to check access at all, it should consider the access of the current 
scope, not just whether the function is private or protected.


Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/05/2017 07:53 PM, Martin Sebor wrote:

On 06/05/2017 01:13 PM, Martin Sebor wrote:

On 06/05/2017 10:07 AM, Martin Sebor wrote:

Maybe I should use a different approach and instead of trying
to see if a function is deleted use trivially_xible to see if
it's usable.  That will mean changing the diagnostics from
"with a deleted special function" to "without trivial special
function" but it will avoid calling synthesized_method_walk
while still avoiding giving bogus suggestions.


Actually, this would check for one possible argument type and not
others, so I think it's better to keep looking at the declarations.
You
can do that by just looking them up (lookup_fnfields_slot) and
iterating
over them, you don't need to call synthesized_method_walk.


You mean using trivially_xible might check assignability or copy
constructibility from const T& but not from T& (or the other way
around), and you think both (or perhaps even other forms) should
be considered?

E.g., given:

  struct S
  {
S& operator= (const S&) = default;
void operator= (S&) = delete;
  };

  void f (S *d, const S *s)
  {
memcpy(d, s, sizeof *d);   // don't warn here
  }

  void g (S *d, S *s)
  {
memcpy(d, s, sizeof *d);   // but warn here
  }

And your suggestion is to iterate over the assignment operator
(and copy ctor) overloads for S looking for one that's trivial,
public, and not deleted?

If that's it, I was thinking of just checking for the const T&
overload (as if by using std::is_trivially_copy_assignable()).

I don't mind trying the approach you suggest.  It should be more
accurate.  I just want to make sure we're on the same page.


Actually, after some more thought and testing the approach I have
a feeling that distinguishing between the two cases above is not
what you meant.

Classes that overload copy assignment or copy ctors on the constness
of the argument are tricky to begin with and using raw memory calls
on them seems suspect and worthy of a warning.

I'm guessing what you meant by "checking for one possible argument
type and not others" is actually checking to make sure all copy
assignment (and copy ctor) overloads are trivial, not jut some,
and at least one of them is accessible.  I'll go with that unless
I hear otherwise.


Attached is an update that implements this simplified approach.


Another update, this one rebased on top of trunk (where OVL_NEXT
doesn't exist).  This version also adds a couple of minor tweaks
to handle template ctors and assignment operators, and to handle
and exercise move ctors and move assignment and const/non-const
overloads of each.  Tested on x86_64-linux.

Martin
PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wclass-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field, maybe_warn_class_memaccess): New
	functions.
	(has_trivial_copy_assign_p, has_trivial_copy_p): Ditto.
	(build_cxx_call): Call maybe_warn_class_memaccess.

gcc/ChangeLog:

	PR c++/80560
	* dumpfile.c (dump_register): Avoid calling memset to initialize
	a class with a default ctor.
	* gcc.c (struct compiler): Remove const qualification.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* omp-low.c (lower_omp_ordered_clauses): Replace memset with
	default ctor.
	* params.h (struct param_info): Make struct members non-const.
	* tree-switch-conversion.c (emit_case_bit_tests): Replace memset
	with default initialization.
	* vec.h (vec_copy_construct, vec_default_construct): New helper
	functions.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	(vect::quick_grow_cleared): Replace memset with default ctor.
	(vect::vec_safe_grow_cleared, vec_safe_grow_cleared): Same.
	* doc/invoke.texi (-Wclass-memaccess): Document.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.
	(GTM::gtm_transaction_cp::commit): Use assignment instead of memcpy
	to copy an object.
	* method-ml.cc (orec_iterator::reinit): Avoid -Wclass-memaccess.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wclass-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 37bb236..363d104 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -804,6 +804,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wclass-memaccess
+C++ ObjC++ Var(warn_class_memaccess) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn for 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/05/2017 01:13 PM, Martin Sebor wrote:

On 06/05/2017 10:07 AM, Martin Sebor wrote:

Maybe I should use a different approach and instead of trying
to see if a function is deleted use trivially_xible to see if
it's usable.  That will mean changing the diagnostics from
"with a deleted special function" to "without trivial special
function" but it will avoid calling synthesized_method_walk
while still avoiding giving bogus suggestions.


Actually, this would check for one possible argument type and not
others, so I think it's better to keep looking at the declarations.  You
can do that by just looking them up (lookup_fnfields_slot) and iterating
over them, you don't need to call synthesized_method_walk.


You mean using trivially_xible might check assignability or copy
constructibility from const T& but not from T& (or the other way
around), and you think both (or perhaps even other forms) should
be considered?

E.g., given:

  struct S
  {
S& operator= (const S&) = default;
void operator= (S&) = delete;
  };

  void f (S *d, const S *s)
  {
memcpy(d, s, sizeof *d);   // don't warn here
  }

  void g (S *d, S *s)
  {
memcpy(d, s, sizeof *d);   // but warn here
  }

And your suggestion is to iterate over the assignment operator
(and copy ctor) overloads for S looking for one that's trivial,
public, and not deleted?

If that's it, I was thinking of just checking for the const T&
overload (as if by using std::is_trivially_copy_assignable()).

I don't mind trying the approach you suggest.  It should be more
accurate.  I just want to make sure we're on the same page.


Actually, after some more thought and testing the approach I have
a feeling that distinguishing between the two cases above is not
what you meant.

Classes that overload copy assignment or copy ctors on the constness
of the argument are tricky to begin with and using raw memory calls
on them seems suspect and worthy of a warning.

I'm guessing what you meant by "checking for one possible argument
type and not others" is actually checking to make sure all copy
assignment (and copy ctor) overloads are trivial, not jut some,
and at least one of them is accessible.  I'll go with that unless
I hear otherwise.


Attached is an update that implements this simplified approach.

Martin

PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wclass-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field, maybe_warn_class_memaccess): New
	functions.
	(has_trivial_copy_assign_p, has_trivial_copy_p): Ditto.
	(build_cxx_call): Call maybe_warn_class_memaccess.

gcc/ChangeLog:

	PR c++/80560
	* dumpfile.c (dump_register): Avoid calling memset to initialize
	a class with a default ctor.
	* gcc.c (struct compiler): Remove const qualification.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* omp-low.c (lower_omp_ordered_clauses): Replace memset with
	default ctor.
	* params.h (struct param_info): Make struct members non-const.
	* tree-switch-conversion.c (emit_case_bit_tests): Replace memset
	with default initialization.
	* vec.h (vec_copy_construct, vec_default_construct): New helper
	functions.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	(vect::quick_grow_cleared): Replace memset with default ctor.
	(vect::vec_safe_grow_cleared, vec_safe_grow_cleared): Same.
	* doc/invoke.texi (-Wclass-memaccess): Document.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.
	(GTM::gtm_transaction_cp::commit): Use assignment instead of memcpy
	to copy an object.
	* method-ml.cc (orec_iterator::reinit): Avoid -Wclass-memaccess.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wclass-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9ad2f6e..f777b8f 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -792,6 +792,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wclass-memaccess
+C++ ObjC++ Var(warn_class_memaccess) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn for unsafe raw memory writes to objects of class types.
+
 Wnon-virtual-dtor
 C++ ObjC++ Var(warn_nonvdtor) Warning LangEnabledBy(C++ ObjC++,Weffc++)
 Warn about non-virtual destructors.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index e348f29..df83688 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8153,6 +8153,376 @@ build_over_call (struct 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 06/05/2017 10:07 AM, Martin Sebor wrote:

Maybe I should use a different approach and instead of trying
to see if a function is deleted use trivially_xible to see if
it's usable.  That will mean changing the diagnostics from
"with a deleted special function" to "without trivial special
function" but it will avoid calling synthesized_method_walk
while still avoiding giving bogus suggestions.


Actually, this would check for one possible argument type and not
others, so I think it's better to keep looking at the declarations.  You
can do that by just looking them up (lookup_fnfields_slot) and iterating
over them, you don't need to call synthesized_method_walk.


You mean using trivially_xible might check assignability or copy
constructibility from const T& but not from T& (or the other way
around), and you think both (or perhaps even other forms) should
be considered?

E.g., given:

  struct S
  {
S& operator= (const S&) = default;
void operator= (S&) = delete;
  };

  void f (S *d, const S *s)
  {
memcpy(d, s, sizeof *d);   // don't warn here
  }

  void g (S *d, S *s)
  {
memcpy(d, s, sizeof *d);   // but warn here
  }

And your suggestion is to iterate over the assignment operator
(and copy ctor) overloads for S looking for one that's trivial,
public, and not deleted?

If that's it, I was thinking of just checking for the const T&
overload (as if by using std::is_trivially_copy_assignable()).

I don't mind trying the approach you suggest.  It should be more
accurate.  I just want to make sure we're on the same page.


Actually, after some more thought and testing the approach I have
a feeling that distinguishing between the two cases above is not
what you meant.

Classes that overload copy assignment or copy ctors on the constness
of the argument are tricky to begin with and using raw memory calls
on them seems suspect and worthy of a warning.

I'm guessing what you meant by "checking for one possible argument
type and not others" is actually checking to make sure all copy
assignment (and copy ctor) overloads are trivial, not jut some,
and at least one of them is accessible.  I'll go with that unless
I hear otherwise.

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

Maybe I should use a different approach and instead of trying
to see if a function is deleted use trivially_xible to see if
it's usable.  That will mean changing the diagnostics from
"with a deleted special function" to "without trivial special
function" but it will avoid calling synthesized_method_walk
while still avoiding giving bogus suggestions.


Actually, this would check for one possible argument type and not
others, so I think it's better to keep looking at the declarations.  You
can do that by just looking them up (lookup_fnfields_slot) and iterating
over them, you don't need to call synthesized_method_walk.


You mean using trivially_xible might check assignability or copy
constructibility from const T& but not from T& (or the other way
around), and you think both (or perhaps even other forms) should
be considered?

E.g., given:

  struct S
  {
S& operator= (const S&) = default;
void operator= (S&) = delete;
  };

  void f (S *d, const S *s)
  {
memcpy(d, s, sizeof *d);   // don't warn here
  }

  void g (S *d, S *s)
  {
memcpy(d, s, sizeof *d);   // but warn here
  }

And your suggestion is to iterate over the assignment operator
(and copy ctor) overloads for S looking for one that's trivial,
public, and not deleted?

If that's it, I was thinking of just checking for the const T&
overload (as if by using std::is_trivially_copy_assignable()).

I don't mind trying the approach you suggest.  It should be more
accurate.  I just want to make sure we're on the same page.

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On 06/04/2017 10:01 PM, Jason Merrill wrote:

On 06/02/2017 05:28 PM, Martin Sebor wrote:

On 05/31/2017 05:34 PM, Jason Merrill wrote:

On 05/27/2017 06:44 PM, Martin Sebor wrote:

+  /* True if the class is trivial and has a trivial non-deleted copy
+ assignment, copy ctor, and default ctor, respectively.  The last
+ one isn't used to issue warnings but only to decide what suitable
+ alternatives to offer as replacements for the raw memory
operation.  */
+  bool trivial = trivial_type_p (desttype);


This comment seems out of date; there's only one variable here now.


+  /* True if the class is has a non-deleted trivial assignment.  Set


s/is//


+  /* True if the class has a (possibly deleted) trivial copy ctor.  */
+  bool trivcopy = trivially_copyable_p (desttype);


"True if the class is trivially copyable."


+  if (delassign)
+warnfmt = G_("%qD writing to an object of type %#qT with "
+ "deleted copy assignment");
+  else if (!trivassign)
+warnfmt = G_("%qD writing to an object of type %#qT with "
+ "no trivial copy assignment");
+  else if (!trivial)
+warnfmt = G_("%qD writing to an object of non-trivial "
+ "type %#qT; use assignment instead");


I'd still like the !trivial test to come first in all the memset cases,
!trivcopy in the copy cases.


The tests are in the order they're in to provide as much useful
detail in the diagnostics as necessary to understand the problem
make the suggestion meaningful.  To what end you want to change
it?


Mostly I was thinking that whether a class is trivial(ly copyable) is 
more to the point, but I guess what you have now is fine.



+static bool
+has_trivial_special_function (tree ctype, special_function_kind sfk,
+  bool *deleted_p)


This seems redundant with type_has_trivial_fn.  If that function is
giving the wrong answer for a class where all of the SFK are deleted,
let's fix that, under check_bases_and_members, rather than introduce a
new function.  I don't want to call synthesized_method_walk every time
we want to check whether a function is trivial.


A deleted special function can be trivial.


I believe that in the language, the triviality of a deleted function 
cannot be determined.  But I believe you're right about the behavior of 
type_has_trivial_fn, which is why I mentioned changing it.



Maybe I should use a different approach and instead of trying
to see if a function is deleted use trivially_xible to see if
it's usable.  That will mean changing the diagnostics from
"with a deleted special function" to "without trivial special
function" but it will avoid calling synthesized_method_walk
while still avoiding giving bogus suggestions.


Actually, this would check for one possible argument type and not 
others, so I think it's better to keep looking at the declarations.  You 
can do that by just looking them up (lookup_fnfields_slot) and iterating 
over them, you don't need to call synthesized_method_walk.


Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On 06/02/2017 05:28 PM, Martin Sebor wrote:

On 05/31/2017 05:34 PM, Jason Merrill wrote:

On 05/27/2017 06:44 PM, Martin Sebor wrote:

+  /* True if the class is trivial and has a trivial non-deleted copy
+ assignment, copy ctor, and default ctor, respectively.  The last
+ one isn't used to issue warnings but only to decide what suitable
+ alternatives to offer as replacements for the raw memory
operation.  */
+  bool trivial = trivial_type_p (desttype);


This comment seems out of date; there's only one variable here now.


+  /* True if the class is has a non-deleted trivial assignment.  Set


s/is//


+  /* True if the class has a (possibly deleted) trivial copy ctor.  */
+  bool trivcopy = trivially_copyable_p (desttype);


"True if the class is trivially copyable."


+  if (delassign)
+warnfmt = G_("%qD writing to an object of type %#qT with "
+ "deleted copy assignment");
+  else if (!trivassign)
+warnfmt = G_("%qD writing to an object of type %#qT with "
+ "no trivial copy assignment");
+  else if (!trivial)
+warnfmt = G_("%qD writing to an object of non-trivial "
+ "type %#qT; use assignment instead");


I'd still like the !trivial test to come first in all the memset cases,
!trivcopy in the copy cases.


The tests are in the order they're in to provide as much useful
detail in the diagnostics as necessary to understand the problem
make the suggestion meaningful.  To what end you want to change
it?


Mostly I was thinking that whether a class is trivial(ly copyable) is 
more to the point, but I guess what you have now is fine.



+static bool
+has_trivial_special_function (tree ctype, special_function_kind sfk,
+  bool *deleted_p)


This seems redundant with type_has_trivial_fn.  If that function is
giving the wrong answer for a class where all of the SFK are deleted,
let's fix that, under check_bases_and_members, rather than introduce a
new function.  I don't want to call synthesized_method_walk every time
we want to check whether a function is trivial.


A deleted special function can be trivial.


I believe that in the language, the triviality of a deleted function 
cannot be determined.  But I believe you're right about the behavior of 
type_has_trivial_fn, which is why I mentioned changing it.



Maybe I should use a different approach and instead of trying
to see if a function is deleted use trivially_xible to see if
it's usable.  That will mean changing the diagnostics from
"with a deleted special function" to "without trivial special
function" but it will avoid calling synthesized_method_walk
while still avoiding giving bogus suggestions.

Is this approach acceptable?


Yes, that makes sense.

Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/31/2017 05:34 PM, Jason Merrill wrote:

On 05/27/2017 06:44 PM, Martin Sebor wrote:

+  /* True if the class is trivial and has a trivial non-deleted copy
+ assignment, copy ctor, and default ctor, respectively.  The last
+ one isn't used to issue warnings but only to decide what suitable
+ alternatives to offer as replacements for the raw memory
operation.  */
+  bool trivial = trivial_type_p (desttype);


This comment seems out of date; there's only one variable here now.


+  /* True if the class is has a non-deleted trivial assignment.  Set


s/is//


+  /* True if the class has a (possibly deleted) trivial copy ctor.  */
+  bool trivcopy = trivially_copyable_p (desttype);


"True if the class is trivially copyable."


+  if (delassign)
+warnfmt = G_("%qD writing to an object of type %#qT with "
+ "deleted copy assignment");
+  else if (!trivassign)
+warnfmt = G_("%qD writing to an object of type %#qT with "
+ "no trivial copy assignment");
+  else if (!trivial)
+warnfmt = G_("%qD writing to an object of non-trivial "
+ "type %#qT; use assignment instead");


I'd still like the !trivial test to come first in all the memset cases,
!trivcopy in the copy cases.


The tests are in the order they're in to provide as much useful
detail in the diagnostics as necessary to understand the problem
make the suggestion meaningful.  To what end you want to change
it?

AFAICS, all it will accomplish is shuffle the tests around
because starting with !trivial means I'll still need to test
for delassign and delcopy before issuing the warning so that
I can include the right suggestion (and avoid suggesting to
use assignment when it's not available).


+static bool
+has_trivial_special_function (tree ctype, special_function_kind sfk,
+  bool *deleted_p)


This seems redundant with type_has_trivial_fn.  If that function is
giving the wrong answer for a class where all of the SFK are deleted,
let's fix that, under check_bases_and_members, rather than introduce a
new function.  I don't want to call synthesized_method_walk every time
we want to check whether a function is trivial.


A deleted special function can be trivial.  type_has_trivial_fn()
only determines whether a function is trivial, not whether it's deleted. 
 I chose not to use the function because when a special

function is deleted I don't want to have the warning suggest it
as an alternative to the raw memory function.

Maybe I should use a different approach and instead of trying
to see if a function is deleted use trivially_xible to see if
it's usable.  That will mean changing the diagnostics from
"with a deleted special function" to "without trivial special
function" but it will avoid calling synthesized_method_walk
while still avoiding giving bogus suggestions.

Is this approach acceptable?

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On 05/27/2017 06:44 PM, Martin Sebor wrote:

+  /* True if the class is trivial and has a trivial non-deleted copy
+ assignment, copy ctor, and default ctor, respectively.  The last
+ one isn't used to issue warnings but only to decide what suitable
+ alternatives to offer as replacements for the raw memory operation.  */
+  bool trivial = trivial_type_p (desttype);


This comment seems out of date; there's only one variable here now.


+  /* True if the class is has a non-deleted trivial assignment.  Set


s/is//


+  /* True if the class has a (possibly deleted) trivial copy ctor.  */
+  bool trivcopy = trivially_copyable_p (desttype);


"True if the class is trivially copyable."


+ if (delassign)
+   warnfmt = G_("%qD writing to an object of type %#qT with "
+"deleted copy assignment");
+ else if (!trivassign)
+   warnfmt = G_("%qD writing to an object of type %#qT with "
+"no trivial copy assignment");
+ else if (!trivial)
+   warnfmt = G_("%qD writing to an object of non-trivial "
+"type %#qT; use assignment instead");


I'd still like the !trivial test to come first in all the memset cases, 
!trivcopy in the copy cases.



+static bool
+has_trivial_special_function (tree ctype, special_function_kind sfk,
+ bool *deleted_p)


This seems redundant with type_has_trivial_fn.  If that function is 
giving the wrong answer for a class where all of the SFK are deleted, 
let's fix that, under check_bases_and_members, rather than introduce a 
new function.  I don't want to call synthesized_method_walk every time 
we want to check whether a function is trivial.


Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

+/* Return true if class TYPE meets a relaxed definition of
standard-layout.
+   In particular, the requirement that it "has all non-static data
members
+   nd bit-fields in the class and its base classes first declared in the
+   same class" is not considered.  */
+
+static bool
+almost_std_layout_p (tree type, tree *field)


It looks like this will only return false when trivial_type_p will also
return false, so its only real function is setting *field.  Can we
remove it and instead make first_non_public_field recursive?


Sure, that simplifies the diagnostic code as well.  Thanks!



Let's put the !trivial check first.


...

Again, let's put the !trivial check first; then we don't need to check
trivcopy in the !trivassign branch.


...

And here put the trivially-copyable test first, and drop checking
trivcopy in the !trivassign branch.



I found some gaps so I've reworked the checking completely.  I've
also added a check for realloc.  Please see the new patch (sorry
for the churn).


+  else if (!trivial
+   && !VOID_TYPE_P (srctype)
+   && !char_type_p (TYPE_MAIN_VARIANT (srctype))
+   && !same_type_ignoring_top_level_qualifiers_p (desttype,
+  srctype))


How can this ever be true?  If !trivial, we would have already
complained about the type being non-trivially-copyable.


The code has changed but the warning here now fires for cases like
the following (see the test(HasDefault*, ...) overload in the new
test).

  struct S { S (); };

  void f (S *s, const int a[])
  {
memcpy (s, a, sizeof *s);
  }



"array"


Fixed, thanks.




+   "assignment or copy-initialization instead",
+   fndecl, desttype, access, fld, srctype);
+}
+  else if (!zero_init_p (desttype)
+   && !VOID_TYPE_P (srctype)
+   && !char_type_p (TYPE_MAIN_VARIANT (srctype))
+   && !same_type_ignoring_top_level_qualifiers_p (desttype,
+  srctype))
+warnfmt = G_("%qD writing to an object of type %#qT containing "
+ "a pointer-to-member; use assignment or copy-"
+ "initialization instead");


This check seems unnecessary; copying from e.g. an array of
pointer-to-members would be fine.  I think we only want to check
zero_init_p for the bzero case above.


Makes sense.

Martin
PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wclass-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field, maybe_warn_class_memaccess): New
	functions.
	(build_cxx_call): Call maybe_warn_class_memaccess.
	* cp-tree.h (locate_ctor, locate_copy_ctor): Declare.
	(locate_copy_assign, has_trivial_assign, has_trivial_copy): Same.
	(has_trivial_dtor): Same.
	* method.c (locate_dtor, locate_copy_ctor): New functions.
	(locate_copy_assign, has_trivial_special_function): Same.
	(has_trivial_assign, has_trivial_copy, has_trivial_default): Same.
	(has_trivial_dtor): Same.

gcc/ChangeLog:

	PR c++/80560
	* dumpfile.c (dump_register): Avoid calling memset to initialize
	a class with a default ctor.
	* gcc.c (struct compiler): Remove const qualification.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* omp-low.c (lower_omp_ordered_clauses): Replace memset with
	default ctor.
	* params.h (struct param_info): Make struct members non-const.
	* tree-switch-conversion.c (emit_case_bit_tests): Replace memset
	with default initialization.
	* vec.h (vec_copy_construct, vec_default_construct): New helper
	functions.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	(vect::quick_grow_cleared): Replace memset with default ctor.
	(vect::vec_safe_grow_cleared, vec_safe_grow_cleared): Same.
	* doc/invoke.texi (-Wclass-memaccess): Document.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.
	(GTM::gtm_transaction_cp::commit): Use assignment instead of memcpy
	to copy an object.
	* method-ml.cc (orec_iterator::reinit): Avoid -Wclass-memaccess.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wclass-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9ad2f6e..f777b8f 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -792,6 +792,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wclass-memaccess
+C++ ObjC++ Var(warn_class_memaccess) Warning 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On 05/24/2017 04:28 PM, Martin Sebor wrote:

Attached is an updated patch with the requested changes.  I've
also renamed the option -Wclass-memaccess to avoid suggesting
that the warning focuses solely on non-trivial types, and
updated its wording and comments throughout to refer to value
initialization rather than default initialization.  Retesting
exposed another problem recently introduced into dumplfile.c
so this revision of the patch also contains a fix for that.


Emacs wasn't done saving the patch so here's the latest one with
the dumpfile.c fix.



+/* Return true if class TYPE meets a relaxed definition of standard-layout.
+   In particular, the requirement that it "has all non-static data members
+   nd bit-fields in the class and its base classes first declared in the
+   same class" is not considered.  */
+
+static bool
+almost_std_layout_p (tree type, tree *field)


It looks like this will only return false when trivial_type_p will also 
return false, so its only real function is setting *field.  Can we 
remove it and instead make first_non_public_field recursive?



+case BUILT_IN_MEMSET:
+  if (!integer_zerop (args[1]))
+   {
+ /* Diagnose setting non-copy-assignable, non-trivial or non-
+standard layout objects (in that order, since the latter
+two are subsets of one another), to (potentially) non-zero
+bytes.  Since the value of the bytes being written is unknown,
+suggest using assignment instead (if one exists).  */
+ if (!trivassign)
+   warnfmt = G_("%qD writing to an object of type %#qT without "
+"trivial copy assignment");
+ else if (!trivial)
+   warnfmt = G_("%qD writing to an object of non-trivial "
+"type %#qT; use assignment instead");


Let's put the !trivial check first.


+ else if (!stdlayout)
+   warnfmt = G_("%qD writing to an object of non-standard-layout "
+"type %#qT; use assignment instead");
+ else if (fld)
+   {
+ const char *access = TREE_PRIVATE (fld) ? "private" : "protected";
+ warned = warning_at (loc, OPT_Wclass_memaccess,
+  "%qD writing to an object of type %#qT with "
+  "%qs member %qD; use assignment instead",
+  fndecl, desttype, access, fld);
+   }
+ else if (!zero_init_p (desttype))
+   warnfmt = G_("%qD writing to an object of type %#qT containing "
+"a pointer to data member; use assignment instead");
+
+ break;
+   }
+  /* Fall through.  */
+
+case BUILT_IN_BZERO:
+  /* Similarly to the above, diagnose clearing non-trivial or non-
+standard layout objects, or objects of types with no assignmenmt.  */
+  if (!trivassign)
+   warnfmt = (trivcopy
+  ? G_("%qD clearing an object of type %#qT without trivial "
+   "copy assignment; use copy constructor instead")
+  : G_("%qD clearing an object of type %#qT without trivial "
+   "copy assignment"));
+  else if (!trivial)
+   warnfmt = G_("%qD clearing an object of non-trivial type "
+"%#qT; use assignment or value-initialization instead");


Again, let's put the !trivial check first; then we don't need to check 
trivcopy in the !trivassign branch.



+  else if (!stdlayout)
+   warnfmt = G_("%qD clearing an object of non-standard-layout type "
+"%#qT; use assignment or value-initialization instead");
+  else if (!zero_init_p (desttype))
+   warnfmt = G_("%qD clearing an object of type %#qT containing "
+"a pointer-to-member; use assignment or value-"
+"initialization instead");
+  break;
+
+case BUILT_IN_BCOPY:
+case BUILT_IN_MEMCPY:
+case BUILT_IN_MEMMOVE:
+case BUILT_IN_MEMPCPY:
+  /* Determine the type of the source object.  */
+  srctype = (TREE_CODE (args[1]) == NOP_EXPR
+? TREE_OPERAND (args[1], 0) : args[1]);
+
+  srctype = TREE_TYPE (TREE_TYPE (srctype));
+
+  if (!trivassign)
+   warnfmt = (trivcopy
+  ? G_("%qD writing to an object of type %#qT without trivial "
+   "copy assignment; use copy constructor instead")
+  :  G_("%qD writing to an object of type %#qT without trivial 
"
+"copy assignment"));
+  else if (!trivially_copyable_p (desttype))
+   warnfmt = G_("%qD writing to an object of non-trivially-copyable type "
+"%#qT; use assignment or copy-initialization instead");


And here put the trivially-copyable test first, and drop checking 
trivcopy in the !trivassign branch.



+  else if (!trivial
+  && !VOID_TYPE_P (srctype)
+

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

Attached is an updated patch with the requested changes.  I've
also renamed the option -Wclass-memaccess to avoid suggesting
that the warning focuses solely on non-trivial types, and
updated its wording and comments throughout to refer to value
initialization rather than default initialization.  Retesting
exposed another problem recently introduced into dumplfile.c
so this revision of the patch also contains a fix for that.


Emacs wasn't done saving the patch so here's the latest one with
the dumpfile.c fix.

Martin

PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wclass-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field): New function.
	(maybe_warn_class_memaccess): Same.
	(build_cxx_call): Call it.
	* cp-tree.h (has_trivial_assign, has_trivial_copy): Declare.
	* method.c (has_trivial_assign, has_trivial_copy): Define.	

gcc/ChangeLog:

	PR c++/80560
	* dumpfile.c (dump_register): Avoid calling memset to initialize
	a class with a default ctor.
	* gcc.c (struct compiler): Remove const qualification.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* omp-low.c (lower_omp_ordered_clauses): Replace memset with
	default ctor.
	* params.h (struct param_info): Make struct members non-const.
	* tree-switch-conversion.c (emit_case_bit_tests): Replace memset
	with default initialization.
	* vec.h (vec_copy_construct, vec_default_construct): New helper
	functions.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	(vect::quick_grow_cleared): Replace memset with default ctor.
	(vect::vec_safe_grow_cleared, vec_safe_grow_cleared): Same.
	* doc/invoke.texi (-Wclass-memaccess): Document.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.
	(GTM::gtm_transaction_cp::commit): Use assignment instead of memcpy
	to copy an object.
	* method-ml.cc (orec_iterator::reinit): Avoid -Wclass-memaccess.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wclass-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9ad2f6e..f777b8f 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -792,6 +792,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wclass-memaccess
+C++ ObjC++ Var(warn_class_memaccess) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn for unsafe raw memory writes to objects of class types.
+
 Wnon-virtual-dtor
 C++ ObjC++ Var(warn_nonvdtor) Warning LangEnabledBy(C++ ObjC++,Weffc++)
 Warn about non-virtual destructors.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index f14c0fa..8e9c583 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8143,6 +8143,259 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
   return call;
 }
 
+/* Return the DECL of the first non-public data member of class TYPE
+   or null if none can be found.  */
+
+static tree
+first_non_public_field (tree type)
+{
+  if (!CLASS_TYPE_P (type))
+return NULL_TREE;
+
+  for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+{
+  if (TREE_CODE (field) != FIELD_DECL)
+	continue;
+  if (TREE_STATIC (field))
+	continue;
+  if (TREE_PRIVATE (field) || TREE_PROTECTED (field))
+	return field;
+}
+
+  return NULL_TREE;
+}
+
+/* Return true if class TYPE meets a relaxed definition of standard-layout.
+   In particular, the requirement that it "has all non-static data members
+   nd bit-fields in the class and its base classes first declared in the
+   same class" is not considered.  */
+
+static bool
+almost_std_layout_p (tree type, tree *field)
+{
+  if (!CLASS_TYPE_P (type))
+return true;
+
+  if (!trivial_type_p (type))
+return false;
+
+  if (tree fld = first_non_public_field (type))
+if (!*field)
+  *field = fld;
+
+  tree binfo, base_binfo;
+
+  int i;
+
+  for (binfo = TYPE_BINFO (type), i = 0;
+   BINFO_BASE_ITERATE (binfo, i, base_binfo); i++)
+{
+  tree base = TREE_TYPE (base_binfo);
+
+  if (!almost_std_layout_p (base, field))
+	return false;
+}
+
+  return true;
+}
+
+/* Issue a warning on a call to the built-in function FNDECL if it is
+   a memory write whose destination is not an object of (something like)
+   trivial or standard layout type with a non-deleted assignment and copy
+   ctor.  Detects const correctness violations, corrupting references,
+   virtual table pointers, and bypassing 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/21/2017 10:50 PM, Jason Merrill wrote:

On Sun, May 21, 2017 at 7:59 PM, Martin Sebor  wrote:

On 05/19/2017 03:42 PM, Jason Merrill wrote:

On Fri, May 19, 2017 at 4:07 PM, Martin Sebor  wrote:

On 05/19/2017 01:07 PM, Jason Merrill wrote:

On Tue, May 16, 2017 at 5:39 PM, Martin Sebor  wrote:

On 05/16/2017 01:41 PM, Jason Merrill wrote:


I'm still not convinced we need to consider standard-layout at all.


I agree.  The patch doesn't make use of is_standard_layout_p().
It defines its own helper called almost_std_layout_p() that
combines trivial_type_p() with tests for non-public members,




That's the part that seems unnecessary.  Why do we care about
non-public members?



Because modifying them breaks encapsulation.



Not if you're clearing/copying the object as a whole.


If I take a legacy struct, make some of its members private,
and define accessors and modifiers to manipulate those members
and maintain invariants between them, I will want to check and
adjust all code that changes objects of the struct in ways that
might violate the invariants.


For a trivial type, worrying about invariants doesn't make sense to
me, since default-initialization won't establish any invariants.  And
bzero or memset(0) will have the same effect as value-initialization
(if zero_init_p (type); we probably want to check that).  If you're
going to establish invariants, I would expect you to write a default
constructor, which would make the class non-trivial.


Thanks for the zero_init_p pointer!  Let me add that to the patch
along with Pedro's suggestion to use the current C++ terminology,
retest and resubmit.

In most cases you're right that defining the default constructor
is the way to go.  There is are a couple of use cases where a ctor
tends to be avoided: when objects the class need to be initialized
statically, and where they need to be PODs.  GCC itself relies on
the latter (e.g., some of the vec templates), apparently because
it stores them in unions.  It doesn't look tome like these vec
templates maintain much of an invariant of any kind, but they
easily could.

An example of the static initialization case is an atomic class
(not necessarily std::atomic though I think this would apply to
it as well).  Its objects usually need to be statically default-
initializable (without running any ctors) but then they must be
explicitly initialized by some sort of an init call to be made
valid, and their state can only be accessed and modified via
member functions (and so their state is private).  Modifying
them by any other means, including by memset or memcpy, is
undefined.




What common use case are you concerned about that isn't more
appropriately expressed using the generated default or copy
constructor or assignment operator?



None; I am concerned about focusing the warning on code that is
actually likely to be problematic.



Hopefully the above explanation resolves that concern.  If not,
please let me know.


I still think that we shouldn't warn about zeroing out a
non-standard-layout object when value-initialization would produce the
exact same result.  If you want to keep the almost_std_layout_p check
for the non-zero memset case, that's fine.


Attached is an updated patch with the requested changes.  I've
also renamed the option -Wclass-memaccess to avoid suggesting
that the warning focuses solely on non-trivial types, and
updated its wording and comments throughout to refer to value
initialization rather than default initialization.  Retesting
exposed another problem recently introduced into dumplfile.c
so this revision of the patch also contains a fix for that.

Martin
PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wclass-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field): New function.
	(maybe_warn_class_memaccess): Same.
	(build_cxx_call): Call it.
	* cp-tree.h (has_trivial_assign, has_trivial_copy): Declare.
	* method.c (has_trivial_assign, has_trivial_copy): Define.	

gcc/ChangeLog:

	PR c++/80560
	* gcc.c (struct compiler): Remove const qualification.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* omp-low.c (lower_omp_ordered_clauses): Replace memset with
	default ctor.
	* params.h (struct param_info): Make struct members non-const.
	* tree-switch-conversion.c (emit_case_bit_tests): Replace memset
	with default initialization.
	* vec.h (vec_copy_construct, vec_default_construct): New helper
	functions.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	(vect::quick_grow_cleared): Replace memset with default ctor.
	(vect::vec_safe_grow_cleared, vec_safe_grow_cleared): Same.
	* doc/invoke.texi 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On Sun, May 21, 2017 at 7:59 PM, Martin Sebor  wrote:
> On 05/19/2017 03:42 PM, Jason Merrill wrote:
>> On Fri, May 19, 2017 at 4:07 PM, Martin Sebor  wrote:
>>> On 05/19/2017 01:07 PM, Jason Merrill wrote:
 On Tue, May 16, 2017 at 5:39 PM, Martin Sebor  wrote:
> On 05/16/2017 01:41 PM, Jason Merrill wrote:
>
>> I'm still not convinced we need to consider standard-layout at all.
>
> I agree.  The patch doesn't make use of is_standard_layout_p().
> It defines its own helper called almost_std_layout_p() that
> combines trivial_type_p() with tests for non-public members,



 That's the part that seems unnecessary.  Why do we care about
 non-public members?
>>>
>>>
>>> Because modifying them breaks encapsulation.
>>
>>
>> Not if you're clearing/copying the object as a whole.
>>
>>> If I take a legacy struct, make some of its members private,
>>> and define accessors and modifiers to manipulate those members
>>> and maintain invariants between them, I will want to check and
>>> adjust all code that changes objects of the struct in ways that
>>> might violate the invariants.
>>
>> For a trivial type, worrying about invariants doesn't make sense to
>> me, since default-initialization won't establish any invariants.  And
>> bzero or memset(0) will have the same effect as value-initialization
>> (if zero_init_p (type); we probably want to check that).  If you're
>> going to establish invariants, I would expect you to write a default
>> constructor, which would make the class non-trivial.
>
> Thanks for the zero_init_p pointer!  Let me add that to the patch
> along with Pedro's suggestion to use the current C++ terminology,
> retest and resubmit.
>
> In most cases you're right that defining the default constructor
> is the way to go.  There is are a couple of use cases where a ctor
> tends to be avoided: when objects the class need to be initialized
> statically, and where they need to be PODs.  GCC itself relies on
> the latter (e.g., some of the vec templates), apparently because
> it stores them in unions.  It doesn't look tome like these vec
> templates maintain much of an invariant of any kind, but they
> easily could.
>
> An example of the static initialization case is an atomic class
> (not necessarily std::atomic though I think this would apply to
> it as well).  Its objects usually need to be statically default-
> initializable (without running any ctors) but then they must be
> explicitly initialized by some sort of an init call to be made
> valid, and their state can only be accessed and modified via
> member functions (and so their state is private).  Modifying
> them by any other means, including by memset or memcpy, is
> undefined.
>
>>
>>> What common use case are you concerned about that isn't more
>>> appropriately expressed using the generated default or copy
>>> constructor or assignment operator?
>>
>>
>> None; I am concerned about focusing the warning on code that is
>> actually likely to be problematic.
>
>
> Hopefully the above explanation resolves that concern.  If not,
> please let me know.

I still think that we shouldn't warn about zeroing out a
non-standard-layout object when value-initialization would produce the
exact same result.  If you want to keep the almost_std_layout_p check
for the non-zero memset case, that's fine.

Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/19/2017 03:42 PM, Jason Merrill wrote:

On Fri, May 19, 2017 at 4:07 PM, Martin Sebor  wrote:

On 05/19/2017 01:07 PM, Jason Merrill wrote:


On Tue, May 16, 2017 at 5:39 PM, Martin Sebor  wrote:


On 05/16/2017 01:41 PM, Jason Merrill wrote:


I'm still not convinced we need to consider standard-layout at all.



I agree.  The patch doesn't make use of is_standard_layout_p().
It defines its own helper called almost_std_layout_p() that
combines trivial_type_p() with tests for non-public members,



That's the part that seems unnecessary.  Why do we care about
non-public members?


Because modifying them breaks encapsulation.


Not if you're clearing/copying the object as a whole.


If I take a legacy struct, make some of its members private,
and define accessors and modifiers to manipulate those members
and maintain invariants between them, I will want to check and
adjust all code that changes objects of the struct in ways that
might violate the invariants.


For a trivial type, worrying about invariants doesn't make sense to
me, since default-initialization won't establish any invariants.  And
bzero or memset(0) will have the same effect as value-initialization
(if zero_init_p (type); we probably want to check that).  If you're
going to establish invariants, I would expect you to write a default
constructor, which would make the class non-trivial.


Thanks for the zero_init_p pointer!  Let me add that to the patch
along with Pedro's suggestion to use the current C++ terminology,
retest and resubmit.

In most cases you're right that defining the default constructor
is the way to go.  There is are a couple of use cases where a ctor
tends to be avoided: when objects the class need to be initialized
statically, and where they need to be PODs.  GCC itself relies on
the latter (e.g., some of the vec templates), apparently because
it stores them in unions.  It doesn't look tome like these vec
templates maintain much of an invariant of any kind, but they
easily could.

An example of the static initialization case is an atomic class
(not necessarily std::atomic though I think this would apply to
it as well).  Its objects usually need to be statically default-
initializable (without running any ctors) but then they must be
explicitly initialized by some sort of an init call to be made
valid, and their state can only be accessed and modified via
member functions (and so their state is private).  Modifying
them by any other means, including by memset or memcpy, is
undefined.




What common use case are you concerned about that isn't more
appropriately expressed using the generated default or copy
constructor or assignment operator?


None; I am concerned about focusing the warning on code that is
actually likely to be problematic.


Hopefully the above explanation resolves that concern.  If not,
please let me know.

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On Fri, May 19, 2017 at 4:07 PM, Martin Sebor  wrote:
> On 05/19/2017 01:07 PM, Jason Merrill wrote:
>>
>> On Tue, May 16, 2017 at 5:39 PM, Martin Sebor  wrote:
>>>
>>> On 05/16/2017 01:41 PM, Jason Merrill wrote:
>>>
 I'm still not convinced we need to consider standard-layout at all.
>>>
>>>
>>> I agree.  The patch doesn't make use of is_standard_layout_p().
>>> It defines its own helper called almost_std_layout_p() that
>>> combines trivial_type_p() with tests for non-public members,
>>
>>
>> That's the part that seems unnecessary.  Why do we care about
>> non-public members?
>
> Because modifying them breaks encapsulation.

Not if you're clearing/copying the object as a whole.

> If I take a legacy struct, make some of its members private,
> and define accessors and modifiers to manipulate those members
> and maintain invariants between them, I will want to check and
> adjust all code that changes objects of the struct in ways that
> might violate the invariants.

For a trivial type, worrying about invariants doesn't make sense to
me, since default-initialization won't establish any invariants.  And
bzero or memset(0) will have the same effect as value-initialization
(if zero_init_p (type); we probably want to check that).  If you're
going to establish invariants, I would expect you to write a default
constructor, which would make the class non-trivial.

> What common use case are you concerned about that isn't more
> appropriately expressed using the generated default or copy
> constructor or assignment operator?

None; I am concerned about focusing the warning on code that is
actually likely to be problematic.

Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/19/2017 01:07 PM, Jason Merrill wrote:

On Tue, May 16, 2017 at 5:39 PM, Martin Sebor  wrote:

On 05/16/2017 01:41 PM, Jason Merrill wrote:


I'm still not convinced we need to consider standard-layout at all.


I agree.  The patch doesn't make use of is_standard_layout_p().
It defines its own helper called almost_std_layout_p() that
combines trivial_type_p() with tests for non-public members,


That's the part that seems unnecessary.  Why do we care about
non-public members?


Because modifying them breaks encapsulation.

If I take a legacy struct, make some of its members private,
and define accessors and modifiers to manipulate those members
and maintain invariants between them, I will want to check and
adjust all code that changes objects of the struct in ways that
might violate the invariants.

What common use case are you concerned about that isn't more
appropriately expressed using the generated default or copy
constructor or assignment operator?

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On Tue, May 16, 2017 at 5:39 PM, Martin Sebor  wrote:
> On 05/16/2017 01:41 PM, Jason Merrill wrote:
>
>> I'm still not convinced we need to consider standard-layout at all.
>
> I agree.  The patch doesn't make use of is_standard_layout_p().
> It defines its own helper called almost_std_layout_p() that
> combines trivial_type_p() with tests for non-public members,

That's the part that seems unnecessary.  Why do we care about
non-public members?

Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Pedro Alves]

[Meant to add this to the other email, but forgot to copy it back.]

On 05/12/2017 03:31 AM, Martin Sebor wrote:
> +  // Zeroing out is diagnosed only because it's difficult not to.
> +  // Otherwise, a class that's non-trivial only because it has
> +  // a non-trivial dtor can be safely zeroed out (that's what
> +  // default-initializing it does).

FWIW, I found references like these to "default initialization" confusing,
until I realized you're likely talking about default initialization in
pre-C++11 terms.  I.e., "new T()" with parens.   I first assumed you're
talking about default initialization like "new T;" or "T t;", but in
that case the object would not the zeroed out, hence the confusion.
It may be beneficial to go over the comments in the patch (and
particularly documentation) and talk in C++11 (and later)
terms -- i.e., say "value-initializing it" instead.

> +  T (bzero, (p, sizeof *p));// { dg-warning "bzero" }
> +  T (memset, (p, 0, sizeof *p));// { dg-warning "memset" }
> +  T (memset, (p, i, sizeof *p));// { dg-warning "memset" }

Thanks,
Pedro Alves

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Pedro Alves]

On 05/17/2017 02:55 AM, Martin Sebor wrote:
> On 05/16/2017 04:48 PM, Pedro Alves wrote:
>> On 05/16/2017 08:41 PM, Jason Merrill wrote:
>>
>>> I agree that it makes sense to
>>> check for a trivial assignment operator specifically.  I guess we want
>>> a slightly stronger "trivially copyable" that also requires a
>>> non-deleted assignment operator.
>>>
>>> It seems to me that the relevant tests are:
>>>
>>> bcopy/memcpy/memmove want trivally copyable + non-deleted assignment.
>>> bzero/memset want trivial + non-deleted assignment.
>>>
>>> I'm still not convinced we need to consider standard-layout at all.
>>
>> What do you think of warning for memset of types with references? 

Having slept, I now realize you had that covered already by the
"non-deleted assignment" requirement...  A reference data member
makes the assignment operator be implicitly deleted.  Sorry for the noise.

>> While at it, maybe the same reasoning would justify warn of memcpy/memset
>> of types with const data members?  

Ditto.

> I did this because objects with references cannot be assigned
> to (the default copy assignment is deleted).  So as a baseline
> rule, I made the warning trigger whenever a native assignment
> or copy isn't valid.  In the IMO unlikely event that a memcpy
> over a reference is intended, the warning is easy to suppress.

Agreed.

I wondered whether we'll end up wanting to distinguish these cases:

#1memcpy (T *, const T *src, size_t n);
#2.1  memcpy (T *, const char *src, size_t n);  // char, void, std::byte...
#2.2  memcpy (char *, const T *src, size_t n);  // char, void, std::byte...
#3memcpy (T *, const U *src, size_t n);

Where:
- T is a trivially copyable type that still triggers the new warning.
- U is a type unrelated to T, and is not (unsigned) char, void or std::byte.

#1 is the case that looks like copy.

#2.1 and 2.2 may well appear in type-erasing code.

#3 would look like a way to work around aliasing issues and (even though
ISTR that it's OK as GNU extension if T and U are trivial) worthy of a
warning even if T is trivial under the definition of the warning.
Reading your updated patch, I see that you warn already when T is trivial
and U is not trivial, but IIUC, not if U is also trivial, even if
unrelated to T.  Anyway, I don't really want to argue about this -- I
started writing this paragraph before actually reading the patch, and
then actually read the patch and was pleasantly surprised with what
I saw.   I think it's looking great.

> I used a similar (though not exactly the same) rationale for
> warning for const members.  They too cannot be assigned to,
> and letting memset or memcpy silently change them violates
> const-correctnes. 

*Nod*

> It's also undefined 

I'm not sure, I think there may be nuances, as usual.  AFAICS, it's generally
valid to memcpy into trivially copyable types that have const members to/from
untyped/char storage, AFAICS.  Might need to apply std::launder afterwards.  
But it
isn't clear to me, since whether memcpy starts a (trivial) object's lifetime is
up in the air, AFAIK.  But I'm not suggesting to really consider that.  The rare
specialized code will be able to work around the spurious warning.

> and the immutability
> of such members an optimization opportunity waiting to be
> exploited.
> 

*nod*

>>> +Wnon-trivial-memaccess
>>> +C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++
>>> ObjC++, Wall)
>>> +Warn for raw memory writes to objects of non-trivial types.
>>
>> May I suggest renaming the warning (and the description above) from
>> -Wnon-trivial-memaccess to something else that avoids "trivial" in
>> its name?  Because it's no longer strictly about "trivial" in the
>> standard C++ sense.  The documentation talks about "The safe way",
>> and "does not warn on safe calls", so maybe call it -Wunsafe-memaccess?
> 
> I debated whether to rename things (not just the warning but
> also the function that implements it in GCC).  Ultimately I
> decided it wasn't necessary because the rules seem close enough
> to be based on some notion of triviality and because no better
> name came to mind. -Wunsafe-memaccess might work.  The one mild
> concern I have with it is that it could suggest it might do more
> than simple type checking (e.g., buffer overflow and what not).
> Let's see what Jason thinks.

Yet another motivation of avoiding "trivial" that crossed my mind is that
you may want to enable the warning in C too, e.g., for warning about
the memcpy of types with const members.  But C as no concept
of "trivial", everything is "trivial".

>> (I spotted a couple typos in the new patch: "otherwse", "becase", btw.)
> 
> I'm a horrible typist.  I'll proofread the patch again and fix
> them up before committing it.

Thanks much for working on this.  I think this will uncover lots of
latent bugs in many codebases.  Looking forward to have this in.

Thanks,
Pedro Alves

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/16/2017 04:48 PM, Pedro Alves wrote:

On 05/16/2017 08:41 PM, Jason Merrill wrote:


I agree that it makes sense to
check for a trivial assignment operator specifically.  I guess we want
a slightly stronger "trivially copyable" that also requires a
non-deleted assignment operator.

It seems to me that the relevant tests are:

bcopy/memcpy/memmove want trivally copyable + non-deleted assignment.
bzero/memset want trivial + non-deleted assignment.

I'm still not convinced we need to consider standard-layout at all.


What do you think of warning for memset of types with references?  Since NULL
references are undefined behavior (N4659, [dcl.ref]/5), IMHO such code is quite
smelly and most likely a sign of incomplete refactoring, not design.

(My original intention for poisoning memset of standard-layout type in gdb was
mainly as proxy/approximation for "type with references".  Other properties
that make a type non-standard-layout are not as interesting to me.)

While at it, maybe the same reasoning would justify warn of memcpy/memset
of types with const data members?  memcpy of such types kind of sounds like a
recipe for subtle breakage that could only be salvaged with std::launder.
And if you know about that, you'll probably be copying objects of type T
to/from raw byte/char storage, not to/from another T.

Actually, now that I look at Martin's new patch, I see he's already warning
about const data members and references, both memcpy and memset.  I'm not
super convinced on warning about memcpy of references (unlike memset), but
I don't feel strongly against it either.  I'd be fine (FWIW) with giving it a
try and see what breaks out there.


I did this because objects with references cannot be assigned
to (the default copy assignment is deleted).  So as a baseline
rule, I made the warning trigger whenever a native assignment
or copy isn't valid.  In the IMO unlikely event that a memcpy
over a reference is intended, the warning is easy to suppress.

I expect calling memset on an object with a reference to almost
certainly be a bug since there's no way to make such a reference
usable.  The only time it might be intentional is when someone
tries to wipe out the storage before deleting the object in
the storage (e.g., in a dtor).  But that's a misuse as well
because such calls are typically eliminated, much to many
a security analyst's shock and horror.  I'd like to eventually
diagnose that too (though possibly under a different warning).

I used a similar (though not exactly the same) rationale for
warning for const members.  They too cannot be assigned to,
and letting memset or memcpy silently change them violates
const-correctnes.  It's also undefined and the immutability
of such members an optimization opportunity waiting to be
exploited.


+Wnon-trivial-memaccess
+C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ ObjC++, 
Wall)
+Warn for raw memory writes to objects of non-trivial types.


May I suggest renaming the warning (and the description above) from
-Wnon-trivial-memaccess to something else that avoids "trivial" in
its name?  Because it's no longer strictly about "trivial" in the
standard C++ sense.  The documentation talks about "The safe way",
and "does not warn on safe calls", so maybe call it -Wunsafe-memaccess?


I debated whether to rename things (not just the warning but
also the function that implements it in GCC).  Ultimately I
decided it wasn't necessary because the rules seem close enough
to be based on some notion of triviality and because no better
name came to mind. -Wunsafe-memaccess might work.  The one mild
concern I have with it is that it could suggest it might do more
than simple type checking (e.g., buffer overflow and what not).
Let's see what Jason thinks.


(I spotted a couple typos in the new patch: "otherwse", "becase", btw.)


I'm a horrible typist.  I'll proofread the patch again and fix
them up before committing it.

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Pedro Alves]

On 05/16/2017 08:41 PM, Jason Merrill wrote:

> I agree that it makes sense to
> check for a trivial assignment operator specifically.  I guess we want
> a slightly stronger "trivially copyable" that also requires a
> non-deleted assignment operator.
> 
> It seems to me that the relevant tests are:
> 
> bcopy/memcpy/memmove want trivally copyable + non-deleted assignment.
> bzero/memset want trivial + non-deleted assignment.
> 
> I'm still not convinced we need to consider standard-layout at all.

What do you think of warning for memset of types with references?  Since NULL
references are undefined behavior (N4659, [dcl.ref]/5), IMHO such code is quite
smelly and most likely a sign of incomplete refactoring, not design.

(My original intention for poisoning memset of standard-layout type in gdb was
mainly as proxy/approximation for "type with references".  Other properties
that make a type non-standard-layout are not as interesting to me.)

While at it, maybe the same reasoning would justify warn of memcpy/memset
of types with const data members?  memcpy of such types kind of sounds like a
recipe for subtle breakage that could only be salvaged with std::launder.
And if you know about that, you'll probably be copying objects of type T
to/from raw byte/char storage, not to/from another T.

Actually, now that I look at Martin's new patch, I see he's already warning
about const data members and references, both memcpy and memset.  I'm not
super convinced on warning about memcpy of references (unlike memset), but
I don't feel strongly against it either.  I'd be fine (FWIW) with giving it a
try and see what breaks out there.

> +Wnon-trivial-memaccess
> +C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ ObjC++, 
> Wall)
> +Warn for raw memory writes to objects of non-trivial types.

May I suggest renaming the warning (and the description above) from
-Wnon-trivial-memaccess to something else that avoids "trivial" in
its name?  Because it's no longer strictly about "trivial" in the
standard C++ sense.  The documentation talks about "The safe way",
and "does not warn on safe calls", so maybe call it -Wunsafe-memaccess?

(I spotted a couple typos in the new patch: "otherwse", "becase", btw.)

Thanks,
Pedro Alves

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/16/2017 01:41 PM, Jason Merrill wrote:

On Thu, May 11, 2017 at 12:23 PM, Martin Sebor  wrote:

The challenge with using memcpy or memset with class types is
figuring out if it's being called to copy-construct a new object
or assign a new value to an existing one.  In general it's not
possible to tell so the conservative assumption is that it's
the latter.

Because of that, relying on the trivially copyable property to
determine whether it's safe to assign a new value to an object
is not sufficient (or even necessary).  The class must be at
a minimum trivially assignable.  But it turns out that even
trivial assignability as defined by C++ isn't enough.  A class
with a const data member or a member of a reference type is
considered "trivially assignable" but its copy assignment is
deleted, and so it can't be assigned to.   Calling memset or
memcpy to write over an object of such a class can violate
the const invariant or corrupt the reference, respectively.


Yes, "trivially copyable" elides the differences between
initialization and assignment context.  I agree that it makes sense to
check for a trivial assignment operator specifically.  I guess we want
a slightly stronger "trivially copyable" that also requires a
non-deleted assignment operator.

It seems to me that the relevant tests are:

bcopy/memcpy/memmove want trivally copyable + non-deleted assignment.
bzero/memset want trivial + non-deleted assignment.


I think that's very close to what the patch does.

bzero/memset: expects a trivial class with no private
members and with a non-deleted trivial assignment.  That looks
the same as what you have above.

bcopy/memcpy/memmove: expects the same plus trivially copyable,
except that the class may be non-trivial if the source of the
copy is any of void*, [signed or unsigned] char*, or a pointer
to the class itself.  In that case, the byte size must either
be non-constant or a multiple of the size of the object, to
help detect inadvertent partial copies.

The test for class triviality is to detect misusing the functions
to initialize (construct) an object of a class with a user-defined
default ctor by copying bytes into it from an object of unrelated
type (void* and char* are allowed for serialization).

I did forget about bcopy.  Let me add it.



I'm still not convinced we need to consider standard-layout at all.


I agree.  The patch doesn't make use of is_standard_layout_p().
It defines its own helper called almost_std_layout_p() that
combines trivial_type_p() with tests for non-public members,
ignoring the requirement that all members of a derived standard
layout class must be defined in the same base (or derived) class.

Is there something you'd like to see done differently in the
latest revision of the patch?

https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00976.html

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

On Thu, May 11, 2017 at 12:23 PM, Martin Sebor  wrote:
> The challenge with using memcpy or memset with class types is
> figuring out if it's being called to copy-construct a new object
> or assign a new value to an existing one.  In general it's not
> possible to tell so the conservative assumption is that it's
> the latter.
>
> Because of that, relying on the trivially copyable property to
> determine whether it's safe to assign a new value to an object
> is not sufficient (or even necessary).  The class must be at
> a minimum trivially assignable.  But it turns out that even
> trivial assignability as defined by C++ isn't enough.  A class
> with a const data member or a member of a reference type is
> considered "trivially assignable" but its copy assignment is
> deleted, and so it can't be assigned to.   Calling memset or
> memcpy to write over an object of such a class can violate
> the const invariant or corrupt the reference, respectively.

Yes, "trivially copyable" elides the differences between
initialization and assignment context.  I agree that it makes sense to
check for a trivial assignment operator specifically.  I guess we want
a slightly stronger "trivially copyable" that also requires a
non-deleted assignment operator.

It seems to me that the relevant tests are:

bcopy/memcpy/memmove want trivally copyable + non-deleted assignment.
bzero/memset want trivial + non-deleted assignment.

I'm still not convinced we need to consider standard-layout at all.

Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

I found a few more instances of the warning and a couple of bugs
in the fixes for it.  The attached update corrects those as well.

On 05/11/2017 02:01 PM, Martin Sebor wrote:

Attached is revision 2 of the patch that addresses of the feedback
I got from Pedro (thanks again).  As I explained in my response
(https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00925.html) it's not
entirely practical to rely on the strict interpretation of the C++
definition of type triviality or the standard layout property to
decide what to warn on.  Doing so results in too much noise for
safe code, and misses some interesting problems.

The warning instead implements its own slightly modified notion of
these concepts to detect common abuses of the functions without
flagging those that are safe in GCC (e.g, calling memset to clear
objects that would be standard layout if they didn't declare data
members in both derived and base classes).

The new warning detected a total of 74 instances (13 unique) in 8
GCC files.  I've gone through and resolved them by either replacing
memcpy/memset with invocations of the copy ctor or copy assignment
operator, or in a couple of instances by removing const quialifiers
from members of the flagged types.

-Wnon-trivial-memaccess Instances:
  /opt/notnfs/msebor/src/gcc/80560/gcc/gcc.c:2319
  /opt/notnfs/msebor/src/gcc/80560/gcc/gcc.c:7468
  /opt/notnfs/msebor/src/gcc/80560/gcc/genattrtab.c:4707
  /opt/notnfs/msebor/src/gcc/80560/gcc/hash-table.h:806
  /opt/notnfs/msebor/src/gcc/80560/gcc/ipa-cp.c:1491
  /opt/notnfs/msebor/src/gcc/80560/gcc/ipa-prop.c:3733
  /opt/notnfs/msebor/src/gcc/80560/gcc/omp-low.c:6295
  /opt/notnfs/msebor/src/gcc/80560/gcc/params.c:74
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:1098
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:1448
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:1613
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:618
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:841

Martin


PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wnon-trivial-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field): New function.
	(maybe_warn_nontrivial_memaccess): Same.
	(build_cxx_call): Call it.
	* cp-tree.h (has_trivial_assign, has_trivial_copy): Declare.
	* method.c (has_trivial_assign, has_trivial_copy): Define.	

gcc/ChangeLog:

	PR c++/80560
	* gcc.c (struct compiler): Remove const qualification.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* omp-low.c (lower_omp_ordered_clauses): Replace memset with
	default ctor.
	* params.h (struct param_info): Make struct members non-const.
	* tree-switch-conversion.c (emit_case_bit_tests): Replace memset
	with default initialization.
	* vec.h (vec_copy_construct, vec_default_construct): New helper
	functions.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	(vect::quick_grow_cleared): Replace memset with default ctor.
	(vect::vec_safe_grow_cleared, vec_safe_grow_cleared): Same.
	* doc/invoke.texi (-Wnon-trivial-memaccess): Document.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.
	(GTM::gtm_transaction_cp::commit): Use assignment instead of memcpy
	to copy an object.
	* method-ml.cc (orec_iterator::reinit): Avoid -Wnon-trivial-memaccess.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wnon-trivial-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 6ecbfca..c32f8a9 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -792,6 +792,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wnon-trivial-memaccess
+C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn for raw memory writes to objects of non-trivial types.
+
 Wnon-virtual-dtor
 C++ ObjC++ Var(warn_nonvdtor) Warning LangEnabledBy(C++ ObjC++,Weffc++)
 Warn about non-virtual destructors.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c15b8e4..4bf984d 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8152,6 +8152,245 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
   return call;
 }
 
+/* Return the DECL of the first non-public data member of class TYPE
+   or null if none can be found.  */
+
+static tree
+first_non_public_field (tree type)
+{
+  if (!CLASS_TYPE_P (type))
+return NULL_TREE;
+
+  for (tree field = TYPE_FIELDS (type); field; field = 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

Attached is revision 2 of the patch that addresses of the feedback
I got from Pedro (thanks again).  As I explained in my response
(https://gcc.gnu.org/ml/gcc-patches/2017-05/msg00925.html) it's not
entirely practical to rely on the strict interpretation of the C++
definition of type triviality or the standard layout property to
decide what to warn on.  Doing so results in too much noise for
safe code, and misses some interesting problems.

The warning instead implements its own slightly modified notion of
these concepts to detect common abuses of the functions without
flagging those that are safe in GCC (e.g, calling memset to clear
objects that would be standard layout if they didn't declare data
members in both derived and base classes).

The new warning detected a total of 74 instances (13 unique) in 8
GCC files.  I've gone through and resolved them by either replacing
memcpy/memset with invocations of the copy ctor or copy assignment
operator, or in a couple of instances by removing const quialifiers
from members of the flagged types.

-Wnon-trivial-memaccess Instances:
  /opt/notnfs/msebor/src/gcc/80560/gcc/gcc.c:2319
  /opt/notnfs/msebor/src/gcc/80560/gcc/gcc.c:7468
  /opt/notnfs/msebor/src/gcc/80560/gcc/genattrtab.c:4707
  /opt/notnfs/msebor/src/gcc/80560/gcc/hash-table.h:806
  /opt/notnfs/msebor/src/gcc/80560/gcc/ipa-cp.c:1491
  /opt/notnfs/msebor/src/gcc/80560/gcc/ipa-prop.c:3733
  /opt/notnfs/msebor/src/gcc/80560/gcc/omp-low.c:6295
  /opt/notnfs/msebor/src/gcc/80560/gcc/params.c:74
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:1098
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:1448
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:1613
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:618
  /opt/notnfs/msebor/src/gcc/80560/gcc/vec.h:841

Martin
PR c++/80560 - warn on undefined memory operations involving non-trivial types

gcc/c-family/ChangeLog:

	PR c++/80560
	* c.opt (-Wnon-trivial-memaccess): New option.

gcc/cp/ChangeLog:

	PR c++/80560
	* call.c (first_non_public_field): New function.
	(maybe_warn_nontrivial_memaccess): Same.
	(build_cxx_call): Call it.
	* cp-tree.h (has_trivial_assign, has_trivial_copy): Declare.
	* method.c (has_trivial_assign, has_trivial_copy): Define.	

gcc/ChangeLog:

	PR c++/80560
	* gcc.c (n_compilers, n_default_compilers): Make unsigned.
	(read_specs): Replace memset with assignment.
	(driver::set_up_specs): Replace memset with ctor.
	(driver::finalize): Use unsigned instead of int.
	* genattrtab.c (gen_insn_reserv): Replace memset with initialization.
	* hash-table.h: Ditto.
	* ipa-cp.c (allocate_and_init_ipcp_value): Replace memset with
	  assignment.
	* ipa-prop.c (ipa_free_edge_args_substructures): Ditto.
	* params.h (struct param_info): Make struct members non-const.
	* vec.h (vec_copy_construct): New helper function.
	(vec::copy, vec::splice, vec::reserve): Replace memcpy
	with vec_copy_construct.
	* doc/invoke.texi (-Wnon-trivial-memaccess): Document.

libcpp/ChangeLog:

	PR c++/80560
	* line-map.c (line_maps::~line_maps): Avoid calling htab_delete
	with a null pointer.
	(linemap_init): Avoid calling memset on an object of a non-trivial
	type.

libitm/ChangeLog:

	PR c++/80560
	* beginend.cc (GTM::gtm_thread::rollback): Avoid calling memset
	on an object of a non-trivial type.
	(GTM::gtm_transaction_cp::commit): Use assignment instead of memcpy
	to copy an object.
	* method-ml.cc (orec_iterator::reinit): Avoid -Wnon-trivial-memaccess.

gcc/testsuite/ChangeLog:

	PR c++/80560
	* g++.dg/Wnon-trivial-memaccess.C: New test.

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 9ad2f6e..dae5e80 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -792,6 +792,10 @@ Wnon-template-friend
 C++ ObjC++ Var(warn_nontemplate_friend) Init(1) Warning
 Warn when non-templatized friend functions are declared within a template.
 
+Wnon-trivial-memaccess
+C++ ObjC++ Var(warn_nontrival_memaccess) Warning LangEnabledBy(C++ ObjC++, Wall)
+Warn for raw memory writes to objects of non-trivial types.
+
 Wnon-virtual-dtor
 C++ ObjC++ Var(warn_nonvdtor) Warning LangEnabledBy(C++ ObjC++,Weffc++)
 Warn about non-virtual destructors.
diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index e348f29..44bc438 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -8153,6 +8153,246 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
   return call;
 }
 
+/* Return the DECL of the first non-public data member of class TYPE
+   or null if none can be found.  */
+
+static tree
+first_non_public_field (tree type)
+{
+  if (!CLASS_TYPE_P (type))
+return NULL_TREE;
+
+  for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
+{
+  if (TREE_CODE (field) != FIELD_DECL)
+	continue;
+  if (TREE_STATIC (field))
+	continue;
+  if (TREE_PRIVATE (field) || TREE_PROTECTED (field))
+	return field;
+}
+
+  return NULL_TREE;
+}
+
+/* Return true if class TYPE meets a relaxed definition of standard-layout.
+   In particular, the requirement that it 

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 05/11/2017 10:34 AM, Jakub Jelinek wrote:

On Thu, May 11, 2017 at 10:23:48AM -0600, Martin Sebor wrote:

Unlike in C, the preferred way to initialize objects in C++
is to use some form of initialization (as opposed to memset).
The preferred way to copy objects is using the copy ctor or
assignment operator (as opposed to memcpy).  Using the special
member functions is clearer, less prone to mistakes and so
safer, and in some cases can also be more efficient.  Memcpy
and memset should be reserved for manipulating raw storage,
not typed objects.


But optimizers should be able to turn the copy ctors/assignment operators
or default ctors into memcpy or memset where possible, making
it efficient again.  And that is something that doesn't work well yet.


I was referring to GCC not expanding inline calls to memcpy with
non-const size, as in function g below, whereas in f, the loop is
fully inlined for (likely) better performance.  This is meant to
be close to the GDB example of a vector-like class that uses
memcpy (or memmove) instead of ordinary assignment.

  struct S { char a[32]; };

  void f (S *p, const S *q, unsigned n)
  {
while (n--)
  *p++ = *q++;
  }

  void g (S *p, const S *q, unsigned n)
  {
__builtin_memcpy (p, q, sizeof *q * n);
  }

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jakub Jelinek]

On Thu, May 11, 2017 at 10:23:48AM -0600, Martin Sebor wrote:
> Unlike in C, the preferred way to initialize objects in C++
> is to use some form of initialization (as opposed to memset).
> The preferred way to copy objects is using the copy ctor or
> assignment operator (as opposed to memcpy).  Using the special
> member functions is clearer, less prone to mistakes and so
> safer, and in some cases can also be more efficient.  Memcpy
> and memset should be reserved for manipulating raw storage,
> not typed objects.

But optimizers should be able to turn the copy ctors/assignment operators
or default ctors into memcpy or memset where possible, making
it efficient again.  And that is something that doesn't work well yet.

Jakub

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 04/30/2017 02:02 PM, Pedro Alves wrote:

Hi Martin,

Thanks much for doing this.  A few comments below, in light of my
experience doing the equivalent checks in the gdb patch linked below,
using standard C++11.


Thanks for the feedback!  It gave me quite a bit to think about.

The challenge with using memcpy or memset with class types is
figuring out if it's being called to copy-construct a new object
or assign a new value to an existing one.  In general it's not
possible to tell so the conservative assumption is that it's
the latter.

Because of that, relying on the trivially copyable property to
determine whether it's safe to assign a new value to an object
is not sufficient (or even necessary).  The class must be at
a minimum trivially assignable.  But it turns out that even
trivial assignability as defined by C++ isn't enough.  A class
with a const data member or a member of a reference type is
considered "trivially assignable" but its copy assignment is
deleted, and so it can't be assigned to.   Calling memset or
memcpy to write over an object of such a class can violate
the const invariant or corrupt the reference, respectively.

On the other hand, relying on the standard layout property
to decide whether or not calling memset on an object is safe,
while on the surface reasonable, is at the same time too strict
and overly permissive.  It's too strict because it warns for
calls where the destination is an object of a trivial derived
class that declares data members in one of its bases as well as
in the derived class (GCC has code like that).  It's not strict
enough because it doesn't catch cases where the class contains
only private or only protected data members (GCC is guilty of
abusing memset to violate encapsulation like that as well).

That said, I'm testing a solution that overcomes these problems.
I adjusted it so it doesn't warn on the GDB code in your example
(or any GDB code on trunk), even though in my opinion "tricks"
like that would best be avoided in favor of safer alternatives.

Unlike in C, the preferred way to initialize objects in C++
is to use some form of initialization (as opposed to memset).
The preferred way to copy objects is using the copy ctor or
assignment operator (as opposed to memcpy).  Using the special
member functions is clearer, less prone to mistakes and so
safer, and in some cases can also be more efficient.  Memcpy
and memset should be reserved for manipulating raw storage,
not typed objects.

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Martin Sebor]

On 04/30/2017 05:39 PM, Joseph Myers wrote:

On Sat, 29 Apr 2017, Martin Sebor wrote:


+The safe way to either initialize or "reset" objects of non-trivial


Should use TeX quotes in Texinfo files, ``reset''.


Heh :) I wrestled with Emacs to get rid of those,  It kept replacing
my plain quotes with the TeX kind but in the end I won.  Sounds like
I should have trusted it.  Let me restore it in the follow-on patch.

Martin

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Jason Merrill]

Pedro's suggestions all sound good to me.

Jason

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Joseph Myers]

On Sat, 29 Apr 2017, Martin Sebor wrote:

> +The safe way to either initialize or "reset" objects of non-trivial

Should use TeX quotes in Texinfo files, ``reset''.

-- 
Joseph S. Myers
jos...@codesourcery.com

Re: [PATCH] warn on mem calls modifying objects of non-trivial types (PR 80560)

[Pedro Alves]

Hi Martin,

Thanks much for doing this.  A few comments below, in light of my
experience doing the equivalent checks in the gdb patch linked below,
using standard C++11.

On 04/29/2017 09:09 PM, Martin Sebor wrote:
> Calling memset, memcpy, or similar to write to an object of
> a non-trivial type (such as one that defines a ctor or dtor,
> or has such a member) can break the invariants otherwise
> maintained by the class and cause undefined behavior.
> 
> The motivating example that prompted this work was a review of
> a change that added to a plain old struct a new member with a ctor
> and dtor (in this instance the member was of type std::vector).
> 
> To help catch problems of this sort some projects (such as GDB)
> have apparently even devised their own clever solutions to detect
> them: https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html.
> 
> The attached patch adds a new warning, -Wnon-trivial-memaccess,
> that has GCC detect these mistakes.  The patch also fixes up
> a handful of instances of the problem in GCC.  These instances
> correspond to the two patterns below:
> 
>   struct A
>   {
> void *p;
> void foo (int n) { p = malloc (n); }
> ~A () { free (p); }
>   };
> 
>   void init (A *a)
>   {
> memset (a, 0, sizeof *a);
>   }
> 
> and
> 
>   struct B
>   {
> int i;
> ~A ();
>   };

(typo: "~B ();")

> 
>   void copy (B *p, const B *q)
>   {
> memcpy (p, q, sizeof *p);
> ...
>}
> 

IMO the check should be relaxed from "type is trivial" to "type is
trivially copyable" (which is what the gdb detection at
https://sourceware.org/ml/gdb-patches/2017-04/msg00378.html
uses for memcpy/memmove).  Checking that the destination is trivial is
going to generate false positives -- specifically, [basic-types]/3
specifies that it's fine to memcpy trivially _copyable_ types, not
trivial types.  A type can be both non-trivial and trivially copyable
at the same time.  For example, this compiles, but triggers
your new warning:

#include 
#include 
#include 

struct NonTrivialButTriviallyCopyable
{
  NonTrivialButTriviallyCopyable () : i (0) {}
  int i;
};

static_assert (!std::is_trivial::value, "");
static_assert 
(std::is_trivially_copyable::value, "");

void copy (NonTrivialButTriviallyCopyable *dst, NonTrivialButTriviallyCopyable 
*src)
{
  memcpy (dst, src, sizeof (*src));
}

$ /opt/gcc/bin/g++ -std=gnu++11 trivial-warn.cc -o trivial-warn -g3 -O0 -Wall 
-Wextra -c
trivial-warn.cc: In function ‘void copy(NonTrivialButTriviallyCopyable*, 
NonTrivialButTriviallyCopyable*)’:
trivial-warn.cc:16:34: warning: calling ‘void* memcpy(void*, const void*, 
size_t)’ with a pointer to a non-trivial type ‘struct 
NonTrivialButTriviallyCopyable’ [-Wnon-trivial-memaccess]
   memcpy (dst, src, sizeof (*src));
  ^
$

Implementations of vector-like classes can very well (and are
encouraged) to make use of std::is_trivially_copyable to know whether
they can copy a range of elements to new storage
using memcpy/memmove/mempcpy.

Running your patch against GDB trips on such a case:

src/gdb/btrace.h: In function ‘btrace_insn_s* 
VEC_btrace_insn_s_quick_insert(VEC_btrace_insn_s*, unsigned int, const 
btrace_insn_s*, const char*, unsigned int)’:
src/gdb/common/vec.h:948:62: error: calling ‘void* memmove(void*, const void*, 
size_t)’ with a pointer to a non-trivial type ‘btrace_insn_s {aka struct 
btrace_insn}’ [-Werror=non-trivial-memaccess]
   memmove (slot_ + 1, slot_, (vec_->num++ - ix_) * sizeof (T));\
  ^

There is nothing wrong with the code being warned here.
While "struct btrace_insn" is trivial (has a user-provided default
ctor), it is still trivially copyable.

Now, this gdb code is using the old VEC (originated from
gcc's C days, it's not the current C++fied VEC implementation),
but the point is that any other random vector-like container out there
is free to optimize copy of a range of non-trivial but trivially
copyable types using memcpy/memmove.

Note that libstdc++ does not actually do that optimization, but
that's just a missed optimization, see PR libstdc++/68350 [1]
"std::uninitialized_copy overly restrictive for
trivially_copyable types".  (libstdc++'s std::vector defers
copy to std::unitialized_copy.)

[1] - https://gcc.gnu.org/bugzilla/show_bug.cgi?id=68350

> These aren't undefined and the patch could be tweaked to allow
> them.  

I think they're undefined because the types are not trivially
copyable (non-trivial destructor with side effects).

> I decided not to invest effort into it because, although
> not strictly erroneous, I think they represent poor practice.

I think that for my suggestion, all you really need is use
call trivially_copyable_p instead of trivial_type_p, for
memcpy/memmove/mempcpy at least.

For memset, I'd suggest to go the other direction actually, and
instead of relaxing the trivial check, to tighten it, by warning about
memset'ting objects of