Re: [committed] Fix -Werror=class-memaccess failures in jit testsuite (PR jit/81144)

Re: [committed] Fix -Werror=class-memaccess failures in jit testsuite (PR jit/81144)

[Martin Sebor]

On 06/20/2017 03:25 PM, David Malcolm wrote:
> This patch fixes a couple of failures of the form:
>
>   error: 'void* memset(void*, int, size_t)' clearing an object of non-trivial
>     type 'struct quadratic_test'; use assignment or value-initialization
>     instead [-Werror=class-memaccess]
>   note: 'struct quadratic_test' declared here
>   cc1plus: all warnings being treated as errors
>
> seen within the jit testsuite, by using zero-initialization instead
> of memset.
>
> (presumably introduced by r249234 aka a324786b4ded9047d05463b4bce9d238b6c6b3ef)
>
> Successfully tested on x86_64-pc-linux-gnu; takes jit.sum from:
>   # of expected passes            9211
>   # of unexpected failures        2
> to:
>   # of expected passes            9349
>
> Martin: it's unclear to me what the benefit of the warning is for these
> cases.  AIUI, it's complaining because the code is calling
> the default ctor for struct quadratic_test, and then that object is
> being clobbered by the memset.
> But if I'm reading things right, the default ctor for this struct
> zero-initializes all fields.  Can't the compiler simply optimize away
> the redundant memset, and not issue a warning?

-Wclass-memaccess is issued because struct quadratic_test contains
members of classes that define a default ctor to initialize their
private members.  The premise behind the warning is that objects
of types with user-defined default and copy ctors should be
initialized by making use of their ctors, and those with private
data members manipulated via member functions rather than by
directly modifying their raw representation.  Using memset to
bypass the default ctor doesn't begin the lifetime of an object,
can violate invariants set up by it, and using it to overwrite
private members breaks encapsulation.  Examples of especially
insidious errors include overwriting const data, references, or
pointer to data members for which zero-initialization isn't
the same as clearing their bytes.

The warning runs early on in the C++ front end and has no knowledge
of either the effects of the type's ctors, dtor, and copy assignment
operator, or whether the raw memory function is called in lieu of
initializing an object (e.g., in storage obtained from malloc or
operator new), or as a shortcut to zero out its members, or when
zeroing them out happens to be safe and doesn't actually do any
of those bad things I mentioned above.

That said, I'm sorry (and a little surprised) that I missed these
errors in my tests.  I thought I had all the languages covered by
using

   --enable-languages=all,ada,c,c++,fortran,go,lto,objc,obj-c++

but I guess jit still isn't implied by all, even after Nathan's
recent change to it.  Let me add jit to my script (IIRC, I once
had it there but it was causing some trouble and I took it out.)

Martin

Re: [committed] Fix -Werror=class-memaccess failures in jit testsuite (PR jit/81144)

[David Malcolm]

On Tue, 2017-06-20 at 17:15 -0600, Martin Sebor wrote:
> On 06/20/2017 03:25 PM, David Malcolm wrote:
> > This patch fixes a couple of failures of the form:
> > 
> >   error: 'void* memset(void*, int, size_t)' clearing an object of
> > non-trivial
> >     type 'struct quadratic_test'; use assignment or value
> > -initialization
> >     instead [-Werror=class-memaccess]
> >   note: 'struct quadratic_test' declared here
> >   cc1plus: all warnings being treated as errors
> > 
> > seen within the jit testsuite, by using zero-initialization instead
> > of memset.
> > 
> > (presumably introduced by r249234 aka
> > a324786b4ded9047d05463b4bce9d238b6c6b3ef)
> > 
> > Successfully tested on x86_64-pc-linux-gnu; takes jit.sum from:
> >   # of expected passes            9211
> >   # of unexpected failures        2
> > to:
> >   # of expected passes            9349
> > 
> > Martin: it's unclear to me what the benefit of the warning is for
> > these
> > cases.  AIUI, it's complaining because the code is calling
> > the default ctor for struct quadratic_test, and then that object is
> > being clobbered by the memset.
> > But if I'm reading things right, the default ctor for this struct
> > zero-initializes all fields.  Can't the compiler simply optimize
> > away
> > the redundant memset, and not issue a warning?

Thanks for the info.

> -Wclass-memaccess is issued because struct quadratic_test contains
> members of classes that define a default ctor to initialize their
> private members.  
> The premise behind the warning is that objects
> of types with user-defined default and copy ctors should be
> initialized by making use of their ctors, and those with private
> data members manipulated via member functions rather than by
> directly modifying their raw representation.  Using memset to
> bypass the default ctor doesn't begin the lifetime of an object,
> can violate invariants set up by it, and using it to overwrite
> private members breaks encapsulation.  Examples of especially
> insidious errors include overwriting const data, references, or
> pointer to data members for which zero-initialization isn't
> the same as clearing their bytes.

If I'm reading my code correctly, all of the default ctors of all of
the members of this struct are "merely" initializing the pointer they
wrap to NULL.

