Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Tom de Vries]

On 12/17/2017 01:01 AM, Martin Sebor wrote:


* c-c++-common/Wrestrict.c: New test.




   681/* The following doesn't overlap but it should trigger 
-Wstrinop-ovewrflow
   682   for writing past the end.  */
   683T ("012", a + sizeof a, a);


For nvptx, the warning actually shows up and is classified as excess error:
...
gcc/testsuite/c-c++-common/Wrestrict.c:683:3: warning: 
'__builtin_memcpy' writing 4 bytes into a region of size 0 overflows the 
destination [-Wstringop-overflow=]

...


   760r = SR (DIFF_MAX - 2, DIFF_MAX - 1);
   761T (8, "012", a + r, a);/* { dg-warning "accessing 4 bytes at offsets 
\\\[\[0-9\]+, \[0-9\]+] and 0 overlaps" "strcpy" } */
   762  


Likewise, the warning triggers here:
...
gcc/testsuite/c-c++-common/Wrestrict.c:761:3: warning: 
'__builtin_memcpy' writing 4 bytes into a region of size 0 overflows the 
destination [-Wstringop-overflow=]

...


>>> * c-c++-common/Warray-bounds-4.c: New test.


66TM ("0123", "",  ma.a5 + i, ma.a5); /* { dg-warning "offset 6 from the object at 
.ma. is out of the bounds of referenced subobject .a5. with type .char\\\[5]. at offset 0" "strcpy" { 
xfail *-*-* } } */
67TM ("", "012345", ma.a7 + i, ma.a7);/* { dg-warning "offset 13 from 
the object at .ma. is out of the bounds of referenced subobject .\(MA::\)?a7. with type .char ?\\\[7]. at 
offset 5" } */


And this warning fails to trigger:
...
FAIL: c-c++-common/Warray-bounds-4.c  -Wc++-compat   (test for warnings, 
line 67)

...

Thanks,
- Tom

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[H.J. Lu]

On Sat, Dec 16, 2017 at 4:01 PM, Martin Sebor  wrote:
> On 12/11/2017 03:27 PM, Jeff Law wrote:
>>
>> On 12/08/2017 12:19 PM, Martin Sebor wrote:
>>>
>>> Attached is revision 8 of the patch with the changes suggested
>>> and/or requested below.
>>
>>
>> [ Big snip. ]
>>
>>>
>>>
>>> gcc-78918.diff
>>>
>>>
>>> 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/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/memcpy-6.c: New test.
>>> * gcc.dg/pr69172.c: Adjust.
>>> * gcc.dg/pr79223.c: Same.
>>> * gcc.dg/Wrestrict-2.c: New test.
>>> * gcc.dg/Wrestrict.c: New test.
>>> * gcc.dg/Wsizeof-pointer-memaccess1.c
>>> * gcc.target/i386/chkp-stropt-17.c: New test.
>>> * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Adjust.
>>
>> OK.  Thanks for your patience.  I know this was a ton of work and even
>> more waiting.
>
>
> Thanks.  In more testing I uncovered a few minor glitches.  I've
> fixed those and committed r255755.  Attached is the committed patch
> for reference.
>
> Martin

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83446

-- 
H.J.

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 12/08/2017 12:19 PM, Martin Sebor wrote:
> Attached is revision 8 of the patch with the changes suggested
> and/or requested below.

[ Big snip. ]

> 
> 
> gcc-78918.diff
> 
> 
> 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/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/memcpy-6.c: New test.
>   * gcc.dg/pr69172.c: Adjust.
>   * gcc.dg/pr79223.c: Same.
>   * gcc.dg/Wrestrict-2.c: New test.
>   * gcc.dg/Wrestrict.c: New test.
>   * gcc.dg/Wsizeof-pointer-memaccess1.c
>   * gcc.target/i386/chkp-stropt-17.c: New test.
>   * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Adjust.
OK.  Thanks for your patience.  I know this was a ton of work and even
more waiting.

jeff

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 12/07/2017 03:23 PM, Jeff Law wrote:

On 11/29/2017 04:36 PM, Martin Sebor wrote:

I've finished reimplementing the patch as a standalone pass.
In the attached revision I also addressed your comments below
as well as Richard's to allowing the strlen optimizations even
for overlapping accesses.

While beefing up the tests I found a few minor issues that
I also fixed (false negatives).

The fallout wasn't quite as bad as I thought, mainly thanks
to the narrow API for the checker.

Syncing up with the latest trunk has led to some more changes
in tree-ssa-strlen.

I've retested the patch with GDB and Glibc with the same results
as before.

The patch seems sizable (over 3KLOC without tests) but it's worth
noting that most of the complexity is actually not in determining
whether or not an overlap exists (that's quite simple) but rather
in computing its offset and size to mention in the warnings and
making sure the information is meaningful to the user even when
ranges are involved.  All the subtly different forms of warnings
also contribute substantially to the overall size.

Martin

[ Huge snip. ]



gcc-78918.diff


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/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/memcpy-6.c: New test.
* gcc.dg/pr69172.c: Adjust.
* gcc.dg/pr79223.c: Same.
* gcc.dg/Wrestrict-2.c: New test.
* gcc.dg/Wrestrict.c: New test.
* gcc.dg/Wsizeof-pointer-memaccess1.c
* gcc.target/i386/chkp-stropt-17.c: New test.
* gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Adjust.

@@ -3874,32 +3885,32 @@ check_strncat_sizes (tree exp, tree objsize)
size_one_node)
 : NULL_TREE);

-  /* Strncat copies at most MAXLEN bytes and always appends the terminating
+  /* Strncat copies at most MAXREAD bytes and always appends the terminating

Nit.  Use "strncat" rather than "Strncat", even when starting a
sentence.  I saw this elsewhere.  You can fix these in a 

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 11/29/2017 04:36 PM, Martin Sebor wrote:
> I've finished reimplementing the patch as a standalone pass.
> In the attached revision I also addressed your comments below
> as well as Richard's to allowing the strlen optimizations even
> for overlapping accesses.
> 
> While beefing up the tests I found a few minor issues that
> I also fixed (false negatives).
> 
> The fallout wasn't quite as bad as I thought, mainly thanks
> to the narrow API for the checker.
> 
> Syncing up with the latest trunk has led to some more changes
> in tree-ssa-strlen.
> 
> I've retested the patch with GDB and Glibc with the same results
> as before.
> 
> The patch seems sizable (over 3KLOC without tests) but it's worth
> noting that most of the complexity is actually not in determining
> whether or not an overlap exists (that's quite simple) but rather
> in computing its offset and size to mention in the warnings and
> making sure the information is meaningful to the user even when
> ranges are involved.  All the subtly different forms of warnings
> also contribute substantially to the overall size.
> 
> Martin
[ Huge snip. ]

> 
> gcc-78918.diff
> 
> 
> 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/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/memcpy-6.c: New test.
>   * gcc.dg/pr69172.c: Adjust.
>   * gcc.dg/pr79223.c: Same.
>   * gcc.dg/Wrestrict-2.c: New test.
>   * gcc.dg/Wrestrict.c: New test.
>   * gcc.dg/Wsizeof-pointer-memaccess1.c
>   * gcc.target/i386/chkp-stropt-17.c: New test.
>   * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Adjust.
> 
> @@ -3874,32 +3885,32 @@ check_strncat_sizes (tree exp, tree objsize)
>   size_one_node)
>: NULL_TREE);
>  
> -  /* Strncat copies at most MAXLEN bytes and always appends the terminating
> +  /* Strncat copies at most MAXREAD bytes and always appends the terminating
Nit.  Use "strncat" rather than "Strncat", even when starting a
sentence.  I saw 

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 12/07/2017 02:28 PM, Martin Sebor wrote:
> On 12/07/2017 02:14 PM, Jeff Law wrote:
>> On 11/29/2017 04:36 PM, Martin Sebor wrote:
>>> I've finished reimplementing the patch as a standalone pass.
>>> In the attached revision I also addressed your comments below
>>> as well as Richard's to allowing the strlen optimizations even
>>> for overlapping accesses.
>>>
>>> While beefing up the tests I found a few minor issues that
>>> I also fixed (false negatives).
>>>
>>> The fallout wasn't quite as bad as I thought, mainly thanks
>>> to the narrow API for the checker.
>> So still reading though this, but wanted to start with a question I hope
>> you can answer quickly.
>>
>> In terms of coverage -- did we lose much in terms of cases that were
>> diagnosed in the original version, but aren't in this version?
> 
> I'm quite pleased to say that with the pass in the right place
> (after vrp) the coverage is the same.
That's awesome.
Jeff

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 12/07/2017 02:14 PM, Jeff Law wrote:

On 11/29/2017 04:36 PM, Martin Sebor wrote:

I've finished reimplementing the patch as a standalone pass.
In the attached revision I also addressed your comments below
as well as Richard's to allowing the strlen optimizations even
for overlapping accesses.

While beefing up the tests I found a few minor issues that
I also fixed (false negatives).

The fallout wasn't quite as bad as I thought, mainly thanks
to the narrow API for the checker.

So still reading though this, but wanted to start with a question I hope
you can answer quickly.

In terms of coverage -- did we lose much in terms of cases that were
diagnosed in the original version, but aren't in this version?


I'm quite pleased to say that with the pass in the right place
(after vrp) the coverage is the same.

Martin

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 11/29/2017 04:36 PM, Martin Sebor wrote:
> I've finished reimplementing the patch as a standalone pass.
> In the attached revision I also addressed your comments below
> as well as Richard's to allowing the strlen optimizations even
> for overlapping accesses.
> 
> While beefing up the tests I found a few minor issues that
> I also fixed (false negatives).
> 
> The fallout wasn't quite as bad as I thought, mainly thanks
> to the narrow API for the checker.
So still reading though this, but wanted to start with a question I hope
you can answer quickly.

In terms of coverage -- did we lose much in terms of cases that were
diagnosed in the original version, but aren't in this version?

jeff

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 11/27/2017 05:44 AM, Richard Biener wrote:
> 
> +
> +  if (const strinfo *chksi = olddsi ? olddsi : dsi)
> +if (si
> +   && !check_bounds_or_overlap (stmt, chksi->ptr, si->ptr, NULL_TREE, 
> len))
> +  /* Avoid transforming strcpy when out-of-bounds offsets or
> +overlapping access is detected.  */
> +  return;
> 
> as I said elsewhere diagnostics should not prevent optimization.  Your warning
> code isn't optimization-grade (that is, false positives are possible).
> 
> +   if (!check_bounds_or_overlap (stmt, dst, sptr, NULL_TREE, slen))
> + /* Avoid transforming strcat when out-of-bounds offsets or
> +overlapping access is detected.  */
> + return;
> +  }
> 
> Likewise.
> 
> +  if (!check_bounds_or_overlap (stmt, dst, sptr, dstlen, srcsize))
> + /* Avoid transforming strcat when out-of-bounds offsets or
> +overlapping access is detected.  */
> +   return;
> 
> Likewise.
I'll note that if we separate diagnostics from optimization these become
a non-issue.  The diagnostic bits simply wouldn't change code, plain and
simple... :-)

