This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] warn for unsafe calls to __builtin_return_address
- From: Segher Boessenkool <segher at kernel dot crashing dot org>
- To: Martin Sebor <msebor at gmail dot com>
- Cc: Jeff Law <law at redhat dot com>, Martin Sebor <msebor at redhat dot com>, Gcc Patch List <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 28 Jul 2015 09:24:06 -0500
- Subject: Re: [PATCH] warn for unsafe calls to __builtin_return_address
- Authentication-results: sourceware.org; auth=none
- References: <555E2FC6 dot 60804 at redhat dot com> <555E50A5 dot 60309 at redhat dot com> <557A0617 dot 6040605 at redhat dot com> <55B1C845 dot 1030000 at redhat dot com> <20150725142035 dot GB7309 at gate dot crashing dot org> <55B6F232 dot 2080402 at gmail dot com>
On Mon, Jul 27, 2015 at 09:08:34PM -0600, Martin Sebor wrote:
> >>So, my suggestion would be to warn for any call with a nonzero value.
> >
> >The current documentation says that you should only use nonzero values
> >for debug purposes. A warning would help yes, how many people read the
> >manual after all :-)
>
> Thank you both for the feedback. Attached is a simplified patch
> to issue a warning for all builtin_xxx_address calls with any
> non-zero argument.
>
> Martin
>
> gcc/ChangeLog
> 2015-07-27 Martin Sebor <msebor@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).
> - if (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_FRAME_ADDRESS)
> - error ("invalid argument to %<__builtin_frame_address%>");
> - else
> - error ("invalid argument to %<__builtin_return_address%>");
> + error ("invalid argument to %qD", fndecl);
That works? Nice.
> {
> - 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"?
> /* 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".
> return const0_rtx;
> }
>
> + if (0 < count)
Yoda :-) You can just say "if (count)" fwiw.
> +Wbuiltin-address
> +C ObjC C++ ObjC++ Var(warn_builtin_address) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
> +Warn when __builtin_frame_address or __builtin_return_address is used unsafely
This is not such a nice warning name, maybe -Wbuiltin-frame-address or
-Wframe-address?
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -8562,8 +8562,11 @@ to determine if the top of the stack has been reached.
> Additional post-processing of the returned value may be needed, see
> @code{__builtin_extract_return_addr}.
>
> -This function should only be used with a nonzero argument for debugging
> -purposes.
> +Calling this function with a nonzero argument can have unpredictable
> +effects, including crashing the calling program. As a result, calls
> +that are considered unsafe are diagnosed when the @option{-Wbuiltin-address}
> +option is in effect. Such calls are typically only useful in debugging
> +situations.
I like the original "should only be used" better than that last line.
Elsewhere there was a "non-zero" btw, but we should use "nonzero" according
to the coding conventions. Huh.
> +void* __attribute__ ((weak))
Not all targets support weak.
Segher