On 06/16/2016 02:32 AM, Aldy Hernandez wrote:
Hi folks!
I've been working on a plugin to warn on unbounded uses of alloca() to
help find questionable uses in glibc and other libraries. It occurred
to me that the broader community could benefit from it, as it has found
quite a few interesting cases. So, I've reimplemented it as an actual
pass, lest it be lost in plugin la-la land and bit-rot.
Before I sink any more time cleaning it up, would this be something
acceptable into the compiler? It doesn't have anything glibc specific,
except possibly the following idiom which I allow:
if (gate_function (length))
alloca(length);
...and the above is probably common enough that we should handle it.
The testcase has a lot of examples of what the pass handles.
Thoughts?
I think detecting potentially problematic uses of alloca would
be useful, especially when done in an intelligent way like in
your patch (as opposed to simply diagnosing every call to
the function regardless of the value of its argument). At
the same time, it seems that an even more reliable solution
than pointing out potentially unsafe calls to the function
and relying on users to modify their code to use malloc for
large/unbounded allocations would be to let GCC do it for
them automatically (i.e., in response to some other option,
emit a call to malloc instead, and insert a call to free when
appropriate).
I applied the patch and experimented with it a bit (I haven't
studied the code in any detail yet) and found a few opportunities
for improvements. I describe them below (Sorry in advance for
the length of my comments!)
I found the "warning: unbounded use of alloca" misleading when
a call to the function was, in fact, bounded but to a limit
that's greater than alloca-max-size as in the program below:
void f (void*);
void g (int n)
{
void *p;
if (n < 4096)
p = __builtin_alloca (n);
else
p = __builtin_malloc (n);
f (p);
}
t.C: In function ‘g’:
t.C:7:7: warning: unbounded use of alloca [-Walloca]
p = __builtin_alloca (n);
I would suggest to rephrase the diagnostic to mention the limit,
e.g.,
warning: calling alloca with an argument in excess of '4000'
bytes
I'm not sure I understand how -Walloca-max-size is supposed to
be used. For example, it has no effect on the test case above
(i.e., I couldn't find a way to use it to raise the limit to
avoid the warning). Maybe the interaction of the two options
is more subtle than I think. I would have expected either
a single option to control whether alloca warnings are to be
emitted and also (optionally) the allocation threshold, or
perhaps two options, one to turn the warning on and off, and
another just to override the threshold (though this latter
approach seems superfluous given that a single option can do
both).
In other words, I would expect a -Walloca alone to diagnose
all calls to alloca with an argument in excess of the default
maximum or with one whose value is unknown, and -Walloca=N
to enable the same warnings but change the threshold to N
bytes. (-Walloca=0 would then diagnose every call to alloca).
As a separate enhancement, since in the (idiomatic) example
above the malloc memory is allowed to leak, issuing a distinct
warning for it would help detect this class of bugs that's
likely to be common as users replace unbounded uses of alloca
with malloc in response to the new option and forget to free
the memory. Diagnosing freeing the alloca memory would be
a nice touch as well.
Aldy
p.s. The pass currently warns on all uses of VLAs. I'm not completely
sold on this idea, so perhaps we could remove it, or gate it with a flag.
I also think that VLA diagnostics would be better controlled
by a separate option, and emit a different diagnostic (one
that mentions VLA rather than alloca). Although again, and
for VLAs even more so than for alloca, providing an option
to have GCC use dynamic allocation, would be an even more
robust solution than issuing warnings. IIRC, this was the
early implementation of VLAs in GCC so there is a precedent
for it. (Though this seems complementary to the warnings.)
In addition, I'm of the opinion that potentially unbounded
VLA allocation should be checked at runtime and made trap on
size overflow in C and throw an exception in C++ (e.g., when
int a [A][B] when A * B * sizeof (int) exceeds SIZE_MAX / 2
or some runtime-configurable limit). My C++ patch for bug
69517 does just that (it needs to be resubmitted with the
runtime configuration limit added).
For a choice of VLA warning options, there already is -Wvla,
though it simply points out every VLA definition regardless
of its size. It also issues a diagnostic that's questionable
in C99 and later modes (that "ISO C90 forbids variable length
array" is only relevant in C90; what matters when -Wvla is
specified in C99 and C11 modes is that a VLA has been seen).
To keep things consistent with -Walloca, perhaps -Wvla could
be extended to take an optional argument to specify the VLA
threshold. (So that -Wvla=4096 would only diagnose VLA
definitions of 4k bytes or more, or unknown size.)
Beyond a separate option for VLAs, in my limited testing I
couldn't figure out how constrain the VLA size so that using
-Walloca wouldn't complain for example on the following code:
void f (void*);
void h (int n)
{
if (n < 100)
{
int a [n];
f (a);
}
}
Martin
