| Message ID | 2db2b46a-b716-4a3c-1e42-a066e40d85c7@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | avoid warning for memcpy to self (PR 83456) | expand |
On Thu, Mar 8, 2018 at 12:01 AM, Martin Sebor <msebor@gmail.com> wrote: > I have become convinced that issuing -Wrestrict in gimple-fold > for calls to memcpy() where the source pointer is the same as > the destination causes more trouble than it's worth, especially > when inlining is involved, as in: > > inline void bar (void *d, void *s, unsigned N) > { > if (s != d) > memcpy (d, s, N); > } > > void foo (void* src) > { > bar (src, src, 1); > } > > It seems that there should be a way to teach GCC to avoid > folding statements in dead blocks (e.g., in a block controlled > by 'if (0 != 0)' as the one below), and that it might even speed > up compilation, but in the meantime it leads to false positive > -Wrestrict warnings. > > The attached patch removes this instance of the warning and > adjusts tests not to expect it. Ok. Richard. > Martin
On 03/08/2018 01:59 AM, Richard Biener wrote: > On Thu, Mar 8, 2018 at 12:01 AM, Martin Sebor <msebor@gmail.com> wrote: >> I have become convinced that issuing -Wrestrict in gimple-fold >> for calls to memcpy() where the source pointer is the same as >> the destination causes more trouble than it's worth, especially >> when inlining is involved, as in: >> >> inline void bar (void *d, void *s, unsigned N) >> { >> if (s != d) >> memcpy (d, s, N); >> } >> >> void foo (void* src) >> { >> bar (src, src, 1); >> } >> >> It seems that there should be a way to teach GCC to avoid >> folding statements in dead blocks (e.g., in a block controlled >> by 'if (0 != 0)' as the one below), and that it might even speed >> up compilation, but in the meantime it leads to false positive >> -Wrestrict warnings. >> >> The attached patch removes this instance of the warning and >> adjusts tests not to expect it. > > Ok. I'm sorry, I just discovered that simply removing the code from gimple-fold would cause a regression with respect to GCC 7.0 which does diagnose memcpy(p, p, N) calls, and the diagnostic is useful to users (pr60256 and pr65452 request one for all string functions, although for reasons other than to detect overlapping accesses). To maintain the warning and at the same time eliminate the pesky false positives from gimple-fold I've partially restored the detection in the front-end. That in turn exposed some missing checking of the no-warning bit, leading to duplicate warnings. I've fixed all that up in the attached slightly bigger patch. Martin PS Even with the change to the front end above, removing the memcpy warning from gimple-fold has the downside of failing to detect bugs like: struct S { char a[32]; } s; memcpy (&s, s.a, sizeof s.a); because the front end doesn't know that &s and s.a are at the same address (copying between distinct members of the same union causes another false negative). I want to revisit this in stage 1 and see about handling it. If you have suggestions/preference for how or where I'd appreciate your input. PR tree-optimization/83456 - -Wrestrict false positive on a non-overlapping memcpy in an inline function gcc/ChangeLog: PR tree-optimization/83456 * gimple-fold.c (gimple_fold_builtin_memory_op): Avoid warning for perfectly overlapping calls to memcpy. (gimple_fold_builtin_memory_chk): Same. (gimple_fold_builtin_strcpy): Handle no-warning. (gimple_fold_builtin_stxcpy_chk): Same. * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Handle no-warning. gcc/c-family/ChangeLog: PR tree-optimization/83456 * gcc/c-family/c-common.c (check_function_restrict): Return bool. Restore checking of bounded built-in functions. (check_function_arguments): Also return the result of warn_for_restrict. * gcc/c-family/c-common.c (check_function_restrict): Return bool. * gcc/c-family/c-warn.c (warn_for_restrict): Return bool. gcc/testsuite/ChangeLog: PR tree-optimization/83456 * c-c++-common/Wrestrict-2.c: Remove test cases. * c-c++-common/Wrestrict.c: Same. * gcc.dg/Wrestrict-12.c: New test. * gcc.dg/Wrestrict-14.c: New test. Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 258366) +++ gcc/c-family/c-common.c (working copy) @@ -5309,10 +5309,11 @@ check_function_sentinel (const_tree fntype, int na } } -/* Check that the same argument isn't passed to restrict arguments - and other arguments. */ +/* Check that the same argument isn't passed to two or more + restrict-qualified formal and issue a -Wrestrict warning + if it is. Return true if a warning has been issued. */ -static void +static bool check_function_restrict (const_tree fndecl, const_tree fntype, int nargs, tree *argarray) { @@ -5322,11 +5323,14 @@ check_function_restrict (const_tree fndecl, const_ if (fndecl && TREE_CODE (fndecl) == FUNCTION_DECL) { - /* Skip checking built-ins here. They are checked in more - detail elsewhere. */ + /* Avoid diagnosing calls built-ins with a zero size/bound + here. They are checked in more detail elsewhere. */ if (DECL_BUILT_IN (fndecl) - && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL) - return; + && DECL_BUILT_IN_CLASS (fndecl) == BUILT_IN_NORMAL + && nargs == 3 + && TREE_CODE (argarray[2]) == INTEGER_CST + && integer_zerop (argarray[2])) + return false; if (DECL_ARGUMENTS (fndecl)) parms = DECL_ARGUMENTS (fndecl); @@ -5335,6 +5339,8 @@ check_function_restrict (const_tree fndecl, const_ for (i = 0; i < nargs; i++) TREE_VISITED (argarray[i]) = 0; + bool warned = false; + for (i = 0; i < nargs && parms && parms != void_list_node; i++) { tree type; @@ -5351,11 +5357,13 @@ check_function_restrict (const_tree fndecl, const_ if (POINTER_TYPE_P (type) && TYPE_RESTRICT (type) && !TYPE_READONLY (TREE_TYPE (type))) - warn_for_restrict (i, argarray, nargs); + warned |= warn_for_restrict (i, argarray, nargs); } for (i = 0; i < nargs; i++) TREE_VISITED (argarray[i]) = 0; + + return warned; } /* Helper for check_function_nonnull; given a list of operands which @@ -5596,8 +5604,10 @@ attribute_fallthrough_p (tree attr) /* Check for valid arguments being passed to a function with FNTYPE. - There are NARGS arguments in the array ARGARRAY. LOC should be used for - diagnostics. Return true if -Wnonnull warning has been diagnosed. */ + There are NARGS arguments in the array ARGARRAY. LOC should be used + for diagnostics. Return true if either -Wnonnull or -Wrestrict has + been issued. */ + bool check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, int nargs, tree *argarray, vec<location_t> *arglocs) @@ -5620,7 +5630,7 @@ check_function_arguments (location_t loc, const_tr check_function_sentinel (fntype, nargs, argarray); if (warn_restrict) - check_function_restrict (fndecl, fntype, nargs, argarray); + warned_p |= check_function_restrict (fndecl, fntype, nargs, argarray); return warned_p; } Index: gcc/c-family/c-common.h =================================================================== --- gcc/c-family/c-common.h (revision 258366) +++ gcc/c-family/c-common.h (working copy) @@ -1269,7 +1269,7 @@ extern void warnings_for_convert_and_check (locati extern void c_do_switch_warnings (splay_tree, location_t, tree, tree, bool, bool); extern void warn_for_omitted_condop (location_t, tree); -extern void warn_for_restrict (unsigned, tree *, unsigned); +extern bool warn_for_restrict (unsigned, tree *, unsigned); /* Places where an lvalue, or modifiable lvalue, may be required. Used to select diagnostic messages in lvalue_error and Index: gcc/c-family/c-warn.c =================================================================== --- gcc/c-family/c-warn.c (revision 258366) +++ gcc/c-family/c-warn.c (working copy) @@ -2372,14 +2372,15 @@ maybe_warn_bool_compare (location_t loc, enum tree } /* Warn if an argument at position param_pos is passed to a - restrict-qualified param, and it aliases with another argument. */ + restrict-qualified param, and it aliases with another argument. + Return true if a warning has been issued. */ -void +bool warn_for_restrict (unsigned param_pos, tree *argarray, unsigned nargs) { tree arg = argarray[param_pos]; if (TREE_VISITED (arg) || integer_zerop (arg)) - return; + return false; location_t loc = EXPR_LOC_OR_LOC (arg, input_location); gcc_rich_location richloc (loc); @@ -2395,13 +2396,13 @@ warn_for_restrict (unsigned param_pos, tree *argar tree current_arg = argarray[i]; if (operand_equal_p (arg, current_arg, 0)) { - TREE_VISITED (current_arg) = 1; + TREE_VISITED (current_arg) = 1; arg_positions.safe_push (i + 1); } } if (arg_positions.is_empty ()) - return; + return false; int pos; FOR_EACH_VEC_ELT (arg_positions, i, pos) @@ -2411,13 +2412,13 @@ warn_for_restrict (unsigned param_pos, tree *argar richloc.add_range (EXPR_LOCATION (arg), false); } - warning_n (&richloc, OPT_Wrestrict, arg_positions.length (), - "passing argument %i to restrict-qualified parameter" - " aliases with argument %Z", - "passing argument %i to restrict-qualified parameter" - " aliases with arguments %Z", - param_pos + 1, arg_positions.address (), - arg_positions.length ()); + return warning_n (&richloc, OPT_Wrestrict, arg_positions.length (), + "passing argument %i to restrict-qualified parameter" + " aliases with argument %Z", + "passing argument %i to restrict-qualified parameter" + " aliases with arguments %Z", + param_pos + 1, arg_positions.address (), + arg_positions.length ()); } /* Callback function to determine whether an expression TP or one of its Index: gcc/gimple-fold.c =================================================================== --- gcc/gimple-fold.c (revision 258366) +++ gcc/gimple-fold.c (working copy) @@ -682,11 +682,7 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterato tree destvar, srcvar; location_t loc = gimple_location (stmt); - tree func = gimple_call_fndecl (stmt); bool nowarn = gimple_no_warning_p (stmt); - bool check_overlap = (DECL_FUNCTION_CODE (func) != BUILT_IN_MEMMOVE - && DECL_FUNCTION_CODE (func) != BUILT_IN_MEMMOVE_CHK - && !nowarn); /* If the LEN parameter is a constant zero or in range where the only valid value is zero, return DEST. */ @@ -713,13 +709,7 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterato { /* Avoid diagnosing exact overlap in calls to __builtin_memcpy. It's safe and may even be emitted by GCC itself (see bug - 32667). However, diagnose it in explicit calls to the memcpy - function. */ - if (check_overlap && *IDENTIFIER_POINTER (DECL_NAME (func)) != '_') - warning_at (loc, OPT_Wrestrict, - "%qD source argument is the same as destination", - func); - + 32667). */ unlink_stmt_vdef (stmt); if (gimple_vdef (stmt) && TREE_CODE (gimple_vdef (stmt)) == SSA_NAME) release_ssa_name (gimple_vdef (stmt)); @@ -1622,11 +1612,14 @@ gimple_fold_builtin_strcpy (gimple_stmt_iterator * /* If SRC and DEST are the same (and not volatile), return DEST. */ if (operand_equal_p (src, dest, 0)) { - tree func = gimple_call_fndecl (stmt); + if (!gimple_no_warning_p (stmt)) + { + tree func = gimple_call_fndecl (stmt); - warning_at (loc, OPT_Wrestrict, - "%qD source argument is the same as destination", - func); + warning_at (loc, OPT_Wrestrict, + "%qD source argument is the same as destination", + func); + } replace_call_with_value (gsi, dest); return true; @@ -2499,15 +2492,6 @@ gimple_fold_builtin_memory_chk (gimple_stmt_iterat (resp. DEST+LEN for __mempcpy_chk). */ if (fcode != BUILT_IN_MEMSET_CHK && operand_equal_p (src, dest, 0)) { - if (fcode != BUILT_IN_MEMMOVE && fcode != BUILT_IN_MEMMOVE_CHK) - { - tree func = gimple_call_fndecl (stmt); - - warning_at (loc, OPT_Wrestrict, - "%qD source argument is the same as destination", - func); - } - if (fcode != BUILT_IN_MEMPCPY_CHK) { replace_call_with_value (gsi, dest); @@ -2609,11 +2593,14 @@ gimple_fold_builtin_stxcpy_chk (gimple_stmt_iterat /* If SRC and DEST are the same (and not volatile), return DEST. */ if (fcode == BUILT_IN_STRCPY_CHK && operand_equal_p (src, dest, 0)) { - tree func = gimple_call_fndecl (stmt); + if (!gimple_no_warning_p (stmt)) + { + tree func = gimple_call_fndecl (stmt); - warning_at (loc, OPT_Wrestrict, - "%qD source argument is the same as destination", - func); + warning_at (loc, OPT_Wrestrict, + "%qD source argument is the same as destination", + func); + } replace_call_with_value (gsi, dest); return true; Index: gcc/tree-ssa-strlen.c =================================================================== --- gcc/tree-ssa-strlen.c (revision 258366) +++ gcc/tree-ssa-strlen.c (working copy) @@ -1801,6 +1801,8 @@ bool maybe_diag_stxncpy_trunc (gimple_stmt_iterator gsi, tree src, tree cnt) { gimple *stmt = gsi_stmt (gsi); + if (gimple_no_warning_p (stmt)) + return false; wide_int cntrange[2]; Index: gcc/testsuite/c-c++-common/Wrestrict-2.c =================================================================== --- gcc/testsuite/c-c++-common/Wrestrict-2.c (revision 258366) +++ gcc/testsuite/c-c++-common/Wrestrict-2.c (working copy) @@ -12,13 +12,13 @@ static void wrap_memcpy (void *d, const void *s, size_t n) { - memcpy (d, s, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ + memcpy (d, s, n); /* { dg-warning "accessing 2 bytes at offsets 0 and 1 overlaps 1 byte at offset 1" "memcpy" } */ } -void call_memcpy (void *d, size_t n) +void call_memcpy (char *d) { - const void *s = d; - wrap_memcpy (d, s, n); + const void *s = d + 1; + wrap_memcpy (d, s, 2); } Index: gcc/testsuite/c-c++-common/Wrestrict.c =================================================================== --- gcc/testsuite/c-c++-common/Wrestrict.c (revision 258366) +++ gcc/testsuite/c-c++-common/Wrestrict.c (working copy) @@ -52,7 +52,6 @@ void test_memcpy_cst (void *d, const void *s) } while (0) T (a, a, 0); - T (a, s = a, 3); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ /* This isn't detected because memcpy calls with small power-of-2 sizes are intentionally folded into safe copies equivalent to memmove. @@ -64,19 +63,6 @@ void test_memcpy_cst (void *d, const void *s) T (a, a + 3, 5); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ { - char a[3] = { 1, 2, 3 }; - - /* Verify that a call to memcpy with an exact overlap is diagnosed - (also tested above) but an excplicit one to __builtin_memcpy is - not. See bug 32667 for the rationale. */ - (memcpy)(a, a, sizeof a); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (a); - - __builtin_memcpy (a, a, sizeof a); - sink (a); - } - - { char a[3][7]; sink (a); @@ -116,11 +102,6 @@ void test_memcpy_cst (void *d, const void *s) memcpy (d, s, sizeof x.a); sink (&x); - d = x.a; - s = x.a; - memcpy (d, s, sizeof x.a); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ - sink (&x); - d = x.a + 4; s = x.b; memcpy (d, s, sizeof x.a); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ @@ -447,22 +428,10 @@ void test_memcpy_var (char *d, const char *s) { size_t n = unsigned_value (); + /* Since no copying takes place no warning should be issued. */ memcpy (d, d, 0); sink (d); - memcpy (d, d, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - - memcpy (d, &d[0], n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - - memcpy (&d[0], d, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - - s = d; - memcpy (d, s, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - /* The following overlaps if n is greater than 1. */ s = d + 1; memcpy (d, s, n); @@ -499,10 +468,6 @@ void test_memcpy_var (char *d, const char *s) s = d + 5; n = 7; memcpy (d, s, n); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ - - n = UR (0, 1); - s = d; - memcpy (d, s, n); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ } @@ -941,7 +906,7 @@ void test_strncpy_range (char *d, size_t n) /* The following overlaps except in the unlikely case that value () is zero, so it's diagnosed. */ - T ("012", a, a, n); /* { dg-warning "source argument is the same as destination " "strncpy" } */ + T ("012", a, a, n); /* { dg-warning "\\\[-Wrestrict]" "strncpy" } */ } Index: gcc/testsuite/gcc.dg/Wrestrict-12.c =================================================================== --- gcc/testsuite/gcc.dg/Wrestrict-12.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wrestrict-12.c (working copy) @@ -0,0 +1,66 @@ +/* PR tree-optimization/83456 - -Wrestrict false positive on + a non-overlapping memcpy in an inline function + { dg-do compile } + { dg-options "-O2 -Wrestrict -ftrack-macro-expansion=0" } */ + +extern void* memcpy (void*, const void*, __SIZE_TYPE__); + +/* Test case from comment #0. */ + +inline void pr83456_comment0 (void *d, void *s, unsigned N) +{ + if (s != d) + memcpy (d, s, N); +} + +void call_pr83456_comment0 (void* src) +{ + pr83456_comment0 (src, src, 1); +} + + +/* Test case from comment #1. */ + +char a[4]; + +void pr83456_comment1 (unsigned n) +{ + for (int i = 0; i < 1; i++) + { + if (!i) + continue; + + memcpy (a, a, n); + } +} + +/* Test case from comment #2. */ + +struct netdevice { + void *priv; +}; + +struct ip_tunnel { + struct netdevice *dev; + int ip6rd[3]; +}; + +struct sit_net { + struct netdevice *fb_tunnel_dev; +}; + +void ipip6_tunnel_clone_6rd (struct netdevice *dev, struct sit_net *sitn) +{ + struct ip_tunnel *t = dev->priv; + if (t->dev == sitn->fb_tunnel_dev) + return; + + struct ip_tunnel *t0 = sitn->fb_tunnel_dev->priv; + memcpy(&t->ip6rd, &t0->ip6rd, sizeof(t->ip6rd)); +} + +void sit_init_net (struct sit_net *sitn, struct netdevice *fb_tunnel_dev) +{ + sitn->fb_tunnel_dev = fb_tunnel_dev; + ipip6_tunnel_clone_6rd (sitn->fb_tunnel_dev, sitn); +} Index: gcc/testsuite/gcc.dg/Wrestrict-14.c =================================================================== --- gcc/testsuite/gcc.dg/Wrestrict-14.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wrestrict-14.c (working copy) @@ -0,0 +1,221 @@ +/* PR tree-optimization/83456 - -Wrestrict false positive on a non-overlapping + memcpy in an inline function + Verify that calls to built-in functions are diagnosed when the pointer + arguments to their restrict-qualified parameters are the same (the absence + of the false positives reported in PR 83456 is tested in Wrestrict-12.c. + { dg-do compile } + { dg-options "-O2 -Wall -Wrestrict -Wno-stringop-truncation" } */ + +typedef __SIZE_TYPE__ size_t; + +extern void* memcpy (void* restrict, const void* restrict, size_t); +extern void* mempcpy (void* restrict, const void* restrict, size_t); +extern char* stpncpy (char* restrict, const char* restrict, size_t); +extern char* stpcpy (char* restrict, const char* restrict); +extern char* strncat (char* restrict, const char* restrict, size_t); +extern char* strcat (char* restrict, const char* restrict); +extern char* strncpy (char* restrict, const char* restrict, size_t); +extern char* strcpy (char* restrict, const char* restrict); + +struct S +{ + char a[4]; + char *p; +} s; + +void sink (void*); + +void test_memcpy (char *p, struct S *q, size_t n) +{ + /* The behavior of memcpy() is undefined only when when copying takes + place between overlapping objects. Since a call with a size of zero + does nothing, it should not be diagnosed. */ + memcpy (p, p, 0); + sink (p); + + memcpy (p, p, 1); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + memcpy (p, p, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + memcpy (q->a, q->a, 0); + sink (q); + + memcpy (q->p, q->p, 1); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + memcpy (&q->a[0], q->a, n); /* { dg-warning "\\\[-Wrestrict]" "bug ????" { xfail *-*-* } } */ + sink (q); + + memcpy (q, q->a, n); /* { dg-warning "\\\[-Wrestrict]" "bug ????" { xfail *-*-* } } */ + sink (q); +} + +void test_mempcpy (char *p, struct S *q, size_t n) +{ + mempcpy (p, p, 0); + sink (p); + + mempcpy (p, p, 1); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + mempcpy (p, p, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + mempcpy (q->a, q->a, 0); + sink (q); + + mempcpy (q->p, q->p, 1); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + mempcpy (&q->a[0], q->a, n); /* { dg-warning "\\\[-Wrestrict]" "bug ????" { xfail *-*-* } } */ + sink (q); + + mempcpy (q, q->a, n); /* { dg-warning "\\\[-Wrestrict]" "bug ????" { xfail *-*-* } } */ + sink (q); +} + +void test_strncat (char *p, struct S *q, size_t n) +{ + strncat (p, p, 0); + sink (p); + + strncat (p, p, 1); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strncat (p, p, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strncat (q->a, q->a, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strncat (&q->a[0], &q->a[0], n);/* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strncat (q->a, &q->a[0], n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strncat (q->p, &q->p[0], n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); +} + +void test_strcat (char *p, struct S *q, size_t n) +{ + strcat (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strcat (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strcat (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strcat (q->a, q->a); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strcat (&q->a[0], &q->a[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strcat (q->a, &q->a[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strcat (q->p, &q->p[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); +} + +void test_stpncpy (char *p, struct S *q, size_t n) +{ + stpncpy (p, p, 0); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + stpncpy (p, p, 1); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + stpncpy (p, p, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + stpncpy (q->a, q->a, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + stpncpy (&q->a[0], &q->a[0], n);/* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + stpncpy (q->a, &q->a[0], n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + stpncpy (q->p, &q->p[0], n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); +} + +void test_stpcpy (char *p, struct S *q, size_t n) +{ + stpcpy (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + stpcpy (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + stpcpy (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + stpcpy (q->a, q->a); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + stpcpy (&q->a[0], &q->a[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + stpcpy (q->a, &q->a[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + stpcpy (q->p, &q->p[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); +} + +void test_strncpy (char *p, struct S *q, size_t n) +{ + strncpy (p, p, 0); + sink (p); + + strncpy (p, p, 1); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strncpy (p, p, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strncpy (q->a, q->a, n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strncpy (&q->a[0], &q->a[0], n);/* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strncpy (q->a, &q->a[0], n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strncpy (q->p, &q->p[0], n); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); +} + +void test_strcpy (char *p, struct S *q, size_t n) +{ + strcpy (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strcpy (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strcpy (p, p); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (p); + + strcpy (q->a, q->a); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strcpy (&q->a[0], &q->a[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strcpy (q->a, &q->a[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); + + strcpy (q->p, &q->p[0]); /* { dg-warning "\\\[-Wrestrict]" } */ + sink (q); +}
On 03/08/2018 05:46 PM, Martin Sebor wrote: > On 03/08/2018 01:59 AM, Richard Biener wrote: >> On Thu, Mar 8, 2018 at 12:01 AM, Martin Sebor <msebor@gmail.com> wrote: >>> I have become convinced that issuing -Wrestrict in gimple-fold >>> for calls to memcpy() where the source pointer is the same as >>> the destination causes more trouble than it's worth, especially >>> when inlining is involved, as in: >>> >>> inline void bar (void *d, void *s, unsigned N) >>> { >>> if (s != d) >>> memcpy (d, s, N); >>> } >>> >>> void foo (void* src) >>> { >>> bar (src, src, 1); >>> } >>> >>> It seems that there should be a way to teach GCC to avoid >>> folding statements in dead blocks (e.g., in a block controlled >>> by 'if (0 != 0)' as the one below), and that it might even speed >>> up compilation, but in the meantime it leads to false positive >>> -Wrestrict warnings. >>> >>> The attached patch removes this instance of the warning and >>> adjusts tests not to expect it. >> >> Ok. > > I'm sorry, I just discovered that simply removing the code from > gimple-fold would cause a regression with respect to GCC 7.0 > which does diagnose memcpy(p, p, N) calls, and the diagnostic > is useful to users (pr60256 and pr65452 request one for all > string functions, although for reasons other than to detect > overlapping accesses). > > To maintain the warning and at the same time eliminate the pesky > false positives from gimple-fold I've partially restored the > detection in the front-end. That in turn exposed some missing > checking of the no-warning bit, leading to duplicate warnings. > I've fixed all that up in the attached slightly bigger patch. > > Martin > > PS Even with the change to the front end above, removing > the memcpy warning from gimple-fold has the downside of > failing to detect bugs like: > > struct S { char a[32]; } s; > memcpy (&s, s.a, sizeof s.a); > > because the front end doesn't know that &s and s.a are at > the same address (copying between distinct members of the > same union causes another false negative). I want to > revisit this in stage 1 and see about handling it. If > you have suggestions/preference for how or where I'd > appreciate your input. I suspect the too-early folding is going to get in the way here. In some ways what you want is to either warn early or have a way to recover the original arguments. I'm not aware of a good way to do either for this scenario though. > > gcc-83456.diff > > > PR tree-optimization/83456 - -Wrestrict false positive on a non-overlapping memcpy in an inline function > > gcc/ChangeLog: > > PR tree-optimization/83456 > * gimple-fold.c (gimple_fold_builtin_memory_op): Avoid warning > for perfectly overlapping calls to memcpy. > (gimple_fold_builtin_memory_chk): Same. > (gimple_fold_builtin_strcpy): Handle no-warning. > (gimple_fold_builtin_stxcpy_chk): Same. > * tree-ssa-strlen.c (maybe_diag_stxncpy_trunc): Handle no-warning. > > gcc/c-family/ChangeLog: > > PR tree-optimization/83456 > * gcc/c-family/c-common.c (check_function_restrict): Return bool. > Restore checking of bounded built-in functions. > (check_function_arguments): Also return the result > of warn_for_restrict. > * gcc/c-family/c-common.c (check_function_restrict): Return bool. > * gcc/c-family/c-warn.c (warn_for_restrict): Return bool. > > gcc/testsuite/ChangeLog: > > PR tree-optimization/83456 > * c-c++-common/Wrestrict-2.c: Remove test cases. > * c-c++-common/Wrestrict.c: Same. > * gcc.dg/Wrestrict-12.c: New test. > * gcc.dg/Wrestrict-14.c: New test. The self-copy via memcpy is particularly annoying. In an ideal world we'd never generate them, but we often do and it's been a source of many complaints. Ideally we'd never generate them. Second best option is to fold them away -- at least those generated by the compiler itself. Anyway, this is OK for the trunk. Feel free to open BZs for issues you want to tackle during gcc-9. Thanks, jeff
On Wed, Mar 7, 2018 at 3:01 PM, Martin Sebor <msebor@gmail.com> wrote: > I have become convinced that issuing -Wrestrict in gimple-fold > for calls to memcpy() where the source pointer is the same as > the destination causes more trouble than it's worth, especially > when inlining is involved, as in: > > inline void bar (void *d, void *s, unsigned N) > { > if (s != d) > memcpy (d, s, N); > } > > void foo (void* src) > { > bar (src, src, 1); > } > > It seems that there should be a way to teach GCC to avoid > folding statements in dead blocks (e.g., in a block controlled > by 'if (0 != 0)' as the one below), and that it might even speed > up compilation, but in the meantime it leads to false positive > -Wrestrict warnings. > > The attached patch removes this instance of the warning and > adjusts tests not to expect it. This caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84919
PR tree-optimization/83456 - -Wrestrict false positive on a non-overlapping memcpy in an inline function gcc/ChangeLog: PR tree-optimization/83456 * gimple-fold.c (gimple_fold_builtin_memory_op): Avoid warning for perfectly overlapping calls to memcpy. (gimple_fold_builtin_memory_chk): Same. gcc/testsuite/ChangeLog: PR tree-optimization/83456 * c-c++-common/Wrestrict-2.c: Remove test cases. * c-c++-common/Wrestrict.c: Same. * gcc.dg/Wrestrict-12.c: New test. Index: gcc/gimple-fold.c =================================================================== --- gcc/gimple-fold.c (revision 258339) +++ gcc/gimple-fold.c (working copy) @@ -713,13 +713,7 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterato { /* Avoid diagnosing exact overlap in calls to __builtin_memcpy. It's safe and may even be emitted by GCC itself (see bug - 32667). However, diagnose it in explicit calls to the memcpy - function. */ - if (check_overlap && *IDENTIFIER_POINTER (DECL_NAME (func)) != '_') - warning_at (loc, OPT_Wrestrict, - "%qD source argument is the same as destination", - func); - + 32667). */ unlink_stmt_vdef (stmt); if (gimple_vdef (stmt) && TREE_CODE (gimple_vdef (stmt)) == SSA_NAME) release_ssa_name (gimple_vdef (stmt)); @@ -2499,15 +2493,6 @@ gimple_fold_builtin_memory_chk (gimple_stmt_iterat (resp. DEST+LEN for __mempcpy_chk). */ if (fcode != BUILT_IN_MEMSET_CHK && operand_equal_p (src, dest, 0)) { - if (fcode != BUILT_IN_MEMMOVE && fcode != BUILT_IN_MEMMOVE_CHK) - { - tree func = gimple_call_fndecl (stmt); - - warning_at (loc, OPT_Wrestrict, - "%qD source argument is the same as destination", - func); - } - if (fcode != BUILT_IN_MEMPCPY_CHK) { replace_call_with_value (gsi, dest); Index: gcc/testsuite/c-c++-common/Wrestrict-2.c =================================================================== --- gcc/testsuite/c-c++-common/Wrestrict-2.c (revision 258339) +++ gcc/testsuite/c-c++-common/Wrestrict-2.c (working copy) @@ -12,13 +12,13 @@ static void wrap_memcpy (void *d, const void *s, size_t n) { - memcpy (d, s, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ + memcpy (d, s, n); /* { dg-warning "accessing 2 bytes at offsets 0 and 1 overlaps 1 byte at offset 1" "memcpy" } */ } -void call_memcpy (void *d, size_t n) +void call_memcpy (char *d) { - const void *s = d; - wrap_memcpy (d, s, n); + const void *s = d + 1; + wrap_memcpy (d, s, 2); } Index: gcc/testsuite/c-c++-common/Wrestrict.c =================================================================== --- gcc/testsuite/c-c++-common/Wrestrict.c (revision 258339) +++ gcc/testsuite/c-c++-common/Wrestrict.c (working copy) @@ -52,7 +52,6 @@ void test_memcpy_cst (void *d, const void *s) } while (0) T (a, a, 0); - T (a, s = a, 3); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ /* This isn't detected because memcpy calls with small power-of-2 sizes are intentionally folded into safe copies equivalent to memmove. @@ -64,19 +63,6 @@ void test_memcpy_cst (void *d, const void *s) T (a, a + 3, 5); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ { - char a[3] = { 1, 2, 3 }; - - /* Verify that a call to memcpy with an exact overlap is diagnosed - (also tested above) but an excplicit one to __builtin_memcpy is - not. See bug 32667 for the rationale. */ - (memcpy)(a, a, sizeof a); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (a); - - __builtin_memcpy (a, a, sizeof a); - sink (a); - } - - { char a[3][7]; sink (a); @@ -116,11 +102,6 @@ void test_memcpy_cst (void *d, const void *s) memcpy (d, s, sizeof x.a); sink (&x); - d = x.a; - s = x.a; - memcpy (d, s, sizeof x.a); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ - sink (&x); - d = x.a + 4; s = x.b; memcpy (d, s, sizeof x.a); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ @@ -450,19 +431,6 @@ void test_memcpy_var (char *d, const char *s) memcpy (d, d, 0); sink (d); - memcpy (d, d, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - - memcpy (d, &d[0], n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - - memcpy (&d[0], d, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - - s = d; - memcpy (d, s, n); /* { dg-warning "source argument is the same as destination" "memcpy" } */ - sink (d); - /* The following overlaps if n is greater than 1. */ s = d + 1; memcpy (d, s, n); @@ -499,10 +467,6 @@ void test_memcpy_var (char *d, const char *s) s = d + 5; n = 7; memcpy (d, s, n); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ - - n = UR (0, 1); - s = d; - memcpy (d, s, n); /* { dg-warning "\\\[-Wrestrict" "memcpy" } */ } Index: gcc/testsuite/gcc.dg/Wrestrict-12.c =================================================================== --- gcc/testsuite/gcc.dg/Wrestrict-12.c (nonexistent) +++ gcc/testsuite/gcc.dg/Wrestrict-12.c (working copy) @@ -0,0 +1,66 @@ +/* PR tree-optimization/83456 - -Wrestrict false positive on + a non-overlapping memcpy in an inline function + { dg-do compile } + { dg-options "-O2 -Wrestrict -ftrack-macro-expansion=0" } */ + +extern void* memcpy (void*, const void*, __SIZE_TYPE__); + +/* Test case from comment #0. */ + +inline void pr83456_comment0 (void *d, void *s, unsigned N) +{ + if (s != d) + memcpy (d, s, N); +} + +void call_pr83456_comment0 (void* src) +{ + pr83456_comment0 (src, src, 1); +} + + +/* Test case from comment #1. */ + +char a[4]; + +void pr83456_comment1 (unsigned n) +{ + for (int i = 0; i < 1; i++) + { + if (!i) + continue; + + memcpy (a, a, n); + } +} + +/* Test case from comment #2. */ + +struct netdevice { + void *priv; +}; + +struct ip_tunnel { + struct netdevice *dev; + int ip6rd[3]; +}; + +struct sit_net { + struct netdevice *fb_tunnel_dev; +}; + +void ipip6_tunnel_clone_6rd (struct netdevice *dev, struct sit_net *sitn) +{ + struct ip_tunnel *t = dev->priv; + if (t->dev == sitn->fb_tunnel_dev) + return; + + struct ip_tunnel *t0 = sitn->fb_tunnel_dev->priv; + memcpy(&t->ip6rd, &t0->ip6rd, sizeof(t->ip6rd)); +} + +void sit_init_net (struct sit_net *sitn, struct netdevice *fb_tunnel_dev) +{ + sitn->fb_tunnel_dev = fb_tunnel_dev; + ipip6_tunnel_clone_6rd (sitn->fb_tunnel_dev, sitn); +}
I have become convinced that issuing -Wrestrict in gimple-fold for calls to memcpy() where the source pointer is the same as the destination causes more trouble than it's worth, especially when inlining is involved, as in: inline void bar (void *d, void *s, unsigned N) { if (s != d) memcpy (d, s, N); } void foo (void* src) { bar (src, src, 1); } It seems that there should be a way to teach GCC to avoid folding statements in dead blocks (e.g., in a block controlled by 'if (0 != 0)' as the one below), and that it might even speed up compilation, but in the meantime it leads to false positive -Wrestrict warnings. The attached patch removes this instance of the warning and adjusts tests not to expect it. Martin