[C++] -Wclass-memaccess fixes and improvements (PR c++/81327)

Hi!

Apparently LLVM allows similar warning to -Wclass-memaccess (is it just
similar or same?; if the latter, perhaps we should use the same option for
that) to be disabled not just by casting the destination pointer to
e.g. (char *) or some other pointer to non-class, but also to (void *),
which is something that Qt uses or wants to use.
The problem is that the warning is diagnosed too late and whether we had
explicit or implicit conversion to void * is lost at that point.

This patch moves the warning tiny bit earlier (from build_cxx_call to the
caller) where we still have information about the original parsed arguments
before conversions to the builtin argument types.

In addition it fixes various small issues I run into when reading the code,
e.g. STRIP_NOPS was changing the argument in the argument array passed to
it, or e.g. the if (!warned && !warnfmt) return; was redundant with the
following code doing just
  if (warnfmt)
    something;
  if (warned)
    somethingelse;
  return;
or not verifying the INTEGER_CSTs fit into uhwi before calling tree_to_uhwi
etc.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2018-01-05  Jakub Jelinek  <jakub@redhat.com>

	PR c++/81327
	* call.c (maybe_warn_class_memaccess): Add forward declaration.
	Change last argument from tree * to const vec<tree, va_gc> *, adjust
	args uses and check number of operands too.  Don't strip away any
	nops.  Use maybe_constant_value when looking for INTEGER_CST args.
	Deal with src argument not having pointer type.  Check
	tree_fits_uhwi_p before calling tree_to_uhwi.  Remove useless
	test.
	(build_over_call): Call maybe_warn_class_memaccess here on the
	original arguments.
	(build_cxx_call): Rather than here on converted arguments.

	* g++.dg/Wclass-memaccess-2.C: Don't expect a warning when explicitly
	cast to void *.


	Jakub

On 01/05/2018 03:02 PM, Jakub Jelinek wrote:
> Hi!
>
> Apparently LLVM allows similar warning to -Wclass-memaccess (is it just
> similar or same?; if the latter, perhaps we should use the same option for
> that) to be disabled not just by casting the destination pointer to
> e.g. (char *) or some other pointer to non-class, but also to (void *),
> which is something that Qt uses or wants to use.

Clang has an option/warning called -Wdynamic-class-memaccess.
It detects a limited subset of the problems -Wclass-memaccess
detects: AFAIK, just calling raw memory functions like memcpy
on objects of polymorphic types, which the standard says is
undefined.

I didn't know about the Clang option when I wrote the GCC code.
Had I found out about it sooner I would have probably considered
splitting up -Wclass-memaccess and providing the same option as
Clang for compatibility, and another for the rest of the checks.
But there might still be time to split it up before the release
if that's important.

Some Qt code apparently uses memcpy to copy polymorphic objects
but gets around the Clang warning by casting the pointers to
void*, which doesn't suppress the GCC warning.  I had considered
allowing the void* cast as a possible suppression mechanism but
ultimately decided against it.  IIRC, partly because it would
have complicated the implementation and partly because I wasn't
convinced that it was a good idea (and I also didn't know about
the Clang escape hatch).

> The problem is that the warning is diagnosed too late and whether we had
> explicit or implicit conversion to void * is lost at that point.
>
> This patch moves the warning tiny bit earlier (from build_cxx_call to the
> caller) where we still have information about the original parsed arguments
> before conversions to the builtin argument types.

I'm still not convinced that letting programs get away with
byte-wise copying polymorphic objects is a good thing, but I
agree that supporting the void* cast suppression will be useful
for portability with Clang (presumably both C-style cast and
C++-style static_cast work).

If we want to make it a feature then we should document it in
the manual.  Otherwise, if it's supposed to be a back door for
poorly written code, then I think it would be helpful to mention
it in comments in the implementation and above the assertion in
the test. I don't like undocumented back doors so I'm leaning
toward documenting it in the manual.  I'm happy to add some
text to the manual if/when this is approved and decided.

Martin

