Message ID | 6c8a8e20-674e-3954-a7e2-63c2eedac828@linux.ibm.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D71E23858003 for <patchwork@sourceware.org>; Thu, 28 Oct 2021 03:18:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D71E23858003 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1635391102; bh=USkM0tj9g7rfLj7qy8V/EA5pk+gIEdL+wheoaqSZkbs=; h=Date:To:Subject:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=r42CyUxWU3DYpH89YeZ20ttpd7fFe+n0r/aGdDVYIIo4ijLh91FTE8F/ln3nzl7Ua /9LT0z19xmIlZ6fox7tU4iz8VPa5mwl369aQklaWmoaNW/WOjCoHzqMrJio1Z5OoBa 7zaaHwVLksBr0nc8sEksS3ZoUYTa70EUpUB3gVOM= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 0E89D385843C for <gcc-patches@gcc.gnu.org>; Thu, 28 Oct 2021 03:17:52 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 0E89D385843C Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 19S3GVgo010826; Thu, 28 Oct 2021 03:17:52 GMT Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 3byktj80u8-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 28 Oct 2021 03:17:51 +0000 Received: from m0098393.ppops.net (m0098393.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 19S3HLoU015577; Thu, 28 Oct 2021 03:17:51 GMT Received: from ppma03wdc.us.ibm.com (ba.79.3fa9.ip4.static.sl-reverse.com [169.63.121.186]) by mx0a-001b2d01.pphosted.com with ESMTP id 3byktj80tv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 28 Oct 2021 03:17:51 +0000 Received: from pps.filterd (ppma03wdc.us.ibm.com [127.0.0.1]) by ppma03wdc.us.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 19S376tr026228; Thu, 28 Oct 2021 03:17:42 GMT Received: from b03cxnp08026.gho.boulder.ibm.com (b03cxnp08026.gho.boulder.ibm.com [9.17.130.18]) by ppma03wdc.us.ibm.com with ESMTP id 3bx4f0yngd-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 28 Oct 2021 03:17:42 +0000 Received: from b03ledav001.gho.boulder.ibm.com (b03ledav001.gho.boulder.ibm.com [9.17.130.232]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 19S3HfJF29163778 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 28 Oct 2021 03:17:41 GMT Received: from b03ledav001.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 6B3406E052; Thu, 28 Oct 2021 03:17:41 +0000 (GMT) Received: from b03ledav001.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 8A5386E054; Thu, 28 Oct 2021 03:17:40 +0000 (GMT) Received: from [9.65.201.56] (unknown [9.65.201.56]) by b03ledav001.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 28 Oct 2021 03:17:40 +0000 (GMT) Message-ID: <6c8a8e20-674e-3954-a7e2-63c2eedac828@linux.ibm.com> Date: Wed, 27 Oct 2021 22:17:39 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.2.1 Content-Language: en-US To: Segher Boessenkool <segher@kernel.crashing.org>, David Edelsohn <dje.gcc@gmail.com> Subject: rs6000: Fix up flag_shrink_wrap handling in presence of -mrop-protect [PR101324] Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-GUID: ybtWDqJfdkpiKMFnSrdy9DTZjs0KOz6Q X-Proofpoint-ORIG-GUID: T5qZVvmHBrLu0-8qkFFPPK4bugWPkJ-d X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.0.607.475 definitions=2021-10-27_07,2021-10-26_01,2020-04-07_01 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 clxscore=1015 impostorscore=0 mlxscore=0 lowpriorityscore=0 spamscore=0 adultscore=0 phishscore=0 bulkscore=0 priorityscore=1501 mlxlogscore=924 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000 definitions=main-2110280012 X-Spam-Status: No, score=-11.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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 <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Peter Bergner via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Peter Bergner <bergner@linux.ibm.com> Cc: Bill Schmidt <wschmidt@linux.ibm.com>, Tulio Magno Quites Machado Filho <tuliom@linux.ibm.com>, GCC Patches <gcc-patches@gcc.gnu.org> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
rs6000: Fix up flag_shrink_wrap handling in presence of -mrop-protect [PR101324]
|
|
Commit Message
Peter Bergner
Oct. 28, 2021, 3:17 a.m. UTC
Sorry for reposting, but I forgot to CC the gcc-patches mailing list. :-( PR101324 shows a problem in disabling shrink-wrapping when using -mrop-protect when there is a attribute optimize/pragma. Martin's patch below moves handling of flag_shrink_wrap so it gets re-disbled when we change or add options. This passed bootstrap and regtesting with no regressions. Segher, you approved Martin's patch in the bugzilla. Is the test case ok too? I'll note the test case uses the "new" rop_ok effective-target function which I submitted as a separate patch. Peter 2021-10-27 Martin Liska <mliska@suse.cz> gcc/ PR target/101324 * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the disabling of shrink-wrapping when using -mrop-protect from here... (rs6000_override_options_after_change): ...to here. 2021-10-27 Peter Bergner <bergner@linux.ibm.com> gcc/testsuite/ PR target/101324 * gcc.target/powerpc/pr101324.c: New test.
Comments
On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: > Sorry for reposting, but I forgot to CC the gcc-patches mailing list. :-( Whoops, and I replied to your original message :-/ > 2021-10-27 Martin Liska <mliska@suse.cz> > > gcc/ > PR target/101324 > * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the > disabling of shrink-wrapping when using -mrop-protect from here... > (rs6000_override_options_after_change): ...to here. > > 2021-10-27 Peter Bergner <bergner@linux.ibm.com> > > gcc/testsuite/ > PR target/101324 > * gcc.target/powerpc/pr101324.c: New test. > +/* Ensure hashst comes after mflr and hashchk comes after ld 0,16(1). */ > +/* { dg-final { scan-assembler "mflr 0.*hashst 0," } } */ > +/* { dg-final { scan-assembler "ld 0,16\\\(1\\\).*hashchk 0," } } */ First: don't use double quotes, or you get double backslashes (or more) as well. Use curlies instead: /* { dg-final { scan-assembler {ld 0,16\(1\).*hashchk 0,} } } */ But, more importantly, "." by default matches anything, newlines as well. You probably do not want that here, because your RE as written can match an "ld" in one function and a "hashchk" many functions later, many million lines later. You can for example do /* { dg-final { scan-assembler {(?p)ld 0,.*\n.*\mhashchk 0,} } } */ (?p) is "partial newline-sensitive matching": it makes "." not match newlines. This is often what you want. This RE also makes sure that "hashchk" is the full mnemonic (not the tail of one), and that it is on the line after that "ld". Similarly you would have /* { dg-final { scan-assembler {(?p)\mmflr 0,.*\n.*\mhashst 0,} } } */ I hope I didn't typo those things, I didn't test them out :-) Okay for trunk with similar robustification. Thanks! Segher
Sorry for taking so long to get back to this. On 10/29/21 4:45 PM, Segher Boessenkool wrote: > On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >> +/* Ensure hashst comes after mflr and hashchk comes after ld 0,16(1). */ >> +/* { dg-final { scan-assembler "mflr 0.*hashst 0," } } */ >> +/* { dg-final { scan-assembler "ld 0,16\\\(1\\\).*hashchk 0," } } */ > > First: don't use double quotes, or you get double backslashes (or more) > as well. Use curlies instead: > > /* { dg-final { scan-assembler {ld 0,16\(1\).*hashchk 0,} } } */ > > But, more importantly, "." by default matches anything, newlines as > well. You probably do not want that here, because your RE as written > can match an "ld" in one function and a "hashchk" many functions later, > many million lines later. > > You can for example do > /* { dg-final { scan-assembler {(?p)ld 0,.*\n.*\mhashchk 0,} } } */ I had to change your suggestion to the following, which works: /* { dg-final { scan-assembler {(?p)\mmflr 0.*\n.*\n.*\mhashst 0,} } } */ /* { dg-final { scan-assembler {(?p)ld 0,.*\n.*\n.*\n.*\mhashchk 0,} } } */ Meaning from the current code generation, the hashst is 2 insns after the mflr insn and the hashck is 3 insns after the ld 0,16(1) insn. But is this fragile? Are we sure we won't schedule the hashst and hashchk to some other location breaking the test case? > (?p) is "partial newline-sensitive matching": it makes "." not match > newlines. This is often what you want. This RE also makes sure that > "hashchk" is the full mnemonic (not the tail of one), and that it is on > the line after that "ld". Well, this test case only has one function and is very small, so there should only be one "mflr 0" and one "ld 0,16(1)". Given the small size, I assumed if the hashst and hashchk were after the mflr/ld 0,16(1), then it's in the correct order and "fixed". But maybe that's just as fragile as assuming how many insns proceed the hashst/hashchk? If you prefer the updated change to your suggestion, let me know and I'll commit it that way. > I'll note the test case uses the "new" rop_ok effective-target function which > I submitted as a separate patch. This test case uses the new rop_ok effective-target which I said I submitted as a separate patch, but I can't seem to find the submission anywhere and it's not even in the archive. Did I forget to submit it? Probably. :-( Let me do that now, as this relies on that patch. Peter
On 10/29/21 4:45 PM, Segher Boessenkool wrote: > On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >> 2021-10-27 Martin Liska <mliska@suse.cz> >> >> gcc/ >> PR target/101324 >> * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the >> disabling of shrink-wrapping when using -mrop-protect from here... >> (rs6000_override_options_after_change): ...to here. >> >> 2021-10-27 Peter Bergner <bergner@linux.ibm.com> >> >> gcc/testsuite/ >> PR target/101324 >> * gcc.target/powerpc/pr101324.c: New test. > > Okay for trunk with similar robustification. Thanks! With the rop_ok change finally committed, I have finally pushed this change with your suggested test case changes. Thanks Martin for the fix and Segher for the reviews. Peter
On 12/3/21 2:39 PM, Peter Bergner wrote: > On 10/29/21 4:45 PM, Segher Boessenkool wrote: >> On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >>> 2021-10-27 Martin Liska <mliska@suse.cz> >>> >>> gcc/ >>> PR target/101324 >>> * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the >>> disabling of shrink-wrapping when using -mrop-protect from here... >>> (rs6000_override_options_after_change): ...to here. >>> >>> 2021-10-27 Peter Bergner <bergner@linux.ibm.com> >>> >>> gcc/testsuite/ >>> PR target/101324 >>> * gcc.target/powerpc/pr101324.c: New test. >> >> Okay for trunk with similar robustification. Thanks! > > With the rop_ok change finally committed, I have finally pushed this > change with your suggested test case changes. Thanks Martin for the > fix and Segher for the reviews. So I just checked and we have the same failure on GCC11 too. Ok for the GCC11 release branch after this has burned-in on trunk for a couple of days? Ditto for the rop_ok testsuite patch that goes with this one? Peter
On 12/3/21 3:27 PM, Peter Bergner wrote: > On 12/3/21 2:39 PM, Peter Bergner wrote: >> On 10/29/21 4:45 PM, Segher Boessenkool wrote: >>> On Wed, Oct 27, 2021 at 10:17:39PM -0500, Peter Bergner wrote: >>>> 2021-10-27 Martin Liska <mliska@suse.cz> >>>> >>>> gcc/ >>>> PR target/101324 >>>> * config/rs6000/rs6000.c (rs6000_option_override_internal): Move the >>>> disabling of shrink-wrapping when using -mrop-protect from here... >>>> (rs6000_override_options_after_change): ...to here. >>>> >>>> 2021-10-27 Peter Bergner <bergner@linux.ibm.com> >>>> >>>> gcc/testsuite/ >>>> PR target/101324 >>>> * gcc.target/powerpc/pr101324.c: New test. >>> >>> Okay for trunk with similar robustification. Thanks! >> >> With the rop_ok change finally committed, I have finally pushed this >> change with your suggested test case changes. Thanks Martin for the >> fix and Segher for the reviews. > > So I just checked and we have the same failure on GCC11 too. > Ok for the GCC11 release branch after this has burned-in on > trunk for a couple of days? > > Ditto for the rop_ok testsuite patch that goes with this one? FYI, both commits backported cleanly and showed no testsuite regressions on powerrpc64le-linux. Peter
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index bac959f4ef4..95e0d2cffdd 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -3484,6 +3484,10 @@ rs6000_override_options_after_change (void) } else if (!OPTION_SET_P (flag_cunroll_grow_size)) flag_cunroll_grow_size = flag_peel_loops || optimize >= 3; + + /* If we are inserting ROP-protect instructions, disable shrink wrap. */ + if (rs6000_rop_protect) + flag_shrink_wrap = 0; } #ifdef TARGET_USES_LINUX64_OPT @@ -4048,10 +4052,6 @@ rs6000_option_override_internal (bool global_init_p) && ((rs6000_isa_flags_explicit & OPTION_MASK_QUAD_MEMORY_ATOMIC) == 0)) rs6000_isa_flags |= OPTION_MASK_QUAD_MEMORY_ATOMIC; - /* If we are inserting ROP-protect instructions, disable shrink wrap. */ - if (rs6000_rop_protect) - flag_shrink_wrap = 0; - /* If we can shrink-wrap the TOC register save separately, then use -msave-toc-indirect unless explicitly disabled. */ if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0 diff --git a/gcc/testsuite/gcc.target/powerpc/pr101324.c b/gcc/testsuite/gcc.target/powerpc/pr101324.c new file mode 100644 index 00000000000..d27cc2876f3 --- /dev/null +++ b/gcc/testsuite/gcc.target/powerpc/pr101324.c @@ -0,0 +1,17 @@ +/* { dg-require-effective-target rop_ok } */ +/* { dg-options "-O1 -mrop-protect -mdejagnu-cpu=power10" } */ + +extern void foo (void); + +long int +__attribute__ ((__optimize__ ("no-inline"))) +func (long int cond) +{ + if (cond) + foo (); + return cond; +} + +/* Ensure hashst comes after mflr and hashchk comes after ld 0,16(1). */ +/* { dg-final { scan-assembler "mflr 0.*hashst 0," } } */ +/* { dg-final { scan-assembler "ld 0,16\\\(1\\\).*hashchk 0," } } */