From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mengyan1223.wang (mengyan1223.wang [89.208.246.23]) by sourceware.org (Postfix) with ESMTPS id 511753857C65 for ; Thu, 10 Mar 2022 11:32:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 511753857C65 Received: from localhost.localdomain (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature ECDSA (P-384) server-digest SHA384) (Client did not present a certificate) (Authenticated sender: xry111@mengyan1223.wang) by mengyan1223.wang (Postfix) with ESMTPSA id 960EC6623D; Thu, 10 Mar 2022 06:32:08 -0500 (EST) Message-ID: Subject: Re: [PATCH v2] cse: avoid signed overflow in compute_const_anchors [PR 104843] From: Xi Ruoyao To: Richard Biener Cc: GCC Patches , Richard Sandiford , Jeff Law Date: Thu, 10 Mar 2022 19:32:06 +0800 In-Reply-To: References: Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.4 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3035.4 required=5.0 tests=BAYES_00, BODY_8BITS, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, JMQ_SPF_NEUTRAL, LOTS_OF_MONEY, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Mar 2022 11:32:13 -0000 On Thu, 2022-03-10 at 09:01 +0100, Richard Biener wrote: > On Wed, Mar 9, 2022 at 5:12 PM Xi Ruoyao > wrote: > > > > On Wed, 2022-03-09 at 15:55 +0100, Richard Biener wrote: > > > > > isn't it better to make targetm.const_anchor unsigned? > > > The & and ~ are not subject to overflow rules. > > > > It's not enough: if n is the minimum value of HOST_WIDE_INT and > > const_anchor = 0x8000 (the value for MIPS), we'll have a signed > > 0x7fff > > in *upper_base.  Then the next line, "*upper_offs = n - > > *upper_base;" > > will be a signed overflow again. > > > > How about the following? > > Hmm, so all this seems to be to round CST up and down to a multiple of > CONST_ANCHOR. > It works on CONST_INT only which is sign-extended, so if there is > overflow the resulting > anchor is broken as far as I can see. On MIPS addiu/daddiu do 2-complement addition, so the overflowed result is still usable. > So instead of papering over this issue > the function should return false when n is negative since then > n & ~(targetm.const_anchor - 1) is also not n rounded down to a > multiple of const_anchor. This function does work for negative n, like: void g (int, int); void f (void) { g(0x8123ffff, 0x81240001); } It should produce: li $4,-2128347136 # 0xffffffff81240000 daddiu $5,$4,1 daddiu $4,$4,-1 jal g But return false for negative n will cause regression for this case, producing: li $5,-2128347136 # 0xffffffff81240000 li $4,-2128412672 # 0xffffffff81230000 ori $5,$5,0x1 ori $4,$4,0xffff jal g That being said, it indeed does not work for: void g (int, int); void f () { g (0x7fffffff, 0x80000001); } It produces: li $5,-2147483648 # 0xffffffff80000000 li $4,2147418112 # 0x7fff0000 daddiu $5,$5,1 ori $4,$4,0xffff jal g Should be: li $5,-2147483648 # 0xffffffff80000000 daddiu $5,$5,1 addiu $4,$5,-1 > > -- >8 -- > > > > With a non-zero const_anchor, the behavior of this function relied on > > signed overflow. > > > > gcc/ > > > >         PR rtl-optimization/104843 > >         * cse.cc (compute_const_anchors): Use unsigned HOST_WIDE_INT for > >         n to perform overflow arithmetics safely. > > --- > >  gcc/cse.cc | 8 ++++---- > >  1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/gcc/cse.cc b/gcc/cse.cc > > index a18b599d324..052fa0c3490 100644 > > --- a/gcc/cse.cc > > +++ b/gcc/cse.cc > > @@ -1169,12 +1169,12 @@ compute_const_anchors (rtx cst, > >                        HOST_WIDE_INT *lower_base, HOST_WIDE_INT *lower_offs, > >                        HOST_WIDE_INT *upper_base, HOST_WIDE_INT *upper_offs) > >  { > > -  HOST_WIDE_INT n = INTVAL (cst); > > - > > -  *lower_base = n & ~(targetm.const_anchor - 1); > > -  if (*lower_base == n) > > +  unsigned HOST_WIDE_INT n = UINTVAL (cst); > > +  unsigned HOST_WIDE_INT lb = n & ~(targetm.const_anchor - 1); > > +  if (lb == n) > >      return false; > > > > +  *lower_base = lb; > >    *upper_base = > >      (n + (targetm.const_anchor - 1)) & ~(targetm.const_anchor - 1); > >    *upper_offs = n - *upper_base; > > -- > > 2.35.1 > > > > > > > -- Xi Ruoyao School of Aerospace Science and Technology, Xidian University