[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: [RFC] [PATCH] Implement -ffortify for C/C++
From:       "Richard Guenther" <richard.guenther () gmail ! com>
Date:       2007-01-30 16:26:46
Message-ID: 84fc9c000701300826s1b485c27lfaaf6f3f24a4744f () mail ! gmail ! com
[Download RAW message or body]

On 1/30/07, Dirk Mueller <dmueller@suse.de> wrote:
>
> Hi,
>
> this patch adds a command line switch -ffortify=1,2 that works simliar to the
> glibc define -D_FORTIFY_SOURCE=1,2. The main difference to the glibc
> implementation is that it also works for C++ code. The glibc implementation
> uses #define's that replace certain string and memory related functions (e.g.
> strcpy) with the strcpy_chk variant. This does not work with C++ due to
> namespace issues and is therefore disabled alltogether for this language. The
> gcc implementation does not suffer from this issue and can also fortify
> namespaced calls, e.g. calls to "std::strcpy" correctly.
>
> The patch is so far bootstrapped for c,c++ and fortran and tested on various
> testcases and real world code. regression test is unaffected since it is not
> enabled by default. A regression test run with unconditionally enabled
> ffortify leads to some failures, which I believe are related to testcases not
> expecting the rewrite (investigation still in progress).
>
> Comments?

First of all thanks for this work!  Now, instead of overloading
-ffortify=@var{n}
can you introduce -Wfortify for the compile-time warnings and -ffortify for the
instrumentation?  I don't know what the higher levels you mention (n > 2) do
and if they are useful, but -ffortify=0 looks useless, -ffortify=1 would be
-Wfortify and -ffortify=2 -ffortify.

There are several over-long lines in the patch which need re-indentation, like

+/* Fortify the str* or mem* related builtin FUNCTION into the appropriate
+   _chk variant given by FCODE, by constructing an additional parameter
+   calling __builtin_object_size with the parameter BOS on the first argument
+   of PARAMS. If anything fails, return NULL_TREE.  */
+
+

Only one line here.

+static tree
+fortify_mem_builtin_fn(enum built_in_function fcode, int bos, tree
function, tree params)
+{
+  if (TREE_VALUE (params))
+    {

Better write as
  if (!TREE_VALUE (params))
    return NULL_TREE;

  ..unindented block..


+static tree
+fortify_printf_builtin_fn (int flag_num, bool append_bos,
+    enum built_in_function fcode, tree function, tree params)

align the second line properly

Richard.

[prev in list] [next in list] [prev in thread] [next in thread]