So the ctors are initializing everything to NULL, and then the memset
redundant re-init's everything to 0 bits (I guess I was going for a
"belt and braces" approach to ensure that things are initialized).

> The warning runs early on in the C++ front end and has no knowledge
> of either the effects of the type's ctors, dtor, and copy assignment
> operator, or whether the raw memory function is called in lieu of
> initializing an object (e.g., in storage obtained from malloc or
> operator new), or as a shortcut to zero out its members, or when
> zeroing them out happens to be safe and doesn't actually do any
> of those bad things I mentioned above.

Aha: so at the place where the warning runs it's not possible to access
the ctors and tell that they're assigning NULL everywhere?

Might it be possible to convert the warning to work in a two-phase way
where it first gathers up a vec of suspicious-looking modifications,
and then flushes them later, filtering against ctor information when it
has the latter?  (so that we don't have to warn for this case at
-Wall?)

Alternatively maybe this is PEBCAK at my end; if so, maybe a case for adding this to the changes.html page?  (and maybe adding some notes on workarounds there, and/or to invoke.texi?)

> 
> That said, I'm sorry (and a little surprised) that I missed these
> errors in my tests.  I thought I had all the languages covered by
> using
> 
>    --enable-languages=all,ada,c,c++,fortran,go,lto,objc,obj-c++
> 
> but I guess jit still isn't implied by all, even after Nathan's
> recent change to it.  Let me add jit to my script (IIRC, I once
> had it there but it was causing some trouble and I took it out.)

Reading r248454 (aka 01b4453cde8f1871495955298043d9fb589e4a36), it
looks like "jit" is only included in "all" if you also pass
  --enable-host-shared
Presumably that's what happened.  Bother.

Thanks; hope this is constructive.
Dave

[committed] Fix -Werror=class-memaccess failures in jit testsuite (PR jit/81144)

[David Malcolm]

This patch fixes a couple of failures of the form:

  error: 'void* memset(void*, int, size_t)' clearing an object of non-trivial
    type 'struct quadratic_test'; use assignment or value-initialization
    instead [-Werror=class-memaccess]
  note: 'struct quadratic_test' declared here
  cc1plus: all warnings being treated as errors

seen within the jit testsuite, by using zero-initialization instead
of memset.

(presumably introduced by r249234 aka a324786b4ded9047d05463b4bce9d238b6c6b3ef)

Successfully tested on x86_64-pc-linux-gnu; takes jit.sum from:
  # of expected passes            9211
  # of unexpected failures        2
to:
  # of expected passes            9349

Martin: it's unclear to me what the benefit of the warning is for these
cases.  AIUI, it's complaining because the code is calling
the default ctor for struct quadratic_test, and then that object is
being clobbered by the memset.
But if I'm reading things right, the default ctor for this struct
zero-initializes all fields.  Can't the compiler simply optimize away
the redundant memset, and not issue a warning?

gcc/testsuite/ChangeLog:
	PR jit/81144
	* jit.dg/test-operator-overloading.cc (make_test_quadratic): Replace
	memset call with zero-initialization.
	* jit.dg/test-quadratic.cc (make_test_quadratic): Likewise.
---
 gcc/testsuite/jit.dg/test-operator-overloading.cc | 3 +--
 gcc/testsuite/jit.dg/test-quadratic.cc            | 3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/gcc/testsuite/jit.dg/test-operator-overloading.cc b/gcc/testsuite/jit.dg/test-operator-overloading.cc
