On 11/30/2016 09:09 PM, Martin Sebor wrote:
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.
Right. This is consistent with the limitations of other similar
warnings such as null pointer dereferences.
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?
The difference vs implicit fallthrough is that new instances of implicit
fallthrus aren't likely to be exposed by changes in IL that occur due to
transformations in the optimizer pipeline.
Given the number of runtime triggers vs warnings, we know there's many
instances of passing 0 to the allocators that we're not diagnosing. I
can easily see differences in the early IL (such as those due to
BRANCH_COST differing for targets) exposing/hiding cases where 0 flows
into the allocator argument. Similarly for changes in inlining
decisions, jump threading, etc for profiled bootstraps. I'd like to
avoid playing wack-a-mole right now.
So I'm being a bit more conservative here. Maybe it'd be appropriate in
Wextra since that's not enabled by default for GCC builds.
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.
There's always ways :-) For example, I wouldn't be at all surprised if
you found PHIs that feed the allocation where one or more of the PHI
arguments are 0.
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.
Agreed on not wanting alloca(0) handling to stall the rest of the patch.
Jeff
