Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Jeff Law]

On 07/28/2015 09:44 AM, Martin Sebor wrote:

gcc/ChangeLog
2015-07-27  Martin Sebor <mse...@redhat.com>

     * c-family/c.opt (-Wbuiltin-address): New warning option.
     * doc/invoke.texi (Wbuiltin-address): Document it.
     * doc/extend.texi (__builtin_frame_addrress,
__builtin_return_addrress):

Typoes (rr).

Fixed.

-      rtx tem
-    = expand_builtin_return_addr (DECL_FUNCTION_CODE (fndecl),
-                      tree_to_uhwi (CALL_EXPR_ARG (exp, 0)));
+      /* Number of frames to scan up the stack.  */
+      const unsigned HOST_WIDE_INT count = tree_to_uhwi
(CALL_EXPR_ARG (exp, 0));
+
+      rtx tem = expand_builtin_return_addr (DECL_FUNCTION_CODE
(fndecl), count);

Do we need to say "const"?

No, we don't. FWIW, I find code easier to think about when it's
explicit about things like this, even if they have no semantic
effect. But since it's not common practice I took the const out.

        /* Some ports cannot access arbitrary stack frames.  */
        if (tem == NULL)
      {
-      if (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_FRAME_ADDRESS)
-        warning (0, "unsupported argument to
%<__builtin_frame_address%>");
-      else
-        warning (0, "unsupported argument to
%<__builtin_return_address%>");
+      warning (0, "invalid argument to %qD", fndecl);

"unsupported argument".

Thanks, fixed.

+      if (0 < count)

Yoda :-)  You can just say "if (count)" fwiw.

Sure.

This is not such a nice warning name, maybe -Wbuiltin-frame-address or
-Wframe-address?

I renamed it to -Wframe-address.

I like the original "should only be used" better than that last line.

Okay, reworded.

Elsewhere there was a "non-zero" btw, but we should use "nonzero"
according
to the coding conventions.  Huh.

Changed.

Not all targets support weak.

I replaced it with __attribute__((noclone, noinline)).

Attached is an updated patch with the changes above.

Thanks
Martin

gcc-Wframe-address.patch


gcc/ChangeLog
2015-07-28  Martin Sebor<mse...@redhat.com>

     * c-family/c.opt (-Wframe-address): New warning option.
     * doc/invoke.texi (Wframe-address): Document it.
     * doc/extend.texi (__builtin_frame_address, __builtin_return_address):
     Clarify possible effects of calling the functions with non-zero
     arguments and mention -Wframe-address.
     * builtins.c (expand_builtin_frame_address): Handle -Wframe-address.

gcc/testsuite/ChangeLog
2015-07-28  Martin Sebor<mse...@redhat.com>

     * g++.dg/Wframe-address-in-Wall.C: New test.
     * g++.dg/Wframe-address.C: New test.
     * g++.dg/Wno-frame-address.C: New test.
     * gcc.dg/Wframe-address-in-Wall.c: New test.
     * gcc.dg/Wframe-address.c: New test.
     * gcc.dg/Wno-frame-address.c: New test.

OK for the trunk.  Sorry for the delay.

jeff