> 
> I have no strong opinion against the "code duplication" Jeff mentions with
> regarding to builtin_access and friends.  The relation to ao_ref and friends
> could be documented a bit and how builtin_memref/builtin_access are
> not suitable for optimization.
At the least documentation around not using those classes driving any
code generation changes/decisions seems wise.  We have the same issue
around compute_objsize/check_memop_sizes for the stringop-overflow patch
if I understand those bits correctly (i've got messages to get back to
on that thread as well..)

JEff

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 11/25/2017 05:53 PM, Martin Sebor wrote:
> On 11/22/2017 04:50 PM, Jeff Law wrote:
>> On 11/16/2017 02:29 PM, Martin Sebor wrote:
 On 10/23/2017 08:42 PM, Martin Sebor wrote:
> Attached is a reworked solution to enhance -Wrestrict while
> avoiding changing tree-vrp.c or any other VRP machinery.  Richard,
> in considering you suggestions I realized that the ao_ref struct
> isn't general enough to detect the kinds of problems I needed to
> etect (storing bit-offsets in HOST_WIDE_INT means out-of-bounds
> offsets cannot be represented or detected, leading to either false
> positives or false negatives).
>> So this seems to be a recurring theme, which makes me wonder if we
>> should have an ao_ref-like structure that deals in bytes rather than
>> bits and make it a first class citizen.   There's certainly clients that
>> work on bits and certainly clients that would prefer to work on bytes.
> 
> The class I introduced serves a different purpose than ao_ref and
> stores a lot more data.
> 
> In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82042#c3 Richard
> says that "this [offset being HOST_WIDE_INT and storing bits] is
> a know deficiency in ao_ref 'offset' (and also size and maxsize).
> Blowing up to offset_int isn't really a good idea."
Understood.  My point was that we seem to be stumbling into the same
class of problems in more than once place.  So we may want to consider
having a class that looks like like ao_ref, but which operates in byte
sized chunks.

I don't think it should be a requirement to go forward through.  It's
more of a long term question/concern.


>> So I realize you don't have any code to answer this question, but your
>> thoughts on how much we might loose effectiveness if we didn't do the
>> warnings within gimple_fold_builtin_, but instead broke out a
>> distinct pass to handle warnings?  My biggest design concern is the
>> warning from within the folder aspects.
> 
> With optimization enabled the folder folds things like this into
> MEM_REF which would prevent the warning unless the pass pass ran
> with early optimizations.
> 
>   struct S { char a[7], b[7]; };
> 
>   void sink (void*);
> 
>   void f (void)
>   {
>     struct S s;
>     sink ();
> 
>     unsigned n = sizeof s.a;
>     memcpy (s.a + 4, s.b, n);
> 
>     sink ();
>   }
> 
> Without optimization it isn't folded and so the memcpy call is
> emitted (but there is no warning).  The overlapping MEM_REF copy
> is safe but the overlapping memcpy call is not, so warning on it
> is helpful.
Understood.  I'm just trying to get a sense for how an implementation as
a separate pass would affect the quality of the diags we generate.  I
expect we'd lose some, the question is do we lose so many that the
ultimate result really isn't useful in practice and to balance that
against the various pros/cons of the different approaches.


> 
> I prototyped a pass over Thanksgiving for the -Wrestict code from
> builtins.c to and ran it just after the sprintf pass.  There are
> still lots of failures in the new test because it's non-trivial
> to compute the same data as builtins.c does (the data is also
> computed for -Wstringop-overflow).  I could move all that code
> to the new pass as well to clear up the failures.  I could also
> arrange for the pass to run multiple times to catch cases like
> the one above.  I suspect this would trigger some false negatives
> and/or positives due to the differences in range information, so
> it would mean some cleanup in the tests.  What I can't do without
> seriously compromising the feature is avoid calling into the new
> pass from tree-ssa-strlen.c, but that would presumably be fine.
I note that you posted that as a follow-up.  I'll take a look at it
momentarily.


> 
> With that said, and although I'm not necessarily opposed to it,
> moving all this code into its own pass would mean a non-trivial
> amount of work for what seems like a questionable benefit.  All
> it would achieve, as far as I can see, is duplicating some of
> the work that's already been done: iterating over the GIMPLE,
> testing for built-ins to handle, and extracting their arguments.
> What exactly do you hope to accomplish by moving it into its own
> pass?  (If it's a matter of keeping the warning code separate
> that can easily be done by moving it to its own file.)
By moving the warning into its own pass we get several benefits.

1. There is a general desire not to mix diagnostics and code
transformations.  Splitting it out is in line with that goal.

2. We get the flexibility to put the diagnostic pass wherever in the
pipeline makes the most sense.  We can even have early/late versions to
try and increase precision

3. Sometimes, but not always the analysis can be shared.  In that model
the underlying analysis engine is where the bulk of the work happens.
The optimizations and diagnostics are just clients of the analysis
module.  It also encourages re-use of the analysis module.  Oh how I
wish 

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 11/27/2017 05:44 AM, Richard Biener wrote:

On Thu, Nov 16, 2017 at 10:29 PM, Martin Sebor  wrote:

Ping.

I've fixed the outstanding false positive exposed by the Linux
kernel.  The kernel builds with four instances of the warning,
all of them valid (perfect overlap in memcpy).

I also built Glibc.  It shows one instance of the warning, also
a true positive (cause by calling a restrict-qualified function
with two copies of the same argument).

Finally, I built Binutils and GDB with no warnings.

The attached patch includes just that one fix, with everything
else being the same.


+ /* Detect invalid bounds and overlapping copies and issue
+either -Warray-bounds or -Wrestrict.  */
+ if (check_bounds_or_overlap (stmt, dest, src, len, len))
+   gimple_set_no_warning (stmt, true);

if (! gimple_no_warning (stmt)
&& ...)

to avoid repeated work if this call doesn't get folded.


Sure.



@@ -7295,3 +7342,4 @@ gimple_stmt_integer_valued_real_p (gimple *stmt,
int depth)
   return false;
 }
 }
+

please don't do unrelated whitespace changes.

+
+  if (const strinfo *chksi = olddsi ? olddsi : dsi)
+if (si
+   && !check_bounds_or_overlap (stmt, chksi->ptr, si->ptr, NULL_TREE, len))
+  /* Avoid transforming strcpy when out-of-bounds offsets or
+overlapping access is detected.  */
+  return;

as I said elsewhere diagnostics should not prevent optimization.  Your warning
code isn't optimization-grade (that is, false positives are possible).


I remember you pointing it out earlier and agreeing that warning
options should not disable/enable optimizations.  In this case,
if GCC can avoid the undefined behavior by folding a call that
would otherwise result from calling the library function, it
makes sense to go ahead and fold it.  I forgot to allow for
that in the last revision of my patch but I've fixed it in
the updated one I just posted.


+   if (!check_bounds_or_overlap (stmt, dst, sptr, NULL_TREE, slen))
+ /* Avoid transforming strcat when out-of-bounds offsets or
+overlapping access is detected.  */
+ return;
+  }

Likewise.

+  if (!check_bounds_or_overlap (stmt, dst, sptr, dstlen, srcsize))
+ /* Avoid transforming strcat when out-of-bounds offsets or
+overlapping access is detected.  */
+   return;

Likewise.

I have no strong opinion against the "code duplication" Jeff mentions with
regarding to builtin_access and friends.  The relation to ao_ref and friends
could be documented a bit and how builtin_memref/builtin_access are
not suitable for optimization.


I added a comment to the classes to make it clearer.  Although
now that the classes are in a dedicated warning pass there is
little risk of someone misusing them for optimization.

Thanks
Martin



Thanks,
Richard.



On 11/09/2017 04:57 PM, Martin Sebor wrote:


Ping:

  https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01642.html

On 10/23/2017 08:42 PM, Martin Sebor wrote:


Attached is a reworked solution to enhance -Wrestrict while
avoiding changing tree-vrp.c or any other VRP machinery.  Richard,
in considering you suggestions I realized that the ao_ref struct
isn't general enough to detect the kinds of problems I needed to
etect (storing bit-offsets in HOST_WIDE_INT means out-of-bounds
offsets cannot be represented or detected, leading to either false
positives or false negatives).

Instead, the solution adds a couple of small classes to builtins.c
to overcome this limitation (I'm contemplating moving them along
with -Wstringop-overflow to a separate file to keep builtins.c
from getting too much bigger).

The detection of out-of-bounds offsets and overlapping accesses
is relatively simple but the rest of the changes are somewhat
involved because of the computation of the offsets and sizes of
the overlaps.

Jeff, as per your suggestion/request in an earlier review (bug
81117) I've renamed some of the existing functions to better
reflect their new function (including renaming strlen_optimize_stmt
in tree-ssa-strlen.c to strlen_check_and_optimize_stmt).  There's
quite a bit of churn due to some of this renaming.  I don't think
this renaming makes the review too difficult but if you feel
differently I can [be persuaded to] split it out into a separate
patch.

To validate the patch I compiled the Linux kernel and Binutils/GDB.
There's one false positive I'm working on resolving that's caused
by an incorrect interpretation of an offset in a range whose lower
bound is greater than its upper bound.  This it so say that while
I'm aware the patch isn't perfect it already works reasonably well
in practice and I think it's close enough to review.

Thanks
Martin

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener]

On Thu, Nov 16, 2017 at 10:29 PM, Martin Sebor  wrote:
> Ping.
>
> I've fixed the outstanding false positive exposed by the Linux
> kernel.  The kernel builds with four instances of the warning,
> all of them valid (perfect overlap in memcpy).
>
> I also built Glibc.  It shows one instance of the warning, also
> a true positive (cause by calling a restrict-qualified function
> with two copies of the same argument).
>
> Finally, I built Binutils and GDB with no warnings.
>
> The attached patch includes just that one fix, with everything
> else being the same.

+ /* Detect invalid bounds and overlapping copies and issue
+either -Warray-bounds or -Wrestrict.  */
+ if (check_bounds_or_overlap (stmt, dest, src, len, len))
+   gimple_set_no_warning (stmt, true);

if (! gimple_no_warning (stmt)
&& ...)

to avoid repeated work if this call doesn't get folded.

@@ -7295,3 +7342,4 @@ gimple_stmt_integer_valued_real_p (gimple *stmt,
int depth)
   return false;
 }
 }
+

please don't do unrelated whitespace changes.

