Re: [PATCH]: New warning option -Walloca

["Manuel López-Ibáñez" <lopezibanez at gmail dot com>]

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.

+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.

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.

+
+/* 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?
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.

Cheers,

Manuel.