Re: A bug in vrp_meet?

[Jeff Law <law@redhat.com>]

On 2/28/19 10:05 AM, Qing Zhao wrote:
> Hi,
> 
> I have been debugging a runtime error caused by value range propagation. and finally located to the following gcc routine:
> 
> vrp_meet_1 in gcc/tree-vrp.c
> 
> ====
> /* Meet operation for value ranges.  Given two value ranges VR0 and
>    VR1, store in VR0 a range that contains both VR0 and VR1.  This
>    may not be the smallest possible such range.  */
> 
> static void 
> vrp_meet_1 (value_range *vr0, const value_range *vr1)
> {
>   value_range saved;
> 
>   if (vr0->type == VR_UNDEFINED)
>     {    
>       set_value_range (vr0, vr1->type, vr1->min, vr1->max, vr1->equiv);
>       return;
>     }    
> 
>   if (vr1->type == VR_UNDEFINED)
>     {    
>       /* VR0 already has the resulting range.  */
>       return;
>     }    
> ====
> 
> In the above, when one of vr0 or vr1 is VR_UNDEFINED,  the meet result of these two will be  the other VALUE. 
> 
> This seems not correct to me. 
That's the optimistic nature of VRP.  It's generally desired behavior.

> 
> For example, the following is the located incorrect value range propagation:  (portion from the dump file *.181t.dom3)
> 
> ====
> Visiting PHI node: i_83 = PHI <_152(20), 0(22)>
>     Argument #0 (20 -> 10 executable)
>         _152: UNDEFINED
>     Argument #1 (22 -> 10 executable)
>         0: [0, 0]
> Meeting
>   UNDEFINED
> and
>   [0, 0]
> to
>   [0, 0]
> Intersecting
>   [0, 0]
> and
>   [0, 65535]
> to
>   [0, 0]
> ====
> 
> 
> In the above, “i_83” is defined as PHI <_152(20), 0(22)>,   the 1st argument is UNDEFINED at this time(but its value range definitely is NOT [0,0]),
>  and the 2nd argument is 0.
If it's value is undefined then it can be any value we want.  We choose
to make it equal to the other argument.

If VRP later finds that _152 changes, then it will go back and
reevaluate the PHI.  That's one of the basic design principles of the
optimistic propagators.

> 
> “vrp_meet” generate a VR_RANGE with [0,0] for “i_83” based on the current algorithm.  Obviously, this result VR_RANGE with [0,0] does NOT 
> contain the value ranges for _152. 
> 
>  the result of “vrp_meet” is Not correct.  and this incorrect value range result finally caused the runtime error. 
> 
> I ‘d like to modify the vrp_meet_1 as following:
> 
> ====
> static void 
> vrp_meet_1 (value_range *vr0, const value_range *vr1)
> {
>   value_range saved;
> 
>   if (vr0->type == VR_UNDEFINED)
>     {    
>       /* VR0 already has the resulting range. */
>       return;
>     }    
> 
>   if (vr1->type == VR_UNDEFINED)
>     {    
>       set_value_range_to_undefined (vr0)
>      return;
>     }    
> ====
> 
> let me know your opinion.
> 
> thanks a lot for the help.
I think we (Richi and I) went through this about a year ago and the
conclusion was we should be looking at how you're getting into the
vrp_meet with the VR_UNDEFINED.

If it happens because the user's code has an undefined use, then, the
consensus has been to go ahead and optimize it.

If it happens for any other reason, then it's likely a bug in GCC.  We
had a couple of these when we made EVRP a re-usable module and started
exploiting its data in the jump threader.

So you need to work backwards from this point to figure out how you got
here.

jeff