[PATCH] clarify doc for __builtin_return_address

[PATCH] clarify doc for __builtin_return_address

[Martin Sebor]

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?

Thanks
Martin

2015-05-21  Martin Sebor  <msebor@redhat.com>

	* extend.texi (Return Address): Clarify possible effects
	of calling the functions with non-zero arguments.

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 7470e40..b37e893 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -7959,7 +7959,8 @@ 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.
+purposes since such calls to it can have unpredictable effects, including
+crashing the calling program.
  @end deftypefn

  @deftypefn {Built-in Function} {void *} __builtin_extract_return_addr 
(void *@var{addr})
@@ -7998,7 +7999,8 @@ 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.
+purposes since such calls to it can have unpredictable effects, including
+crashing the calling program.
  @end deftypefn

  @node Vector Extensions

Re: [PATCH] clarify doc for __builtin_return_address

[Sandra Loosemore]

On 05/21/2015 01:19 PM, Martin Sebor wrote:

> 2015-05-21  Martin Sebor  <msebor@redhat.com>
>
>      * extend.texi (Return Address): Clarify possible effects
>      of calling the functions with non-zero arguments.
>
> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> index 7470e40..b37e893 100644
> --- a/gcc/doc/extend.texi
> +++ b/gcc/doc/extend.texi
> @@ -7959,7 +7959,8 @@ 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.
> +purposes since such calls to it can have unpredictable effects, including
> +crashing the calling program.
>   @end deftypefn
>
>   @deftypefn {Built-in Function} {void *} __builtin_extract_return_addr
> (void *@var{addr})

I think the problem is that the original sentence parses ambiguously -- 
is it telling you that you must pass a nonzero argument to use it for 
debugging purposes, or telling you that you must use calls with a 
nonzero argument only for debugging?  And adding an additional clause 
onto the end only makes it harder to parse.

I suggest rewriting it as something like

Calling this function with a nonzero argument can have unpredictable 
effects, including crashing the calling program.  Such calls are 
typically only useful in debugging situations.

> @@ -7998,7 +7999,8 @@ 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.
> +purposes since such calls to it can have unpredictable effects, including
> +crashing the calling program.
>   @end deftypefn
>
>   @node Vector Extensions
>

Same applies here as well.

-Sandra

Re: [PATCH] clarify doc for __builtin_return_address

[Martin Sebor]

On 05/21/2015 02:05 PM, Sandra Loosemore wrote:
> On 05/21/2015 01:19 PM, Martin Sebor wrote:
>
>> 2015-05-21  Martin Sebor  <msebor@redhat.com>
>>
>>      * extend.texi (Return Address): Clarify possible effects
>>      of calling the functions with non-zero arguments.
>>
>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
>> index 7470e40..b37e893 100644
>> --- a/gcc/doc/extend.texi
>> +++ b/gcc/doc/extend.texi
>> @@ -7959,7 +7959,8 @@ 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.
>> +purposes since such calls to it can have unpredictable effects,
>> including
>> +crashing the calling program.
>>   @end deftypefn
>>
>>   @deftypefn {Built-in Function} {void *} __builtin_extract_return_addr
>> (void *@var{addr})
>
> I think the problem is that the original sentence parses ambiguously --
> is it telling you that you must pass a nonzero argument to use it for
> debugging purposes, or telling you that you must use calls with a
> nonzero argument only for debugging?  And adding an additional clause
> onto the end only makes it harder to parse.
>
> I suggest rewriting it as something like
>
> Calling this function with a nonzero argument can have unpredictable
> effects, including crashing the calling program.  Such calls are
> typically only useful in debugging situations.

Thanks. I agree that's better. I'll wait for further comments
or approval to commit your version.

Martin

Re: [PATCH] clarify doc for __builtin_return_address

[Pedro Alves]

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

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 3743 bytes --]

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  <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):
	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  <msebor@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
>


