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.
So - if you want to move it I'd applaud such effort!
> I think just like a general purpose API to query value ranges that
> cam be called from any pass, another API to query object sizes, and
> another to query string lengths, would be useful.
>
> > OTOH making a warning part of an optimization pass naturally
> > limits its effect to when the specific optimization is enabled.
> > In theory it's possible to do -Warray-bounds at -O0 - we are in
> > SSA form after all - but of course you don't want to enable VRP at -O0.
>
> True. Better warnings at -O0 would be great. The drawback would
> be that the warnings would then have to implement all the same
> analyses as the optimization passes they rely on. We'd end up
> having to implement a static analyzer on top of basic GIMPLE.
>
> >> I don't know if there's the -Walloca pass would benefit from merging
> >> with any of the others or vice versa, but superficially it seems like
> >> it might be worth thinking about integrating the -Walloc-larger-than
> >> warnings into the -Walloca pass, if only to keep similar functionality
> >> in the same place.
> >
> > -Wrestrict and all the format warning stuff seems related as well.
>
> I think so. All of strlen, sprintf, Wrestrict, -Wstringop-overflow
> and -Walloca/-Walloc-larger-than, and even objsize, would benefit
> from sharing the same IL traversal and the same data. As would
> -Warray-bounds to detect out-of-bounds accesses to dynamically
> allocated objects. There probably are other warnings and likely
> also optimizations that would benefit from closer coupling.
>
> Martin
>
>
> >
> >>> I also see that the Og pass pipeline misses the second walloca pass
> >>> completely (and also the warn_restrict pass).
> >>
> >> That's worth fixing.
> >>
> >> Martin
> >>
> >>>
> >>> Given code sinkings obvious effects on SSA value-range representation
> >>> it may make sense to add another instance of that pass earlier.
> >>>
> >>>
> >>>
> >>>>
> >>>> --
> >>>> Marc Glisse
> >>
>
