This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH]: New warning option -Walloca
- From: "Raksit Ashok" <raksit dot ashok at gmail dot com>
- To: "Manuel López-Ibáñez" <lopezibanez at gmail dot com>
- Cc: "Dave Korn" <dave dot korn at artimi dot com>, tromey at redhat dot com, gcc-patches at gcc dot gnu dot org, "Kaveh R. GHAZI" <ghazi at caip dot rutgers dot edu>, "Seongbae Park (박성배, 朴成培)" <spark at google dot com>, briangrant at google dot com, cgd at google dot com
- Date: Tue, 18 Sep 2007 14:24:24 -0700
- Subject: Re: [PATCH]: New warning option -Walloca
- References: <49886b970708301427r645c9dr58443fc40385cd6e@mail.gmail.com> <m3bqcoobpz.fsf@fleche.redhat.com> <00d501c7eb69$32d51fb0$2e08a8c0@CAM.ARTIMI.COM> <49886b970709171449q3f860a49q3746d53c76502af7@mail.gmail.com> <6c33472e0709180210p25e0cdddh9b8956f7e3b5f5c7@mail.gmail.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