Bug 78918 - missing -Wrestrict on memcpy copying over self
Summary: missing -Wrestrict on memcpy copying over self
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 7.0
: P3 normal
Target Milestone: ---
Assignee: Martin Sebor
URL:
Keywords: diagnostic, patch
Depends on:
Blocks: Wrestrict
  Show dependency treegraph
 
Reported: 2016-12-24 00:32 UTC by Martin Sebor
Modified: 2018-03-09 02:59 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-06-21 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Sebor 2016-12-24 00:32:37 UTC
Calling memcpy to copy objects that overlap violates the strict aliasing rules and is undefined.  As the dump below shows, GCC eliminates some such calls implying it "knows" the objects overlap (even though optimizing undefined code seems questionable) but it doesn't issue any diagnostic.  With the -Wrestrict warning GCC should flag at least the basic cases.

$ cat z.c && gcc -O2 -S -Wall -Wextra -Wpedantic -Wrestrict -fdump-tree-optimized=/dev/stdout z.c
void f (void *p, void *q, unsigned n)
{
  p = q;
  __builtin_memcpy (p, q, n);
}

;; Function f (f, funcdef_no=0, decl_uid=1797, cgraph_uid=0, symbol_order=0)

f (void * p, void * q, unsigned int n)
{
  <bb 2> [100.00%]:
  return;

}
Comment 1 Martin Sebor 2016-12-24 00:33:13 UTC
See also bug 60256 for a similar case involving strcpy.
Comment 2 Martin Sebor 2017-06-21 18:53:42 UTC
One reason for the missing warning in the test case in comment #0 is that GCC built-ins aren't declared using the restrict qualifier.  In particular, both memcpy and memmove are declared in builtins.def like so:

DEF_LIB_BUILTIN_CHKP   (BUILT_IN_MEMCPY, "memcpy", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)
DEF_LIB_BUILTIN_CHKP   (BUILT_IN_MEMMOVE, "memmove", BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_RET1_NOTHROW_NONNULL_LEAF)

with BT_FN_PTR_PTR_CONST_PTR_SIZE specifying the same signature.  BT_PTR expands to void_ptr_node, with no restrict qualifier.

The second reason is that the -Wrestrict warning is implemented in the front-end, with no ability to determine relationships between distinct pointers.  So while memcpy(p, p, n) is diagnosed (provided memcpy is declared restrict), memcpy(p, q, n) is not.  That limitation limits the warning to just the most basic and obvious cases, missing the more interesting problems.

Confirming on that basis.  Adding a reference to bug 35503 under which the warning was implemented (in r242366).
Comment 3 Florian Weimer 2017-06-21 18:56:41 UTC
PR32667 says that GCC itself uses memcpy with exact overlaps, so we probably should not add restrict qualifiers until that is fixed.
Comment 4 Martin Sebor 2017-07-16 23:48:58 UTC
Patch posted for review:
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00917.html
Comment 5 Martin Sebor 2017-12-16 23:59:05 UTC
Author: msebor
Date: Sat Dec 16 23:58:34 2017
New Revision: 255755

URL: https://gcc.gnu.org/viewcvs?rev=255755&root=gcc&view=rev
Log:
PR tree-optimization/78918 - missing -Wrestrict on memcpy copying over self

gcc/c-family/ChangeLog:

	PR tree-optimization/78918
	* c-common.c (check_function_restrict): Avoid checking built-ins.
	* c.opt (-Wrestrict): Include in -Wall.

gcc/ChangeLog:

	PR tree-optimization/78918
	* Makefile.in (OBJS): Add gimple-ssa-warn-restrict.o.
	* builtins.c (check_sizes): Rename...
	(check_access): ...to this.  Rename function arguments for clarity.
	(check_memop_sizes): Adjust names.
	(expand_builtin_memchr, expand_builtin_memcpy): Same.
	(expand_builtin_memmove, expand_builtin_mempcpy): Same.
	(expand_builtin_strcat, expand_builtin_stpncpy): Same.
	(check_strncat_sizes, expand_builtin_strncat): Same.
	(expand_builtin_strncpy, expand_builtin_memset): Same.
	(expand_builtin_bzero, expand_builtin_memcmp): Same.
	(expand_builtin_memory_chk, maybe_emit_chk_warning): Same.
	(maybe_emit_sprintf_chk_warning): Same.
	(expand_builtin_strcpy): Adjust.
	(expand_builtin_stpcpy): Same.
	(expand_builtin_with_bounds): Detect out-of-bounds accesses
	in pointer-checking forms of memcpy, memmove, and mempcpy.
	(gcall_to_tree_minimal, max_object_size): Define new functions.
	* builtins.h (max_object_size): Declare.
	* calls.c (alloc_max_size): Call max_object_size instead of
	hardcoding ssizetype limit.
	(get_size_range): Handle new argument.
	* calls.h (get_size_range): Add a new argument.
	* cfgexpand.c (expand_call_stmt): Propagate no-warning bit.
	* doc/invoke.texi (-Wrestrict): Adjust, add example.
	* gimple-fold.c (gimple_fold_builtin_memory_op): Detect overlapping
	operations.
	(gimple_fold_builtin_memory_chk): Same.
	(gimple_fold_builtin_stxcpy_chk): New function.
	* gimple-ssa-warn-restrict.c: New source.
	* gimple-ssa-warn-restrict.h: New header.
	* gimple.c (gimple_build_call_from_tree): Propagate location.
	* passes.def (pass_warn_restrict): Add new pass.
	* tree-pass.h (make_pass_warn_restrict): Declare.
	* tree-ssa-strlen.c (handle_builtin_strcpy): Detect overlapping
	operations.
	(handle_builtin_strcat): Same.
	(strlen_optimize_stmt): Rename...
	(strlen_check_and_optimize_stmt): ...to this.  Handle strncat,
	stpncpy, strncpy, and their checking forms.

