On 11/23/2016 01:57 PM, Jeff Law wrote:
On 11/20/2016 04:06 PM, Martin Sebor wrote:
On 11/20/2016 01:03 AM, Bernd Edlinger wrote:
On 11/20/16 00:43, Martin Sebor wrote:
As best I can tell the result isn't actually used (the code that
uses the result gets branched over). GCC just doesn't see it.
I verified this by changing the XALLOCAVEC macro to
#define XALLOCAVEC(T, N) (N ? alloca (sizeof (T) * N) : 0)
and bootstrapping and running the regression test suite with
no apparent regressions.
I would like this solution with braces around N better than
allocating one element when actually zero were requested.
The disadvantage of N+1 or N+!N is that it will hide true
programming errors from the sanitizer.
I agree. Let me post an updated patch with the above fix and
leave it to others to choose which approach to go with.
Just so I'm clear, this part of the discussions is talking about
mitigation strategies within GCC itself, right?
That's right.
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'm not sure we need to break things up. I haven't seen a good argument
for that yet.
Is there an updated patch to post? Or has it already been posted?
Given the above I suggest going with one of the attached two patches.
Martin
include/ChangeLog:
* libiberty.h (XALLOCAVEC): Return null for zero-size allocations.
Index: include/libiberty.h
===================================================================
--- include/libiberty.h (revision 242768)
+++ include/libiberty.h (working copy)
@@ -353,7 +353,8 @@ extern unsigned int xcrc32 (const unsigned char *,
/* Array allocators. */
-#define XALLOCAVEC(T, N) ((T *) alloca (sizeof (T) * (N)))
+#define XALLOCAVEC(T, N) \
+ (T *) (((N) != 0) ? alloca (sizeof (T) * (N)) : 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)))
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)))
