RFA: new pass to warn on questionable uses of alloca() and VLAs

[New thread now that I actually have a tested patch :)].

> 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).

As Jeff said, we were thinking the other way around: notice a malloced 
area that doesn't escape and replace it with a call to alloca.  But all 
this is beyond the scope of this patch.

>
> 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!)

BTW, thank you so much for taking the time to look into this.  Your 
feedback has been invaluable.

>
> 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:

I have added various levels of granularity for the warning, along with 
appropriately different messages:

// Possible problematic uses of alloca.
enum alloca_type {
   // Alloca argument is within known bounds that are appropriate.
   ALLOCA_OK,

   // Alloca argument is KNOWN to have a value that is too large.
   ALLOCA_BOUND_DEFINITELY_LARGE,

   // Alloca argument may be too large.
   ALLOCA_BOUND_MAYBE_LARGE,

   // Alloca argument is bounded but of an indeterminate size.
   ALLOCA_BOUND_UNKNOWN,

   // Alloca argument was casted from a signed integer.
   ALLOCA_CAST_FROM_SIGNED,

   // Alloca appears in a loop.
   ALLOCA_IN_LOOP,

   // Alloca call is unbounded.  That is, there is no controlling
   // predicate for its argument.
   ALLOCA_UNBOUNDED
};

Of course, there are plenty of cases where we can't get the exact 
diagnosis (due to the limitations on our range info) and we fall back to 
ALLOCA_UNBOUNDED or ALLOCA_BOUND_MAYBE_LARGE.  In practice, I'm 
wondering whether we should lump everything into 2-3 warnings instead of 
trying so hard to get the exact reason for the problematic use of 
alloca.  (More details on upcoming changes to range info further down.)

>
>   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);

Well, in this particular case you are using a signed int, so n < 4096 
can cause the value passed to alloca  to be rather large in the case of 
n < 0.

>
> I would suggest to rephrase the diagnostic to mention the limit,
> e.g.,
>
>   warning: calling alloca with an argument in excess of '4000'
>   bytes

In the attached patch I try to diagnose these cases with:

a.c: In function ‘g’:
a.c:7:10: warning: cast from signed type in alloca [-Walloca]
p = __builtin_alloca (n);

I'm not 100% convinced this the best idea, and I could be easily 
convinced to narrow the wide variety of warning cases I currently have 
into just a handful less specific ones.

>
> 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

If by subtle you mean buggy, then yes-- thank you for your kind words 
:).  I have fixed it all, and those found responsible have been sacked.

> 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).
...
> I also think that VLA diagnostics would be better controlled
> by a separate option, and emit a different diagnostic (one

I have overhauled the options and added extensive documentation to 
invoke.texi explaining them.  See the included testcases.  I have tried 
to add a testcase for everything the pass currently handles.

In the interest of keeping a consistent relationship with -Wvla, we now 
have:

-Walloca:	Warn on every use of alloca (not VLAs).
-Walloca=999:	Warn on unbounded uses of alloca, bounded uses with
		no known limit, and bounded uses where the number of
		bytes is greater than 999.
-Wvla:		Behaves as currently (warn on every use of VLAs).
-Wvla=999:	Similar to -Walloca=999, but for -Wvla.

Notice plain -Walloca doesn't have a default, and just warns on every 
use of alloca, just like -Wvla does for VLAs.  This way we can be 
consistent with -Wvla, and just add the -Wvla=999 variant.

I have added appropriate warnings to avoid confusion with -Wvla=0 and 
-Walloca=0, because they would conflict with -Wno-vla and -Wno-alloca 
(setting warn_vla=0 and warn_alloca=0 respectively).

The option handling is a bit convoluted because no -Wvla passed is 
warn_vla == -1, and CPP and the FE's depend on this information to know 
when no -Wvla was passed (pedantic warnings and some CPP pre-defines). 
So to keep everything consistent I had to differentiate between no -Wvla 
passed, plain -Wvla passed, and -Wvla=999 passed, without getting 
confused by -Wvla=0 and -Wno-vla.  Ughhh...

> 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);
>     }
>   }

Fixed.  Also, you have a signed int here as well.  That could cause a 
very large allocation.

This patch depends on range information, which is less than stellar past 
the VRP pass.  To get better range information, we'd have to incorporate 
this somehow into the VRP pass and use the ASSERT_EXPRs therein.  And 
still, VRP gets awfully confused with PHI merge points.  Andrew Macleod 
is working on a new fancy range info infrastructure that should 
eliminate a lot of the false positives we may get and/or help us 
diagnose a wider range of cases.  Hence the reason that I'm not bending 
over backwards to incorporate this pass into tree-vrp.c.

FYI, I have a few XFAILed tests in this patch, to make sure we don't 
lose track of possible improvements (which in this case should be 
handled by better range info).  What's the blessed way of doing this? 
Are adding new XFAILed tests acceptable?

I have regstrapped this patch on x86-64 Linux, and have tested the 
resulting compiler by building glibc with:

/source/glibc/configure --prefix=/usr CC="/path/bin/gcc -Walloca=5000" 
--disable-werror

This of course, pointed out all sorts of interesting things!

Fire away!

Aldy
gcc/

	* Makefile.in (OBJS): Add gimple-ssa-warn-walloca.o.
	* common.opt (Walloca): New.
	(Walloca=): New.
	* passes.def: Add pass_walloca.
	* tree-pass.h (make_pass_walloca): New.
	* gimple-ssa-warn-walloca.c: New file.
	* opts.c (finish_options): Warn on specific variants of -Walloca
	and -Wvla=.
	(common_handle_option): Add cases for OPT_Walloca and OPT_Walloca_.
	* doc/invoke.texi: Document -Walloca, -Walloca=, and -Wvla
	options.

gcc/c-family/

	* c-cppbuiltin.c (c_cpp_builtins): Make sure not all negative
	numbers get treated the same when evaluating warn_vla.
	* c-opts.c (c_common_handle_option): Add case for OPT_Wvla and
	OPT_Wvla_.
	* c.opt: Add entries for Walloca, Walloca=, and Wvla=.

gcc/c/

	* c-decl.c (warn_variable_length_array): Do not warn when
	-Wvla=999 is being used.
	* c-errors.c (pedwarn_c90): Not all negative numbers case a
	warning to be silenced.

gcc/cp/

	* decl.c (compute_array_index_type): Handle -Wvla=999 warning.

RFA: new pass to warn on questionable uses of alloca() and VLAs

Commit Message

Comments

Patch