+
+  if (const strinfo *chksi = olddsi ? olddsi : dsi)
+if (si
+   && !check_bounds_or_overlap (stmt, chksi->ptr, si->ptr, NULL_TREE, len))
+  /* Avoid transforming strcpy when out-of-bounds offsets or
+overlapping access is detected.  */
+  return;

as I said elsewhere diagnostics should not prevent optimization.  Your warning
code isn't optimization-grade (that is, false positives are possible).

+   if (!check_bounds_or_overlap (stmt, dst, sptr, NULL_TREE, slen))
+ /* Avoid transforming strcat when out-of-bounds offsets or
+overlapping access is detected.  */
+ return;
+  }

Likewise.

+  if (!check_bounds_or_overlap (stmt, dst, sptr, dstlen, srcsize))
+ /* Avoid transforming strcat when out-of-bounds offsets or
+overlapping access is detected.  */
+   return;

Likewise.

I have no strong opinion against the "code duplication" Jeff mentions with
regarding to builtin_access and friends.  The relation to ao_ref and friends
could be documented a bit and how builtin_memref/builtin_access are
not suitable for optimization.

Thanks,
Richard.

>
> On 11/09/2017 04:57 PM, Martin Sebor wrote:
>>
>> Ping:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01642.html
>>
>> On 10/23/2017 08:42 PM, Martin Sebor wrote:
>>>
>>> Attached is a reworked solution to enhance -Wrestrict while
>>> avoiding changing tree-vrp.c or any other VRP machinery.  Richard,
>>> in considering you suggestions I realized that the ao_ref struct
>>> isn't general enough to detect the kinds of problems I needed to
>>> etect (storing bit-offsets in HOST_WIDE_INT means out-of-bounds
>>> offsets cannot be represented or detected, leading to either false
>>> positives or false negatives).
>>>
>>> Instead, the solution adds a couple of small classes to builtins.c
>>> to overcome this limitation (I'm contemplating moving them along
>>> with -Wstringop-overflow to a separate file to keep builtins.c
>>> from getting too much bigger).
>>>
>>> The detection of out-of-bounds offsets and overlapping accesses
>>> is relatively simple but the rest of the changes are somewhat
>>> involved because of the computation of the offsets and sizes of
>>> the overlaps.
>>>
>>> Jeff, as per your suggestion/request in an earlier review (bug
>>> 81117) I've renamed some of the existing functions to better
>>> reflect their new function (including renaming strlen_optimize_stmt
>>> in tree-ssa-strlen.c to strlen_check_and_optimize_stmt).  There's
>>> quite a bit of churn due to some of this renaming.  I don't think
>>> this renaming makes the review too difficult but if you feel
>>> differently I can [be persuaded to] split it out into a separate
>>> patch.
>>>
>>> To validate the patch I compiled the Linux kernel and Binutils/GDB.
>>> There's one false positive I'm working on resolving that's caused
>>> by an incorrect interpretation of an offset in a range whose lower
>>> bound is greater than its upper bound.  This it so say that while
>>> I'm aware the patch isn't perfect it already works reasonably well
>>> in practice and I think it's close enough to review.
>>>
>>> Thanks
>>> Martin
>>
>>
>

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 11/22/2017 04:50 PM, Jeff Law wrote:

On 11/16/2017 02:29 PM, Martin Sebor wrote:

Ping.

I've fixed the outstanding false positive exposed by the Linux
kernel.  The kernel builds with four instances of the warning,
all of them valid (perfect overlap in memcpy).

I also built Glibc.  It shows one instance of the warning, also
a true positive (cause by calling a restrict-qualified function
with two copies of the same argument).

Finally, I built Binutils and GDB with no warnings.

The attached patch includes just that one fix, with everything
else being the same.

On 11/09/2017 04:57 PM, Martin Sebor wrote:

Ping:

  https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01642.html

On 10/23/2017 08:42 PM, Martin Sebor wrote:

Attached is a reworked solution to enhance -Wrestrict while
avoiding changing tree-vrp.c or any other VRP machinery.  Richard,
in considering you suggestions I realized that the ao_ref struct
isn't general enough to detect the kinds of problems I needed to
etect (storing bit-offsets in HOST_WIDE_INT means out-of-bounds
offsets cannot be represented or detected, leading to either false
positives or false negatives).

So this seems to be a recurring theme, which makes me wonder if we
should have an ao_ref-like structure that deals in bytes rather than
bits and make it a first class citizen.   There's certainly clients that
work on bits and certainly clients that would prefer to work on bytes.


The class I introduced serves a different purpose than ao_ref and
stores a lot more data.

In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82042#c3 Richard
says that "this [offset being HOST_WIDE_INT and storing bits] is
a know deficiency in ao_ref 'offset' (and also size and maxsize).
Blowing up to offset_int isn't really a good idea."


Instead, the solution adds a couple of small classes to builtins.c
to overcome this limitation (I'm contemplating moving them along
with -Wstringop-overflow to a separate file to keep builtins.c
from getting too much bigger).

The detection of out-of-bounds offsets and overlapping accesses
is relatively simple but the rest of the changes are somewhat
involved because of the computation of the offsets and sizes of
the overlaps.

Jeff, as per your suggestion/request in an earlier review (bug
81117) I've renamed some of the existing functions to better
reflect their new function (including renaming strlen_optimize_stmt
in tree-ssa-strlen.c to strlen_check_and_optimize_stmt).  There's
quite a bit of churn due to some of this renaming.  I don't think
this renaming makes the review too difficult but if you feel
differently I can [be persuaded to] split it out into a separate
patch.

Understood.  FWIW, one way to deal with this that I've found works
reasonably well is to have an initial patch that just does the renaming
you want to do.  That separates the mechanical stuff from the meat of
the change.  They can be git squashed together just before committing or
committed as two changes back-to-back to eliminate or minimize the time
where the variable and function names are inconsistent.

It's also the case that independent little fixes should just go upstream
immediately.  I didn't see many, but the alloc_max_size fix for KB seems
like it should have just gone forward independent of the rest of the
changes.


Sure, I think that change was incidental, not intentional.  There
should be no others (AFAIK).


To validate the patch I compiled the Linux kernel and Binutils/GDB.
There's one false positive I'm working on resolving that's caused
by an incorrect interpretation of an offset in a range whose lower
bound is greater than its upper bound.  This it so say that while
I'm aware the patch isn't perfect it already works reasonably well
in practice and I think it's close enough to review.

Thanks
Martin





gcc-78918.diff


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
* builtins.c (builtin_memref, builtin_access): New classes.
(check_bounds_or_overlap, maybe_diag_overlap): New static functions.
(maybe_diag_offset_bounds): Same.
(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): 

