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

[Aldy Hernandez <aldyh at redhat dot com>]

On 07/10/2016 07:41 PM, Manuel López-Ibáñez wrote:
> > +Walloca
> > +LangEnabledBy(C ObjC C++ ObjC++)
> > +; in common.opt
> > +
> > +Walloca=
> > +LangEnabledBy(C ObjC C++ ObjC++)
> > +; in common.opt
> > +
>
> I'm not sure what you think the above means, but this is an invalid use
> of LangEnabledBy(). (It would be nice if the .awk scripts were able to
> catch it and give an error.)

I was following the practice for -Warray-bounds in c-family/c.opt:

Warray-bounds
LangEnabledBy(C ObjC C++ ObjC++,Wall)
; in common.opt

Warray-bounds=
LangEnabledBy(C ObjC C++ ObjC++,Wall,1,0)
; in common.opt

...as well as the generic version in common.opt:

Warray-bounds
Common Var(warn_array_bounds) Warning
Warn if an array is accessed out of bounds.

Warray-bounds=
Common Joined RejectNegative UInteger Var(warn_array_bounds) Warning
Warn if an array is accessed out of bounds.

I *thought* you defined the option in common.opt, and narrowed the 
language variants in c-family/c.opt.  ??

> > +;; warn_vla == 0 for -Wno-vla
> > +;; warn_vla == -1 for nothing passed.
> > +;; warn_vla == -2 for -Wvla passed
> > +;; warn_vla == NUM for -Wvla=NUM
> >  Wvla
> >  C ObjC C++ ObjC++ Var(warn_vla) Init(-1) Warning
> >  Warn if a variable length array is used.
> >
> > +Wvla=
> > +C ObjC C++ ObjC++ Var(warn_vla) Warning Joined RejectNegative UInteger
> > +-Wvla=<number> Warn on unbounded uses of variable-length arrays, and
> > +warn on bounded uses of variable-length arrays whose bound can be
> > +larger than <number> bytes.
> > +
>
> This overloading of warn_vla seems confusing (as shown by all the places
> that require updating). Why not call it Wvla-larger-than= and use a
> different warn_vla_larger_than variable? We already have -Wlarger-than=
> and -Wframe-larger-than=.

Hey, I'm all for different options.  It would definitely be easier for 
me :).  I wast trying really hard to use -Wvla= and keep the current 
-Wvla meaning the same.  Though I see your point about using -Wvla* but 
using different variables for representing -Wvla and -Wvla=blah.

The easiest thing would be:

-Wvla: Current behavior (warn on everything).

-Wvla-larger-than=xxxx: Warn on unbounded VLA and bounded VLAs > xxxx.

-Walloca: Warn on every use of alloca (analogous to -Wvla).

-Walloca-larger-than=xxxx: Warn on unbounded alloca and bounded allocas 
> xxxx.

This seems straightforward and avoids all the overloading you see. 
Would this be ok with everyone?

> Using warn_vla_larger_than (even if you wish to keep -Wvla= as the
> option name) as a variable distinct from warn_vla would make things
> simpler:
>
> -Wvla => warn_vla == 1
> -Wno-vla => warn_vla == 0
> -Wvla=N => warn_vla_larger_than == N (where N == 0 means disable).
>
> If you wish that -Wno-vla implies -Wvla=0 then you'll need to handle
> that manually. I don't think we have support for that in the .opt files.
> But that seems far less complex than having a single shared Var().
>
> Cheers,
>      Manuel.

Thanks.
Aldy