From patchwork Thu Jun 11 22:05:11 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Martin Sebor X-Patchwork-Id: 483315 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 6B93D140218 for ; Fri, 12 Jun 2015 08:05:25 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=P5atQzoy; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type; q=dns; s=default; b=kB2nS80UAswcXhfsU JPKfxuMvAmmdI6TQnQS13xU2OGvW/1TggEnsVbiQNeyA3JYsh/HAqAk626NUUWan VlWrZvPFdKwHcR5QfAqt3fIdk5ySzWtZWoFMCL9X10FWqvqfaYDzzmb7LghWoqCo a9W3RVnECji5PvcOXqKRGAR1LQ= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type; s=default; bh=coBeWTJj9gk2Qxnd8VtPds+ 8A4U=; b=P5atQzoyUQ6SkVpBEGW6uuajgUEmtzxG7oA4sMNoLlyo8ndMnBSD6v6 umAv9J1quA25Z2p8ziQNrhUhUuU9vyPRhXAoDIgE4fz99SYfFiQONYPAvaWr3Rmd qFjS8In+AfqWEiHRZn0GcvyOkidSfw0CCWolM5uhFzgtAWdsE3oc= Received: (qmail 114580 invoked by alias); 11 Jun 2015 22:05:17 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 114569 invoked by uid 89); 11 Jun 2015 22:05:16 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.2 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, SPF_HELO_PASS, T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 11 Jun 2015 22:05:14 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (Postfix) with ESMTPS id 6E63835C51A for ; Thu, 11 Jun 2015 22:05:13 +0000 (UTC) Received: from [10.10.61.119] (vpn-61-119.rdu2.redhat.com [10.10.61.119]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t5BM5B7J017819 for ; Thu, 11 Jun 2015 18:05:12 -0400 Message-ID: <557A0617.6040605@redhat.com> Date: Thu, 11 Jun 2015 16:05:11 -0600 From: Martin Sebor User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Gcc Patch List Subject: Re: [PATCH] warn for unsafe calls to __builtin_return_address References: <555E2FC6.60804@redhat.com> <555E50A5.60309@redhat.com> In-Reply-To: <555E50A5.60309@redhat.com> X-IsSubscribed: yes 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 * 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 * 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"