index cbb1e98..f57b3fc 100644
--- a/gcc/testsuite/jit.dg/test-operator-overloading.cc
+++ b/gcc/testsuite/jit.dg/test-operator-overloading.cc
@@ -272,8 +272,7 @@ make_test_quadratic (quadratic_test &testcase)
 void
 create_code (gcc_jit_context *ctxt, void *user_data)
 {
-  struct quadratic_test testcase;
-  memset (&testcase, 0, sizeof (testcase));
+  struct quadratic_test testcase = {};
   testcase.ctxt = ctxt;
   make_types (testcase);
   make_sqrt (testcase);
diff --git a/gcc/testsuite/jit.dg/test-quadratic.cc b/gcc/testsuite/jit.dg/test-quadratic.cc
index f347669..61b5cdd 100644
--- a/gcc/testsuite/jit.dg/test-quadratic.cc
+++ b/gcc/testsuite/jit.dg/test-quadratic.cc
@@ -328,8 +328,7 @@ make_test_quadratic (quadratic_test &testcase)
 void
 create_code (gcc_jit_context *ctxt, void *user_data)
 {
-  struct quadratic_test testcase;
-  memset (&testcase, 0, sizeof (testcase));
+  struct quadratic_test testcase = {};
   testcase.ctxt = ctxt;
   make_types (testcase);
   make_sqrt (testcase);
-- 
1.8.5.3

Re: [committed] Fix -Werror=class-memaccess failures in jit testsuite (PR jit/81144)

[Martin Sebor]

On 06/20/2017 06:54 PM, David Malcolm wrote:
> On Tue, 2017-06-20 at 17:15 -0600, Martin Sebor wrote:
>> On 06/20/2017 03:25 PM, David Malcolm wrote:
>>> This patch fixes a couple of failures of the form:
>>>
>>>   error: 'void* memset(void*, int, size_t)' clearing an object of
>>> non-trivial
>>>     type 'struct quadratic_test'; use assignment or value
>>> -initialization
>>>     instead [-Werror=class-memaccess]
>>>   note: 'struct quadratic_test' declared here
>>>   cc1plus: all warnings being treated as errors
>>>
>>> seen within the jit testsuite, by using zero-initialization instead
>>> of memset.
>>>
>>> (presumably introduced by r249234 aka
>>> a324786b4ded9047d05463b4bce9d238b6c6b3ef)
>>>
>>> Successfully tested on x86_64-pc-linux-gnu; takes jit.sum from:
>>>   # of expected passes            9211
>>>   # of unexpected failures        2
>>> to:
>>>   # of expected passes            9349
>>>
>>> Martin: it's unclear to me what the benefit of the warning is for
>>> these
>>> cases.  AIUI, it's complaining because the code is calling
>>> the default ctor for struct quadratic_test, and then that object is
>>> being clobbered by the memset.
>>> But if I'm reading things right, the default ctor for this struct
>>> zero-initializes all fields.  Can't the compiler simply optimize
>>> away
>>> the redundant memset, and not issue a warning?
>
> Thanks for the info.
>
>> -Wclass-memaccess is issued because struct quadratic_test contains
>> members of classes that define a default ctor to initialize their
>> private members.
>> The premise behind the warning is that objects
>> of types with user-defined default and copy ctors should be
>> initialized by making use of their ctors, and those with private
>> data members manipulated via member functions rather than by
>> directly modifying their raw representation.  Using memset to
>> bypass the default ctor doesn't begin the lifetime of an object,
>> can violate invariants set up by it, and using it to overwrite
>> private members breaks encapsulation.  Examples of especially
>> insidious errors include overwriting const data, references, or
>> pointer to data members for which zero-initialization isn't
>> the same as clearing their bytes.
>
> If I'm reading my code correctly, all of the default ctors of all of
> the members of this struct are "merely" initializing the pointer they
> wrap to NULL.

Yes, that's my reading as well.

>
> So the ctors are initializing everything to NULL, and then the memset
> redundant re-init's everything to 0 bits (I guess I was going for a
> "belt and braces" approach to ensure that things are initialized).
>
>> The warning runs early on in the C++ front end and has no knowledge
>> of either the effects of the type's ctors, dtor, and copy assignment
>> operator, or whether the raw memory function is called in lieu of
>> initializing an object (e.g., in storage obtained from malloc or
>> operator new), or as a shortcut to zero out its members, or when
>> zeroing them out happens to be safe and doesn't actually do any
>> of those bad things I mentioned above.
>
> Aha: so at the place where the warning runs it's not possible to access
> the ctors and tell that they're assigning NULL everywhere?

Right, though I view it less as a limitation of the choice to
implement the warning in the FE and more as a feature.

>
> Might it be possible to convert the warning to work in a two-phase way
> where it first gathers up a vec of suspicious-looking modifications,
> and then flushes them later, filtering against ctor information when it
> has the latter?  (so that we don't have to warn for this case at
> -Wall?)

With some effort I suppose it might be possible to do something
sophisticated like that but based on the warnings we've seen so
far I'm not convinced it's necessary or that it would time well
spent.  In my view, classes with user-defined ctors and other
special functions (dtors, copy assignment), i.e., basically non
trivial types, are preferably manipulated using these special
functions, and memset and friends should only be used only for
raw memory operations, not as a substitute for the former.  In
this case (as in most others I've seen, including the one in your
patch), the code is clearer, more concise, and in general, also
safer (and much easier for GCC to analyze for correctness than
calls to memset et al.)

> Alternatively maybe this is PEBCAK at my end; if so, maybe a case for adding this to the changes.html page?  (and maybe adding some notes on workarounds there, and/or to invoke.texi?)

Sure, that sounds good to me.  Let me make a mental note to add
something to the manual.

>
>>
>> That said, I'm sorry (and a little surprised) that I missed these
>> errors in my tests.  I thought I had all the languages covered by
>> using
>>
>>    --enable-languages=all,ada,c,c++,fortran,go,lto,objc,obj-c++
>>
>> but I guess jit still isn't implied by all, even after Nathan's
>> recent change to it.  Let me add jit to my script (IIRC, I once
>> had it there but it was causing some trouble and I took it out.)
>
> Reading r248454 (aka 01b4453cde8f1871495955298043d9fb589e4a36), it
> looks like "jit" is only included in "all" if you also pass
>   --enable-host-shared
> Presumably that's what happened.  Bother.

Ah.  I forgot about that bit.  Thanks.

Martin