| Message ID | bc2f8f9a-b44d-7794-c720-0bee805dd97c@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 11/26/2016 05:52 PM, Martin Sebor wrote: > On 11/25/2016 12:51 PM, Jeff Law wrote: >> On 11/23/2016 06:15 PM, Martin Sebor wrote: >>> >>> gcc_assert works only in some instances (e.g., in c-ada-spec.c:191) >>> but not in others because some actually do make the alloca(0) call >>> at runtime: at a minimum, lto.c:3285, reg-stack.c:2008, and >>> tree-ssa-threadedge.c:344 assert during bootstrap. >> You might have the wrong line number of reg-stack.c and lto. You've >> pointed to the start of subst_asm_stack_regs and lto_main respectively. >> It'd probably be better if you posted the line with a bit of context. > > I must have copied the wrong line numbers or had stale sources > in my tree. Sorry about that. In lto.c, there are two calls > to XALLOCAVEC. I believe the first one is the one where the > alloca(0) call takes place: > > 1580 > 1581 tree *map = XALLOCAVEC (tree, 2 * len); > 1582 for (tree_scc *pscc = *slot; pscc; pscc = pscc->next) > -- > 1610 { > 1611 tree *map2 = XALLOCAVEC (tree, 2 * len); > 1612 for (unsigned i = 0; i < len; ++i) I'm not at all familiar with this code, but something looks real fishy here. Essentially if pscc->entry_len is >= 1 and len == 0, then we'll read map[0] and map[1] which were never allocated (see compare_tree_sccs and compare_tree_sccs_1). It may be the case that pscc->entry_len and len are related in such a way that never happens, but I can't easily prove it. I'd really like Richi to chime in on how this stuff is supposed to work. > > In reg-stack.c it's these three: > > 2052 > 2053 note_reg = XALLOCAVEC (rtx, i); > 2054 note_loc = XALLOCAVEC (rtx *, i); > 2055 note_kind = XALLOCAVEC (enum reg_note, i); > 2056 So for reg-stack.c I think we move the n_notes initialization before the XALLOCAVEC, then wrap the XALLOCAVEC calls and the subsequent loop over the notes inside an if (i > 0) conditional. Damn you for making me look at reg-stack.c. It's been years and hopefully it'll be years before I have to do it again :-) n_notes = 0; if (i > 0) { note_reg = note_loc = note_kind = for (note = REG_NOTES (insn); ...) { ... } } I'm pretty sure I can twiddle the tree-ssa-threadedge code to avoid the problem in there. > To find all such calls I modified GCC to emit an inform call for > every XALLOCAVEC invocation with a zero argument, configured the > patched GCC on x86_64 with all languages (including lto), > bootstrapped it, ran the full test suite, and extracted the set > of unique notes from the logs. Attached in the .log file is > the output along with counts of each. Curiously, neither of > the two above shows up, even though adding asserts for them > broke bootstrap. I haven't investigated why. Thanks. That's interesting data -- every one of those should be deeply investigated. I suspect we'd probably trip even more if we did a test with config-list.mk and perhaps even more if we took those cross compilers and built the target libraries. 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. 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. Jeff
> 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
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. I should also add that for GCC abd provided the main concern is non-unique pointers, the warning find just the right subset of calls, (other concerns are portability to non-GCC compilers or to library implementations). GCC makes sure the size is a multiple of stack alignment. When the argument is constant and a multiple of stack size GCC does nothing, and so when the size is zero it just returns the top of stack, resulting in non-unique pointers. When it's not constant, GCC emits code to round up the size to a multiple of stack alignment, which makes each pointer unique. > >> 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
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
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.
On 12/2/16, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > 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 ... > Don't the gcc docs say that -Wextra is the preferred name for -W? Why is gcc still using the deprecated name for the flag when building itself? Seems like it'd help to avoid this confusion if the flags read -Wextra instead of -W...
diff --git a/gcc/tree-core.h b/gcc/tree-core.h index 3e3f31e..24c8c32 100644 --- a/gcc/tree-core.h +++ b/gcc/tree-core.h @@ -22,6 +22,12 @@ along with GCC; see the file COPYING3. If not see #include "symtab.h" +extern void inform (location_t, const char *, ...); + +#undef WARN_ALLOCA_ZERO +#define WARN_ALLOCA_ZERO() \ + inform (0, "%s:%i: %s: alloca called with a zero argument", \ + __FILE__, __LINE__, __PRETTY_FUNCTION__) /* This file contains all the data structures that define the 'tree' type. There are no accessor macros nor functions in this file. Only the basic data structures, extern declarations and type definitions. */ diff --git a/include/libiberty.h b/include/libiberty.h index 605ff56..c293533 100644 --- a/include/libiberty.h +++ b/include/libiberty.h @@ -352,8 +352,11 @@ extern unsigned int xcrc32 (const unsigned char *, int, unsigned int); #define XDELETE(P) free ((void*) (P)) /* Array allocators. */ +#ifndef WARN_ALLOCA_ZERO +# define WARN_ALLOCA_ZERO() (void)0 +#endif -#define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N))) +#define XALLOCAVEC(T, N) ((N) != 0 ? (T *) alloca (sizeof (T) * (N)) : (WARN_ALLOCA_ZERO (), (T *)0)) #define XNEWVEC(T, N) ((T *) xmalloc (sizeof (T) * (N))) #define XCNEWVEC(T, N) ((T *) xcalloc ((N), sizeof (T))) #define XDUPVEC(T, P, N) ((T *) xmemdup ((P), sizeof (T) * (N), sizeof (T) * (N)))