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

[Aldy Hernandez <aldyh at redhat dot com>]

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

Attachment: curr
Description: Text document