On 5/29/19 1:21 AM, Richard Biener wrote:
> On Tue, May 28, 2019 at 5:34 PM Martin Sebor <mse...@gmail.com> wrote:
>>
>> On 5/21/19 3:53 AM, Richard Biener wrote:
>>> On Tue, May 21, 2019 at 4:13 AM Martin Sebor <mse...@gmail.com> wrote:
>>>>
>>>> On 5/20/19 3:16 AM, Richard Biener wrote:
>>>>> On Mon, May 20, 2019 at 10:16 AM Marc Glisse <marc.gli...@inria.fr> wrote:
>>>>>>
>>>>>> On Mon, 20 May 2019, Richard Biener wrote:
>>>>>>
>>>>>>> On Sun, May 19, 2019 at 6:16 PM Marc Glisse <marc.gli...@inria.fr>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> 2 pieces:
>>>>>>>>
>>>>>>>> - the first one handles the case where the denominator is negative. It
>>>>>>>> doesn't happen often with exact_div, so I don't handle it everywhere,
>>>>>>>> but
>>>>>>>> this one looked trivial
>>>>>>>>
>>>>>>>> - handle the case where a pointer difference is cast to an unsigned
>>>>>>>> type
>>>>>>>> before being compared to a constant (I hit this in std::vector). With
>>>>>>>> some
>>>>>>>> range info we could probably handle some non-constant cases as well...
>>>>>>>>
>>>>>>>> The second piece breaks Walloca-13.c (-Walloca-larger-than=100 -O2)
>>>>>>>>
>>>>>>>> void f (void*);
>>>>>>>> void g (int *p, int *q)
>>>>>>>> {
>>>>>>>> __SIZE_TYPE__ n = (__SIZE_TYPE__)(p - q);
>>>>>>>> if (n < 100)
>>>>>>>> f (__builtin_alloca (n));
>>>>>>>> }
>>>>>>>>
>>>>>>>> At the time of walloca2, we have
>>>>>>>>
>>>>>>>> _1 = p_5(D) - q_6(D);
>>>>>>>> # RANGE [-2305843009213693952, 2305843009213693951]
>>>>>>>> _2 = _1 /[ex] 4;
>>>>>>>> # RANGE ~[2305843009213693952, 16140901064495857663]
>>>>>>>> n_7 = (long unsigned intD.10) _2;
>>>>>>>> _11 = (long unsigned intD.10) _1;
>>>>>>>> if (_11 <= 396)
>>>>>>>> [...]
>>>>>>>> _3 = allocaD.1059 (n_7);
>>>>>>>>
>>>>>>>> and warn.
>>>>>>>
>>>>>>> That's indeed to complicated relation of _11 to n_7 for
>>>>>>> VRP predicate discovery.
>>>>>>>
>>>>>>>> However, DOM3 later produces
>>>>>>>>
>>>>>>>> _1 = p_5(D) - q_6(D);
>>>>>>>> _11 = (long unsigned intD.10) _1;
>>>>>>>> if (_11 <= 396)
>>>>>>>
>>>>>>> while _11 vs. _1 works fine.
>>>>>>>
>>>>>>>> [...]
>>>>>>>> # RANGE [0, 99] NONZERO 127
>>>>>>>> _2 = _1 /[ex] 4;
>>>>>>>> # RANGE [0, 99] NONZERO 127
>>>>>>>> n_7 = (long unsigned intD.10) _2;
>>>>>>>> _3 = allocaD.1059 (n_7);
>>>>>>>>
>>>>>>>> so I am tempted to say that the walloca2 pass is too early, xfail the
>>>>>>>> testcase and file an issue...
>>>>>>>
>>>>>>> Hmm, there's a DOM pass before walloca2 already and moving
>>>>>>> walloca2 after loop opts doesn't look like the best thing to do?
>>>>>>> I suppose it's not DOM but sinking that does the important transform
>>>>>>> here? That is,
>>>>>>>
>>>>>>> Index: gcc/passes.def
>>>>>>> ===================================================================
>>>>>>> --- gcc/passes.def (revision 271395)
>>>>>>> +++ gcc/passes.def (working copy)
>>>>>>> @@ -241,9 +241,9 @@ along with GCC; see the file COPYING3.
>>>>>>> NEXT_PASS (pass_optimize_bswap);
>>>>>>> NEXT_PASS (pass_laddress);
>>>>>>> NEXT_PASS (pass_lim);
>>>>>>> - NEXT_PASS (pass_walloca, false);
>>>>>>> NEXT_PASS (pass_pre);
>>>>>>> NEXT_PASS (pass_sink_code);
>>>>>>> + NEXT_PASS (pass_walloca, false);
>>>>>>> NEXT_PASS (pass_sancov);
>>>>>>> NEXT_PASS (pass_asan);
>>>>>>> NEXT_PASS (pass_tsan);
>>>>>>>
>>>>>>> fixes it?
>>>>>>
>>>>>> I will check, but I don't think walloca uses any kind of on-demand VRP,
>>>>>> so
>>>>>> we still need some pass to update the ranges after sinking, which doesn't
>>>>>> seem to happen until the next DOM pass.
>>>>>
>>>>> Oh, ok... Aldy, why's this a separate pass anyways? I think similar
>>>>> other warnigns are emitted from RTL expansion? So maybe we can
>>>>> indeed move the pass towards warn_restrict or late_warn_uninit.
>>>>
>>>> I thought there was a preference to add new middle-end warnings
>>>> into passes of their own rather than into existing passes. Is
>>>> that not so (either in general or in this specific case)?
>>>
>>> The preference was to add them not into optimization passes. But
>>> of course having 10+ warning passes, each going over the whole IL
>>> is excessive. Also each of the locally computing ranges or so.
>>>
>>> Given the simplicity of Walloca I wonder why it's not part of another
>>> warning pass - since it's about tracking "sizes" again there are plenty
>>> that fit ;)
>>
>> -Walloca doesn't need to track object sizes in the same sense
>> as objsize and strlen do. It just examines calls to allocation
>> functions, same as -Walloc-larger-than. It would make sense to
>> merge the implementation of two warnings. They don't need to
>> run as a pass of their own.
>>
>>>> From my POV, the main (only?) benefit of putting warnings in their
>>>> own passes is modularity. Are there any others?
>>>>
>>>> The biggest drawback I see is that it makes it hard to then share
>>>> data across multiple passes. The sharing can help not just
>>>> warnings (reduce both false positive and false negative rates) but
>>>> also optimization. That's why I'm merging the strlen and sprintf
>>>> passes, and want to eventually also look into merging
>>>> the -Wstringop-overflow warnings there (also emitted just before
>>>> RTL expansion. Did I miss any downsides?
>>>
>>> When things fit together they are fine to merge obviously.
>>>
>>> One may not like -Warray-bounds inside VRP but it really "fits".
>>
>> It fits because it has access to data that's not (yet) available
>> outside VRP, right? It's not an ideal fit because by being in VRP
>> it doesn't have access to allocated object sizes so out-of-bounds
>> access to dynamically allocated objects aren't detected. To detect
>> those, I think the only pass it could go in is strlen. So again
>> an optimization pass. It doesn't fit there very well at all, but
>> there is no other pass that tracks sizes of all objects (objsize
>> does, though only in response to calls to __builtin_object_size).
>
> All true - now that we have warning passes computing range-information
> it definitely fits better there. At the time we added the warning there
> wasn't even per-SSA name range information.
Right. Global ranges attached to SSA_NAMEs is a relatively recent
capability and one that I believe we'll continue to find uses for.
WRT out of bounds array indexing. What we do right now in VRP is so
lame it makes me want to cry. Essentially we only warn if the entire
range of the index is out of bounds. We can and should be able to do
better.
Global ranges aren't the solution for this problem. They're too
imprecise, particularly after join points. Living inside VRP is
problematical, not just because it's gross, but because I think we
should be turning out of bounds array references into traps (after
warning of course). In fact, it was problems in this space that were
the prime motivation behind the ranger project :-)
A common idiom I've seen during analysis is to use -1 to denote an error
case. It's relatively common as a result to end up with stuff like this:
index = PHI (1, 2, 3, -1)
y = array[index]
I think we want a warning for this kind of stuff. Furthermore, I think
we want to isolate the path which results in -1 being assigned to index.
Once that path is isolated, we can insert a trap on the isolated path
and cleanup the CFG.
Jeff