[-- Attachment #2: gcc-Wbuiltin-address.patch --]
[-- Type: text/x-patch, Size: 13529 bytes --]

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"

[PING] Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

Are there any concerns with or suggestions for changes to
the following patch?

   https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00886.html

Thanks
Martin

Re: [PING 2] Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

Is this patch okay for trunk?

On 06/18/2015 11:15 AM, Martin Sebor wrote:
> Are there any concerns with or suggestions for changes to
> the following patch?
>
>    https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00886.html
>
> Thanks
> Martin

[PING 3] Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

This is a small change to diagnose unsafe calls to
__builtin_{frame,return}_address (with an argument > 2) than
tend to return bogus values or lead to crashes at runtime.

A review would be appreciated.

Thanks
Martin

On 06/26/2015 05:49 PM, Martin Sebor wrote:
> Is this patch okay for trunk?
>
> On 06/18/2015 11:15 AM, Martin Sebor wrote:
>> Are there any concerns with or suggestions for changes to
>> the following patch?
>>
>>    https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00886.html
>>
>> Thanks
>> Martin
>

Re: [PING 3] Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Pedro Alves]

On 07/07/2015 04:41 AM, Martin Sebor wrote:
> This is a small change to diagnose unsafe calls to
> __builtin_{frame,return}_address (with an argument > 2) than
> tend to return bogus values or lead to crashes at runtime.
> 

I hadn't realized you went through and implemented the
suggestion.  Thanks for doing this.  I hope it gets approved.

> A review would be appreciated.

(It may be the relevant maintainers haven't noticed there's a
code patch here because this is nested within
the "[PATCH] clarify doc for __builtin_return_address" thread).)

Thanks,
Pedro Alves

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Jeff Law]

On 06/11/2015 04:05 PM, Martin Sebor wrote:
> 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  <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):
>      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  <msebor@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.
You have to start thinking creatively.  For example, consider ports 
without dwarf2 unwinders :-)  Consider ports where the linker inserts 
stubs between caller & callee for various reasons  Consider cases where 
the next outermost frame is compiled by something other than GCC, or 
perhaps was written in pure assembly.  Or consider the case when we're 
currently in a signal handling frame...






>
> 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.
I don't think this is really an issue with calling with a non-zero 
argument near the top of the stack, but more an issue of the general 
difficulty in unwinding beyond the current frame.

It's relatively easy for the compiler to get the return address of the 
current frame -- after all, the compiler knows everything about the 
current function and can even arrange to avoid things which may make 
finding the current frame's address difficult.

However, once you want to go back one frame, you're in an arbitrary hunk 
of code and digging the prior frame out can be non-trivial.

You can get a sense of the complexities by looking at the GDB code to 
find the saved pc and base of an arbitrary frame.  It's insane, 
especially in a world without dwarf2 unwind records.

So, my suggestion would be to warn for any call with a nonzero value.

Jeff

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Segher Boessenkool]

On Thu, Jul 23, 2015 at 11:08:21PM -0600, Jeff Law wrote:
> >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.

A quick grep shows aarch64.  Also many other ports, for
__builtin_return_address.  For __builtin_frame_address there is no
non-hacky way for ports to say "I cannot access other frames"; see
mmix_dynamic_chain_address for more complaints about this.

> You have to start thinking creatively.  For example, consider ports 
> without dwarf2 unwinders :-)  Consider ports where the linker inserts 
> stubs between caller & callee for various reasons  Consider cases where 
> the next outermost frame is compiled by something other than GCC, or 
> perhaps was written in pure assembly.  Or consider the case when we're 
> currently in a signal handling frame...

Or simply the case where the caller does not have a frame pointer, and
the ABI does not provide another way to get at (the size of) stack frames.

> 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 :-)


Segher

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 400 bytes --]

>> 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


[-- Attachment #2: gcc-Wbuiltin-address.patch --]
[-- Type: text/x-patch, Size: 13282 bytes --]

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):
    Clarify possible effects of calling the functions with non-zero
    arguments and mention -Wbuiltin-address.
    * builtins.c (expand_builtin_frame_address): Handle -Wbuiltin-address.

