From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 119220 invoked by alias); 12 Dec 2018 15:13:47 -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 118889 invoked by uid 89); 12 Dec 2018 15:13:46 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-21.0 required=5.0 tests=BAYES_05,BODY_8BITS,GARBLED_BODY,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=8:=d0=bf, mandatory, 52813, 11807?= X-HELO: mail-vs1-f42.google.com Received: from mail-vs1-f42.google.com (HELO mail-vs1-f42.google.com) (209.85.217.42) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Dec 2018 15:13:44 +0000 Received: by mail-vs1-f42.google.com with SMTP id x1so11210526vsc.10 for ; Wed, 12 Dec 2018 07:13:44 -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; bh=l0mj1b2vf/S8ZZisNvACI/vqkYgwNFJr+mJhHS3IiL0=; b=Nm4ABxwOkYiw7ju7rMy+4pcR6BWpfAolk05Y9aJivG7KpcFvLHR6us1z+WhxMwtXGo gmehu1blwir49sT2jeA29u3c50m/2br8XP0ORwVA8TYl+fe0gKkD+s1ovF0p/L7Zv9lF LQ7ylFf8BTjJTAnb69mE7tuaIuPF3f0XaMxhw= 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 15:13: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: multipart/mixed; boundary="000000000000b842fe057cd4a4af" X-IsSubscribed: yes X-SW-Source: 2018-12/txt/msg00823.txt.bz2 --000000000000b842fe057cd4a4af Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-length: 3974 On Wed, 12 Dec 2018 at 14:19, Christophe Lyon wrote: > > 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 stat= us. > My understanding is that since this patch was committed, if an asm statem= ent > clobbers sp, it is now allowed to actually declare it as clobber (this pa= tch > 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= "? > I'm attaching a small patch which adds a more verbose error message along the lines of what I understand of the current status. I'm pretty sure I got (at least) the formatting wrong :) Christophe > > > 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= EET Richard Sandiford wrote: > > > > > >> Dimitar Dimitrov writes: > > > > > >> > I have tested this fix on x86_64 host, and found no regressi= on in the C > > > > > >> > and C++ testsuites. I'm marking this patch as RFC simply be= cause I don't > > > > > >> > have experience with other architectures, and I don't have a= setup 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 fun= ction. > > > > > >> > > > > > > >> > 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 thi= s 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 --000000000000b842fe057cd4a4af Content-Type: text/plain; charset="US-ASCII"; name="pr52813.chlog.txt" Content-Disposition: attachment; filename="pr52813.chlog.txt" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_jplbcije0 Content-length: 204 MjAxOC0xMi0xMiAgQ2hyaXN0b3BoZSBMeW9uICA8Y2hyaXN0b3BoZS5seW9u QGxpbmFyby5vcmc+CgoJZ2NjLwoJKiBjZmdleHBhbmQuYyAoYXNtX2Nsb2Ji ZXJfcmVnX2lzX3ZhbGlkKTogQWRkIGEgbW9yZSBkZXNjcmlwdGl2ZQoJZXJy b3IgbWVzc2FnZS4KCg== --000000000000b842fe057cd4a4af Content-Type: text/plain; charset="US-ASCII"; name="pr52813.patch.txt" Content-Disposition: attachment; filename="pr52813.patch.txt" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_jplbcik21 Content-length: 1456 ZGlmZiAtLWdpdCBhL2djYy9jZmdleHBhbmQuYyBiL2djYy9jZmdleHBhbmQu YwppbmRleCAwZDA0YmJjLi5lMWYyYmZmIDEwMDY0NAotLS0gYS9nY2MvY2Zn ZXhwYW5kLmMKKysrIGIvZ2NjL2NmZ2V4cGFuZC5jCkBAIC0yODcwLDEyICsy ODcwLDIwIEBAIGFzbV9jbG9iYmVyX3JlZ19pc192YWxpZCAoaW50IHJlZ25v LCBpbnQgbnJlZ3MsIGNvbnN0IGNoYXIgKnJlZ25hbWUpCiAgICAgewogICAg ICAgLyogPz8/IERpYWdub3NlIGR1cmluZyBnaW1wbGlmaWNhdGlvbj8gICov CiAgICAgICBlcnJvciAoIlBJQyByZWdpc3RlciBjbG9iYmVyZWQgYnkgJXFz IGluICU8YXNtJT4iLCByZWduYW1lKTsKKyAgICAgIGVycm9yICgiU3VjaCBj bG9iYmVycyBhcmUgbm90IHN1cHBvcnRlZCBieSBHQ0MuICIKKwkgICAgICJJ ZiB5b3UgcmVhbGx5IHdhbnQgdG8gb3ZlcndyaXRlIHRoZSBQSUMgcmVnaXN0 ZXIsICIKKwkgICAgICJyZW1vdmUgaXQgZnJvbSB0aGUgY2xvYmJlciBsaXN0 IGluIHRoZSAlPGFzbSU+IGF0IHlvdXIgb3duIHJpc2s6ICIKKwkgICAgICJH Q0Mgd2lsbCBub3Qgc2F2ZSBpdC4iKTsKICAgICAgIGlzX3ZhbGlkID0gZmFs c2U7CiAgICAgfQogICAvKiBDbG9iYmVyaW5nIHRoZSBTVEFDSyBQT0lOVEVS IHJlZ2lzdGVyIGlzIGFuIGVycm9yLiAgKi8KICAgaWYgKG92ZXJsYXBzX2hh cmRfcmVnX3NldF9wIChyZWdzZXQsIFBtb2RlLCBTVEFDS19QT0lOVEVSX1JF R05VTSkpCiAgICAgewogICAgICAgZXJyb3IgKCJTdGFjayBQb2ludGVyIHJl Z2lzdGVyIGNsb2JiZXJlZCBieSAlcXMgaW4gJTxhc20lPiIsIHJlZ25hbWUp OworICAgICAgZXJyb3IgKCJTdWNoIGNsb2JiZXJzIGFyZSBub3Qgc3VwcG9y dGVkIGJ5IEdDQy4gIgorCSAgICAgIklmIHlvdSByZWFsbHkgd2FudCB0byBv dmVyd3JpdGUgdGhlIFN0YWNrIFBvaW50ZXIsICIKKwkgICAgICJyZW1vdmUg aXQgZnJvbSB0aGUgY2xvYmJlciBsaXN0IGluIHRoZSAlPGFzbSU+IGF0IHlv dXIgb3duIHJpc2s6ICIKKwkgICAgICJHQ0Mgd2lsbCBub3Qgc2F2ZSBpdC4i KTsKICAgICAgIGlzX3ZhbGlkID0gZmFsc2U7CiAgICAgfQogCg== --000000000000b842fe057cd4a4af--