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

[Jeff Law <law at redhat dot com>]

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 follow-up which
you can consider pre-approved.

So please correct me if I'm wrong.  WRT overall structure, you've got a
pass which walks the IL looking for suitable calls and potentially warns.

Additionally, that pass exports check_bounds_or_overlap which we use to
varying degress in the folder and strlen optimization pass.  It doesn't
really query any pass specific state, it's just an exported interface
into the meat of your optimization pass.

Presumably you have those exported interfaces so that you can diagnose
things prior to folding or other transformations that potentially
obfuscate the calls you want to diagnose?  Right?

The main pass (which runs after VRP) presumably runs to capture more
stuff that is exposed by the optimizers.  ie, the optimizers may
const/copy propagate things which are necessary to expose some of the
cases we want to capture.

Assuming I'm correct on those issues, this starts to look a lot like how
we structure early/late warnings.

--


I believe standard practice is to put the pass class into the anonymous
namespace -- pass_data_wstrict & pass_wrestrict as well as any methods
you define for them.  I'm sure there's a good reason why we do that,
though I find it annoying given GDB's poor support for anonymous
namespaces.  BUt please follow convention here.

Consider walking the blocks in dominator order.  If the pass can benefit
from range data it'll make adding the evrp analyzer easier.  As a
reference see how I changed the sprintf warnings bits to use a dominator
walk order.

On pass_wrestrict::check_call, put its return value type onto a separate
line.

It looks like you have to create an expression for each and every
builtin call statement you check.  Is there any way to defer creating
that CALL_EXPR until you know you need it in a diagnostic?  Can the uses
prior to the actual diagnostic get their data from elsewhere?

It looks like you're doing a lot more than just restrict checking in the
new pass.  I see array bounds checking and what appears to be general
pointer-out-of-bounds checking in the new pass. Can you comment on that?

Overall it looks pretty good.  I want to make sure I understand the
overall structure and reasoning behind that structure.

Jeff