From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 70680 invoked by alias); 12 Dec 2018 13:19:45 -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 70662 invoked by uid 89); 12 Dec 2018 13:19:43 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=2.6 required=5.0 tests=BAYES_00,BODY_8BITS,GARBLED_BODY,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=no version=3.3.2 spammy=8:=d0=bf, mandatory, 11807, 52813?= X-HELO: mail-vs1-f50.google.com Received: from mail-vs1-f50.google.com (HELO mail-vs1-f50.google.com) (209.85.217.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Dec 2018 13:19:41 +0000 Received: by mail-vs1-f50.google.com with SMTP id g68so10990033vsd.11 for ; Wed, 12 Dec 2018 05:19:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=JbM5u8EGK6Uh3wXpANFkb/IdynPfJDKnt5GsObANYos=; b=CKwlzwlNKfSnmyGtxuEba0yxT7IPT5EcaoooWe+SqvZXTtyTvUIBlOwsOo6rBNQnvJ +TBzRWvVbcG2mQAnFUPodo+guw9auDp0Tyiqqf4JLjtqMQASZeAQ/fMIaESs0q/3EmLP gYbxTfWR50AnL1N9fWqgVOf07K8M8dHJrzbXQ= MIME-Version: 1.0 References: <20181209100856.14051-1-dimitar@dinux.eu> <87woohsk32.fsf@arm.com> <2807771.CbC4dySGB1@tpdeb> <87woog9i32.fsf@arm.com> In-Reply-To: From: Christophe Lyon Date: Wed, 12 Dec 2018 13:19:00 -0000 Message-ID: Subject: Re: [PATCH] [RFC] PR target/52813 and target/11807 To: Thomas Preudhomme Cc: dimitar@dinux.eu, gcc Patches , Richard Sandiford , "Thomas Preud'homme" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00815.txt.bz2 On Wed, 12 Dec 2018 at 12:21, Thomas Preudhomme wrote: > > So my understanding is that the original code (CMSIS library) used to > clobber sp because the asm statement was actually changing the sp. > That in turn led GCC to try to save and restore sp which is not what > CMSIS was expecting to happen. Changing sp without clobber as done now > is probably the right solution and r242693 can be reverted. That will > remove the failing test. > OK, I read PR52813 too, but I'm not sure to fully understand the new status. My understanding is that since this patch was committed, if an asm statement clobbers sp, it is now allowed to actually declare it as clobber (this patch generates an error in such a case). So the user is now expected to lie to the compiler when writing to this kind of register (sp, pic register), by not declaring it as "clobber"? > Best regards, > > Thomas > On Wed, 12 Dec 2018 at 10:30, Thomas Preudhomme > wrote: > > > > Hi Christophe, > > > > That PR was about a bug occuring when sp was clobbered so if it cannot > > be clobbered anymore the whole commit (r242693) can be removed. Let me > > check the original code that lead to the PR why it's clobbering sp > > though. > > > > Best regards, > > > > Thomas > > On Wed, 12 Dec 2018 at 09:43, Christophe Lyon > > wrote: > > > > > > On Tue, 11 Dec 2018 at 16:52, Richard Sandiford > > > wrote: > > > > > > > > Dimitar Dimitrov writes: > > > > > On =D0=BF=D0=BE=D0=BD=D0=B5=D0=B4=D0=B5=D0=BB=D0=BD=D0=B8=D0=BA, = 10 =D0=B4=D0=B5=D0=BA=D0=B5=D0=BC=D0=B2=D1=80=D0=B8 2018 =D0=B3. 11:21:53 E= ET Richard Sandiford wrote: > > > > >> Dimitar Dimitrov writes: > > > > >> > I have tested this fix on x86_64 host, and found no regression= in the C > > > > >> > and C++ testsuites. I'm marking this patch as RFC simply beca= use I don't > > > > >> > have experience with other architectures, and I don't have a s= etup to > > > > >> > test all architectures supported by GCC. > > > > >> > > > > > >> > gcc/ChangeLog: > > > > >> > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > >> > > > > > >> > * cfgexpand.c (asm_clobber_reg_is_valid): Also produce > > > > >> > error when stack pointer is clobbered. > > > > >> > (expand_asm_stmt): Refactor clobber check in separate funct= ion. > > > > >> > > > > > >> > gcc/testsuite/ChangeLog: > > > > >> > > > > > >> > 2018-12-07 Dimitar Dimitrov > > > > >> > > > > > >> > * gcc.target/i386/pr52813.c: New test. > > > > >> > > > > > >> > Signed-off-by: Dimitar Dimitrov > > > > >> > > > > >> LGTM. Do you have a copyright assignment on file? 'Fraid this = is > > > > >> probably big enough to need one. > > > > > Yes, I have copyright assignment. > > > > > > > > OK, great. I went ahead and applied the patch. > > > > > > > > > > Hi, > > > > > > This patch introduces a regression on arm: > > > FAIL: gcc.target/arm/pr77904.c (test for excess errors) > > > Excess errors: > > > /gcc/testsuite/gcc.target/arm/pr77904.c:7:3: error: Stack Pointer > > > register clobbered by 'sp' in 'asm' > > > > > > Indeed the testcase has an explicit: > > > __asm volatile ("" : : : "sp"); > > > which is now rejected. > > > > > > Thomas, is that mandatory to test your code to fix pr77904? > > > > > > Thanks, > > > > > > Christophe > > > > > > > Thanks, > > > > Richard