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