On Tue, 14 May 2024, Qing Zhao wrote:
>
>
> > On May 14, 2024, at 09:08, Richard Biener <rguent...@suse.de> 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
As it's been quite some time I think I remeber that I thought of
constructing the diagnostic path at jump threading time and associating
that with the location. But I don't remember exactly where I wanted to
put it - I think it was on an extra stmt to avoid having too many
ad-hoc locations as I'm not sure of their cost. It would of course
need experimenting since we can end up moving stmts and merging blocks
though the linear traces created by jump threading should be quite
stable (as opposed to say the unrolling case where multiple instances
of the loop body likely will end up in the exact same basic block).
> Yes, this is a good idea.
>
> The current major issue with the warning is: the constant index value 4 is
> not in the source code, it’s a compiler generated intermediate value (even
> though it’s a correct value -:)). Such warning messages confuse the end-users
> with information that cannot be connected directly to the source code.
>
> With the above recorded “events” information, the warning messages should
> make good sense to the end user, and also help the end user to locate the
> place where the fix in the source code can be added.
>
> Actually, with the above warning information, the user can locate the place
> “line 3” to add fixes as following:
>
> if (*index >= 4)
> {
> warn();
> *index = 3;
> }
>
> I.e.
> [109071]$ diff t_org.c t.c
> 2c2
> < static inline void assign(int val, int *regs, int index)
> ---
> > static inline void assign(int val, int *regs, int *index)
> 4c4,5
> < if (index >= 4)
> ---
> > if (*index >= 4)
> > {
> 5a7,8
> > *index = 3;
> > }
> 14,15c17,18
> < assign(0, ptr, index);
> < assign(*val, ptr, index);
> ---
> > assign(0, ptr, &index);
> > assign(*val, ptr, &index);
>
> >
> > 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.
>
> Recording such info to each stmt might be more reliable?
>
> >
> > 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.
>
> I agree that the current warning is correct (that’s the reason I only moved
> it from -Warray-bounds=1 to -Warray-bounds=2 -:)), the major issue is that
> it’s not clear and confusing the end-user.
>
> With more information, we can help the user to locate the issue and fix it in
> the source code.
> >
> > Note very similar issues happen when unrolling a loop.
> >
> > Note all late diagnostics are prone to these kind of issues.
>
> Are there any open PRs for these similar issues?
You can search bugzilla for diagnostic bugs or bugs blocking
the symbolic Warray-bounds bug.
Richard.
> Thanks.
>
> Qing
> >
> > 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)
>
>
>
--
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)
