From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89380 invoked by alias); 8 Jan 2019 12:03:14 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 89363 invoked by uid 89); 8 Jan 2019 12:03:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-10.6 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_NUMSUBJECT,SPF_PASS autolearn=ham version=3.3.2 spammy=straight, Meanwhile, H*Ad:D*eu X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 08 Jan 2019 12:03:10 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 22D701596; Tue, 8 Jan 2019 04:03:09 -0800 (PST) Received: from localhost (unknown [10.32.98.35]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B3D543F70D; Tue, 8 Jan 2019 04:03:07 -0800 (PST) From: Richard Sandiford To: Bernd Edlinger Mail-Followup-To: Bernd Edlinger ,Jakub Jelinek , Dimitar Dimitrov , Segher Boessenkool , Christophe Lyon , Thomas Preudhomme , "gcc-patches\@gcc.gnu.org" , richard.sandiford@arm.com Cc: Jakub Jelinek , Dimitar Dimitrov , Segher Boessenkool , Christophe Lyon , Thomas Preudhomme , "gcc-patches\@gcc.gnu.org" Subject: Re: [PATCH] [RFC] PR target/52813 and target/11807 References: <85840089.MtehzfUrTt@tpdeb> <20190107092337.GM30353@tucnak> Date: Tue, 08 Jan 2019 12:03:00 -0000 In-Reply-To: (Bernd Edlinger's message of "Mon, 7 Jan 2019 21:51:25 +0000") Message-ID: <87lg3vicg5.fsf@arm.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-SW-Source: 2019-01/txt/msg00386.txt.bz2 Bernd Edlinger writes: > On 1/7/19 10:23 AM, Jakub Jelinek wrote: >> On Sun, Dec 16, 2018 at 06:13:57PM +0200, Dimitar Dimitrov wrote: >>> - /* Clobbering the STACK POINTER register is an error. */ >>> + /* Clobbered STACK POINTER register is not saved/restored by GCC, >>> + which is often unexpected by users. See PR52813. */ >>> if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) >>> { >>> - error ("Stack Pointer register clobbered by %qs in %", regname); >>> + warning (0, "Stack Pointer register clobbered by %qs in %", >>> + regname); >>> + warning (0, "GCC has always ignored Stack Pointer % clobbers"); >> >> Why do we write Stack Pointer rather than stack pointer? That is really >> weird. The second warning would be a note based on the first one, i.e. >> if (warning ()) note (); >> and better have some -W* option to silence the warning. >> > > Yes, thanks for this suggestion. > > Meanwhile I found out, that the stack clobber has only been ignored up to > gcc-5 (at least with lra targets, not really sure about reload targets). > From gcc-6 on, with the exception of PR arm/77904 which was a regression due > to the underlying lra change, but fixed later, and back-ported to gcc-6.3.0, > this works for all targets I tried so far. > > To me, it starts to look like a rather unique and useful feature, that I would > like to keep working. Not sure what you mean by "unique". But forcing a frame is a bit of a slippery concept. Force it where? For the asm only, or the whole function? This depends on optimisation and hasn't been consistent across GCC versions, since it depends on the shrink-wrapping optimisation. (There was a similar controversy a while ago about to what extent -fno-omit-frame-pointer should "force a frame".) The effect on the redzone seems like something that should be specified explicitly rather than as an (accidental?) side effect of listing the sp in the clobber list. Maybe this would be another use for the "asm attributes" proposal. "noreturn" was another attribute suggested on IRC yesterday. But either way, the general feeling seems to be that going straight to a hard error is too harsh, since there's quite a bit of existing code that has the clobber. This patch implements the compromise discussed on IRC yesterday of making it a -Wdeprecated warning instead. Tested on x86_64-linux-gnu and aarch64-linux-gnu. OK to install? Richard Dimitar: sorry the run-around on this patch, and thanks for the submission. It looks from all the controversy like it was a long-festering PR for a reason. :-/ 2019-01-07 Richard Sandiford gcc/ PR inline-asm/52813 * doc/extend.texi: Document that listing the stack pointer in the clobber list of an asm is a deprecated feature. * common.opt (Wdeprecated): Moved from c-family/c.opt. * cfgexpand.c (asm_clobber_reg_is_valid): Issue a -Wdeprecated warning instead of an error for clobbers of the stack pointer. Add a note explaining why. gcc/c-family/ PR inline-asm/52813 * c.opt (Wdeprecated): Move documentation and variable to common.opt. gcc/d/ PR inline-asm/52813 * lang.opt (Wdeprecated): Reference common.opt instead of c.opt. gcc/testsuite/ PR inline-asm/52813 * gcc.target/i386/pr52813.c (test1): Turn the diagnostic into a -Wdeprecated warning and expect a following note:. Index: gcc/doc/extend.texi =================================================================== --- gcc/doc/extend.texi 2019-01-07 12:14:31.699490485 +0000 +++ gcc/doc/extend.texi 2019-01-08 11:40:20.807906878 +0000 @@ -9441,6 +9441,15 @@ When the compiler selects which register operands, it does not use any of the clobbered registers. As a result, clobbered registers are available for any use in the assembler code. +Another restriction is that the clobber list should not contain the +stack pointer register. This is because the compiler requires the +value of the stack pointer to be the same after an @code{asm} +statement as it was on entry to the statement. However, previous +versions of GCC did not enforce this rule and allowed the stack +pointer to appear in the list, with unclear semantics. This behavior +is deprecated and listing the stack pointer may become an error in +future versions of GCC@. + Here is a realistic example for the VAX showing the use of clobbered registers: Index: gcc/common.opt =================================================================== --- gcc/common.opt 2019-01-04 11:39:27.178246751 +0000 +++ gcc/common.opt 2019-01-08 11:40:20.803906912 +0000 @@ -579,6 +579,10 @@ Wattribute-warning Common Var(warn_attribute_warning) Init(1) Warning Warn about uses of __attribute__((warning)) declarations. +Wdeprecated +Common Var(warn_deprecated) Init(1) Warning +Warn if a deprecated compiler feature, class, method, or field is used. + Wdeprecated-declarations Common Var(warn_deprecated_decl) Init(1) Warning Warn about uses of __attribute__((deprecated)) declarations. Index: gcc/cfgexpand.c =================================================================== --- gcc/cfgexpand.c 2019-01-07 12:14:32.031487693 +0000 +++ gcc/cfgexpand.c 2019-01-08 11:40:20.803906912 +0000 @@ -2872,12 +2872,16 @@ asm_clobber_reg_is_valid (int regno, int error ("PIC register clobbered by %qs in %", regname); is_valid = false; } - /* Clobbering the STACK POINTER register is an error. */ - if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM)) - { - error ("Stack Pointer register clobbered by %qs in %", regname); - is_valid = false; - } + /* Clobbering the stack pointer register is deprecated. GCC expects + the value of the stack pointer after an asm statement to be the same + as it was before, so no asm can validly clobber the stack pointer in + the usual sense. Adding the stack pointer to the clobber list has + traditionally had some undocumented and somewhat obscure side-effects. */ + if (overlaps_hard_reg_set_p (regset, Pmode, STACK_POINTER_REGNUM) + && warning (OPT_Wdeprecated, "listing the stack pointer register" + " %qs in a clobber list is deprecated", regname)) + inform (input_location, "the value of the stack pointer after an %" + " statement must be the same as it was before the statement"); return is_valid; } Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt 2019-01-04 11:39:24.538269283 +0000 +++ gcc/c-family/c.opt 2019-01-08 11:40:20.799906946 +0000 @@ -477,8 +477,8 @@ C++ ObjC++ Var(warn_delnonvdtor) Warning Warn about deleting polymorphic objects with non-virtual destructors. Wdeprecated -C C++ ObjC ObjC++ CPP(cpp_warn_deprecated) CppReason(CPP_W_DEPRECATED) Var(warn_deprecated) Init(1) Warning -Warn if a deprecated compiler feature, class, method, or field is used. +C C++ ObjC ObjC++ CPP(cpp_warn_deprecated) CppReason(CPP_W_DEPRECATED) +; Documented in common.opt Wdeprecated-copy C++ ObjC++ Var(warn_deprecated_copy) Warning LangEnabledBy(C++ ObjC++, Wextra) Index: gcc/d/lang.opt =================================================================== --- gcc/d/lang.opt 2019-01-04 11:39:24.702267882 +0000 +++ gcc/d/lang.opt 2019-01-08 11:40:20.803906912 +0000 @@ -124,7 +124,7 @@ Warn about casts that will produce a nul Wdeprecated D -; Documented in C +; Documented in common.opt Werror D Index: gcc/testsuite/gcc.target/i386/pr52813.c =================================================================== --- gcc/testsuite/gcc.target/i386/pr52813.c 2018-12-11 15:50:44.668823294 +0000 +++ gcc/testsuite/gcc.target/i386/pr52813.c 2019-01-08 11:40:20.807906878 +0000 @@ -5,5 +5,6 @@ void test1 (void) { - asm volatile ("" : : : "%esp"); /* { dg-error "Stack Pointer register clobbered" } */ + asm volatile ("" : : : "%esp"); /* { dg-warning "listing the stack pointer register '%esp' in a clobber list is deprecated" } */ + /* { dg-message "note: the value of the stack pointer after an 'asm' statement must be the same as it was before the statement" "" { target *-*-* } .-1 } */ }