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.
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
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
