From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate.crashing.org (gate.crashing.org [63.228.1.57]) by sourceware.org (Postfix) with ESMTP id 977393853560 for ; Fri, 12 Aug 2022 17:29:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 977393853560 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=kernel.crashing.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=kernel.crashing.org Received: from gate.crashing.org (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.14.1) with ESMTP id 27CHS93D030485; Fri, 12 Aug 2022 12:28:10 -0500 Received: (from segher@localhost) by gate.crashing.org (8.14.1/8.14.1/Submit) id 27CHS9hk030483; Fri, 12 Aug 2022 12:28:09 -0500 X-Authentication-Warning: gate.crashing.org: segher set sender to segher@kernel.crashing.org using -f Date: Fri, 12 Aug 2022 12:28:09 -0500 From: Segher Boessenkool To: "Kewen.Lin" Cc: GCC Patches , David Edelsohn , amodra@gmail.com Subject: Re: [PATCH v2] rs6000: Rework ELFv2 support for -fpatchable-function-entry* [PR99888] Message-ID: <20220812172809.GI25951@gate.crashing.org> References: <878358db-5d17-043c-cf09-b34ff9f4449a@linux.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <878358db-5d17-043c-cf09-b34ff9f4449a@linux.ibm.com> User-Agent: Mutt/1.4.2.3i X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: Fri, 12 Aug 2022 17:29:13 -0000 Hi! On Fri, Aug 12, 2022 at 05:40:06PM +0800, Kewen.Lin wrote: > This patch is to update the NOPs patched before and after > local entry, it looks like: As I said before, please don't say NOPs. I know some documentation does. That docvumentation needs fixing. This is not an acronym or similar: "nop" is short for "noop" or "no-op", meaning "no operation". It also is a well-accepted term. It also is an assembler mnemonic, and has to be written as all lower case there as well. Just say "nops" please. > --- a/gcc/testsuite/c-c++-common/patchable_function_entry-default.c > +++ b/gcc/testsuite/c-c++-common/patchable_function_entry-default.c > @@ -1,6 +1,7 @@ > /* { dg-do compile { target { ! { nvptx*-*-* visium-*-* } } } } */ > /* { dg-options "-O2 -fpatchable-function-entry=3,1" } */ > /* { dg-additional-options "-fno-pie" { target sparc*-*-* } } */ > +/* { dg-additional-options "-fpatchable-function-entry=3,2" { target powerpc_elfv2 } } */ Add a comment why this is needed? People looking at this testcase in the future (including yourself!) will thank you. > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr99888-1.c > @@ -0,0 +1,45 @@ > +/* { dg-require-effective-target powerpc_elfv2 } */ Does this not work on other PowerPC targets? > +/* Verify no errors for different NOPs after local entry. */ (Add "on ELFv2" if you make the test run everywhere :-) ) > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr99888-2.c > @@ -0,0 +1,45 @@ > +/* { dg-require-effective-target powerpc_elfv2 } */ > + > +/* Verify no errors for 2, 6 and 14 NOPs before local entry. */ Similar. > --- /dev/null > +++ b/gcc/testsuite/gcc.target/powerpc/pr99888-3.c > @@ -0,0 +1,12 @@ > +/* { dg-require-effective-target powerpc_elfv2 } */ > +/* { dg-options "-fpatchable-function-entry=1" } */ > + > +/* Verify no errors, using command line option instead of function > + attribute. */ > + > +extern int a; > + > +int test (int b) { > + return a + b; > +} And more. Rest looks good, thanks! Segher