From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x242.google.com (mail-oi1-x242.google.com [IPv6:2607:f8b0:4864:20::242]) by sourceware.org (Postfix) with ESMTPS id 585FD3857031 for ; Wed, 1 Jul 2020 12:32:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 585FD3857031 Received: by mail-oi1-x242.google.com with SMTP id t198so7886791oie.7 for ; Wed, 01 Jul 2020 05:32:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kNwCGDc6gFt/US3AlYIeMIptldfxTLmanz6RaJgLr+0=; b=GQdlvfiMS48bq9wBUij1HOPEVWLAPpULlNyq1yYbx60ZcUDMTZeBMJtVrCmnb/TOu5 s5w4sLATfRWQpoXcstDZPyFdJyp6wUAyWCekqcnVtsytHl0kPHkRmQnbCSmDHSmK9UUN oPnlEErBxEPjP1s7TpIcJpRW4b8C4Wyi6WkIMwDU+As47brw4jbddEWJcuGnji2wa5PM wB/DOqiugHd1PF+DvqxjITbP2Oerl9lqdExoN3KzKKLkcS8+ROeOLlEiBvybTrgoFuX+ sfyVaQB1pw2dGe/lsz7RqEP/rMN+YElQJ+aqx5KaJpx+PKsTaJYOpypir4HZE5RsdE7C quOg== X-Gm-Message-State: AOAM532+8g9ZaTNEfF8aUcqTPEs5d7oCipVSXho+LDaBQW02FE/8Qwwi NJH5asVYpNUJGg7w31SYNgqEijv8R6pyzUIaEyh5lQ== X-Google-Smtp-Source: ABdhPJwqyGpxeRwhqBz4EIQqp6oUWIUkmeO8tPYrw+Foo8kpBaOxKraO6LbhE78eCFGjaLGdnXSaPGdeUzlPK+8yJM8= X-Received: by 2002:aca:af55:: with SMTP id y82mr9880472oie.64.1593606758505; Wed, 01 Jul 2020 05:32:38 -0700 (PDT) MIME-Version: 1.0 References: <1588173552-10417-1-git-send-email-christophe.lyon@linaro.org> <1588173552-10417-2-git-send-email-christophe.lyon@linaro.org> In-Reply-To: From: Christophe Lyon Date: Wed, 1 Jul 2020 14:32:27 +0200 Message-ID: Subject: Re: [PATCH] arm: Warn if IRQ handler is not compiled with -mgeneral-regs-only [PR target/94743] To: Kyrylo Tkachov Cc: "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 01 Jul 2020 12:32:41 -0000 On Tue, 30 Jun 2020 at 15:34, Kyrylo Tkachov wrote: > > > > > -----Original Message----- > > From: Christophe Lyon > > Sent: 30 June 2020 14:32 > > To: Kyrylo Tkachov > > Cc: gcc-patches@gcc.gnu.org > > Subject: Re: [PATCH] arm: Warn if IRQ handler is not compiled with - > > mgeneral-regs-only [PR target/94743] > > > > On Tue, 30 Jun 2020 at 15:16, Kyrylo Tkachov > > wrote: > > > > > > Hi Christophe, > > > > > > Sorry for the delay. > > > > > > > -----Original Message----- > > > > From: Gcc-patches On Behalf Of > > > > Christophe Lyon via Gcc-patches > > > > Sent: 29 April 2020 16:19 > > > > To: gcc-patches@gcc.gnu.org > > > > Subject: [PATCH] arm: Warn if IRQ handler is not compiled with - > > mgeneral- > > > > regs-only [PR target/94743] > > > > > > > > The interrupt attribute does not guarantee that the FP registers are > > > > saved, which can result in problems difficult to debug. > > > > > > > > Saving the FP registers and status registers can be a large penalty, > > > > so it's probably not desirable to do that all the time. > > > > > > > > If the handler calls other functions, we'd likely need to save all of > > > > them, for lack of knowledge of which registers they actually use. > > > > > > > > This is even more obscure for the end-user when the compiler inserts > > > > calls to helper functions such as memcpy (some multilibs do use FP > > > > registers to speed it up). > > > > > > > > In the PR, we discussed adding routines in libgcc to save the FP > > > > context and saving only locally-clobbered FP registers, but I think > > > > this is too intrusive for stage 4. > > > > > > > > In the mean time, emit a warning to suggest re-compiling with > > > > -mgeneral-regs-only. Note that this can lead to errors if the code > > > > uses floating-point and -mfloat-abi=hard, eg: > > > > argument of type 'double' not permitted with -mgeneral-regs-only > > > > > > > > This can be troublesome for the user, but at least this would make > > > > them aware of the latent issue. > > > > > > > > The patch adds two testcases: > > > > - pr94734-1.c checks that a warning is emitted. One function can make > > > > implicit calls to runtime floating-point routines, the other one > > > > doesn't. We can improve the diagnostic later not to warn in the > > > > second case. > > > > > > > > - pr94734-2.c checks that no warning is emitted when using > > > > -mgeneral-regs-only. > > > > > > > > 2020-04-29 Christophe Lyon > > > > > > > > PR target/94743 > > > > gcc/ > > > > * config/arm/arm.c (arm_handle_isr_attribute): Warn if > > > > -mgeneral-regs-only is not used. > > > > > > > > gcc/testsuite/ > > > > * gcc.target/arm/pr94743-1.c: New test. > > > > * gcc.target/arm/pr94743-2.c: New test. > > > > --- > > > > gcc/config/arm/arm.c | 5 +++++ > > > > gcc/testsuite/gcc.target/arm/pr94743-1.c | 20 ++++++++++++++++++++ > > > > gcc/testsuite/gcc.target/arm/pr94743-2.c | 17 +++++++++++++++++ > > > > 3 files changed, 42 insertions(+) > > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-1.c > > > > create mode 100644 gcc/testsuite/gcc.target/arm/pr94743-2.c > > > > > > > > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c > > > > index 6a6e804..34aad1d 100644 > > > > --- a/gcc/config/arm/arm.c > > > > +++ b/gcc/config/arm/arm.c > > > > @@ -7176,6 +7176,11 @@ arm_handle_isr_attribute (tree *node, tree > > name, > > > > tree args, int flags, > > > > name); > > > > *no_add_attrs = true; > > > > } > > > > + else if (TARGET_VFP_BASE) > > > > + { > > > > + warning (OPT_Wattributes, "FP registers might be clobbered > > > > despite %qE attribute: compile with -mgeneral-regs-only", > > > > + name); > > > > + } > > > > > > Let's use %< and %> to quote -mgeneral-regs-only properly. > > > Ok with that change. > > > > Hi Kyrill, > > > > Thanks for the review, however I have posted v2 here: > > https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545747.html > > (you approved v1) > > > > It includes a few more testcases and updates to existing ones. > > > > Is v2 OK with the quotation marks fixed? > > > > Oops, sorry. Yes that looks ok too (with the quotation fixed). > Kyrill > Thanks, pushed as r11-1732. There were two follow-ups: r11-1752 because I forgot to update the expected warning message in the testcases after I changed the quotation) and r11-1759 because I missed that gcc.target/arm/handler-align.c has to be compiled with -mgeneral-regs-only like other testcases. Sorry for the noise, Christophe > > Thanks > > > > Christophe > > > > > Thanks, > > > Kyrill > > > > > > > /* FIXME: the argument if any is checked for type attributes; > > > > should it be checked for decl ones? */ > > > > } > > > > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-1.c > > > > b/gcc/testsuite/gcc.target/arm/pr94743-1.c > > > > new file mode 100644 > > > > index 0000000..67700c6 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-1.c > > > > @@ -0,0 +1,20 @@ > > > > +/* PR target/94743 */ > > > > +/* { dg-do compile } */ > > > > + > > > > +typedef struct { > > > > + double fpdata[32]; > > > > +} dummy_t; > > > > + > > > > +dummy_t global_d; > > > > +dummy_t global_d1; > > > > + > > > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void) > > > > +{ /* { dg-warning { FP registers might be clobbered despite 'interrupt' > > > > attribute: compile with -mgeneral-regs-only} "" { target *-*-* } . } */ > > > > + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3]; > > > > +} > > > > + > > > > + > > > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test2(void) > > > > +{ /* { dg-warning { FP registers might be clobbered despite 'interrupt' > > > > attribute: compile with -mgeneral-regs-only} "" { target *-*-* } . } */ > > > > + global_d.fpdata[3] = 1.0; > > > > +} > > > > diff --git a/gcc/testsuite/gcc.target/arm/pr94743-2.c > > > > b/gcc/testsuite/gcc.target/arm/pr94743-2.c > > > > new file mode 100644 > > > > index 0000000..745fd36 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.target/arm/pr94743-2.c > > > > @@ -0,0 +1,17 @@ > > > > +/* PR target/94743 */ > > > > +/* { dg-do compile } */ > > > > +/* { dg-options "-mgeneral-regs-only" } */ > > > > + > > > > +typedef struct { > > > > + /* Do not use floating-point types, which are not be compatible with > > > > + -mgeneral-regs-only under -mfloat-abi=hard */ > > > > + int fpdata[32]; > > > > +} dummy_t; > > > > + > > > > +dummy_t global_d; > > > > +dummy_t global_d1; > > > > + > > > > +__attribute__ ((interrupt("IRQ"))) void IRQ_HDLR_Test(void) > > > > +{ > > > > + global_d.fpdata[3] += global_d.fpdata[3] * global_d1.fpdata[3]; > > > > +} > > > > -- > > > > 2.7.4 > > >