Attached is an updated patch for both GCC and the manual.
The patch implements the suggested warning, -Wbuiltin-address,
that issues diagnostics for unsafe calls of the builtin address
functions. Safe calls are those with arguments 0 or 1 anywhere
in a program and argument 2 outside of the main function (since
every function has main as its direct or indirect caller).
Tested on powerpc64le and x86_64 Linux.
Martin
The ChangeLog entries for gcc and testsuite:
2015-06-11 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):
Clarify possible effects of calling the functions with
non-zero arguments and mention -Wbuiltin-address.
* builtins.c (expand_builtin_frame_address): Handle
-Wbuiltin-address.
2015-06-11 Martin Sebor <mse...@redhat.com>
* g++.dg/Wbuiltin-address-in-Wall.C: New test.
* g++.dg/Wbuiltin-address.C: New test.
* g++.dg/Wno-builtin-address.C: New test.
* gcc.dg/Wbuiltin-address-in-Wall.c: New test.
* gcc.dg/Wbuiltin-address.c: New test.
* gcc.dg/Wno-builtin-address.c: New test.
PS A few notes about the changes.
There's the following comment in expand_builtin_frame_address:
/* Some ports cannot access arbitrary stack frames. */
just before a block of code where the function can lead to
an "invalid argument" warning which would cause the newly
added tests to fail (since the newly added warning wouldn't
be issued).
I tried to determine what ports these might be so I could add
conditionals to the tests to prevent false positives there but
couldn't find any.
I wanted to also issue a warning for calls at file scope with
arguments greater than 1 (just like in main) but couldn't find
a way to determine that.
I also wanted to make the special treatment of main conditional
on whether or not -ffreestanding is in effect but flag_hosted
is not declared in builtins.c and bringing it into scope seemed
like too much of a change.
I'd be happy to modify the patch and add any of the above if
someone can suggest a way to do it without disrupting too much
code.
On 05/21/2015 03:39 PM, Pedro Alves wrote:
On 05/21/2015 08:19 PM, Martin Sebor wrote:
A program I instrumented to help me debug an otherwise unrelated
problem in 5.1.0 has been crashing in calls to
__builtin_return_address. After checking the manual, I didn't
think I was doing anything wrong. I then did some debugging and
found that the function simply isn't safe to call with non-zero
arguments near the top of the stack. That seemed like a bug to
me so I created a small test case and ran it on a few targets
to see if the problem was isolated to just powerpc (where I'm
working at the moment) or more general. It turned out not to
be target-specific. Before opening a bug, I checked Bugzilla
to see if it's already been reported but couldn't find any open
reports. To be sure I wasn't missing something, I expanded my
search to already resolved bugs. That's when I finally found
pr8743 which had been closed years ago as a documentation issue,
after adding the following to the manual:
This function should only be used with a nonzero argument
for debugging purposes.
Since I was using the function exactly for this purpose, I'd
like to propose the patch below to clarify the effects of the
function to set the right expectations and help others avoid
the effort it took me to figure out this is by design.
Does anyone have any concerns with this update or is it okay
to check in?
Sounds like a good candidate for a warning.
Thanks,
Pedro Alves
diff --git a/gcc/builtins.c b/gcc/builtins.c
index e8fe3db..58b0e13 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -4564,34 +4564,49 @@ 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. */
+ 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);
/* 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);
return const0_rtx;
}
+ if (1 < count)
+ {
+ /* The number of frames the function can safely scan.
+ Assume main has a caller, and (at least in a hosted
+ implementation) every other function has main as its
+ caller. */
+ unsigned safe_count = 2;
+
+ if (DECL_NAME (current_function_decl)
+ && MAIN_NAME_P (DECL_NAME (current_function_decl))
+ && DECL_FILE_SCOPE_P (current_function_decl))
+ safe_count = 1;
+
+ if (safe_count < count)
+ warning (OPT_Wbuiltin_address, "calling %qD with "
+ "an argument greater than %u is unsafe here",
+ fndecl, safe_count);
+ }
+
/* 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..bdff624 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
+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
+
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..1b4596c 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{-Wbuiltin-address}
+option is in effect. Such calls are typically only useful 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{-Wbuiltin-address}
+option is in effect. Such calls are typically only useful in debugging
+situations.
@end deftypefn
@node Vector Extensions
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 5903c75..92887a8 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 -Wbuiltin-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,15 @@ if ((n > 1) == 2) @{ @dots{} @}
@end smallexample
This warning is enabled by @option{-Wall}.
+@item -Wbuiltin-address
+@opindex Wno-builtin-address
+@opindex Wbuiltin-address
+Warn when the @samp{__builtin_frame_address} or @samp{__builtin_return_address}
+is used unsafely. Unsafe uses include calling the function with an argument
+greater than 1 in @samp{main} or greater than 2 anywhere else in a program.
+Such calls may return ndeterminate 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/Wbuiltin-address-in-Wall.C b/gcc/testsuite/g++.dg/Wbuiltin-address-in-Wall.C
new file mode 100644
index 0000000..aa9e17f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wbuiltin-address-in-Wall.C
@@ -0,0 +1,14 @@
+// { dg-do compile }
+// { dg-options "-Wall" }
+
+// Verify that -Wbuiltin-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/Wbuiltin-address.C b/gcc/testsuite/g++.dg/Wbuiltin-address.C
new file mode 100644
index 0000000..0be0244
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wbuiltin-address.C
@@ -0,0 +1,70 @@
+// { dg-do compile }
+// { dg-options "-Wbuiltin-address" }
+
+static void* const fa[] = {
+ __builtin_frame_address (0), // { dg-bogus "builtin_frame_address" }
+ __builtin_frame_address (1), // { dg-bogus "builtin_frame_address" }
+ __builtin_frame_address (2), // { dg-bogus "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), // { dg-bogus "builtin_return_address" }
+ __builtin_return_address (1), // { dg-bogus "builtin_return_address" }
+ __builtin_return_address (2), // { dg-bogus "builtin_return_address" }
+ __builtin_return_address (3), // { dg-warning "builtin_return_address" }
+ __builtin_return_address (4) // { dg-warning "builtin_return_address" }
+};
+
+
+void* __attribute__ ((weak))
+test_builtin_frame_address (unsigned i)
+{
+ void* const fa[] = {
+ __builtin_frame_address (0), // { dg-bogus "builtin_frame_address" }
+ __builtin_frame_address (1), // { dg-bogus "builtin_frame_address" }
+ __builtin_frame_address (2), // { dg-bogus "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__ ((weak))
+test_builtin_return_address (unsigned i)
+{
+ void* const ra[] = {
+ __builtin_return_address (0), // { dg-bogus "builtin_return_address" }
+ __builtin_return_address (1), // { dg-bogus "builtin_return_address" }
+ __builtin_return_address (2), // { dg-bogus "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), // { dg-bogus "builtin_frame_address" }
+ __builtin_frame_address (1), // { dg-bogus "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), // { dg-bogus "builtin_return_address" }
+ __builtin_return_address (1), // { dg-bogus "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-builtin-address.C b/gcc/testsuite/g++.dg/Wno-builtin-address.C
new file mode 100644
index 0000000..a460649
--- /dev/null
+++ b/gcc/testsuite/g++.dg/Wno-builtin-address.C
@@ -0,0 +1,6 @@
+// { dg-do compile }
+// { dg-options "-Werror" }
+
+// Verify that -Wbuiltin-address is not enabled by default by enabling
+// -Werror and verifying the test still compiles.
+#include "Wbuiltin-address.C"
diff --git a/gcc/testsuite/gcc.dg/Wbuiltin-address-in-Wall.c b/gcc/testsuite/gcc.dg/Wbuiltin-address-in-Wall.c
new file mode 100644
index 0000000..9f73810
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wbuiltin-address-in-Wall.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+/* Verify that -Wbuiltin-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/Wbuiltin-address.c b/gcc/testsuite/gcc.dg/Wbuiltin-address.c
new file mode 100644
index 0000000..224ff30
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wbuiltin-address.c
@@ -0,0 +1,57 @@
+/* { dg-do compile } */
+/* { dg-options "-Wbuiltin-address" } */
+
+void* __attribute__ ((weak))
+test_builtin_frame_address (unsigned i)
+{
+ void* const fa[] = {
+ __builtin_frame_address (0), /* { dg-bogus "builtin_frame_address" } */
+ __builtin_frame_address (1), /* { dg-bogus "builtin_frame_address" } */
+ __builtin_frame_address (2), /* { dg-bogus "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__ ((weak))
+test_builtin_return_address (unsigned i)
+{
+ void* const ra[] = {
+ __builtin_return_address (0), /* { dg-bogus "builtin_return_address" } */
+ __builtin_return_address (1), /* { dg-bogus "builtin_return_address" } */
+ __builtin_return_address (2), /* { dg-bogus "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);
+
+ // In main, calling the builtins is safe with an argument one less
+ // than in the rest of the program because all other callers have
+ // main as a (possibly indeirect) caller of their own.
+ void* const a[] = {
+ __builtin_frame_address (0), /* { dg-bogus "builtin_frame_address" } */
+ __builtin_frame_address (1), /* { dg-bogus "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), /* { dg-bogus "builtin_return_address" } */
+ __builtin_return_address (1), /* { dg-bogus "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-builtin-address.c b/gcc/testsuite/gcc.dg/Wno-builtin-address.c
new file mode 100644
index 0000000..c2724d1
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wno-builtin-address.c
@@ -0,0 +1,6 @@
+/* { dg-do compile } */
+/* { dg-options "-Werror" } */
+
+/* Verify that -Wbuiltin-address is not enabled by default by enabling
+ -Werror and verifying the test still compiles. */
+#include "Wbuiltin-address.c"