Re: [PING 2][PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 11/16/2017 02:29 PM, Martin Sebor wrote:
> Ping.
> 
> I've fixed the outstanding false positive exposed by the Linux
> kernel.  The kernel builds with four instances of the warning,
> all of them valid (perfect overlap in memcpy).
> 
> I also built Glibc.  It shows one instance of the warning, also
> a true positive (cause by calling a restrict-qualified function
> with two copies of the same argument).
> 
> Finally, I built Binutils and GDB with no warnings.
> 
> The attached patch includes just that one fix, with everything
> else being the same.
> 
> On 11/09/2017 04:57 PM, Martin Sebor wrote:
>> Ping:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01642.html
>>
>> On 10/23/2017 08:42 PM, Martin Sebor wrote:
>>> Attached is a reworked solution to enhance -Wrestrict while
>>> avoiding changing tree-vrp.c or any other VRP machinery.  Richard,
>>> in considering you suggestions I realized that the ao_ref struct
>>> isn't general enough to detect the kinds of problems I needed to
>>> etect (storing bit-offsets in HOST_WIDE_INT means out-of-bounds
>>> offsets cannot be represented or detected, leading to either false
>>> positives or false negatives).
So this seems to be a recurring theme, which makes me wonder if we
should have an ao_ref-like structure that deals in bytes rather than
bits and make it a first class citizen.   There's certainly clients that
work on bits and certainly clients that would prefer to work on bytes.



>>> Instead, the solution adds a couple of small classes to builtins.c
>>> to overcome this limitation (I'm contemplating moving them along
>>> with -Wstringop-overflow to a separate file to keep builtins.c
>>> from getting too much bigger).
>>>
>>> The detection of out-of-bounds offsets and overlapping accesses
>>> is relatively simple but the rest of the changes are somewhat
>>> involved because of the computation of the offsets and sizes of
>>> the overlaps.
>>>
>>> Jeff, as per your suggestion/request in an earlier review (bug
>>> 81117) I've renamed some of the existing functions to better
>>> reflect their new function (including renaming strlen_optimize_stmt
>>> in tree-ssa-strlen.c to strlen_check_and_optimize_stmt).  There's
>>> quite a bit of churn due to some of this renaming.  I don't think
>>> this renaming makes the review too difficult but if you feel
>>> differently I can [be persuaded to] split it out into a separate
>>> patch.
Understood.  FWIW, one way to deal with this that I've found works
reasonably well is to have an initial patch that just does the renaming
you want to do.  That separates the mechanical stuff from the meat of
the change.  They can be git squashed together just before committing or
committed as two changes back-to-back to eliminate or minimize the time
where the variable and function names are inconsistent.

It's also the case that independent little fixes should just go upstream
immediately.  I didn't see many, but the alloc_max_size fix for KB seems
like it should have just gone forward independent of the rest of the
changes.


>>>
>>> To validate the patch I compiled the Linux kernel and Binutils/GDB.
>>> There's one false positive I'm working on resolving that's caused
>>> by an incorrect interpretation of an offset in a range whose lower
>>> bound is greater than its upper bound.  This it so say that while
>>> I'm aware the patch isn't perfect it already works reasonably well
>>> in practice and I think it's close enough to review.
>>>
>>> Thanks
>>> Martin
>>
> 
> 
> gcc-78918.diff
> 
> 
> 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
>   * builtins.c (builtin_memref, builtin_access): New classes.
>   (check_bounds_or_overlap, maybe_diag_overlap): New static functions.
>   (maybe_diag_offset_bounds): Same.
>   (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/overlapping
>   accesses in pointer-checking forms of memcpy, memmove, and mempcpy.
>   (gcall_to_tree_minimal, check_bounds_or_overlap, max_object_size):
>   

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 08/28/2017 06:27 PM, Martin Sebor wrote:
>> Correct.  I wound my way through this mess a while back.  Essentially
>> Red Hat had a customer with code that had overlapped memcpy arguments.
>> We had them use the memstomp interposition library to start tracking
>> these problems down.
>>
>> One of the things that popped up was structure/class copies which were
>> implemented via calls to memcpy.In the case of self assignment, the
>> interposition library would note the overlap and (rightly IMHO) complain.
> 
> Is this bug 32667?  I'm not having any luck reproducing it with
> any of the test cases there and varying struct sizes, or with
> the test case in the duplicate bug 65029 I filed for the same
> thing last year.  It would be nice to have a test case.
It certainly seems to be in the same class as 32667.  I'm not sure if
it's exactly the same root cause in terms of the front end interactions,
but the net result is the same.

I just reviewed the discussion in my outbox -- I didn't see sample code
or a reference to 32667, but I'm only looking at one side of the
conversation.


> 
>> One could argue that GCC should emit memmove by default for structure
>> assignments, only using memcpy when it knows its not doing self
>> assignment (which may be hard to determine).  Furthermore, GCC should
>> eliminate self structure/class assignment.
> 
> If it's still a problem emitting memmove seems like the right
> thing to do.  From what I've read the performance advantage of
> memcpy over memmove seems debatable at best.  Most performance
> sensitive code avoids making copies of very large objects so
> the only code that can be impacted doesn't care about efficiency
> quite so much.  For small enough objects, inlining the copy as
> GCC already does would obviate the efficiency concern altogether.
I personally feel we reached the point of diminishing returns years ago
--  in attempts to inline/unroll the copies in GCC, in terms of glibc's
copier implementations and in the gain of using memcpy over memmove vs
the pain of folks that can't be bothered to do it right WRT overlapped
copies.

But that's a bit of a digression.  THe issue at handle is structure
assignments that GCC is turning into memcpys.  That's a  much smaller
subset of calls to memcpy and more likely to not be a performance issue
to change.


>> A self-copy should just be folded away.  It's no different than x = x on
>> scalars except that we've dropped it to a memcpy in the IL.  Doing so
>> makes the code more efficient and removes false positives from tools
>> like the memstomp interposition library, making those tools more useful.
> 
> It's possible to do in simple cases but not in general.  I agree
> that in the general case when overlap is possible the only safe
> solution, short of actually testing for it at runtime, is to call
> memmove.
So let's zap those where we know its a self copy.  I'd look favorably on
a patch that use memmove on the others, but I won't immediately approve
it without wider buy-in as I expect it could well be controversial.
These should be separate patches as the former should go forward
immediately.

Jeff

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 08/28/2017 06:20 PM, Martin Sebor wrote:
>>
>> Warning for memcpy (p, p, ...) is going to fire false positives all
>> around
>> given the C++ FE emits those in some cases and optimization can
>> expose that we are dealing with self-assignments.  And *p = *p is
>> valid.
> 
> I changed it to only warn for calls to the library function and
> not to the __builtin_xxx.  Though I haven't been able to reproduce
> the problem you referring to (bug 32667) which makes me wonder if
> it's been fixed.
I doubt it it's been fully addressed.  I don't know if it's possible,
but if we could somehow flag the assignments as they're generated by the
front-ends for the structure copy, that would probably be best becuase
we could then ignore those, but detect on all the others.

> 
>>
>> @@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op
>> (gimple_stmt_iterator *gsi,
>> }
>> }
>>
>> +  /* Avoid folding the call if overlap is detected.  */
>> +  if (check_overlap && detect_overlap (loc, stmt, dest, src, len))
>> +   return false;
>> +
>>
>> no, please not.  You diagnosed the call (which might be a false positive)
>> so why keep it undefined?  The folded stmt will either have the same
>> semantics (aggregate copies expanded as memcpy) or have all reads
>> performed before writes.
> 
> My goal was to follow the approach reflected in the comments
> elsewhere in the file:
> 
> /* Out of bound array access.  Value is undefined,
>but don't fold.  */
So I misunderstood a bit from our call today.  As long as we have the
same semantics I don't have a real strong opinion about folding here.
There are cases where folding is going to enable further optimizations
and cases where it's likely to inhibit optimization.  A lot depends on
the exact context of the nearby code, the operands (particularly the size).

I doubt there's any consensus to be had on this topic because the
implications of folding at point likely aren't very predictable.

My personal opinion is we should only be folding this stuff early if we
collapse down to 1 or 2 stores.   Those are the cases that are most
likely going to result in further optimization.  The rest can probably
be deferred with marginal, if any, performance penalty.


> While gimple_fold_builtin_memory_op may be able to provide well-
> defined behavior for the otherwise undefined semantics in a small
> subset of cases, it doesn't attempt to fold many more that it
> otherwise could (it only folds calls with sizes that are powers
> of 2).  So it seems of dubious value to make an effort in this
> relatively small subset of cases.
I agree its of dubious value.  Again, no strong opinion from me, I'll go
with a majority opinion here.

> 
> In my experience, users also don't appreciate optimizations that
> "rely on" undefined behavior one way or the other.  What they would
> like to see instead is that when their compiler detects undefined
> behavior it diagnoses it but either doesn't use it to make
> optimization decisions, or uses it to disable them.  For calls to
> library functions, that in my view means making the call and not
> folding it.  (Btw., do we have some sort of a policy or guideline
> for how to handle such cases in general?)
It's never a fine crisp line.  There's almost always two sides to this
kind of question and they simply will never agree because they have
different priorities.

So we end up having to make a bit of a guess about how often the
particular undefined situation occurs in practice, what the implications
of optimizing around it might be, etc.  It's almost always the case that
some people complain, but that in and of itself is not enough to justify
never optimizing these cases.  It's a judgment call.

Jeff

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener]

On Tue, Aug 29, 2017 at 2:20 AM, Martin Sebor  wrote:
> On 08/22/2017 02:45 AM, Richard Biener wrote:
>>
>> On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor  wrote:
>>>
>>> On 08/09/2017 10:14 AM, Jeff Law wrote:


 On 08/06/2017 05:08 PM, Martin Sebor wrote:

>>
>> Well, simply because the way as implemented isn't a must-alias query
>> but maybe one that's good enough for warnings (reduces false positives
>> but surely doesn't eliminate them).
>
>
>
> I'm very interested in reducing the rate of false positives in
> these and all other warnings.  As I mentioned in my comments,
> I did my best to weed them out of the implementation by building
> GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
> a guarantee that there aren't any.  But the first implementation
> of any non-trivial feature is never perfect, and hardly any
> warning of sufficient complexity is free of false positives, no
> matter here it's implemented (the middle-end, front-end, or
> a standalone static analysis tool).  If you spotted some cases
> I had missed I'd certainly be grateful for examples.  Otherwise,
> they will undoubtedly be reported as more software is exposed
> to the warning and, if possible, fixed, as happens with all
> other warnings.


 I think Richi is saying that the must alias query you've built isn't
 proper/correct.  It's less about false positives for Richi and more
 about building a proper must alias query if I understand him correctly.

 I suspect he's also saying that you can't reasonably build must-alias on
 top of a may-alias query framework.  They're pretty different queries.

 If you need something that is close to, but not quite a must alias
 query, then you're going to have to make a argument for that and you
 can't call it a must alias query.
>>>
>>>
>>>
>>> Attached is an updated and simplified patch that avoids making
>>> changes to any of the may-alias functions.  It turns out that
>>> all the information the logic needs to determine the overlap
>>> is already in the ao_ref structures populated by
>>> ao_ref_init_from_ptr_and_size.  The only changes to the pass
>>> are the enhancement to ao_ref_init_from_ptr_and_size to make
>>> use of range info and the addition of the two new functions
>>> used by the -Wrestrict clients outside the pass.
>>
>>
>> Warning for memcpy (p, p, ...) is going to fire false positives all around
>> given the C++ FE emits those in some cases and optimization can
>> expose that we are dealing with self-assignments.  And *p = *p is
>> valid.
>
>
> I changed it to only warn for calls to the library function and
> not to the __builtin_xxx.  Though I haven't been able to reproduce
> the problem you referring to (bug 32667) which makes me wonder if
> it's been fixed.

@@ -699,6 +706,15 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
  DEST{,+LEN,+LEN-1}.  */
   if (operand_equal_p (src, dest, 0))
 {
+  /* 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,

You can have explicit calls to __builtin_memcpy so that check looks bogus to me.
I think there's no way to distinguish the cases and I'd simply remove the check
and set TREE_NO_WARNING on the implicit calls generated by frontends.

>>
>> @@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator
>> *gsi,
>> }
>> }
>>
>> +  /* Avoid folding the call if overlap is detected.  */
>> +  if (check_overlap && detect_overlap (loc, stmt, dest, src, len))
>> +   return false;
>> +
>>
>> no, please not.  You diagnosed the call (which might be a false positive)
>> so why keep it undefined?  The folded stmt will either have the same
>> semantics (aggregate copies expanded as memcpy) or have all reads
>> performed before writes.
>
>
> My goal was to follow the approach reflected in the comments
> elsewhere in the file:
>
> /* Out of bound array access.  Value is undefined,
>but don't fold.  */

This is done to keep -Warray-bounds triggering IIRC.  Also there's no
good value to fold to (well, we used zero).  I also vetoed emitting
warnings from folding code given that's inherently fragile ... (I think
I already said I don't like warning from gimple_fold_builtin_memory_op
too much either).

> While gimple_fold_builtin_memory_op may be able to provide well-
> defined behavior for the otherwise undefined semantics in a small
> subset of cases, it doesn't attempt to fold many more that it
> otherwise could (it only folds calls with sizes that are powers
> of 2).  So it seems of dubious value to make 

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 08/24/2017 04:09 PM, Jeff Law wrote:

On 08/22/2017 02:45 AM, Richard Biener wrote:

On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor  wrote:

On 08/09/2017 10:14 AM, Jeff Law wrote:


On 08/06/2017 05:08 PM, Martin Sebor wrote:



Well, simply because the way as implemented isn't a must-alias query
but maybe one that's good enough for warnings (reduces false positives
but surely doesn't eliminate them).



I'm very interested in reducing the rate of false positives in
these and all other warnings.  As I mentioned in my comments,
I did my best to weed them out of the implementation by building
GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
a guarantee that there aren't any.  But the first implementation
of any non-trivial feature is never perfect, and hardly any
warning of sufficient complexity is free of false positives, no
matter here it's implemented (the middle-end, front-end, or
a standalone static analysis tool).  If you spotted some cases
I had missed I'd certainly be grateful for examples.  Otherwise,
they will undoubtedly be reported as more software is exposed
to the warning and, if possible, fixed, as happens with all
other warnings.


I think Richi is saying that the must alias query you've built isn't
proper/correct.  It's less about false positives for Richi and more
about building a proper must alias query if I understand him correctly.

I suspect he's also saying that you can't reasonably build must-alias on
top of a may-alias query framework.  They're pretty different queries.

If you need something that is close to, but not quite a must alias
query, then you're going to have to make a argument for that and you
can't call it a must alias query.



