From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 125309 invoked by alias); 29 May 2019 16:12:10 -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 125299 invoked by uid 89); 29 May 2019 16:12:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 29 May 2019 16:12:08 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BF52330C252E; Wed, 29 May 2019 16:12:06 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-2.rdu2.redhat.com [10.10.112.2]) by smtp.corp.redhat.com (Postfix) with ESMTP id 349C663BB8; Wed, 29 May 2019 16:12:03 +0000 (UTC) Subject: Re: undefined behavior in value_range::equiv_add()? To: Aldy Hernandez , Richard Biener Cc: gcc-patches References: <8ded6f74-091a-880a-3ad6-db78b7b427c9@redhat.com> <1226a18a-fd1d-ce99-9d78-b160d5ece242@redhat.com> From: Jeff Law Openpgp: preference=signencrypt Message-ID: Date: Wed, 29 May 2019 16:15:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <1226a18a-fd1d-ce99-9d78-b160d5ece242@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg01937.txt.bz2 On 5/29/19 9:58 AM, Aldy Hernandez wrote: > On 5/29/19 9:24 AM, Richard Biener wrote: >> On Wed, May 29, 2019 at 2:18 PM Aldy Hernandez wrote: >>> >>> As per the API, and the original documentation to value_range, >>> VR_UNDEFINED and VR_VARYING should never have equivalences.  However, >>> equiv_add is tacking on equivalences blindly, and there are various >>> regressions that happen if I fix this oversight. >>> >>> void >>> value_range::equiv_add (const_tree var, >>>                          const value_range *var_vr, >>>                          bitmap_obstack *obstack) >>> { >>>     if (!m_equiv) >>>       m_equiv = BITMAP_ALLOC (obstack); >>>     unsigned ver = SSA_NAME_VERSION (var); >>>     bitmap_set_bit (m_equiv, ver); >>>     if (var_vr && var_vr->m_equiv) >>>       bitmap_ior_into (m_equiv, var_vr->m_equiv); >>> } >>> >>> Is this a bug in the documentation / API, or is equiv_add incorrect and >>> we should fix the fall-out elsewhere? >> >> I think this must have been crept in during the classification.  If you >> go back to say GCC 7 you shouldn't see value-ranges with >> UNDEFINED/VARYING state in the lattice that have equivalences. >> >> It may not be easy to avoid with the new classy interface but we're >> certainly not tacking on them "blindly".  At least we're not supposed >> to.  As usual the intermediate state might be "broken" but >> intermediateness is not sth the new class "likes". > > It looks like extract_range_from_stmt (by virtue of > vrp_visit_assignment_or_call and then extract_range_from_ssa_name) > returns one of these intermediate ranges.  It would seem to me that an > outward looking API method like vr_values::extract_range_from_stmt > shouldn't be returning inconsistent ranges.  Or are there no guarantees > for value_ranges from within all of vr_values? ISTM that if we have an implementation constraint that says a VR_VARYING or VR_UNDEFINED range can't have equivalences, then we need to honor that at the minimum for anything returned by an external API. Returning an inconsistent state is bad. I'd even state that we should try damn hard to avoid it in internal APIs as well. > > Perhaps I should give a little background.  As part of your > value_range_base re-factoring last year, you mentioned that you didn't > split out intersect like you did union because of time or oversight.  I > have code to implement intersect (attached), for which I've noticed that > I must leave equivalences intact, even when transitioning to VR_UNDEFINED: > > [from the attached patch] > +  /* If THIS is varying we want to pick up equivalences from OTHER. > +     Just special-case this here rather than trying to fixup after the > +     fact.  */ > +  if (this->varying_p ()) > +    this->deep_copy (other); > +  else if (this->undefined_p ()) > +    /* ?? Leave any equivalences already present in an undefined. > +       This is technically not allowed, but we may get an in-flight > +       value_range in an intermediate state.  */ Where/when does this happen? > +    ; > > What is happening is that in evrp's record_ranges_from_stmt, we call > extract_range_from_stmt which returns an inconsistent VR_UNDEFINED with > an equivalence, which is then fed to update_value_range() and finally > value_range::intersect.  The VR_UNDEFINED equivalence must not be > removed in the intersect, because update_value_range() will get confused > as to whether this is a new VR or not (because VR_UNDEFINED with no > equivalences is not the same as VR_UNDEFINED with equivalences-- see > "is_new" in update_value_range). Ugh. I hate some of the gyrations we have to do for update_value_range. Regardless I tend to think the problem is in the inconsistent state we get back from extract_range_from_stmt. Jeff