On Tue, 2024-05-14 at 15:08 +0200, Richard Biener wrote: > On Mon, 13 May 2024, Qing Zhao wrote: > > > -Warray-bounds is an important option to enable linux kernal to > > keep > > the array out-of-bound errors out of the source tree. > > > > However, due to the false positive warnings reported in PR109071 > > (-Warray-bounds false positive warnings due to code duplication > > from > > jump threading), -Warray-bounds=1 cannot be added on by default. > > > > Although it's impossible to elinimate all the false positive > > warnings > > from -Warray-bounds=1 (See PR104355 Misleading -Warray-bounds > > documentation says "always out of bounds"), we should minimize the > > false positive warnings in -Warray-bounds=1. > > > > The root reason for the false positive warnings reported in > > PR109071 is: > > > > When the thread jump optimization tries to reduce the # of branches > > inside the routine, sometimes it needs to duplicate the code and > > split into two conditional pathes. for example: > > > > The original code: > > > > void sparx5_set (int * ptr, struct nums * sg, int index) > > { > > if (index >= 4) > > warn (); > > *ptr = 0; > > *val = sg->vals[index]; > > if (index >= 4) > > warn (); > > *ptr = *val; > > > > return; > > } > > > > With the thread jump, the above becomes: > > > > void sparx5_set (int * ptr, struct nums * sg, int index) > > { > > if (index >= 4) > > { > > warn (); > > *ptr = 0; // Code duplications since "warn" does > > return; > > *val = sg->vals[index]; // same this line. > > // In this path, since it's under > > the condition > > // "index >= 4", the compiler knows > > the value > > // of "index" is larger then 4, > > therefore the > > // out-of-bound warning. > > warn (); > > } > > else > > { > > *ptr = 0; > > *val = sg->vals[index]; > > } > > *ptr = *val; > > return; > > } > > > > We can see, after the thread jump optimization, the # of branches > > inside > > the routine "sparx5_set" is reduced from 2 to 1, however, due to > > the > > code duplication (which is needed for the correctness of the code), > > we > > got a false positive out-of-bound warning. > > > > In order to eliminate such false positive out-of-bound warning, > > > > A. Add one more flag for GIMPLE: is_splitted. > > B. During the thread jump optimization, when the basic blocks are > > duplicated, mark all the STMTs inside the original and > > duplicated > > basic blocks as "is_splitted"; > > C. Inside the array bound checker, add the following new heuristic: > > > > If > > 1. the stmt is duplicated and splitted into two conditional > > paths; > > + 2. the warning level < 2; > > + 3. the current block is not dominating the exit block > > Then not report the warning. > > > > The false positive warnings are moved from -Warray-bounds=1 to > > -Warray-bounds=2 now. > > > > Bootstrapped and regression tested on both x86 and aarch64. > > adjusted > > -Warray-bounds-61.c due to the false positive warnings. > > > > Let me know if you have any comments and suggestions. > > At the last Cauldron I talked with David Malcolm about these kind of > issues and thought of instead of suppressing diagnostics to record > how a block was duplicated. For jump threading my idea was to record > the condition that was proved true when entering the path and do this > by recording the corresponding locations so that in the end we can > use the diagnostic-path infrastructure to say > > warning: array index always above array bounds > events 1: > > > 3 | if (index >= 4) > | > (1) when index >= 4 > > it would be possible to record the info as part of the ad-hoc > location data on each duplicated stmt or, possibly simpler, > as part of a debug stmt of new kind. > > I'm not sure pruning the warnings is a good thing to do. One > would argue we should instead isolate such path as unreachable > since it invokes undefined behavior. In particular your > example is clearly a bug and should be diagnosed. > > Note very similar issues happen when unrolling a loop. > > Note all late diagnostics are prone to these kind of issues.
To recap our chat at Cauldron: any GCC diagnostic can potentially have a diagnostic_path associated with it (not just the analyzer). The current mechanism is: (a) use a rich_location for the diagnostic, and (b) create an instance of some diagnostic_path subclass (e.g. simple_diagnostic_path, or something else), and (c) call richloc.set_path (&path); to associate the path with the rich_location See gcc/testsuite/gcc.dg/plugin/diagnostic_plugin_test_paths.c for an example of using simple_diagnostic_path that doesn't use the analyzer. If we want *every* late diagnostic to potentially have a path, it sounds like we might want some extra infrastructure (perhaps a hook that autogenerates the paths from the ad-hoc data???) But probably best to get it working for just a specific example first before trying to be fancy and generalize. Dave > > Richard. > > > Thanks. > > > > Qing > > > > > > PR tree optimization/109071 > > > > gcc/ChangeLog: > > > > * gimple-array-bounds.cc (check_out_of_bounds_and_warn): > > Add two new > > arguments for the new heuristic to not issue warnings. > > (array_bounds_checker::check_array_ref): Call the new > > prototype of the > > routine check_out_of_bounds_and_warn. > > (array_bounds_checker::check_mem_ref): Add one new argument > > for the > > new heuristic to not issue warnings. > > (array_bounds_checker::check_addr_expr): Call the new > > prototype of the > > routine check_mem_ref, add new heuristic for not issue > > warnings. > > (array_bounds_checker::check_array_bounds): Call the new > > prototype of > > the routine check_mem_ref. > > * gimple-array-bounds.h: New prototype of check_mem_ref. > > * gimple.h (struct GTY): Add one new flag is_splitted for > > gimple. > > (gimple_is_splitted_p): New function. > > (gimple_set_is_splitted): New function. > > * tree-ssa-threadupdate.cc (set_stmts_in_bb_is_splitted): > > New function. > > (back_jt_path_registry::duplicate_thread_path): Mark all > > the stmts in > > both original and copied blocks as IS_SPLITTED. > > > > gcc/testsuite/ChangeLog: > > > > * gcc.dg/Warray-bounds-61.c: Adjust testing case. > > * gcc.dg/pr109071-1.c: New test. > > * gcc.dg/pr109071.c: New test. > > --- > > gcc/gimple-array-bounds.cc | 46 > > +++++++++++++++++++++---- > > gcc/gimple-array-bounds.h | 2 +- > > gcc/gimple.h | 21 +++++++++-- > > gcc/testsuite/gcc.dg/Warray-bounds-61.c | 6 ++-- > > gcc/testsuite/gcc.dg/pr109071-1.c | 22 ++++++++++++ > > gcc/testsuite/gcc.dg/pr109071.c | 22 ++++++++++++ > > gcc/tree-ssa-threadupdate.cc | 15 ++++++++ > > 7 files changed, 122 insertions(+), 12 deletions(-) > > create mode 100644 gcc/testsuite/gcc.dg/pr109071-1.c > > create mode 100644 gcc/testsuite/gcc.dg/pr109071.c > > > > diff --git a/gcc/gimple-array-bounds.cc b/gcc/gimple-array- > > bounds.cc > > index 008071cd5464..4a2975623bc1 100644 > > --- a/gcc/gimple-array-bounds.cc > > +++ b/gcc/gimple-array-bounds.cc > > @@ -264,7 +264,9 @@ check_out_of_bounds_and_warn (location_t > > location, tree ref, > > tree up_bound, tree up_bound_p1, > > const value_range *vr, > > bool ignore_off_by_one, bool > > for_array_bound, > > - bool *out_of_bound) > > + bool *out_of_bound, > > + bool is_splitted, > > + bool is_dominate_exit) > > { > > tree min, max; > > tree low_bound = array_ref_low_bound (ref); > > @@ -273,6 +275,13 @@ check_out_of_bounds_and_warn (location_t > > location, tree ref, > > bool warned = false; > > *out_of_bound = false; > > > > + /* If the stmt is duplicated and splitted, the warning level is > > not 2, > > + and the current block is not dominating the exit block, not > > report > > + the warning. */ > > + if (is_splitted && warn_array_bounds < 2 > > + && !is_dominate_exit) > > + return false; > > + > > /* Empty array. */ > > if (up_bound && tree_int_cst_equal (low_bound, up_bound_p1)) > > { > > @@ -386,12 +395,17 @@ array_bounds_checker::check_array_ref > > (location_t location, tree ref, > > } > > } > > > > + bool is_dominate_exit = dominated_by_p (CDI_DOMINATORS, > > + EXIT_BLOCK_PTR_FOR_FN > > (fun), > > + gimple_bb (stmt)); > > + > > warned = check_out_of_bounds_and_warn (location, ref, > > low_sub_org, low_sub, > > up_sub, > > up_bound, up_bound_p1, > > &vr, > > ignore_off_by_one, > > warn_array_bounds, > > - &out_of_bound); > > - > > + &out_of_bound, > > + gimple_is_splitted_p > > (stmt), > > + is_dominate_exit); > > > > if (!warned && sam == special_array_member::int_0) > > warned = warning_at (location, OPT_Wzero_length_bounds, > > @@ -476,7 +490,7 @@ array_bounds_checker::check_array_ref > > (location_t location, tree ref, > > > > bool > > array_bounds_checker::check_mem_ref (location_t location, tree > > ref, > > - bool ignore_off_by_one) > > + gimple *stmt, bool > > ignore_off_by_one) > > { > > if (warning_suppressed_p (ref, OPT_Warray_bounds_)) > > return false; > > @@ -574,6 +588,16 @@ array_bounds_checker::check_mem_ref > > (location_t location, tree ref, > > } > > } > > > > + /* If the stmt is duplicated and splitted, the warning level is > > not 2, > > + and the current block is not dominating the exit block, not > > report > > + the warning. */ > > + bool is_dominate_exit = dominated_by_p (CDI_DOMINATORS, > > + EXIT_BLOCK_PTR_FOR_FN > > (fun), > > + gimple_bb (stmt)); > > + if (gimple_is_splitted_p (stmt) && warn_array_bounds < 2 > > + && !is_dominate_exit) > > + return false; > > + > > bool warned = false; > > if (lboob) > > { > > @@ -654,7 +678,7 @@ array_bounds_checker::check_addr_expr > > (location_t location, tree t, > > ignore_off_by_one = false; > > } > > else if (TREE_CODE (t) == MEM_REF) > > - warned = check_mem_ref (location, t, ignore_off_by_one); > > + warned = check_mem_ref (location, t, stmt, > > ignore_off_by_one); > > > > if (warned) > > suppress_warning (t, OPT_Warray_bounds_); > > @@ -690,6 +714,16 @@ array_bounds_checker::check_addr_expr > > (location_t location, tree t, > > if (!mem_ref_offset (t).is_constant (&idx)) > > return; > > > > + /* If the stmt is duplicated and splitted, the warning level is > > not 2, > > + and the current block is not dominating the exit block, not > > report > > + the warning. */ > > + bool is_dominate_exit = dominated_by_p (CDI_DOMINATORS, > > + EXIT_BLOCK_PTR_FOR_FN > > (fun), > > + gimple_bb (stmt)); > > + if (gimple_is_splitted_p (stmt) && warn_array_bounds < 2 > > + && !is_dominate_exit) > > + return; > > + > > bool warned = false; > > idx = wi::sdiv_trunc (idx, wi::to_offset (el_sz)); > > if (idx < 0) > > @@ -809,7 +843,7 @@ array_bounds_checker::check_array_bounds (tree > > *tp, int *walk_subtree, > > warned = checker->check_array_ref (location, t, wi->stmt, > > false/*ignore_off_by_one*/); > > else if (TREE_CODE (t) == MEM_REF) > > - warned = checker->check_mem_ref (location, t, > > + warned = checker->check_mem_ref (location, t, wi->stmt, > > false /*ignore_off_by_one*/); > > else if (TREE_CODE (t) == ADDR_EXPR) > > { > > diff --git a/gcc/gimple-array-bounds.h b/gcc/gimple-array-bounds.h > > index 3e077d0178ff..7c98f02204c9 100644 > > --- a/gcc/gimple-array-bounds.h > > +++ b/gcc/gimple-array-bounds.h > > @@ -33,7 +33,7 @@ public: > > private: > > static tree check_array_bounds (tree *tp, int *walk_subtree, > > void *data); > > bool check_array_ref (location_t, tree, gimple *, bool > > ignore_off_by_one); > > - bool check_mem_ref (location_t, tree, bool ignore_off_by_one); > > + bool check_mem_ref (location_t, tree, gimple *, bool > > ignore_off_by_one); > > void check_addr_expr (location_t, tree, gimple *); > > void get_value_range (irange &r, const_tree op, gimple *); > > > > diff --git a/gcc/gimple.h b/gcc/gimple.h > > index bd315ffc2dd4..08f52d107084 100644 > > --- a/gcc/gimple.h > > +++ b/gcc/gimple.h > > @@ -254,8 +254,8 @@ struct GTY((desc ("gimple_statement_structure > > (&%h)"), tag ("GSS_BASE"), > > /* Nonzero if this statement contains volatile operands. */ > > unsigned has_volatile_ops : 1; > > > > - /* Padding to get subcode to 16 bit alignment. */ > > - unsigned pad : 1; > > + /* Nonzero if this statement is duplicated and splitted to two > > pathes. */ > > + unsigned is_splitted : 1; > > > > /* The SUBCODE field can be used for tuple-specific flags for > > tuples > > that do not require subcodes. Note that SUBCODE should be at > > @@ -2327,6 +2327,23 @@ gimple_set_has_volatile_ops (gimple *stmt, > > bool volatilep) > > stmt->has_volatile_ops = (unsigned) volatilep; > > } > > > > +/* Return true if statement G's is_splitted field has > > + been set. */ > > + > > +inline bool > > +gimple_is_splitted_p (const gimple *g) > > +{ > > + return (bool) g->is_splitted; > > +} > > + > > +/* Set the IS_SPLITTED flag to IS_SPLITTEDP. */ > > + > > +inline void > > +gimple_set_is_splitted (gimple *s, bool is_splittedp) > > +{ > > + s->is_splitted = (unsigned) is_splittedp; > > +} > > + > > /* Return true if STMT is in a transaction. */ > > > > inline bool > > diff --git a/gcc/testsuite/gcc.dg/Warray-bounds-61.c > > b/gcc/testsuite/gcc.dg/Warray-bounds-61.c > > index 5b66cdc0aab1..cb3c64a813d7 100644 > > --- a/gcc/testsuite/gcc.dg/Warray-bounds-61.c > > +++ b/gcc/testsuite/gcc.dg/Warray-bounds-61.c > > @@ -23,7 +23,7 @@ void test_ua3_ua0_a0 (int i) > > > > if (i < __LINE__) > > i = 5; > > - ua3_a0.a0[i] = 0; // { dg-warning "\\\[-Warray-bounds" > > } > > + ua3_a0.a0[i] = 0; // { dg-bogus "\\\[-Warray-bounds" } > > > > if (i > -1) > > i = -1; > > @@ -44,7 +44,7 @@ void test_ua3_ua0_a1 (int i) > > > > if (i > -1) > > i = -1; > > - ua3_a0.a1[i] = 0; // { dg-warning "\\\[-Warray-bounds" > > } > > + ua3_a0.a1[i] = 0; // { dg-bogus "\\\[-Warray-bounds" } > > > > if (i < 7) > > i = 7; > > @@ -60,7 +60,7 @@ void test_ua3_ua0_a2 (int i) > > > > if (i < __LINE__) > > i = __LINE__; > > - ua3_a0.a2[i] = 0; // { dg-warning "\\\[-Warray-bounds" > > } > > + ua3_a0.a2[i] = 0; // { dg-bogus "\\\[-Warray-bounds" } > > > > if (i > -1) > > i = -1; > > diff --git a/gcc/testsuite/gcc.dg/pr109071-1.c > > b/gcc/testsuite/gcc.dg/pr109071-1.c > > new file mode 100644 > > index 000000000000..a405c80bd549 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/pr109071-1.c > > @@ -0,0 +1,22 @@ > > +/* PR tree-optimization/109071 -Warray-bounds false positive > > warnings > > + due to code duplication from jump threading > > + { dg-do compile } > > + { dg-options "-O2 -Warray-bounds=2" } > > + */ > > + > > +extern void warn(void); > > +static inline void assign(int val, int *regs, int index) > > +{ > > + if (index >= 4) > > + warn(); > > + *regs = val; > > +} > > +struct nums {int vals[4];}; > > + > > +void sparx5_set (int *ptr, struct nums *sg, int index) > > +{ > > + int *val = &sg->vals[index]; /* { dg-warning "is above array > > bounds" } */ > > + > > + assign(0, ptr, index); > > + assign(*val, ptr, index); > > +} > > diff --git a/gcc/testsuite/gcc.dg/pr109071.c > > b/gcc/testsuite/gcc.dg/pr109071.c > > new file mode 100644 > > index 000000000000..782dfad84ea2 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.dg/pr109071.c > > @@ -0,0 +1,22 @@ > > +/* PR tree-optimization/109071 -Warray-bounds false positive > > warnings > > + due to code duplication from jump threading > > + { dg-do compile } > > + { dg-options "-O2 -Wall" } > > + */ > > + > > +extern void warn(void); > > +static inline void assign(int val, int *regs, int index) > > +{ > > + if (index >= 4) > > + warn(); > > + *regs = val; > > +} > > +struct nums {int vals[4];}; > > + > > +void sparx5_set (int *ptr, struct nums *sg, int index) > > +{ > > + int *val = &sg->vals[index]; /* { dg-bogus "is above array > > bounds" } */ > > + > > + assign(0, ptr, index); > > + assign(*val, ptr, index); > > +} > > diff --git a/gcc/tree-ssa-threadupdate.cc b/gcc/tree-ssa- > > threadupdate.cc > > index fa61ba9512b7..9f338dd4d54d 100644 > > --- a/gcc/tree-ssa-threadupdate.cc > > +++ b/gcc/tree-ssa-threadupdate.cc > > @@ -2371,6 +2371,17 @@ > > back_jt_path_registry::adjust_paths_after_duplication (unsigned > > curr_path_num) > > } > > } > > > > +/* Set all the stmts in the basic block BB as IS_SPLITTED. */ > > + > > +static void > > +set_stmts_in_bb_is_splitted (basic_block bb) > > +{ > > + gimple_stmt_iterator gsi; > > + for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) > > + gimple_set_is_splitted (gsi_stmt (gsi), true); > > + return; > > +} > > + > > /* Duplicates a jump-thread path of N_REGION basic blocks. > > The ENTRY edge is redirected to the duplicate of the region. > > > > @@ -2418,6 +2429,10 @@ back_jt_path_registry::duplicate_thread_path > > (edge entry, > > basic_block *region_copy = XNEWVEC (basic_block, n_region); > > copy_bbs (region, n_region, region_copy, &exit, 1, &exit_copy, > > loop, > > split_edge_bb_loc (entry), false); > > + /* Mark all the stmts in both original and copied basic blocks > > + as IS_SPLITTED. */ > > + set_stmts_in_bb_is_splitted (*region); > > + set_stmts_in_bb_is_splitted (*region_copy); > > > > /* Fix up: copy_bbs redirects all edges pointing to copied > > blocks. The > > following code ensures that all the edges exiting the jump- > > thread path are > > >