| Message ID | b8460b00-29d4-a766-f96c-0e37fc55bd73@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Nov 23, 2016 at 06:15:11PM -0700, Martin Sebor wrote: > >Can't we just > >gcc_assert (x != 0) before the problematical calls? That avoids > >unnecessary over-allocation and gets us a clean ICE if we were to try > >and call alloca with a problematical value. > > 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. > > After reviewing a few more of the XALLOCAVEC calls in the affected > files I suspect that those to alloca(0) pointed out by the warning > may be just a subset that GCC happens to see thanks to constant > propagation. If that's so then changing this subset to > alloca(N + !N) or some such is probably not the right approach > because it will miss all the other calls where GCC doesn't see that > N is zero. I think the most robust solution is to do as Bernd > suggests: change XALLOCAVEC as shown above. That will let us > prevent any and all unsafe assumptions about the result of > alloca(0) being either non-null or distinct. The other approach > would be to change XALLOCAVEC to add 1 to the byte count. That > would be in line with what XMALLOC does. I still fail to see why you want to change anything at least for hosts where you know XALLOCAVEC is __builtin_alloca. Show me one place which assumes the result of alloca (0) in gcc sources is distinct, or non-NULL. For 0 elements the pointer simply isn't used. And whether for the malloc based alloca it GCs or not really doesn't matter for us. Jakub
On 11/24/2016 12:42 AM, Jakub Jelinek wrote: >> After reviewing a few more of the XALLOCAVEC calls in the affected >> files I suspect that those to alloca(0) pointed out by the warning >> may be just a subset that GCC happens to see thanks to constant >> propagation. If that's so then changing this subset to >> alloca(N + !N) or some such is probably not the right approach >> because it will miss all the other calls where GCC doesn't see that >> N is zero. I think the most robust solution is to do as Bernd >> suggests: change XALLOCAVEC as shown above. That will let us >> prevent any and all unsafe assumptions about the result of >> alloca(0) being either non-null or distinct. The other approach >> would be to change XALLOCAVEC to add 1 to the byte count. That >> would be in line with what XMALLOC does. > > I still fail to see why you want to change anything at least for > hosts where you know XALLOCAVEC is __builtin_alloca. > Show me one place which assumes the result of alloca (0) in gcc sources is > distinct, or non-NULL. For 0 elements the pointer simply isn't used. > And whether for the malloc based alloca it GCs or not really doesn't matter > for us. I think for host/build, we ought to assume that alloca is __builtin_alloca. I think we stopped supporting the alloca-on-top-of-malloc host/build systems long ago. But I still think we ought to be "clean" in regard to zero sized allocations. It sounds like an assert may not be sufficient, so we need to look at another approach. Jeff
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 can trivially fix the threadedge issue. Jeff
include/ChangeLog: * libiberty.h (XALLOCAVEC): Avoid calling alloca with a zero argument. Index: include/libiberty.h =================================================================== --- include/libiberty.h (revision 242768) +++ include/libiberty.h (working copy) @@ -353,7 +353,7 @@ extern unsigned int xcrc32 (const unsigned char *, /* Array allocators. */ -#define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N))) +#define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N) + 1)) #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)))