Attached is an updated and simplified patch that avoids making
changes to any of the may-alias functions.  It turns out that
all the information the logic needs to determine the overlap
is already in the ao_ref structures populated by
ao_ref_init_from_ptr_and_size.  The only changes to the pass
are the enhancement to ao_ref_init_from_ptr_and_size to make
use of range info and the addition of the two new functions
used by the -Wrestrict clients outside the pass.


Warning for memcpy (p, p, ...) is going to fire false positives all around
given the C++ FE emits those in some cases and optimization can
expose that we are dealing with self-assignments.  And *p = *p is
valid.

Correct.  I wound my way through this mess a while back.  Essentially
Red Hat had a customer with code that had overlapped memcpy arguments.
We had them use the memstomp interposition library to start tracking
these problems down.

One of the things that popped up was structure/class copies which were
implemented via calls to memcpy.In the case of self assignment, the
interposition library would note the overlap and (rightly IMHO) complain.


Is this bug 32667?  I'm not having any luck reproducing it with
any of the test cases there and varying struct sizes, or with
the test case in the duplicate bug 65029 I filed for the same
thing last year.  It would be nice to have a test case.


One could argue that GCC should emit memmove by default for structure
assignments, only using memcpy when it knows its not doing self
assignment (which may be hard to determine).  Furthermore, GCC should
eliminate self structure/class assignment.


If it's still a problem emitting memmove seems like the right
thing to do.  From what I've read the performance advantage of
memcpy over memmove seems debatable at best.  Most performance
sensitive code avoids making copies of very large objects so
the only code that can be impacted doesn't care about efficiency
quite so much.  For small enough objects, inlining the copy as
GCC already does would obviate the efficiency concern altogether.






@@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
}
}

+  /* Avoid folding the call if overlap is detected.  */
+  if (check_overlap && detect_overlap (loc, stmt, dest, src, len))
+   return false;
+

no, please not.  You diagnosed the call (which might be a false positive)
so why keep it undefined?  The folded stmt will either have the same
semantics (aggregate copies expanded as memcpy) or have all reads
performed before writes.

So can we distinguish here between overlap and the self-copy case?


Yes, but only in a limited subset of cases.



A self-copy should just be folded away.  It's no different than x = x on
scalars except that we've dropped it to a memcpy in the IL.  Doing so
makes the code more efficient and removes false positives from tools
like the memstomp interposition library, making those tools more useful.


It's possible to do in simple cases but not in general.  I agree
that in the general case when overlap is possible the only safe
solution, short of actually testing for it at runtime, is to call
memmove.

Martin

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 08/22/2017 02:45 AM, Richard Biener wrote:

On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor  wrote:

On 08/09/2017 10:14 AM, Jeff Law wrote:


On 08/06/2017 05:08 PM, Martin Sebor wrote:



Well, simply because the way as implemented isn't a must-alias query
but maybe one that's good enough for warnings (reduces false positives
but surely doesn't eliminate them).



I'm very interested in reducing the rate of false positives in
these and all other warnings.  As I mentioned in my comments,
I did my best to weed them out of the implementation by building
GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
a guarantee that there aren't any.  But the first implementation
of any non-trivial feature is never perfect, and hardly any
warning of sufficient complexity is free of false positives, no
matter here it's implemented (the middle-end, front-end, or
a standalone static analysis tool).  If you spotted some cases
I had missed I'd certainly be grateful for examples.  Otherwise,
they will undoubtedly be reported as more software is exposed
to the warning and, if possible, fixed, as happens with all
other warnings.


I think Richi is saying that the must alias query you've built isn't
proper/correct.  It's less about false positives for Richi and more
about building a proper must alias query if I understand him correctly.

I suspect he's also saying that you can't reasonably build must-alias on
top of a may-alias query framework.  They're pretty different queries.

If you need something that is close to, but not quite a must alias
query, then you're going to have to make a argument for that and you
can't call it a must alias query.



Attached is an updated and simplified patch that avoids making
changes to any of the may-alias functions.  It turns out that
all the information the logic needs to determine the overlap
is already in the ao_ref structures populated by
ao_ref_init_from_ptr_and_size.  The only changes to the pass
are the enhancement to ao_ref_init_from_ptr_and_size to make
use of range info and the addition of the two new functions
used by the -Wrestrict clients outside the pass.


Warning for memcpy (p, p, ...) is going to fire false positives all around
given the C++ FE emits those in some cases and optimization can
expose that we are dealing with self-assignments.  And *p = *p is
valid.


I changed it to only warn for calls to the library function and
not to the __builtin_xxx.  Though I haven't been able to reproduce
the problem you referring to (bug 32667) which makes me wonder if
it's been fixed.



@@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
}
}

+  /* Avoid folding the call if overlap is detected.  */
+  if (check_overlap && detect_overlap (loc, stmt, dest, src, len))
+   return false;
+

no, please not.  You diagnosed the call (which might be a false positive)
so why keep it undefined?  The folded stmt will either have the same
semantics (aggregate copies expanded as memcpy) or have all reads
performed before writes.


My goal was to follow the approach reflected in the comments
elsewhere in the file:

/* Out of bound array access.  Value is undefined,
   but don't fold.  */

While gimple_fold_builtin_memory_op may be able to provide well-
defined behavior for the otherwise undefined semantics in a small
subset of cases, it doesn't attempt to fold many more that it
otherwise could (it only folds calls with sizes that are powers
of 2).  So it seems of dubious value to make an effort in this
relatively small subset of cases.

In my experience, users also don't appreciate optimizations that
"rely on" undefined behavior one way or the other.  What they would
like to see instead is that when their compiler detects undefined
behavior it diagnoses it but either doesn't use it to make
optimization decisions, or uses it to disable them.  For calls to
library functions, that in my view means making the call and not
folding it.  (Btw., do we have some sort of a policy or guideline
for how to handle such cases in general?)

With all that said, I don't see a big problem with proceeding with
the folding as you suggest either, so I did and added a comment
documenting it and a test to verify this guarantee.

I should also acknowledge that in my approach I forgot that once
the overlap has been diagnosed and the no-warning bit set, the
next call to gimple_fold_builtin_memory_op() with the same
statement would just go ahead and fold it anyway.  So the tests
were ineffective regardless.


The ao_ref_init_from_ptr_and_size change misses a changelog entry.

+detect_overlap (location_t loc, gimple *stmt, tree dst, tree src, tree size,
+   bool adjust /* = false */)
+{
+  ao_ref dstref, srcref;
+  unsigned HOST_WIDE_INT range[2];
+
+  /* Initialize and store the lower bound of a constant offset (in
+ bits), disregarding the offset for the destination.  */
+  ao_ref_init_from_ptr_and_size (, 

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 08/22/2017 02:45 AM, Richard Biener wrote:
> On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor  wrote:
>> On 08/09/2017 10:14 AM, Jeff Law wrote:
>>>
>>> On 08/06/2017 05:08 PM, Martin Sebor wrote:
>>>
>
> Well, simply because the way as implemented isn't a must-alias query
> but maybe one that's good enough for warnings (reduces false positives
> but surely doesn't eliminate them).


 I'm very interested in reducing the rate of false positives in
 these and all other warnings.  As I mentioned in my comments,
 I did my best to weed them out of the implementation by building
 GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
 a guarantee that there aren't any.  But the first implementation
 of any non-trivial feature is never perfect, and hardly any
 warning of sufficient complexity is free of false positives, no
 matter here it's implemented (the middle-end, front-end, or
 a standalone static analysis tool).  If you spotted some cases
 I had missed I'd certainly be grateful for examples.  Otherwise,
 they will undoubtedly be reported as more software is exposed
 to the warning and, if possible, fixed, as happens with all
 other warnings.
>>>
>>> I think Richi is saying that the must alias query you've built isn't
>>> proper/correct.  It's less about false positives for Richi and more
>>> about building a proper must alias query if I understand him correctly.
>>>
>>> I suspect he's also saying that you can't reasonably build must-alias on
>>> top of a may-alias query framework.  They're pretty different queries.
>>>
>>> If you need something that is close to, but not quite a must alias
>>> query, then you're going to have to make a argument for that and you
>>> can't call it a must alias query.
>>
>>
>> Attached is an updated and simplified patch that avoids making
>> changes to any of the may-alias functions.  It turns out that
>> all the information the logic needs to determine the overlap
>> is already in the ao_ref structures populated by
>> ao_ref_init_from_ptr_and_size.  The only changes to the pass
>> are the enhancement to ao_ref_init_from_ptr_and_size to make
>> use of range info and the addition of the two new functions
>> used by the -Wrestrict clients outside the pass.
> 
> Warning for memcpy (p, p, ...) is going to fire false positives all around
> given the C++ FE emits those in some cases and optimization can
> expose that we are dealing with self-assignments.  And *p = *p is
> valid.
Correct.  I wound my way through this mess a while back.  Essentially
Red Hat had a customer with code that had overlapped memcpy arguments.
We had them use the memstomp interposition library to start tracking
these problems down.

One of the things that popped up was structure/class copies which were
implemented via calls to memcpy.In the case of self assignment, the
interposition library would note the overlap and (rightly IMHO) complain.


One could argue that GCC should emit memmove by default for structure
assignments, only using memcpy when it knows its not doing self
assignment (which may be hard to determine).  Furthermore, GCC should
eliminate self structure/class assignment.


> 
> @@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator 
> *gsi,
> }
> }
> 
> +  /* Avoid folding the call if overlap is detected.  */
> +  if (check_overlap && detect_overlap (loc, stmt, dest, src, len))
> +   return false;
> +
> 
> no, please not.  You diagnosed the call (which might be a false positive)
> so why keep it undefined?  The folded stmt will either have the same
> semantics (aggregate copies expanded as memcpy) or have all reads
> performed before writes.
So can we distinguish here between overlap and the self-copy case?

A self-copy should just be folded away.  It's no different than x = x on
scalars except that we've dropped it to a memcpy in the IL.  Doing so
makes the code more efficient and removes false positives from tools
like the memstomp interposition library, making those tools more useful.




Jeff

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener]

On Mon, Aug 21, 2017 at 10:10 PM, Martin Sebor  wrote:
> On 08/09/2017 10:14 AM, Jeff Law wrote:
>>
>> On 08/06/2017 05:08 PM, Martin Sebor wrote:
>>

 Well, simply because the way as implemented isn't a must-alias query
 but maybe one that's good enough for warnings (reduces false positives
 but surely doesn't eliminate them).