gcc/testsuite/ChangeLog
2015-07-27  Martin Sebor  <msebor@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.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index e8fe3db..48379f8 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.  */
+      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 (0 < count)
+	{
+	  /* Warn since no effort is made to ensure that any frame
+	     beyond the current one exists or can be safely reached.  */
+	  warning (OPT_Wbuiltin_address, "calling %qD with "
+		   "a non-zero 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..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..4e64108 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,13 @@ 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 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/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..5e6ff74
--- /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),
+  __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__ ((weak))
+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__ ((weak))
+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-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..5483f5d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/Wbuiltin-address.c
@@ -0,0 +1,54 @@
+/* { dg-do compile } */
+/* { dg-options "-Wbuiltin-address" } */
+
+void* __attribute__ ((weak))
+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__ ((weak))
+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-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"

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Segher Boessenkool]

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

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

[-- Attachment #1: Type: text/plain, Size: 1915 bytes --]

>> 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

[-- Attachment #2: gcc-Wframe-address.patch --]
[-- Type: text/x-patch, Size: 13223 bytes --]

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"

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 <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-Wframe-address.patch
>
>
> 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.
OK for the trunk.  Sorry for the delay.

jeff

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

> OK for the trunk.  Sorry for the delay.

Thank you. Committed in revision 226480.

Martin

[BUILDROBOT] Go runtime: calling ‘__builtin_frame_address’ with a nonzero argument is unsafe (was: warn for unsafe calls to __builtin_return_address)

[Jan-Benedict Glaw]

[-- Attachment #1: Type: text/plain, Size: 3620 bytes --]

On Sun, 2015-08-02 17:15:27 -0600, Martin Sebor <msebor@gmail.com> wrote:
> >OK for the trunk.  Sorry for the delay.
> 
> Thank you. Committed in revision 226480.

...und breaks native builds. When doing builds using config-list.mk, I
first build a GCC for the build machine, then re-build a
cross-configured GCC with that.

  While building GCC targeting the build=host machine, I get ie:

http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=459572

/bin/bash ./libtool --tag=CC   --mode=compile /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/xgcc -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/bin/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/lib/ -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/include -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/sys-include    -DHAVE_CONFIG_H -I. -I/home/jbglaw/repos-configlist_mk/gcc/libgo  -I /home/jbglaw/repos-configlist_mk/gcc/libgo/runtime -I/home/jbglaw/repos-configlist_mk/gcc/libgo/../libffi/include -I../libffi/include -pthread  -fexceptions -fnon-call-exceptions -fplan9-extensions -fsplit-stack -Wall -Wextra -Wwrite-strings -Wcast-qual -Werror -minline-all-stringops -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libgcc -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libbacktrace -I ../../gcc/include -g -O2 -MT mprof.lo -MD -MP -MF .deps/mprof.Tpo -c -o mprof.lo mprof.c
libtool: compile:  /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/xgcc -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/bin/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/lib/ -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/include -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/sys-include -DHAVE_CONFIG_H -I. -I/home/jbglaw/repos-configlist_mk/gcc/libgo -I /home/jbglaw/repos-configlist_mk/gcc/libgo/runtime -I/home/jbglaw/repos-configlist_mk/gcc/libgo/../libffi/include -I../libffi/include -pthread -fexceptions -fnon-call-exceptions -fplan9-extensions -fsplit-stack -Wall -Wextra -Wwrite-strings -Wcast-qual -Werror -minline-all-stringops -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libgcc -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libbacktrace -I ../../gcc/include -g -O2 -MT mprof.lo -MD -MP -MF .deps/mprof.Tpo -c mprof.c  -fPIC -DPIC -o .libs/mprof.o
/home/jbglaw/repos-configlist_mk/gcc/libgo/runtime/mprof.goc: In function ‘runtime_Stack’:
/home/jbglaw/repos-configlist_mk/gcc/libgo/runtime/mprof.goc:408:5: error: calling ‘__builtin_frame_address’ with a nonzero argument is unsafe [-Werror=frame-address]
  sp = runtime_getcallersp(&b);
     ^
cc1: all warnings being treated as errors
Makefile:2613: recipe for target 'mprof.lo' failed
make[4]: *** [mprof.lo] Error 1



Seems Go's runtime uses that feature.

MfG, JBG

-- 
      Jan-Benedict Glaw      jbglaw@lug-owl.de              +49-172-7608481
 Signature of:                      http://perl.plover.com/Questions.html
 the second  :

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [BUILDROBOT] Go runtime: calling ‘__builtin_frame_address’ with a nonzero argument is unsafe

[Martin Sebor]

On 08/03/2015 05:55 AM, Jan-Benedict Glaw wrote:
> On Sun, 2015-08-02 17:15:27 -0600, Martin Sebor <msebor@gmail.com> wrote:
>>> OK for the trunk.  Sorry for the delay.
>>
>> Thank you. Committed in revision 226480.
>
> ...und breaks native builds. When doing builds using config-list.mk, I
> first build a GCC for the build machine, then re-build a
> cross-configured GCC with that.

I don't know if pragma GCC diagnostic is valid in Go (still waiting
for my build to finish to confirm) but disabling the warning in
cases where the calls are known to be safe should fix the compilation
error.

Index: runtime/mprof.goc
===================================================================
--- runtime/mprof.goc    (revision 226505)
+++ runtime/mprof.goc    (working copy)
@@ -404,10 +404,15 @@
  func Stack(b Slice, all bool) (n int) {
      byte *pc, *sp;
      bool enablegc;
-
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wframe-address"
+
      sp = runtime_getcallersp(&b);
      pc = (byte*)(uintptr)runtime_getcallerpc(&b);

+#pragma GCC diagnostic pop
+
      if(all) {
          runtime_semacquire(&runtime_worldsema, false);
          runtime_m()->gcing = 1;

Martin

>
>    While building GCC targeting the build=host machine, I get ie:
>
> http://toolchain.lug-owl.de/buildbot/show_build_details.php?id=459572
>
> /bin/bash ./libtool --tag=CC   --mode=compile /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/xgcc -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/bin/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/lib/ -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/include -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/sys-include    -DHAVE_CONFIG_H -I. -I/home/jbglaw/repos-configlist_mk/gcc/libgo  -I /home/jbglaw/repos-configlist_mk/gcc/libgo/runtime -I/home/jbglaw/repos-configlist_mk/gcc/libgo/../libffi/include -I../libffi/include -pthread  -fexceptions -fnon-call-exceptions -fplan9-extensions -fsplit-stack -Wall -Wextra -Wwrite-strings -Wcast-qual -Werror -minline-all-stringops -D_GNU_SOURCE -D_LAR
GEFILE_SOURCE -D_FILE_OFFSET_BITS=64 -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libgcc -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libbacktrace -I ../../gcc/include -g -O2 -MT mprof.lo -MD -MP -MF .deps/mprof.Tpo -c -o mprof.lo mprof.c
> libtool: compile:  /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/xgcc -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/build-gcc/native-compiler-build/./gcc/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/bin/ -B/home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/lib/ -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/include -isystem /home/jbglaw/build-configlist_mk/rs6000-ibm-aix5.1.0/_install_/x86_64-pc-linux-gnu/sys-include -DHAVE_CONFIG_H -I. -I/home/jbglaw/repos-configlist_mk/gcc/libgo -I /home/jbglaw/repos-configlist_mk/gcc/libgo/runtime -I/home/jbglaw/repos-configlist_mk/gcc/libgo/../libffi/include -I../libffi/include -pthread -fexceptions -fnon-call-exceptions -fplan9-extensions -fsplit-stack -Wall -Wextra -Wwrite-strings -Wcast-qual -Werror -minline-all-stringops -D_GNU_SOURCE -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BIT
S=64 -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libgcc -I /home/jbglaw/repos-configlist_mk/gcc/libgo/../libbacktrace -I ../../gcc/include -g -O2 -MT mprof.lo -MD -MP -MF .deps/mprof.Tpo -c mprof.c  -fPIC -DPIC -o .libs/mprof.o
> /home/jbglaw/repos-configlist_mk/gcc/libgo/runtime/mprof.goc: In function ‘runtime_Stack’:
> /home/jbglaw/repos-configlist_mk/gcc/libgo/runtime/mprof.goc:408:5: error: calling ‘__builtin_frame_address’ with a nonzero argument is unsafe [-Werror=frame-address]
>    sp = runtime_getcallersp(&b);
>       ^
> cc1: all warnings being treated as errors
> Makefile:2613: recipe for target 'mprof.lo' failed
> make[4]: *** [mprof.lo] Error 1
>
>
>
> Seems Go's runtime uses that feature.
>
> MfG, JBG
>

Re: [BUILDROBOT] Go runtime: calling ‘__builtin_frame_address’ with a nonzero argument is unsafe

[Jeff Law]

On 08/03/2015 08:48 AM, Martin Sebor wrote:
> On 08/03/2015 05:55 AM, Jan-Benedict Glaw wrote:
>> On Sun, 2015-08-02 17:15:27 -0600, Martin Sebor <msebor@gmail.com> wrote:
>>>> OK for the trunk.  Sorry for the delay.
>>>
>>> Thank you. Committed in revision 226480.
>>
>> ...und breaks native builds. When doing builds using config-list.mk, I
>> first build a GCC for the build machine, then re-build a
>> cross-configured GCC with that.
>
> I don't know if pragma GCC diagnostic is valid in Go (still waiting
> for my build to finish to confirm) but disabling the warning in
> cases where the calls are known to be safe should fix the compilation
> error.
My understanding is the runtime is shared across gcc-go and golang.  So 
the push/pop diagnostics may not be appropriate.

Ian Taylor should have the final say about the best way forward.

Jeff

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Andreas Schwab]

Martin Sebor <msebor@gmail.com> writes:

> 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.

FAIL: g++.dg/Wno-frame-address.C  -std=gnu++11 (test for excess errors)
Excess errors:
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:42:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:43:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:44:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:45:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:65:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:66:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:67:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:68:30: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:15:28: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:16:28: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:17:28: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/g++.dg/Wframe-address.C:18:28: error: unsupported argument to 'void* __builtin_return_address(unsigned int)' [-Werror]

FAIL: gcc.dg/Wno-frame-address.c (test for excess errors)
Excess errors:
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:24:5: error: unsupported argument to '__builtin_return_address' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:25:5: error: unsupported argument to '__builtin_return_address' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:26:5: error: unsupported argument to '__builtin_return_address' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:27:5: error: unsupported argument to '__builtin_return_address' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:47:5: error: unsupported argument to '__builtin_return_address' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:48:5: error: unsupported argument to '__builtin_return_address' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:49:5: error: unsupported argument to '__builtin_return_address' [-Werror]
/usr/local/gcc/gcc-20150805/gcc/testsuite/gcc.dg/Wframe-address.c:50:5: error: unsupported argument to '__builtin_return_address' [-Werror]

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Jiong Wang]

On 28/07/15 16:44, Martin Sebor wrote:
>
> Attached is an updated patch with the changes above.
>

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.

noticed the new added "Wno-frame-address.c" fail on arm-none-linux-gnueabihf native test.

from the comments in the testcase:

+/* Verify that -Wframe-address is not enabled by default by enabling
+   -Werror and verifying the test still compiles.  */

seems you want to make sure -Wframe-address work correctly with -Werror, while for arm,
return_address hook is defined to only support level 0, NULL_RTX returned for all other levels,
so this caused Wno-frame-address.c failed in those tem != NULL check for builtin_return_address.

Regards,
Jiong

Re: [PATCH] warn for unsafe calls to __builtin_return_address

[Martin Sebor]

On 08/05/2015 10:02 AM, Jiong Wang wrote:
>
> On 28/07/15 16:44, Martin Sebor wrote:
>>
>> Attached is an updated patch with the changes above.
>>
>
> 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.
>
> noticed the new added "Wno-frame-address.c" fail on
> arm-none-linux-gnueabihf native test.
>
> from the comments in the testcase:
>
> +/* Verify that -Wframe-address is not enabled by default by enabling
> +   -Werror and verifying the test still compiles.  */
>
> seems you want to make sure -Wframe-address work correctly with -Werror,
> while for arm,
> return_address hook is defined to only support level 0, NULL_RTX
> returned for all other levels,
> so this caused Wno-frame-address.c failed in those tem != NULL check for
> builtin_return_address.

Thanks. I'm traveling the next 10 days and most likely won't be
able to get to this until I get back on the 17th.

Martin