gcc/testsuite/ChangeLog:

	PR tree-optimization/78918
	* c-c++-common/Warray-bounds.c: New test.
	* c-c++-common/Warray-bounds-2.c: New test.
	* c-c++-common/Warray-bounds-3.c: New test.
	* c-c++-common/Warray-bounds-4.c: New test.
	* c-c++-common/Warray-bounds-5.c: New test.
	* c-c++-common/Wrestrict-2.c: New test.
	* c-c++-common/Wrestrict.c: New test.
	* c-c++-common/Wrestrict.s: New test.
	* c-c++-common/Wsizeof-pointer-memaccess1.c: Adjust
	* c-c++-common/Wsizeof-pointer-memaccess2.c: Same.
	* g++.dg/torture/Wsizeof-pointer-memaccess1.C: Same.
	* g++.dg/torture/Wsizeof-pointer-memaccess2.C: Same.
	* gcc.dg/range.h: New header.
	* gcc.dg/memcpy-6.c: New test.
	* gcc.dg/pr69172.c: Adjust.
	* gcc.dg/pr79223.c: Same.
	* gcc.dg/pr81345.c: Adjust.
	* gcc.dg/Wobjsize-1.c: Same.
	* gcc.dg/Wrestrict-2.c: New test.
	* gcc.dg/Wrestrict.c: New test.
	* gcc.dg/Wsizeof-pointer-memaccess1.c: Adjust.
	* gcc.dg/builtin-stpncpy.c: Same.
	* gcc.dg/builtin-stringop-chk-1.c: Same.
	* gcc.target/i386/chkp-stropt-17.c: New test.
	* gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Adjust.


Added:
    trunk/gcc/gimple-ssa-warn-restrict.c
    trunk/gcc/gimple-ssa-warn-restrict.h
    trunk/gcc/testsuite/c-c++-common/Warray-bounds-2.c
    trunk/gcc/testsuite/c-c++-common/Warray-bounds-3.c
    trunk/gcc/testsuite/c-c++-common/Warray-bounds-4.c
    trunk/gcc/testsuite/c-c++-common/Warray-bounds-5.c
    trunk/gcc/testsuite/c-c++-common/Wrestrict-2.c
    trunk/gcc/testsuite/c-c++-common/Wrestrict.c
    trunk/gcc/testsuite/gcc.dg/Wrestrict-2.c
    trunk/gcc/testsuite/gcc.dg/Wrestrict.c
    trunk/gcc/testsuite/gcc.dg/memcpy-6.c
    trunk/gcc/testsuite/gcc.dg/range.h
    trunk/gcc/testsuite/gcc.target/i386/chkp-stropt-17.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/Makefile.in
    trunk/gcc/builtins.c
    trunk/gcc/builtins.h
    trunk/gcc/c-family/ChangeLog
    trunk/gcc/c-family/c-common.c
    trunk/gcc/c-family/c.opt
    trunk/gcc/calls.c
    trunk/gcc/calls.h
    trunk/gcc/cfgexpand.c
    trunk/gcc/doc/invoke.texi
    trunk/gcc/gimple-fold.c
    trunk/gcc/passes.def
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/c-c++-common/Warray-bounds.c
    trunk/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess1.c
    trunk/gcc/testsuite/c-c++-common/Wsizeof-pointer-memaccess2.c
    trunk/gcc/testsuite/g++.dg/torture/Wsizeof-pointer-memaccess1.C
    trunk/gcc/testsuite/g++.dg/torture/Wsizeof-pointer-memaccess2.C
    trunk/gcc/testsuite/gcc.dg/Wobjsize-1.c
    trunk/gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c
    trunk/gcc/testsuite/gcc.dg/builtin-stpncpy.c
    trunk/gcc/testsuite/gcc.dg/builtin-stringop-chk-1.c
    trunk/gcc/testsuite/gcc.dg/pr69172.c
    trunk/gcc/testsuite/gcc.dg/pr79223.c
    trunk/gcc/testsuite/gcc.dg/pr81345.c
    trunk/gcc/testsuite/gcc.dg/torture/Wsizeof-pointer-memaccess1.c
    trunk/gcc/tree-pass.h
    trunk/gcc/tree-ssa-strlen.c
