What I think this tells us is that we're not at a place where we're
clean.  But we can incrementally get there.  The warning is only
catching a fairly small subset of the cases AFAICT.  That's not unusual
and analyzing why it didn't trigger on those cases might be useful as well.

The warning has no smarts.  It relies on constant propagation and
won't find a call unless it sees it's being made with a constant
zero.  Looking at the top two on the list the calls are in extern
functions not called from the same source file, so it probably just
doesn't see that the functions are being called from another file
with a zero.  Building GCC with LTO might perhaps help.

So where does this leave us for gcc-7?  I'm wondering if we drop the
warning in, but not enable it by default anywhere.  We fix the cases we
can (such as reg-stack,c tree-ssa-threadedge.c, maybe others) before
stage3 closes, and shoot for the rest in gcc-8, including improvign the
warning (if there's something we can clearly improve), and enabling the
warning in -Wall or -Wextra.

I'm fine with deferring the GCC fixes and working on the cleanup
over time but I don't think that needs to gate enabling the option
with -Wextra.  The warnings can be suppressed or prevented from
causing errors during a GCC build either via a command line option
or by pragma in the code.  AFAICT, from the other warnings I see
go by, this is what has been done for -Wno-implicit-fallthrough
while those warnings are being cleaned up.  Why not take the same
approach here?

As much as I would like to improve the warning itself I'm also not
sure I see much of an opportunity for it.  It's not prone to high
rates of false positives (hardly any in fact) and the cases it
misses are those where it simply doesn't see the argument value
because it's not been made available by constant propagation.

That said, I consider the -Walloc-size-larger-than warning to be
the more important part of the patch by far.  I'd hate a lack of
consensus on how to deal with GCC's handful of instances of
alloca(0) to stall the rest of the patch.

Thanks
Martin

Reply via email to