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

[Qing Zhao <qing.zhao@oracle.com>]

> On May 22, 2024, at 03:38, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Tue, May 21, 2024 at 11:36 PM David Malcolm <dmalcolm@redhat.com> wrote:
>> 
>> 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.
> 
> While I think some of this might be an improvement to those vast
> number of "false positive" diagnostics we have from (too) late diagnostic
> passes I do not have the cycles to work on this.

I can study a little bit more on how to improve the diagnostics for PR 109071 first. 

FYI, I just updated PR109071’s description as: Need more context for -Warray-bounds warnings due to code duplication from jump threading. 

As we already agreed, this is NOT a false positive. It caught a real bug in linux kernel that need to be patched in the kernel source. (See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109071#c11)

In order to add more context to the diagnostics to help the end user locate the bug accurately in their source code, compiler needs:

1. During jump threading phase, record the following information for each duplicated STMT:
	A. A pointer to the COND stmt;
	B. True/false for such COND 
2. During array out-of-bound checking phase, whenever locate an out-of-bound case, 
	A. Use a rich_location for the diagnostic;
	B. Create an instance of diagnositic_path, add events to this diagnostic_path based on the COND stmt, True/False info recorded in the STMT;
	C. Call richloc.set_path() to associate the path with the rich_location;
        D. Then issue warning with this rich_location instead of the current regular location. 

Any comment and suggestion to the above? 
Thanks.

Qing


> 
> Richard.
> 
>> But feel free to ask me any questions about the diagnostic_path
>> machinery within the diagnostics subsystem.
>> 
>> [...snip...]
>> 
>> Regards
>> Dave