Re: [PATCH]: New warning option -Walloca

["Raksit Ashok" <raksit dot ashok at gmail dot com>]

On 9/18/07, Manuel López-Ibáñez <lopezibanez@gmail.com> wrote:
> On 17/09/2007, Raksit Ashok <raksit.ashok@gmail.com> wrote:
> > On 8/30/07, Dave Korn <dave.korn@artimi.com> wrote:
> > >   It seems a bit highly specialised to me... a generic
> > > "-Wdisallowed=alloca,foo,bar,..." option would be nicer, if you felt like
> > > extending it.
> >
> > Following your advice, Dave, I have implemented a more general
> > warning: -Wdisallowed-function-list=foo,bar,alloca,... : emit warning
> > on calls to these functions.
> >
> > OK for mainline?
> >
>
> I am not a maintainer, so I cannot approve (or reject) your patch:
>
> +@item -Wdisallowed-function-list=@var{sym},@var{sym},@dots{}
> +@opindex wdisallowed-function-list
> +
> +If any of @var{sym} is called, GCC will issue a warning. This can be useful
> +in enforcing coding conventions that ban calls to certain functions, for
> +example, @code{alloca}, @code{malloc}, etc.
>  @end table
>
> The opindex should be the same case as the option name.

Good catch! I will fix this.


>
> +Wdisallowed-function-list=
> +Common RejectNegative Joined Warning
> +-Wdisallowed-function-list=foo,bar,...  Warn on calls to these functions.
> +
>
> would be better as:
>
> +Wdisallowed-function-list=
> +C ObjC C++ ObjC++ RejectNegative Joined Warning
> +Warn on calls to these functions.
>
> And also move it to c.opt since it your implementation is C-C++-only currently.

OK. I will move this to c.opt

>
> warn_disallowed_function_p() definition would be better in a different
> file from opts.c. In your case, I think that c-common.c since
> currently it is c-c++-only.
>
> Similarly:
> --- gcc/flags.h (revision 128551)
> +++ gcc/flags.h (working copy)
> @@ -285,6 +285,13 @@
>     instrumentation.  */
>  extern bool flag_instrument_functions_exclude_p (tree fndecl);
>
> +/* Return whether the function call is disallowed under
> +   -Wdisallowed-function-list=...  */
> +extern bool warn_disallowed_function_p (const_tree fncall);
> +
>
> I think that this function should declared in c-common.h.

Keeping warn_disallowed_function_p() in opts.c (instead of
c-common.c), and handling this new option in opts.c (instead of
c-opts.c) makes the code a lot cleaner. This is because all this new
code shares some common definitions (DEF_VEC_P of char_p), and
functions (the refactored add_comma_separated_to_vector() function)
with the -finstrument-functions-exclude* handling code in opts.c

But if you feel strongly about moving warn_disallowed_function_p() to
c-common.c, and handling the option in c-opts.c, I will make the
changes you suggested.

>
> +
> +/* Return whether this function call is disallowed.  */
> +bool
> +warn_disallowed_function_p (const_tree exp)
> +{
> +  if (TREE_CODE(exp) == CALL_EXPR
> +      && VEC_length (char_p, warning_disallowed_functions) > 0)
> +    {
> +      int i;
> +      char *s;
> +      const char *fnname =
> +          IDENTIFIER_POINTER (DECL_NAME (get_callee_fndecl (exp)));
> +      for (i = 0; VEC_iterate (char_p, warning_disallowed_functions, i, s);
> +           ++i)
> +        {
> +          if (strcmp (fnname, s) == 0)
> +            return true;
> +        }
> +    }
> +  return false;
> +}
> +
>
> Since the list of functions is not modified frequently, it would be
> nice to sort it when building it and then do a binary search. Do we
> have functions to handle this?

I imagined that this list of functions would be typically small (upto
5 functions say). So sorting and binary search seems like overkill to
me. I don't know of functions that do this already in gcc.

> Also, why not include the code for the warning in warn_disallowed_function_p():
>
> +            {
> +              const_tree fndecl = get_callee_fndecl (expr.value);
> +              const char *fnname = IDENTIFIER_POINTER (DECL_NAME (fndecl));
> +              warning (OPT_Wdisallowed_function_list_,
> +                       "disallowed call to %qs", fnname);
> +            }
>
> That will avoid some duplication and also avoid calculating the
> function name twice. Then you will simply do:
>
> warn_for_disallowed_function (expr.value);
>
> or warn_if_disallowed_function(expr.value), or some similar name.

Great idea, will change this.

>
> Cheers,
>
> Manuel.

Thanks a lot for the feedback!

-raksit