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

On 07/16/2016 05:07 PM, Martin Sebor wrote:

[Addressed all of Manu's suggestions as well.]

>> Done.  -Walloca and -Wvla warn on any use of alloca and VLAs
>> accordingly, with or without optimization.  I sorry() on the bounded
>> cases.
>
> I think it's an improvement though I suspect we each have a slightly
> different understanding of what the sorry message is meant to be used
> for.  It's documented in the Diagnostics Conventions section of the
> GCC Coding Conventions as:
>
>    sorry is for correct user input programs but unimplemented
>    functionalities.
>
> I take that to mean that it should be used for what is considered
> valid user input that cannot be processed because the functionality
> is not yet implemented (but eventually will be).  So unless this
> case falls into this category I would expect GCC to issue a warning
> saying that the options have no effect (or limited effect, whatever
> the case may be) without optimization. But maybe I'm not reading
> the coding conventions text right.

Technically we could add the functionality later.  I don't know whether 
the new range info work can be made to work with lower optimization 
levels.  But really, I don't care :).  Adjusted to a warning.

>
>>> 2) When passed an argument of a signed type, GCC prints
>>>
>>>    warning: cast from signed type in alloca
>>>
>>> even though there is no explicit cast in the code.  It may not
>>> be obvious why the conversion is a problem in this context.  I
>>> would suggest to rephrase the warning along the lines of
>>> -Wsign-conversion which prints:
>>>
>>>    conversion to ‘long unsigned int’ from ‘int’ may change the sign of
>>> the result
>>>
>>> and add why it's a potential problem.  Perhaps something like:
>>>
>>>    argument to alloca may be too large due to conversion from
>>>    'int to 'long unsigned int'
>>
>> Fixed:
>
> Cool.
>
> FWIW, by coincidence I was just educated about the subtle nuances
> of quoting in GCC messages in a discussion with David and Manu.
> Types, functions, variables, and literals that appear in the source
> code should be referenced in diagnostics by using the "%qT", "%qD",
> and "%qE" directives so that GCC can add the right quotes and
> highlighting.  Enclosing "'%T'" in quotes will not use the same
> kind of quotes as "%qT" and won't highlight the type name.

Fixed.

>
>    https://gcc.gnu.org/wiki/DiagnosticsGuidelines
>
>> There is a "documented" reason for this: :)
>>
>>        // Do not warn on VLAs occurring in a loop, since VLAs are
>>        // guaranteed to be cleaned up when they go out of scope.
>>        // That is, there is a corresponding __builtin_stack_restore
>>        // at the end of the scope in which the VLA occurs.
>
> Yes, I understand that VLAs in loops are treated differently than
> alloca.  But I don't think this is quite how the logic should work.
> I.e., an excessively large VLA should be diagnosed regardless of
> whether it's in a loop or outside.  Consider the following case
> where with the patch as is, the warning is issued only for one
> of the two functions, even though they both allocate a VLA in
> excess of the threshold.

Agreed...

>
>    #define FOO(n) if (1) { \
>        char a [n]; \
>        f (a); \
>      } else (void)0
>
>    #define BAR(n) do { \
>        char a [n]; \
>        f (a); \
>      } while (0)
>
>    void f (void*);
>
>    void foo (void)
>    {
>      int n = 8000;
>      FOO (n);        // warning with -Wla-larger-than=4000
>    }
>
>    void bar (void)
>    {
>      int n = 8000;
>      BAR (n);        // no warning
>    }

...though it looks like your testcases may get optimized away.

I've added this testcase:

void
f6 (unsigned stuff)
{
   int n = 7000;
   do {
     char a[n]; // { dg-warning "variable-length array is too large" }
     f0 (a);
   } while (stuff--);
}

I've changed the logic so we warn on large allocas whether they're for 
VLAs or otherwise, but no warning given on VLAs within a loop when we 
know the bounds.

>
>>> 5) The -Wvla=N logic only seems to take into consideration the number
>>> of elements but not the size of the element type. For example, I wasn't
>>> able to get it to warn on the following with -Wvla=255 or greater:
>>>
>>>    void f0 (void*);
>>>
>>>    void f1 (unsigned char a)
>>>    {
>>>      int x [a];   // or even char a [n][__INT_MAX__];
>>>      f0 (x);
>>>    }
>>
>> That was a huge oversight (or should I say over-engineering) on my part.
>>   Fixed.
>
> Looks good.
>
> I did notice one minor glitch, though not one caused by your patch.
> GCC apparently transforms simple VLAs that are 256 bytes or less
> in size into ordinary arrays (i.e., it doesn't call
> __builtin_alloca_with_align).  Because of that, specifying
> -Wvla-larger-than=N with N less than 256 may not give a warning,
> as in the example below.  I suspect there may not be anything
> the Walloca pass can do about this so perhaps just mentioning
> it in the manual might be enough to avoid bug reports about false
> negatives.

Documentation updated.

>
>    void f0 (void*);
>
>    unsigned f1 (void) { return 256; }
>
>    void f2 (void)
>    {
>      unsigned n = f1 ();
>      char a [n];
>      f0 (a);
>    }
>
> GCC doesn't do the same transformation for alloca calls so the
> -Walloca-larger-than warning doesn't have this quirk.
>
>> This is a problem with the generic machinery so I would prefer someone
>> file a bugzilla report :), as this affects other options.
>
> I raised bug 71905 for this.
>
>> Ughhh...you're making me write user friendly stuff.  The reason I got
>> into compilers was so I wouldn't have to deal with the user :).
>>
>>    if (n < 2000)
>>      {
>>      p = __builtin_alloca (n);
>>      f (p);
>>      }
>>
>> ./cc1 a.c -fdump-tree-all-vops-alias-details-graph -quiet -I/tmp
>> -Walloca-larger-than=100 -O2
>> a.c: In function ‘g2’:
>> a.c:9:7: warning: argument to alloca may be too large
>> [-Walloca-larger-than=]
>>       p = __builtin_alloca (n);
>>       ~~^~~~~~~~~~~~~~~~~~~~~~
>> a.c:9:7: note: limit is '100' bytes, but argument may be '1999'
>>
>> Happy? :-)
>
> I along with untold numbers of users thank you! :)
>
> (As another quoting nit, unlike functions, variables, and constants
> that appear in the source code, ordinary numbers apparently aren't
> supposed to be quoted in GCC messages.  (I'm guilty of making this
> mistake in at least two of my own patches and will be correcting
> it.)

Fixed.

How does this look?

Aldy
gcc/

	* Makefile.in (OBJS): Add gimple-ssa-warn-walloca.o.
	* passes.def: Add two instances of pass_walloca.
	* tree-pass.h (make_pass_walloca): New.
	* gimple-ssa-warn-walloca.c: New file.
	* opts.c (finish_options): Warn when using -Wvla-larger-than= and
	-Walloca-larger-than= without -O2 or greater.
	* doc/invoke.texi: Document -Walloca, -Walloca-larger-than=, and
	-Wvla-larger-than= options.

gcc/c-family/

	* c.opt (Walloca): New.
	(Walloca-larger-than=): New.
	(Wvla-larger-than=): New.

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

Commit Message

Comments

Patch