Re: [RFC][PATCH] PR tree-optimization/109071 - -Warray-bounds false positive warnings due to code duplication from jump threading

[David Malcolm <dmalcolm@redhat.com>]

On Tue, 2024-05-21 at 15:13 +0000, Qing Zhao wrote:
> Thanks for the comments and suggestions.
> 
> > On May 15, 2024, at 10:00, David Malcolm <dmalcolm@redhat.com>
> > wrote:
> > 
> > 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
> 
> Is only recording the location for the TRUE path  enough?
> We might need to record the corresponding locations for both TRUE and
> FALSE paths since the VRP might be more accurate on both paths. 
> Is only recording the location is enough? 
> Do we need to record the pointer to the original condition stmt?

Just to be clear: I don't plan to work on this myself (I have far too
much already to work on...); I'm assuming Richard Biener is
hoping/planning to implement this himself.

But feel free to ask me any questions about the diagnostic_path
machinery within the diagnostics subsystem.

[...snip...]

Regards
Dave