Bug 83510 - [8 Regression] Recent changes for -Warray-bounds trigger false positive
Summary: [8 Regression] Recent changes for -Warray-bounds trigger false positive
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 8.0
: P3 normal
Target Milestone: 8.0
Assignee: David Malcolm
URL:
Keywords: diagnostic, patch
Depends on:
Blocks: Warray-bounds
  Show dependency treegraph
 
Reported: 2017-12-20 13:42 UTC by Franz Sirl
Modified: 2018-01-23 11:12 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 6.4.0, 7.2.0
Known to fail: 8.0
Last reconfirmed: 2017-12-20 00:00:00


Attachments
testcase (238 bytes, text/plain)
2017-12-20 13:42 UTC, Franz Sirl
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Franz Sirl 2017-12-20 13:42:28 UTC
Created attachment 42933 [details]
testcase

The attached testcase started to produce a warning with trunk somewhere between r255501 and r255678, with -O2 -Warray-bounds:

test-Warray-bounds.c: In function 'g':
test-Warray-bounds.c:18:13: warning: array subscript 4294967286 is above array bounds of 'unsigned int[10]' [-Warray-bounds=]
   return arr[number - 0xa];
          ~~~^~~~~~~~~~~~~~

I believe it is a false positive.


unsigned int arr[10];

struct xyz {
	unsigned int a0;
};

extern void wfm(struct xyz *, int, unsigned int);

static unsigned int f(struct xyz * ctx, unsigned int number)
{
	switch (number) {
	case 0x9:
		return ctx->a0;
	case 0xA: case 0xB:
	case 0xC: case 0xD: case 0xE: case 0xF:
	case 0x10: case 0x11: case 0x12: case 0x13:
		return arr[number - 0xa];
	}
	return 0;
}

int g(struct xyz * ctx) {
	int i;

	for (i = 0; i < 10; i++) {
		wfm(ctx, i, f(ctx, i));
	}

	return 0;
}
Comment 1 Martin Sebor 2017-12-20 18:20:10 UTC
Confirmed.  Looks like this has started very recently (sometime in the last week).  But a VRP dump from GCC 7 shows the same range:

  _9 = i.0_1 + 4294967286;
  _10 = arr[_9];
  ...
Found new range for _9: [4294967286, +INF]
  ...
_9: [4294967286, +INF]
Comment 2 Franz Sirl 2018-01-12 13:31:20 UTC
It started with r255649:

2017-12-14  David Malcolm  <dmalcolm@redhat.com>

        PR tree-optimization/83312
        * domwalk.h (dom_walker::dom_walker): Fix typo in comment.
        * tree-cfg.c (find_taken_edge): Update to handle NULL_TREE for
        "val" param, and to cope with arbitrary basic blocks.
        (find_taken_edge_cond_expr): Add "cond_stmt" param and use it to
        handle NULL_TREE for "val", dropping "bb" param.
        (find_taken_edge_switch_expr): Make "switch_stmt" param const and
        drop "bb" param.  Handle NULL_TREE for "val".
        (find_case_label_for_value): Make "switch_stmt" param const.
        * tree-vrp.c (class check_array_bounds_dom_walker): New subclass
        of dom_walker.
        (vrp_prop::check_all_array_refs): Reimplement as...
        (check_array_bounds_dom_walker::before_dom_children): ...this new
        vfunc.  Replace linear search through BB block list, excluding
        those with non-executable in-edges via dominator walk.
Comment 3 Martin Sebor 2018-01-12 16:56:59 UTC
Yes, that's the one.  Thanks for chasing it down.  The warning appears to be due to the change in the order the pass now walks the basic blocks of the function.
Comment 4 David Malcolm 2018-01-16 17:43:12 UTC
Thanks; am looking at it
Comment 5 David Malcolm 2018-01-18 23:34:16 UTC
Candidate patch:
  https://gcc.gnu.org/ml/gcc-patches/2018-01/msg01693.html
Comment 6 Franz Sirl 2018-01-19 13:40:30 UTC
The patch in comment 5 applied to r256877 fixes the warning in both the testcase and the original code.
Comment 7 David Malcolm 2018-01-23 11:11:19 UTC
Author: dmalcolm
Date: Tue Jan 23 11:10:47 2018
New Revision: 256980

URL: https://gcc.gnu.org/viewcvs?rev=256980&root=gcc&view=rev
Log:
-Warray-bounds: Fix false positive in some "switch" stmts (PR tree-optimization/83510)