> In addition it fixes various small issues I run into when reading the code,
> e.g. STRIP_NOPS was changing the argument in the argument array passed to
> it, or e.g. the if (!warned && !warnfmt) return; was redundant with the
> following code doing just
>   if (warnfmt)
>     something;
>   if (warned)
>     somethingelse;
>   return;
> or not verifying the INTEGER_CSTs fit into uhwi before calling tree_to_uhwi
> etc.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2018-01-05  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR c++/81327
> 	* call.c (maybe_warn_class_memaccess): Add forward declaration.
> 	Change last argument from tree * to const vec<tree, va_gc> *, adjust
> 	args uses and check number of operands too.  Don't strip away any
> 	nops.  Use maybe_constant_value when looking for INTEGER_CST args.
> 	Deal with src argument not having pointer type.  Check
> 	tree_fits_uhwi_p before calling tree_to_uhwi.  Remove useless
> 	test.
> 	(build_over_call): Call maybe_warn_class_memaccess here on the
> 	original arguments.
> 	(build_cxx_call): Rather than here on converted arguments.
>
> 	* g++.dg/Wclass-memaccess-2.C: Don't expect a warning when explicitly
> 	cast to void *.
>
> --- gcc/cp/call.c.jj	2018-01-04 00:43:16.109702768 +0100
> +++ gcc/cp/call.c	2018-01-05 15:03:40.255312975 +0100
> @@ -147,6 +147,8 @@ static int equal_functions (tree, tree);
>  static int joust (struct z_candidate *, struct z_candidate *, bool,
>  		  tsubst_flags_t);
>  static int compare_ics (conversion *, conversion *);
> +static void maybe_warn_class_memaccess (location_t, tree,
> +					const vec<tree, va_gc> *);
>  static tree build_over_call (struct z_candidate *, int, tsubst_flags_t);
>  #define convert_like(CONV, EXPR, COMPLAIN)			\
>    convert_like_real ((CONV), (EXPR), NULL_TREE, 0,		\
> @@ -8180,6 +8182,12 @@ build_over_call (struct z_candidate *can
>        && !mark_used (fn, complain))
>      return error_mark_node;
>
> +  /* Warn if the built-in writes to an object of a non-trivial type.  */
> +  if (warn_class_memaccess
> +      && vec_safe_length (args) >= 2
> +      && DECL_BUILT_IN_CLASS (fn) == BUILT_IN_NORMAL)
> +    maybe_warn_class_memaccess (input_location, fn, args);
> +
>    if (DECL_VINDEX (fn) && (flags & LOOKUP_NONVIRTUAL) == 0
>        /* Don't mess with virtual lookup in instantiate_non_dependent_expr;
>  	 virtual functions can't be constexpr.  */
> @@ -8360,7 +8368,8 @@ has_trivial_copy_p (tree type, bool acce
>     assignments.  */
>
>  static void
> -maybe_warn_class_memaccess (location_t loc, tree fndecl, tree *args)
> +maybe_warn_class_memaccess (location_t loc, tree fndecl,
> +			    const vec<tree, va_gc> *args)
>  {
>    /* Except for bcopy where it's second, the destination pointer is
>       the first argument for all functions handled here.  Compute
> @@ -8368,15 +8377,10 @@ maybe_warn_class_memaccess (location_t l
>    unsigned dstidx = DECL_FUNCTION_CODE (fndecl) == BUILT_IN_BCOPY;
>    unsigned srcidx = !dstidx;
>
> -  tree dest = args[dstidx];
> -  if (!dest || !TREE_TYPE (dest) || !POINTER_TYPE_P (TREE_TYPE (dest)))
> +  tree dest = (*args)[dstidx];
> +  if (!TREE_TYPE (dest) || !POINTER_TYPE_P (TREE_TYPE (dest)))
>      return;
>
> -  /* Remove the outermost (usually implicit) conversion to the void*
> -     argument type.  */
> -  if (TREE_CODE (dest) == NOP_EXPR)
> -    dest = TREE_OPERAND (dest, 0);
> -
>    tree srctype = NULL_TREE;
>
>    /* Determine the type of the pointed-to object and whether it's
> @@ -8436,7 +8440,7 @@ maybe_warn_class_memaccess (location_t l
>    switch (DECL_FUNCTION_CODE (fndecl))
>      {
>      case BUILT_IN_MEMSET:
> -      if (!integer_zerop (args[1]))
> +      if (!integer_zerop (maybe_constant_value ((*args)[1])))
>  	{
>  	  /* Diagnose setting non-copy-assignable or non-trivial types,
>  	     or types with a private member, to (potentially) non-zero
> @@ -8497,8 +8501,11 @@ maybe_warn_class_memaccess (location_t l
>      case BUILT_IN_MEMMOVE:
>      case BUILT_IN_MEMPCPY:
>        /* Determine the type of the source object.  */
> -      srctype = STRIP_NOPS (args[srcidx]);
> -      srctype = TREE_TYPE (TREE_TYPE (srctype));
> +      srctype = TREE_TYPE ((*args)[srcidx]);
> +      if (!srctype || !POINTER_TYPE_P (srctype))
> +	srctype = void_type_node;
> +      else
> +	srctype = TREE_TYPE (srctype);
>
>        /* Since it's impossible to determine wheter the byte copy is
>  	 being used in place of assignment to an existing object or
> @@ -8547,13 +8554,16 @@ maybe_warn_class_memaccess (location_t l
>  			       "assignment or copy-initialization instead",
>  			       fndecl, desttype, access, fld, srctype);
>  	}
> -      else if (!trivial && TREE_CODE (args[2]) == INTEGER_CST)
> +      else if (!trivial && vec_safe_length (args) > 2)
>  	{
> +	  tree sz = maybe_constant_value ((*args)[2]);
> +	  if (!tree_fits_uhwi_p (sz))
> +	    break;
> +
>  	  /* Finally, warn on partial copies.  */
>  	  unsigned HOST_WIDE_INT typesize
>  	    = tree_to_uhwi (TYPE_SIZE_UNIT (desttype));
> -	  if (unsigned HOST_WIDE_INT partial
> -	      = tree_to_uhwi (args[2]) % typesize)
> +	  if (unsigned HOST_WIDE_INT partial = tree_to_uhwi (sz) % typesize)
>  	    warned = warning_at (loc, OPT_Wclass_memaccess,
>  				 (typesize - partial > 1
>  				  ? G_("%qD writing to an object of "
> @@ -8577,16 +8587,17 @@ maybe_warn_class_memaccess (location_t l
>        else if (!get_dtor (desttype, tf_none))
>  	warnfmt = G_("%qD moving an object of type %#qT with deleted "
>  		     "destructor");
> -      else if (!trivial
> -	       && TREE_CODE (args[1]) == INTEGER_CST
> -	       && tree_int_cst_lt (args[1], TYPE_SIZE_UNIT (desttype)))
> +      else if (!trivial)
>  	{
> -	  /* Finally, warn on reallocation into insufficient space.  */
> -	  warned = warning_at (loc, OPT_Wclass_memaccess,
> -			       "%qD moving an object of non-trivial type "
> -			       "%#qT and size %E into a region of size %E",
> -			       fndecl, desttype, TYPE_SIZE_UNIT (desttype),
> -			       args[1]);
> +	  tree sz = maybe_constant_value ((*args)[1]);
> +	  if (TREE_CODE (sz) == INTEGER_CST
> +	      && tree_int_cst_lt (sz, TYPE_SIZE_UNIT (desttype)))
> +	    /* Finally, warn on reallocation into insufficient space.  */
> +	    warned = warning_at (loc, OPT_Wclass_memaccess,
> +				 "%qD moving an object of non-trivial type "
> +				 "%#qT and size %E into a region of size %E",
> +				 fndecl, desttype, TYPE_SIZE_UNIT (desttype),
> +				 sz);
>  	}
>        break;
>
> @@ -8594,9 +8605,6 @@ maybe_warn_class_memaccess (location_t l
>        return;
>      }
>
> -  if (!warned && !warnfmt)
> -    return;
> -
>    if (warnfmt)
>      {
>        if (suggest)
> @@ -8643,10 +8651,6 @@ build_cxx_call (tree fn, int nargs, tree
>        if (!check_builtin_function_arguments (EXPR_LOCATION (fn), vNULL, fndecl,
>  					     nargs, argarray))
>  	return error_mark_node;
> -
> -      /* Warn if the built-in writes to an object of a non-trivial type.  */
> -      if (nargs)
> -	maybe_warn_class_memaccess (loc, fndecl, argarray);
>      }
>
>    if (VOID_TYPE_P (TREE_TYPE (fn)))
> --- gcc/testsuite/g++.dg/Wclass-memaccess-2.C.jj	2017-06-27 09:16:08.184683033 +0200
> +++ gcc/testsuite/g++.dg/Wclass-memaccess-2.C	2018-01-05 14:52:01.277207043 +0100
> @@ -53,9 +53,7 @@ void c_cast_uchar (NonTrivial *p)
>    __builtin_memset ((unsigned char*)p, 0, sizeof *p);
>  }
>
> -// A cast to void* does not suppress the warning.  That is (or can be)
> -// considered a feature.
>  void c_cast_void (NonTrivial *p)
>  {
> -  __builtin_memset ((void*)p, 0, sizeof *p);   // { dg-warning "\\\[-Wclass-memaccess]" }
> +  __builtin_memset ((void*)p, 0, sizeof *p);   // { dg-bogus "\\\[-Wclass-memaccess]" }
>  }
>
> 	Jakub
>

On Fri, Jan 5, 2018 at 6:37 PM, Martin Sebor <msebor@gmail.com> wrote:
> On 01/05/2018 03:02 PM, Jakub Jelinek wrote:
>>
>> Hi!
>>
>> Apparently LLVM allows similar warning to -Wclass-memaccess (is it just
>> similar or same?; if the latter, perhaps we should use the same option for
>> that) to be disabled not just by casting the destination pointer to
>> e.g. (char *) or some other pointer to non-class, but also to (void *),
>> which is something that Qt uses or wants to use.
>
> Clang has an option/warning called -Wdynamic-class-memaccess.
> It detects a limited subset of the problems -Wclass-memaccess
> detects: AFAIK, just calling raw memory functions like memcpy
> on objects of polymorphic types, which the standard says is
> undefined.
>
> I didn't know about the Clang option when I wrote the GCC code.
> Had I found out about it sooner I would have probably considered
> splitting up -Wclass-memaccess and providing the same option as
> Clang for compatibility, and another for the rest of the checks.
> But there might still be time to split it up before the release
> if that's important.
>
> Some Qt code apparently uses memcpy to copy polymorphic objects
> but gets around the Clang warning by casting the pointers to
> void*, which doesn't suppress the GCC warning.  I had considered
> allowing the void* cast as a possible suppression mechanism but
> ultimately decided against it.  IIRC, partly because it would
> have complicated the implementation and partly because I wasn't
> convinced that it was a good idea (and I also didn't know about
> the Clang escape hatch).
>
>> The problem is that the warning is diagnosed too late and whether we had
>> explicit or implicit conversion to void * is lost at that point.
>>
>> This patch moves the warning tiny bit earlier (from build_cxx_call to the
>> caller) where we still have information about the original parsed
>> arguments before conversions to the builtin argument types.

OK.

> I'm still not convinced that letting programs get away with
> byte-wise copying polymorphic objects is a good thing, but I
> agree that supporting the void* cast suppression will be useful
> for portability with Clang (presumably both C-style cast and
> C++-style static_cast work).

IMO It's good to have a way to suppress any warning by being more
verbose.  Warnings are to highlight inadventent mistakes, not
deliberate ones.

> If we want to make it a feature then we should document it in
> the manual.  Otherwise, if it's supposed to be a back door for
> poorly written code, then I think it would be helpful to mention
> it in comments in the implementation and above the assertion in
> the test. I don't like undocumented back doors so I'm leaning
> toward documenting it in the manual.  I'm happy to add some
> text to the manual if/when this is approved and decided.

I agree that this should be documented.

Jason

On Wed, Jan 10, 2018 at 10:46:06AM -0500, Jason Merrill wrote:
> >> This patch moves the warning tiny bit earlier (from build_cxx_call to the
> >> caller) where we still have information about the original parsed
> >> arguments before conversions to the builtin argument types.
> 
> OK.

Is that an ack for the whole patch and docs can be handled incrementally
(e.g. if Martin volunteers), or shall that be documented first?
Though, even before this patch memcpy ((char *) whatever, ...)
already disabled the warning and wasn't documented either.

	Jakub

On Wed, Jan 10, 2018 at 11:25 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jan 10, 2018 at 10:46:06AM -0500, Jason Merrill wrote:
>> >> This patch moves the warning tiny bit earlier (from build_cxx_call to the
>> >> caller) where we still have information about the original parsed
>> >> arguments before conversions to the builtin argument types.
>>
>> OK.
>
> Is that an ack for the whole patch and docs can be handled incrementally
> (e.g. if Martin volunteers), or shall that be documented first?
> Though, even before this patch memcpy ((char *) whatever, ...)
> already disabled the warning and wasn't documented either.

The docs can follow on.

Jason