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: Martin Sebor <msebor at gmail dot com>
- To: Segher Boessenkool <segher at kernel dot crashing dot org>
- 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:44:40 -0600
- 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> <20150728142406 dot GA14409 at gate dot crashing dot org>
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).
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/ChangeLog
2015-07-28 Martin Sebor <msebor@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 <msebor@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.
diff --git a/gcc/builtins.c b/gcc/builtins.c
index e8fe3db..b7c5572 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -4564,34 +4564,38 @@ expand_builtin_frame_address (tree fndecl, tree exp)
{
/* The argument must be a nonnegative integer constant.
It counts the number of frames to scan up the stack.
- The value is the return address saved in that frame. */
+ The value is either the frame pointer value or the return
+ address saved in that frame. */
if (call_expr_nargs (exp) == 0)
/* Warning about missing arg was already issued. */
return const0_rtx;
else if (! tree_fits_uhwi_p (CALL_EXPR_ARG (exp, 0)))
{
- 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);
return const0_rtx;
}
else
{
- 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. */
+ unsigned HOST_WIDE_INT count = tree_to_uhwi (CALL_EXPR_ARG (exp, 0));
+
+ rtx tem = expand_builtin_return_addr (DECL_FUNCTION_CODE (fndecl), count);
/* 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, "unsupported argument to %qD", fndecl);
return const0_rtx;
}
+ if (count)
+ {
+ /* Warn since no effort is made to ensure that any frame
+ beyond the current one exists or can be safely reached. */
+ warning (OPT_Wframe_address, "calling %qD with "
+ "a nonzero argument is unsafe", fndecl);
+ }
+
/* For __builtin_frame_address, return what we've got. */
if (DECL_FUNCTION_CODE (fndecl) == BUILT_IN_FRAME_ADDRESS)
return tem;
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 285952e..ccbb399 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -295,6 +295,10 @@ Wbool-compare
C ObjC C++ ObjC++ Var(warn_bool_compare) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
Warn about boolean expression compared with an integer value different from true/false
+Wframe-address
+C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
+Warn when __builtin_frame_address or __builtin_return_address is used unsafely
+
Wbuiltin-macro-redefined
C ObjC C++ ObjC++ CPP(warn_builtin_macro_redefined) CppReason(CPP_W_BUILTIN_MACRO_REDEFINED) Var(cpp_warn_builtin_macro_redefined) Init(1) Warning
Warn when a built-in preprocessor macro is undefined or redefined
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 9ad2b68..96b8b80 100644
--- 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{-Wframe-address}
+option is in effect. Such calls should only be made in debugging
+situations.
@end deftypefn
@deftypefn {Built-in Function} {void *} __builtin_extract_return_addr (void *@var{addr})
@@ -8601,8 +8604,11 @@ any function other than the current one; in such cases, or when the top
of the stack has been reached, this function returns @code{0} if
the first frame pointer is properly initialized by the startup code.
-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{-Wframe-address}
+option is in effect. Such calls should only be made in debugging
+situations.
@end deftypefn
@node Vector Extensions
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5903c75..189d408 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -241,7 +241,7 @@ Objective-C and Objective-C++ Dialects}.
-pedantic-errors @gol
-w -Wextra -Wall -Waddress -Waggregate-return @gol
-Waggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} @gol
--Wbool-compare @gol
+-Wbool-compare -Wframe-address @gol
-Wno-attributes -Wno-builtin-macro-redefined @gol
-Wc90-c99-compat -Wc99-c11-compat @gol
-Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol
@@ -4436,6 +4436,13 @@ if ((n > 1) == 2) @{ @dots{} @}
@end smallexample
This warning is enabled by @option{-Wall}.
+@item -Wframe-address
+@opindex Wno-frame-address
+@opindex Wframe-address
+Warn when the @samp{__builtin_frame_address} or @samp{__builtin_return_address}
+is called with an argument greater than 0. Such calls may return indeterminate
+values or crash the program. The warning is included in @option{-Wall}.
+
@item -Wno-discarded-qualifiers @r{(C and Objective-C only)}
@opindex Wno-discarded-qualifiers
@opindex Wdiscarded-qualifiers
diff --git a/gcc/testsuite/g++.dg/Wframe-address-in-Wall.C b/gcc/testsuite/g++.dg/Wframe-address-in-Wall.C
new file mode 100644
index 0000000..2d945e5
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wframe-address-in-Wall.C
@@ -0,0 +1,14 @@
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+// Verify that -Wframe-address is included in -Wall.
+
+void* test_builtin_address (unsigned i)
+{
+ void* const ba[] = {
+ __builtin_frame_address (4), // { dg-warning "builtin_frame_address" }
+ __builtin_return_address (4) // { dg-warning "builtin_return_address" }
+ };
+
+ return ba [i];
+}
diff --git a/gcc/testsuite/g++.dg/Wframe-address.C b/gcc/testsuite/g++.dg/Wframe-address.C
new file mode 100644
index 0000000..229004e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wframe-address.C
@@ -0,0 +1,70 @@
+// { dg-do compile }
+// { dg-options "-Wframe-address" }
+
+static void* const fa[] = {
+ __builtin_frame_address (0),
+ __builtin_frame_address (1), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (2), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (3), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (4) // { dg-warning "builtin_frame_address" }
+};
+
+
+static void* const ra[] = {
+ __builtin_return_address (0),
+ __builtin_return_address (1), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (2), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (3), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (4) // { dg-warning "builtin_return_address" }
+};
+
+
+void* __attribute__ ((noclone, noinline))
+test_builtin_frame_address (unsigned i)
+{
+ void* const fa[] = {
+ __builtin_frame_address (0),
+ __builtin_frame_address (1), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (2), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (3), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (4) // { dg-warning "builtin_frame_address" }
+ };
+
+ return fa [i];
+}
+
+
+void* __attribute__ ((noclone, noinline))
+test_builtin_return_address (unsigned i)
+{
+ void* const ra[] = {
+ __builtin_return_address (0),
+ __builtin_return_address (1), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (2), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (3), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (4) // { dg-warning "builtin_return_address" }
+ };
+ return ra [i];
+}
+
+
+int main ()
+{
+ test_builtin_frame_address (0);
+
+ test_builtin_return_address (0);
+
+ void* const a[] = {
+ __builtin_frame_address (0),
+ __builtin_frame_address (1), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (2), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (3), // { dg-warning "builtin_frame_address" }
+ __builtin_frame_address (4), // { dg-warning "builtin_frame_address" }
+
+ __builtin_return_address (0),
+ __builtin_return_address (1), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (2), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (3), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (4) // { dg-warning "builtin_return_address" }
+ };
+}
diff --git a/gcc/testsuite/g++.dg/Wno-frame-address.C b/gcc/testsuite/g++.dg/Wno-frame-address.C
new file mode 100644
index 0000000..b19cb43
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wno-frame-address.C
@@ -0,0 +1,6 @@
+// { dg-do compile }
+// { dg-options "-Werror" }
+
+// Verify that -Wframe-address is not enabled by default by enabling
+// -Werror and verifying the test still compiles.
+#include "Wframe-address.C"
diff --git a/gcc/testsuite/gcc.dg/Wframe-address-in-Wall.c b/gcc/testsuite/gcc.dg/Wframe-address-in-Wall.c
new file mode 100644
index 0000000..70da9c8
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wframe-address-in-Wall.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+/* Verify that -Wframe-address is included in -Wall. */
+
+void* test_builtin_address (unsigned i)
+{
+ void* const ba[] = {
+ __builtin_frame_address (4), /* { dg-warning "builtin_frame_address" } */
+ __builtin_return_address (4) /* { dg-warning "builtin_return_address" } */
+ };
+
+ return ba [i];
+}
diff --git a/gcc/testsuite/gcc.dg/Wframe-address.c b/gcc/testsuite/gcc.dg/Wframe-address.c
new file mode 100644
index 0000000..7481baf
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wframe-address.c
@@ -0,0 +1,54 @@
+/* { dg-do compile } */
+/* { dg-options "-Wframe-address" } */
+
+void* __attribute__ ((noclone, noinline))
+test_builtin_frame_address (unsigned i)
+{
+ void* const fa[] = {
+ __builtin_frame_address (0),
+ __builtin_frame_address (1), /* { dg-warning "builtin_frame_address" } */
+ __builtin_frame_address (2), /* { dg-warning "builtin_frame_address" } */
+ __builtin_frame_address (3), /* { dg-warning "builtin_frame_address" } */
+ __builtin_frame_address (4) /* { dg-warning "builtin_frame_address" } */
+ };
+
+ return fa [i];
+}
+
+
+void* __attribute__ ((noclone, noinline))
+test_builtin_return_address (unsigned i)
+{
+ void* const ra[] = {
+ __builtin_return_address (0),
+ __builtin_return_address (1), /* { dg-warning "builtin_return_address" } */
+ __builtin_return_address (2), /* { dg-warning "builtin_return_address" } */
+ __builtin_return_address (3), /* { dg-warning "builtin_return_address" } */
+ __builtin_return_address (4) /* { dg-warning "builtin_return_address" } */
+ };
+ return ra [i];
+}
+
+
+int main (void)
+{
+ test_builtin_frame_address (0);
+
+ test_builtin_return_address (0);
+
+ void* const a[] = {
+ __builtin_frame_address (0),
+ __builtin_frame_address (1), /* { dg-warning "builtin_frame_address" } */
+ __builtin_frame_address (2), /* { dg-warning "builtin_frame_address" } */
+ __builtin_frame_address (3), /* { dg-warning "builtin_frame_address" } */
+ __builtin_frame_address (4), /* { dg-warning "builtin_frame_address" } */
+
+ __builtin_return_address (0),
+ __builtin_return_address (1), /* { dg-warning "builtin_return_address" } */
+ __builtin_return_address (2), /* { dg-warning "builtin_return_address" } */
+ __builtin_return_address (3), /* { dg-warning "builtin_return_address" } */
+ __builtin_return_address (4) /* { dg-warning "builtin_return_address" } */
+ };
+
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/Wno-frame-address.c b/gcc/testsuite/gcc.dg/Wno-frame-address.c
new file mode 100644
index 0000000..f48b91a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wno-frame-address.c
@@ -0,0 +1,6 @@
+/* { dg-do compile } */
+/* { dg-options "-Werror" } */
+
+/* Verify that -Wframe-address is not enabled by default by enabling
+ -Werror and verifying the test still compiles. */
+#include "Wframe-address.c"