>>>
>>>
>>> I'm very interested in reducing the rate of false positives in
>>> these and all other warnings.  As I mentioned in my comments,
>>> I did my best to weed them out of the implementation by building
>>> GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
>>> a guarantee that there aren't any.  But the first implementation
>>> of any non-trivial feature is never perfect, and hardly any
>>> warning of sufficient complexity is free of false positives, no
>>> matter here it's implemented (the middle-end, front-end, or
>>> a standalone static analysis tool).  If you spotted some cases
>>> I had missed I'd certainly be grateful for examples.  Otherwise,
>>> they will undoubtedly be reported as more software is exposed
>>> to the warning and, if possible, fixed, as happens with all
>>> other warnings.
>>
>> I think Richi is saying that the must alias query you've built isn't
>> proper/correct.  It's less about false positives for Richi and more
>> about building a proper must alias query if I understand him correctly.
>>
>> I suspect he's also saying that you can't reasonably build must-alias on
>> top of a may-alias query framework.  They're pretty different queries.
>>
>> If you need something that is close to, but not quite a must alias
>> query, then you're going to have to make a argument for that and you
>> can't call it a must alias query.
>
>
> Attached is an updated and simplified patch that avoids making
> changes to any of the may-alias functions.  It turns out that
> all the information the logic needs to determine the overlap
> is already in the ao_ref structures populated by
> ao_ref_init_from_ptr_and_size.  The only changes to the pass
> are the enhancement to ao_ref_init_from_ptr_and_size to make
> use of range info and the addition of the two new functions
> used by the -Wrestrict clients outside the pass.

Warning for memcpy (p, p, ...) is going to fire false positives all around
given the C++ FE emits those in some cases and optimization can
expose that we are dealing with self-assignments.  And *p = *p is
valid.

@@ -1028,6 +1066,10 @@ gimple_fold_builtin_memory_op (gimple_stmt_iterator *gsi,
}
}

+  /* Avoid folding the call if overlap is detected.  */
+  if (check_overlap && detect_overlap (loc, stmt, dest, src, len))
+   return false;
+

no, please not.  You diagnosed the call (which might be a false positive)
so why keep it undefined?  The folded stmt will either have the same
semantics (aggregate copies expanded as memcpy) or have all reads
performed before writes.

The ao_ref_init_from_ptr_and_size change misses a changelog entry.

