On 12/01/16 19:39, Jeff Law wrote: > 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. >
actually it is enabled, by -W -Wall ... > >> >> 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. I'd say negative arguments on alloca should always diagnosed, as that does increment the stack in reverse direction. And that is always very dangerous. I think the default of -Walloc-size-larger-than should be changed as Martin suggested to SIZE_T_MAX/2, I would make that the default. And keep alloca and malloc size limits separate warnings. Bernd.