PR tree-optimization/83510 reports that r255649 (for
PR tree-optimization/83312) introduced a false positive for
-Warray-bounds for array accesses within certain switch statements:
those for which value-ranges allow more than one case to be reachable,
but for which one or more of the VR-unreachable cases contain
out-of-range array accesses.

In the reproducer, after the switch in f is inlined into g, we have 3 cases
for the switch (case 9, case 10-19, and default), within a loop that
ranges from 0..9.

With both the old and new code, vr_values::simplify_switch_using_ranges clears
the EDGE_EXECUTABLE flag on the edge to the "case 10-19" block.  This
happens during the dom walk within the substitute_and_fold_engine.

With the old code, the clearing of that EDGE_EXECUTABLE flag led to the
      /* Skip blocks that were found to be unreachable.  */
code in the old implementation of vrp_prop::check_all_array_refs skipping
the "case 10-19" block.

With the new code, we have a second dom walk, and that dom_walker's ctor
sets all edges to be EDGE_EXECUTABLE, losing that information.

Then, dom_walker::before_dom_children (here, the subclass'
check_array_bounds_dom_walker::before_dom_children) can return one edge, if
there's a unique successor edge, and dom_walker::walk filters the dom walk
to just that edge.

Here we have two VR-valid edges (case 9 and default), and an VR-invalid
successor edge (case 10-19).  There's no *unique* valid successor edge,
and hence taken_edge is NULL, and the filtering in dom_walker::walk
doesn't fire.

Hence we've lost the filtering of the "case 10-19" BB, hence the false
positive.

The issue is that we have two dom walks: first within vr_values'
substitute_and_fold_dom_walker (which has skip_unreachable_blocks == false),
then another within vrp_prop::check_all_array_refs (with
skip_unreachable_blocks == true).

Each has different "knowledge" about ruling out edges due to value-ranges,
but we aren't combining that information.  The former "knows" about
out-edges at a particular control construct (e.g. at a switch), the latter
"knows" about dominance, but only about unique successors (hence the
problem when two out of three switch cases are valid).

This patch combines the information by preserving the EDGE_EXECUTABLE
flags from the first dom walk, and using it in the second dom walk,
potentially rejecting additional edges.

Doing so fixes the false positive.

I attempted an alternative fix, merging the two dom walks into one, but
that led to crashes in identify_jump_threads, so I went with this, as
a less invasive fix.

gcc/ChangeLog:
	PR tree-optimization/83510
	* domwalk.c (set_all_edges_as_executable): New function.
	(dom_walker::dom_walker): Convert bool param
	"skip_unreachable_blocks" to enum reachability.  Move setup of
	edge flags to set_all_edges_as_executable and only do it when
	reachability is REACHABLE_BLOCKS.
	* domwalk.h (enum dom_walker::reachability): New enum.
	(dom_walker::dom_walker): Convert bool param
	"skip_unreachable_blocks" to enum reachability.
	(set_all_edges_as_executable): New decl.
	* graphite-scop-detection.c  (gather_bbs::gather_bbs): Convert
	from false for "skip_unreachable_blocks" to ALL_BLOCKS for
	"reachability".
	* tree-ssa-dom.c (dom_opt_dom_walker::dom_opt_dom_walker): Likewise,
	but converting true to REACHABLE_BLOCKS.
	* tree-ssa-sccvn.c (sccvn_dom_walker::sccvn_dom_walker): Likewise.
	* tree-vrp.c
	(check_array_bounds_dom_walker::check_array_bounds_dom_walker):
	Likewise, but converting it to REACHABLE_BLOCKS_PRESERVING_FLAGS.
	(vrp_dom_walker::vrp_dom_walker): Likewise, but converting it to
	REACHABLE_BLOCKS.
	(vrp_prop::vrp_finalize): Call set_all_edges_as_executable
	if check_all_array_refs will be called.

gcc/testsuite/ChangeLog:
	PR tree-optimization/83510
	* gcc.c-torture/compile/pr83510.c: New test case.


Added:
    trunk/gcc/testsuite/gcc.c-torture/compile/pr83510.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/domwalk.c
    trunk/gcc/domwalk.h
    trunk/gcc/graphite-scop-detection.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/tree-ssa-dom.c
    trunk/gcc/tree-ssa-sccvn.c
    trunk/gcc/tree-vrp.c
Comment 8 David Malcolm 2018-01-23 11:12:09 UTC
Should be fixed by r256980.