+detect_overlap (location_t loc, gimple *stmt, tree dst, tree src, tree size,
+   bool adjust /* = false */)
+{
+  ao_ref dstref, srcref;
+  unsigned HOST_WIDE_INT range[2];
+
+  /* Initialize and store the lower bound of a constant offset (in
+ bits), disregarding the offset for the destination.  */
+  ao_ref_init_from_ptr_and_size (, dst, size, range);
+  ao_ref_init_from_ptr_and_size (, src, size, range);

just pass NULL range to the first call?

-  ref->ref = NULL_TREE;
+
+  if (offrng)
+offrng[0] = offrng[1] = 0;
+
+ ref->ref = NULL_TREE;

bogus indent

+ else if (offrng && TREE_CODE (offset) == SSA_NAME)
+   {
+ wide_int min, max;
+ value_range_type rng = get_range_info (offset, , );
+ if (rng == VR_RANGE && wi::fits_uhwi_p (min))
+   {
+ ptr = gimple_assign_rhs1 (stmt);
+ offrng[0] = BITS_PER_UNIT * min.to_uhwi ();
+ offrng[1] = BITS_PER_UNIT * max.to_uhwi ();
+
+ extra_offset = offrng[0];

you didnt' check whether max fits uhwi.  The effect of passing offrng
is not documented.

+ else
+   /* Offset range is indeterminate.  */
+   offrng[0] = offrng[1] = HOST_WIDE_INT_M1U;

I believe a cleaner interface would be to do

void
ao_ref_init_from_ptr_and_size (ao_ref *ref, tree ptr, tree size, tree
*var_byte_offset)

and set *var_byte_offset to your 'offset' above, leaving 'ref'
unchanged.  The caller
can then get at the range info of var_byte_offset and adjust
ref->offset if desired.
The indeterminate state is then much cleaner - NULL_TREE.

+unsigned HOST_WIDE_INT
+refs_overlap (ao_ref *ref1, ao_ref *ref2, unsigned HOST_WIDE_INT *aloff)
+{

bool
refs_must_overlap_p (ao_ref *, ao_ref *, unsigned HOST_WIDE_INT *off,
unsinged HOST_WIDE_INT *size)

your return values are in bytes thus

+
+  // Compare pointers.
+  offset1 += mem_ref_offset (base1) << 

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 08/09/2017 10:14 AM, Jeff Law wrote:

On 08/06/2017 05:08 PM, Martin Sebor wrote:



Well, simply because the way as implemented isn't a must-alias query
but maybe one that's good enough for warnings (reduces false positives
but surely doesn't eliminate them).


I'm very interested in reducing the rate of false positives in
these and all other warnings.  As I mentioned in my comments,
I did my best to weed them out of the implementation by building
GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
a guarantee that there aren't any.  But the first implementation
of any non-trivial feature is never perfect, and hardly any
warning of sufficient complexity is free of false positives, no
matter here it's implemented (the middle-end, front-end, or
a standalone static analysis tool).  If you spotted some cases
I had missed I'd certainly be grateful for examples.  Otherwise,
they will undoubtedly be reported as more software is exposed
to the warning and, if possible, fixed, as happens with all
other warnings.

I think Richi is saying that the must alias query you've built isn't
proper/correct.  It's less about false positives for Richi and more
about building a proper must alias query if I understand him correctly.

I suspect he's also saying that you can't reasonably build must-alias on
top of a may-alias query framework.  They're pretty different queries.

If you need something that is close to, but not quite a must alias
query, then you're going to have to make a argument for that and you
can't call it a must alias query.


Attached is an updated and simplified patch that avoids making
changes to any of the may-alias functions.  It turns out that
all the information the logic needs to determine the overlap
is already in the ao_ref structures populated by
ao_ref_init_from_ptr_and_size.  The only changes to the pass
are the enhancement to ao_ref_init_from_ptr_and_size to make
use of range info and the addition of the two new functions
used by the -Wrestrict clients outside the pass.

Martin
PR middle-end/78918 - missing -Wrestrict on memcpy copying over self

gcc/ChangeLog:

	PR middle-end/78918
	* builtins.c (warn_for_overlap, maybe_warn_for_overlap): New.
	(check_sizes): Add argument and call maybe_warn_for_overlap.
	Rename function arguments for clarity.
	(check_memop_sizes): Adjust.
	(expand_builtin_memchr): Ditto.
	(expand_builtin_strcat): Ditto.
	(expand_builtin_strcpy): Ditto.
	(expand_builtin_stpcpy): Ditto.
	(expand_builtin_stpncpy): Ditto.
	(expand_builtin_strncpy): Ditto.
	(expand_builtin_memcmp): Ditto.
	(expand_builtin_memory_chk): Ditto.
	(check_strncat_sizes): Ditto.  Rename locals for clarity.
	(expand_builtin_strncat): Ditto.
	(maybe_emit_chk_warning): Ditto.
	(maybe_emit_sprintf_chk_warning): Adjust.
	* cfgexpand.c (expand_call_stmt): Set TREE_NO_WARNING.
	* gimple-fold.c (gimple_fold_builtin_memory_op): Handle -Wrestrict.
	(gimple_fold_builtin_strcpy): Ditto.
	(gimple_fold_builtin_memory_chk): Ditto.
	(gimple_fold_builtin_stxcpy_chk): Ditto.
	* gimple.c (gimple_build_call_from_tree): Set call location.
	* tree-ssa-alias.h (refs_overlap, detect_overlap): New functions.
	* tree-ssa-alias.c (refs_overlap, detect_overlap): Define.
	* tree-ssa-strlen.c (handle_builtin_strcpy): Handle -Wrestrict.
	(handle_builtin_strcat): Ditto.

gcc/c-family/ChangeLog:

	PR middle-end/78918
	* c-common.c (check_function_restrict): Suppress warning for
	built-in functions.
	* c.opt (-Wrestrict): Include in -Wall.

gcc/testsuite/ChangeLog:

	PR middle-end/78918
	* c-c++-common/Wrestrict.c: New test.
	* gcc.dg/Walloca-1.c: Suppress macro expansion tracking.
	* gcc.dg/pr69172.c: Prune -Wrestrict.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 2deef72..e0fd9a7 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -3036,39 +3036,303 @@ expand_builtin_memcpy_args (tree dest, tree src, tree len, rtx target, tree exp)
   return dest_addr;
 }
 
+/* Issue a warning for a restricted copy call expression EXP to a built-in
+   function FUNC, with a destination of size DSTSIZE, size of copy in RANGE,
+   and with OVERLAP bytes at offset OFFRANGE.  MUST_OVERLAP is true when
+   the overlap is certain, false when it is likely.  */
+
+static void
+warn_for_overlap (tree exp, tree func, bool must_overlap, tree dstsize,
+		  const tree range[2], unsigned HOST_WIDE_INT overlap,
+		  const unsigned HOST_WIDE_INT offrange[2])
+{
+  location_t loc = tree_nonartificial_location (exp);
+  loc = expansion_point_location_if_in_system_header (loc);
+
+  /* To avoid combinatorial explosion of diagnostics format the offset
+ or its range as a string and use it in the warning calls below.  */
+  char offstr[64];
+  if (offrange[0] == offrange[1] || offrange[1] > HOST_WIDE_INT_MAX)
+sprintf (offstr, "%llu", (long long) offrange[0]);
+  else
+sprintf (offstr, "%llu - %llu", (long long) offrange[0],
+	 (long long) offrange[1]);
+
+  /* The text uses the term "writing N bytes" even though most operations
+ 

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 08/06/2017 05:08 PM, Martin Sebor wrote:

>>
>> Well, simply because the way as implemented isn't a must-alias query
>> but maybe one that's good enough for warnings (reduces false positives
>> but surely doesn't eliminate them).
> 
> I'm very interested in reducing the rate of false positives in
> these and all other warnings.  As I mentioned in my comments,
> I did my best to weed them out of the implementation by building
> GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
> a guarantee that there aren't any.  But the first implementation
> of any non-trivial feature is never perfect, and hardly any
> warning of sufficient complexity is free of false positives, no
> matter here it's implemented (the middle-end, front-end, or
> a standalone static analysis tool).  If you spotted some cases
> I had missed I'd certainly be grateful for examples.  Otherwise,
> they will undoubtedly be reported as more software is exposed
> to the warning and, if possible, fixed, as happens with all
> other warnings.
I think Richi is saying that the must alias query you've built isn't
proper/correct.  It's less about false positives for Richi and more
about building a proper must alias query if I understand him correctly.

I suspect he's also saying that you can't reasonably build must-alias on
top of a may-alias query framework.  They're pretty different queries.

If you need something that is close to, but not quite a must alias
query, then you're going to have to make a argument for that and you
can't call it a must alias query.


> 
>> There's a must alias query already, in stmt_kills_ref_p.  It's a matter
>> of refactoring to make a refs_must_alias_p.
>>
>> Then propose that "overlap range" stuff separately.
> 
> I appreciate constructive suggestions for improvements  and I will
> look into the stmt_kills_ref_p suggestion.  But since my work
> elicited such an strong response from you I feel I should explain:
> I tried to make my changes as unintrusive as possible.  The alias
> oracle is a new area for me and I didn't want to make mistakes in
> the process of making overly extensive modifications to it.
Note that I've got a reasonably good handle on how we use
stmt_kills_ref_p.  So I can help with questions on that side.

Jeff

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 08/03/2017 02:45 AM, Richard Biener wrote:
> 
> Well, simply because the way as implemented isn't a must-alias query
> but maybe one that's good enough for warnings (reduces false positives
> but surely doesn't eliminate them).
OK.  So it's more about building a proper must-alias query and less
about exploiting the alias oracle to give us more precise information.
That was the source of my confusion.

> 
> There's a must alias query already, in stmt_kills_ref_p.  It's a matter
> of refactoring to make a refs_must_alias_p.
Funny you should mention that -- stmt_kills_ref_p is what I had in mind
when Martin first posted this work.  DSE essentially raises a must-alias
query via that interface and I'd fully support refactoring
stmt_kills_ref_p so that it would be used for DSE as well as Martin's work.

> 
> Then propose that "overlap range" stuff separately.
That works for me.

> 
> But I'm against lumping this all in as an innocent change suggesting
> the machinery can do sth (must alias) when it really can't.  I also
> do not like adding a "must alias" bool to the may-alias APIs as
> the implementation is fundamentally may-alias, must-alias really
> is very different.
That works for me.


> 
> And to repeat, no, I do not want a "good-enough-for-warnings" must-alias
> in an API that's supposed to be used by optimizations where "good enough"
> is not good enough.
Understood and agreed.

I'll note that I suspect Martin's desire to reduce false positives and
potentially weaken things here is driven by the pushback he's had on
implementing warnings with a "too high" false positive rate in the past.

jeff

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener]

On Mon, Aug 7, 2017 at 1:08 AM, Martin Sebor  wrote:
> On 08/03/2017 02:45 AM, Richard Biener wrote:
>>
>> On Wed, Aug 2, 2017 at 7:10 PM, Jeff Law  wrote:
>>>
>>> On 08/01/2017 03:25 AM, Richard Biener wrote:

 On Tue, Aug 1, 2017 at 11:23 AM, Richard Biener
  wrote:
>
> On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor  wrote:
>>
>> Richard,
>>
>> in discussing this work Jeff mentioned that your comments on
>> the tree-ssa-alias.c parts would be helpful.  When you have
>> a chance could you please give it a once over and let me know
>> if you have any suggestions or concerns?  There are no visible
>> changes to existing clients of the pass, just extensions that
>> are relied on only by the new diagnostics.
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html
>>
>> I expect to revisit the sprintf %s patch mentioned below and
>> see how to simplify it by taking advantage of the changes
>> implemented here.
>
>
> Your ptr_deref_alias_decl_p returns true when must_alias is true
> but there is no must-alias relationship.
>
> The ao_ref_init_from_ptr_and_size doesn't belong there.
>
> I dislike all of the changes related to returning an alias "size".
>
> Please keep away from the alias machinery.
>
> Warning during folding is similarly bad design.  Please don't add
> more such things.
>
> Thanks for putting this on my radar though.
> Richard.


 Oh, for a constructive comment this all feels like sth for either
 sanitizers or a proper static analysis tool.  As I outlined repeatedly
 I belive GCC could host a static analysis tool but it surely should
 not be interwinded into it's optimization passes or tools.
>>>
>>> I disagree strongly here.
>>>
>>> Sanitiers catch problems after the fact and only if you've compiled your
>>> code with sanitization and manage to find a way to trigger the problem
>>> path.
>>>
>>> Other static analysis tools are useful, but they aren't an integral part
>>> of the edit, compile, debug cycle and due to their cost are often run
>>> long after code was committed.
>>>
>>> Finding useful warnings that can be issued as part of the compile, edit,
>>> debug cycle is, IMHO, far more useful than sanitizers or independent
>>> static checkers.
>>>
>>> --
>>>
>>>
>>> I think finding a way to exploit information that our various analyzers
>>> can provide to give precise warnings is a good thing.  So it seemed
>>> natural that if we wanted to ask a must-alias question that we should be
>>> querying the alias oracle rather than implementing that kind of query
>>> within the sprintf warnings.  I'm not sure why you'd say "Please keep
>>> away from the alias machinery".
>>>
>>> I'm a little confused here...
>>
>>
>> Well, simply because the way as implemented isn't a must-alias query
>> but maybe one that's good enough for warnings (reduces false positives
>> but surely doesn't eliminate them).
>
>
> I'm very interested in reducing the rate of false positives in
> these and all other warnings.  As I mentioned in my comments,
> I did my best to weed them out of the implementation by building
> GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
> a guarantee that there aren't any.  But the first implementation
> of any non-trivial feature is never perfect, and hardly any
> warning of sufficient complexity is free of false positives, no
> matter here it's implemented (the middle-end, front-end, or
> a standalone static analysis tool).  If you spotted some cases
> I had missed I'd certainly be grateful for examples.  Otherwise,
> they will undoubtedly be reported as more software is exposed
> to the warning and, if possible, fixed, as happens with all
> other warnings.
>
>> There's a must alias query already, in stmt_kills_ref_p.  It's a matter
>> of refactoring to make a refs_must_alias_p.
>>
>> Then propose that "overlap range" stuff separately.
>
>
> I appreciate constructive suggestions for improvements  and I will
> look into the stmt_kills_ref_p suggestion.  But since my work
> elicited such an strong response from you I feel I should explain:
> I tried to make my changes as unintrusive as possible.  The alias
> oracle is a new area for me and I didn't want to make mistakes in
> the process of making overly extensive modifications to it.
>
>> But I'm against lumping this all in as an innocent change suggesting
>> the machinery can do sth (must alias) when it really can't.  I also
>> do not like adding a "must alias" bool to the may-alias APIs as
>> the implementation is fundamentally may-alias, must-alias really
>> is very different.
>
>
> I certainly want to do the right thing and implement the warning
> in a way that makes the most sense.  As I said, I'll look into
> the refactoring, but since my testing shows the current code to

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Martin Sebor]

On 08/03/2017 02:45 AM, Richard Biener wrote:

On Wed, Aug 2, 2017 at 7:10 PM, Jeff Law  wrote:

On 08/01/2017 03:25 AM, Richard Biener wrote:

On Tue, Aug 1, 2017 at 11:23 AM, Richard Biener
 wrote:

On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor  wrote:

Richard,

in discussing this work Jeff mentioned that your comments on
the tree-ssa-alias.c parts would be helpful.  When you have
a chance could you please give it a once over and let me know
if you have any suggestions or concerns?  There are no visible
changes to existing clients of the pass, just extensions that
are relied on only by the new diagnostics.

  https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html

I expect to revisit the sprintf %s patch mentioned below and
see how to simplify it by taking advantage of the changes
implemented here.


Your ptr_deref_alias_decl_p returns true when must_alias is true
but there is no must-alias relationship.

The ao_ref_init_from_ptr_and_size doesn't belong there.

I dislike all of the changes related to returning an alias "size".

Please keep away from the alias machinery.

Warning during folding is similarly bad design.  Please don't add
more such things.

Thanks for putting this on my radar though.
Richard.


Oh, for a constructive comment this all feels like sth for either
sanitizers or a proper static analysis tool.  As I outlined repeatedly
I belive GCC could host a static analysis tool but it surely should
not be interwinded into it's optimization passes or tools.

I disagree strongly here.

Sanitiers catch problems after the fact and only if you've compiled your
code with sanitization and manage to find a way to trigger the problem path.

Other static analysis tools are useful, but they aren't an integral part
of the edit, compile, debug cycle and due to their cost are often run
long after code was committed.

Finding useful warnings that can be issued as part of the compile, edit,
debug cycle is, IMHO, far more useful than sanitizers or independent
static checkers.

--


I think finding a way to exploit information that our various analyzers
can provide to give precise warnings is a good thing.  So it seemed
natural that if we wanted to ask a must-alias question that we should be
querying the alias oracle rather than implementing that kind of query
within the sprintf warnings.  I'm not sure why you'd say "Please keep
away from the alias machinery".

I'm a little confused here...


Well, simply because the way as implemented isn't a must-alias query
but maybe one that's good enough for warnings (reduces false positives
but surely doesn't eliminate them).


I'm very interested in reducing the rate of false positives in
these and all other warnings.  As I mentioned in my comments,
I did my best to weed them out of the implementation by building
GDB, Glibc, Busybox, and the Linux kernel.  That of course isn't
a guarantee that there aren't any.  But the first implementation
of any non-trivial feature is never perfect, and hardly any
warning of sufficient complexity is free of false positives, no
matter here it's implemented (the middle-end, front-end, or
a standalone static analysis tool).  If you spotted some cases
I had missed I'd certainly be grateful for examples.  Otherwise,
they will undoubtedly be reported as more software is exposed
to the warning and, if possible, fixed, as happens with all
other warnings.


There's a must alias query already, in stmt_kills_ref_p.  It's a matter
of refactoring to make a refs_must_alias_p.

Then propose that "overlap range" stuff separately.


I appreciate constructive suggestions for improvements  and I will
look into the stmt_kills_ref_p suggestion.  But since my work
elicited such an strong response from you I feel I should explain:
I tried to make my changes as unintrusive as possible.  The alias
oracle is a new area for me and I didn't want to make mistakes in
the process of making overly extensive modifications to it.


But I'm against lumping this all in as an innocent change suggesting
the machinery can do sth (must alias) when it really can't.  I also
do not like adding a "must alias" bool to the may-alias APIs as
the implementation is fundamentally may-alias, must-alias really
is very different.


I certainly want to do the right thing and implement the warning
in a way that makes the most sense.  As I said, I'll look into
the refactoring, but since my testing shows the current code to
work well as is, it would be helpful if you could provide more
details about what it is that concerns you with it and what cases
of false positives you are worried about.  (Examples of code that
demonstrate the false positives would be especially valuable.)

That being said, after much thought, I do have to let you know
that I take offense at both your tone and your insinuation that
I tried to sneak in some subversive changes.  I did what I thought
was right.  If it can be done better I'm glad to hear the 

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener]

On Wed, Aug 2, 2017 at 7:10 PM, Jeff Law  wrote:
> On 08/01/2017 03:25 AM, Richard Biener wrote:
>> On Tue, Aug 1, 2017 at 11:23 AM, Richard Biener
>>  wrote:
>>> On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor  wrote:
 Richard,

 in discussing this work Jeff mentioned that your comments on
 the tree-ssa-alias.c parts would be helpful.  When you have
 a chance could you please give it a once over and let me know
 if you have any suggestions or concerns?  There are no visible
 changes to existing clients of the pass, just extensions that
 are relied on only by the new diagnostics.

   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html

 I expect to revisit the sprintf %s patch mentioned below and
 see how to simplify it by taking advantage of the changes
 implemented here.
>>>
>>> Your ptr_deref_alias_decl_p returns true when must_alias is true
>>> but there is no must-alias relationship.
>>>
>>> The ao_ref_init_from_ptr_and_size doesn't belong there.
>>>
>>> I dislike all of the changes related to returning an alias "size".
>>>
>>> Please keep away from the alias machinery.
>>>
>>> Warning during folding is similarly bad design.  Please don't add
>>> more such things.
>>>
>>> Thanks for putting this on my radar though.
>>> Richard.
>>
>> Oh, for a constructive comment this all feels like sth for either
>> sanitizers or a proper static analysis tool.  As I outlined repeatedly
>> I belive GCC could host a static analysis tool but it surely should
>> not be interwinded into it's optimization passes or tools.
> I disagree strongly here.
>
> Sanitiers catch problems after the fact and only if you've compiled your
> code with sanitization and manage to find a way to trigger the problem path.
>
> Other static analysis tools are useful, but they aren't an integral part
> of the edit, compile, debug cycle and due to their cost are often run
> long after code was committed.
>
> Finding useful warnings that can be issued as part of the compile, edit,
> debug cycle is, IMHO, far more useful than sanitizers or independent
> static checkers.
>
> --
>
>
> I think finding a way to exploit information that our various analyzers
> can provide to give precise warnings is a good thing.  So it seemed
> natural that if we wanted to ask a must-alias question that we should be
> querying the alias oracle rather than implementing that kind of query
> within the sprintf warnings.  I'm not sure why you'd say "Please keep
> away from the alias machinery".
>
> I'm a little confused here...

Well, simply because the way as implemented isn't a must-alias query
but maybe one that's good enough for warnings (reduces false positives
but surely doesn't eliminate them).

There's a must alias query already, in stmt_kills_ref_p.  It's a matter
of refactoring to make a refs_must_alias_p.

Then propose that "overlap range" stuff separately.

But I'm against lumping this all in as an innocent change suggesting
the machinery can do sth (must alias) when it really can't.  I also
do not like adding a "must alias" bool to the may-alias APIs as
the implementation is fundamentally may-alias, must-alias really
is very different.

And to repeat, no, I do not want a "good-enough-for-warnings" must-alias
in an API that's supposed to be used by optimizations where "good enough"
is not good enough.

Richard.

>
>
> Jeff

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Jeff Law]

On 08/01/2017 03:25 AM, Richard Biener wrote:
> On Tue, Aug 1, 2017 at 11:23 AM, Richard Biener
>  wrote:
>> On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor  wrote:
>>> Richard,
>>>
>>> in discussing this work Jeff mentioned that your comments on
>>> the tree-ssa-alias.c parts would be helpful.  When you have
>>> a chance could you please give it a once over and let me know
>>> if you have any suggestions or concerns?  There are no visible
>>> changes to existing clients of the pass, just extensions that
>>> are relied on only by the new diagnostics.
>>>
>>>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html
>>>
>>> I expect to revisit the sprintf %s patch mentioned below and
>>> see how to simplify it by taking advantage of the changes
>>> implemented here.
>>
>> Your ptr_deref_alias_decl_p returns true when must_alias is true
>> but there is no must-alias relationship.
>>
>> The ao_ref_init_from_ptr_and_size doesn't belong there.
>>
>> I dislike all of the changes related to returning an alias "size".
>>
>> Please keep away from the alias machinery.
>>
>> Warning during folding is similarly bad design.  Please don't add
>> more such things.
>>
>> Thanks for putting this on my radar though.
>> Richard.
> 
> Oh, for a constructive comment this all feels like sth for either
> sanitizers or a proper static analysis tool.  As I outlined repeatedly
> I belive GCC could host a static analysis tool but it surely should
> not be interwinded into it's optimization passes or tools.
I disagree strongly here.

Sanitiers catch problems after the fact and only if you've compiled your
code with sanitization and manage to find a way to trigger the problem path.

Other static analysis tools are useful, but they aren't an integral part
of the edit, compile, debug cycle and due to their cost are often run
long after code was committed.

Finding useful warnings that can be issued as part of the compile, edit,
debug cycle is, IMHO, far more useful than sanitizers or independent
static checkers.

--


I think finding a way to exploit information that our various analyzers
can provide to give precise warnings is a good thing.  So it seemed
natural that if we wanted to ask a must-alias question that we should be
querying the alias oracle rather than implementing that kind of query
within the sprintf warnings.  I'm not sure why you'd say "Please keep
away from the alias machinery".

I'm a little confused here...


Jeff

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener]

On Tue, Aug 1, 2017 at 11:23 AM, Richard Biener
 wrote:
> On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor  wrote:
>> Richard,
>>
>> in discussing this work Jeff mentioned that your comments on
>> the tree-ssa-alias.c parts would be helpful.  When you have
>> a chance could you please give it a once over and let me know
>> if you have any suggestions or concerns?  There are no visible
>> changes to existing clients of the pass, just extensions that
>> are relied on only by the new diagnostics.
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html
>>
>> I expect to revisit the sprintf %s patch mentioned below and
>> see how to simplify it by taking advantage of the changes
>> implemented here.
>
> Your ptr_deref_alias_decl_p returns true when must_alias is true
> but there is no must-alias relationship.
>
> The ao_ref_init_from_ptr_and_size doesn't belong there.
>
> I dislike all of the changes related to returning an alias "size".
>
> Please keep away from the alias machinery.
>
> Warning during folding is similarly bad design.  Please don't add
> more such things.
>
> Thanks for putting this on my radar though.
> Richard.

Oh, for a constructive comment this all feels like sth for either
sanitizers or a proper static analysis tool.  As I outlined repeatedly
I belive GCC could host a static analysis tool but it surely should
not be interwinded into it's optimization passes or tools.

Richard.

>
>> Thanks
>> Martin
>>
>>
>> On 07/24/2017 09:13 PM, Martin Sebor wrote:
>>>
>>> Ping:
>>>
>>>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html
>>>
>>> This change is related to
>>>
>>>   [PATCH] enhance -Wrestrict for sprintf %s arguments
>>>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01176.html
>>>
>>> On 07/20/2017 02:45 PM, Martin Sebor wrote:

 With more testing (building GDB, Glibc, Busybox, and the Linux
 kernel) I found a few bugs and weaknesses in the initial patch.
 Attached is version 2 that fixes the uncovered problems and makes
 further enhancements to handle more cases.

 Martin

 On 07/16/2017 05:47 PM, Martin Sebor wrote:
>
> Being implemented in the front end, the -Wrestrict warning
> detects only trivial instances of violations.  The attached
> patch extends the implementation to the middle-end where
> data flow and alias analysis can be combined to detect even
> complex cases of overlap.  This work is independent of but
> follows on the patch below (waiting for review):
>
>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00036.html
>
> The changes rely on extending the tree-ssa-{alias,structalias}
> machinery in a simple way to answer the "must-alias" kind of
> question in addition to the current "may-alias."
>
> The rest of the changes are in gimple-fold.c, tree-ssa-strlen.c,
> and builtins.c.
>
> Even though this change makes -Wrestrict a lot more useful, it's
> not a complete implementation.  Not all built-ins are handled yet
> (e.g., strncat), and support for user-defined functions is still
> subject to the limitations of the front end implementation.  To
> complete the support, handlers for the missing string built-ins
> will need to be added to tree-ssa-strlen.c, and the remaining
> bits should be moved from the front end to somewhere in
> the middle-end (e.g., calls.c).
>
> Martin


>>>
>>

Re: [PING #2] [PATCH] enhance -Wrestrict to handle string built-ins (PR 78918)

[Richard Biener]

On Tue, Aug 1, 2017 at 4:27 AM, Martin Sebor  wrote:
> Richard,
>
> in discussing this work Jeff mentioned that your comments on
> the tree-ssa-alias.c parts would be helpful.  When you have
> a chance could you please give it a once over and let me know
> if you have any suggestions or concerns?  There are no visible
> changes to existing clients of the pass, just extensions that
> are relied on only by the new diagnostics.
>
>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html
>
> I expect to revisit the sprintf %s patch mentioned below and
> see how to simplify it by taking advantage of the changes
> implemented here.

Your ptr_deref_alias_decl_p returns true when must_alias is true
but there is no must-alias relationship.

The ao_ref_init_from_ptr_and_size doesn't belong there.

I dislike all of the changes related to returning an alias "size".

Please keep away from the alias machinery.

Warning during folding is similarly bad design.  Please don't add
more such things.

Thanks for putting this on my radar though.
Richard.


> Thanks
> Martin
>
>
> On 07/24/2017 09:13 PM, Martin Sebor wrote:
>>
>> Ping:
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01264.html
>>
>> This change is related to
>>
>>   [PATCH] enhance -Wrestrict for sprintf %s arguments
>>   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg01176.html
>>
>> On 07/20/2017 02:45 PM, Martin Sebor wrote:
>>>
>>> With more testing (building GDB, Glibc, Busybox, and the Linux
>>> kernel) I found a few bugs and weaknesses in the initial patch.
>>> Attached is version 2 that fixes the uncovered problems and makes
>>> further enhancements to handle more cases.
>>>
>>> Martin
>>>
>>> On 07/16/2017 05:47 PM, Martin Sebor wrote:

 Being implemented in the front end, the -Wrestrict warning
 detects only trivial instances of violations.  The attached
 patch extends the implementation to the middle-end where
 data flow and alias analysis can be combined to detect even
 complex cases of overlap.  This work is independent of but
 follows on the patch below (waiting for review):

   https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00036.html

 The changes rely on extending the tree-ssa-{alias,structalias}
 machinery in a simple way to answer the "must-alias" kind of
 question in addition to the current "may-alias."

 The rest of the changes are in gimple-fold.c, tree-ssa-strlen.c,
 and builtins.c.

 Even though this change makes -Wrestrict a lot more useful, it's
 not a complete implementation.  Not all built-ins are handled yet
 (e.g., strncat), and support for user-defined functions is still
 subject to the limitations of the front end implementation.  To
 complete the support, handlers for the missing string built-ins
 will need to be added to tree-ssa-strlen.c, and the remaining
 bits should be moved from the front end to somewhere in
 the middle-end (e.g., calls.c).

 Martin
>>>
>>>
>>
>