Comment 6 Martin Sebor 2017-12-17 00:02:38 UTC
Patch committed in r255755.
Comment 7 Dmitry G. Dyachenko 2017-12-17 12:38:40 UTC
I'm a bit confused:
-- warning about `static inline bar()' inlined into `foo()'
-- and no warning about `baz()'

What is the difference?
May be no warnings will be more consistent?

$ cat x.c
#include <string.h>
static inline void bar(void *d, void *s, unsigned N)
{
    if(s != d)
	memcpy(d, s, N);
}

void baz(void *d, void *s, unsigned N)
{
    if(s != d)
	memcpy(d, s, N);
}

void foo(void* src)
{
    bar(src, src, 1);
}

$ gcc -O2 -S -Wall -Wextra -Wpedantic -Wrestrict x.c
x.c: In function ‘foo’:
x.c:5:2: warning: ‘memcpy’ source argument is the same as destination [-Wrestrict]
  memcpy(d, s, N);
  ^~~~~~~~~~~~~~~

$ gcc -v
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/local/gcc_current/libexec/gcc/x86_64-pc-linux-gnu/8.0.0/lto-wrapper
OFFLOAD_TARGET_NAMES=nvptx-none
Target: x86_64-pc-linux-gnu
Configured with: /home/dimhen/src/gcc_current/configure --prefix=/usr/local/gcc_current --enable-checking=yes,df,fold,rtl,extra --enable-languages=c,c++,lto --disable-multilib --enable-shared --enable-threads=posix --enable-__cxa_atexit --disable-libunwind-exceptions --enable-gnu-unique-object --enable-linker-build-id --with-linker-hash-style=gnu --enable-plugin --enable-initfini-array --with-isl --enable-libmpx --enable-offload-targets=nvptx-none --without-cuda-driver --enable-gnu-indirect-function --with-tune=native
Thread model: posix
gcc version 8.0.0 20171217 (experimental) [trunk revision 255759] (GCC) 

$ gcc -O2 -S -Wall -Wextra -Wpedantic -Wrestrict -fdump-tree-optimized=/dev/stdout x.c
x.c: In function ‘foo’:
x.c:5:2: warning: ‘memcpy’ source argument is the same as destination [-Wrestrict]
  memcpy(d, s, N);
  ^~~~~~~~~~~~~~~

;; Function baz (baz, funcdef_no=1, decl_uid=2076, cgraph_uid=1, symbol_order=1)

baz (void * d, void * s, unsigned int N)
{
  long unsigned int _1;

  <bb 2> [local count: 1073741825]:
  if (s_3(D) != d_4(D))
    goto <bb 3>; [53.47%]
  else
    goto <bb 4>; [46.53%]

  <bb 3> [local count: 574129753]:
  _1 = (long unsigned int) N_6(D);
  memcpy (d_4(D), s_3(D), _1); [tail call]

  <bb 4> [local count: 1073741825]:
  return;

}



;; Function foo (foo, funcdef_no=2, decl_uid=2079, cgraph_uid=2, symbol_order=2)

foo (void * src)
{
  <bb 2> [local count: 1073741825]:
  return;

}
Comment 8 Martin Sebor 2017-12-17 23:10:07 UTC
It's an unintended effect of warning during folding.  The folder sees the arguments propagated before the if conditional has been evaluated and before the call has been eliminated as unreachable.  The only way of avoiding this false positive that I can think of is to defer the folding but that's not a popular alternative.  I've opened bug 83456 to deal with it.  Please add your further comments there.
Comment 9 Christophe Lyon 2017-12-18 11:14:03 UTC
I've noticed that a new test (memcpy-6.c) fails on arm* targets:
FAIL:    gcc.dg/memcpy-6.c  (test for warnings, line 21)
FAIL:    gcc.dg/memcpy-6.c scan-tree-dump-not optimized "memcpy"
FAIL:    gcc.dg/memcpy-6.c scan-tree-dump-not optimized "memmove"

I've also noticed new failures on aarch64 and arm targets:
FAIL:    c-c++-common/Warray-bounds-4.c  -Wc++-compat   (test for warnings, line 67)

XPASS: c-c++-common/Warray-bounds-3.c  -Wc++-compat  strcpy (test for warnings, line 359)
Comment 10 Martin Sebor 2017-12-20 16:58:30 UTC
The memcpy-6.c failures are tracked in bug 83483.