From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 5B8123858D32 for ; Mon, 27 Feb 2023 16:08:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5B8123858D32 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1677514109; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=vWsGDSqfs/pI8p3O/DYNODPVybKCo0SFgRinRtAgdak=; b=Ec12QqrMN7KNLPesmSyAGtd1cu2YXs+deKGr+zQwKiw2ac2NjuCpnY5dH3cz2DAswXfttp DgHGL22+QcF7zldn4vcA7uvWpwmYea0loqnOTxF9rsr6iAJeS+REBiuRRo8V/YYgr/ztfk NlCdefNPxA+G4NBaLi2MxAD1QarWTpY= Received: from mail-wr1-f70.google.com (mail-wr1-f70.google.com [209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-381-Xxo-gbOoOX-JBUN6dkJAag-1; Mon, 27 Feb 2023 11:08:28 -0500 X-MC-Unique: Xxo-gbOoOX-JBUN6dkJAag-1 Received: by mail-wr1-f70.google.com with SMTP id o3-20020a5d6483000000b002cc4fe0f7fcso282771wri.7 for ; Mon, 27 Feb 2023 08:08:28 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:cc:content-language :references:to:subject:from:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=vWsGDSqfs/pI8p3O/DYNODPVybKCo0SFgRinRtAgdak=; b=2ebvBtfpuPXk1w84UECatgHtIH5+v6zoYnp/zuEKX6rBeQD1f8OIxnWMnz5Rgwbw2l Z3ZDHEZF3+nYP/w8sfqhz/TwK3MQBqHK8NTvchXO4m4zGMDvPfVe9X8MjZrWRMpvMVmR OfNgydvz2oKW6rtnPjax0+RaLqGtwfImKNzJg6gX/jrv7TR11jsg6sVJ3IW+JhgrDHEI r3X8jKd4Q+o3IKIJb3KcJsq1DrJXCKnuxT+/XzQzxRbHmgTxGtapslXbEL3gmKQProHd Aseyz233SYHx7TaA86Vi+34QM2+42Xh2+NaP8slTrCYPBQ8MXVrZh5bYdMuqgq9NXdZE n0wA== X-Gm-Message-State: AO0yUKXyXEln5q0wPGSKIUCw5zCrWMOUpg3g/FdrVJavN6nSWJ4eHP4G yjYFadAL3/Yc/aidTyQ9pnh/0xVq8/qxv8QUR+WUzOxbGJFt5j8iYwun04czZkpfWYaBtQA5Qrw E8jBtDC9spLDZClioLg== X-Received: by 2002:adf:de05:0:b0:2c7:1aaa:2b49 with SMTP id b5-20020adfde05000000b002c71aaa2b49mr11866799wrm.23.1677514107030; Mon, 27 Feb 2023 08:08:27 -0800 (PST) X-Google-Smtp-Source: AK7set+cyqMWnoElR8W3033MKMQXWijREmf3loCmvnG6dH8BGCNqSW0HbZGD5PFC9Hz9FoVRvul1yQ== X-Received: by 2002:adf:de05:0:b0:2c7:1aaa:2b49 with SMTP id b5-20020adfde05000000b002c71aaa2b49mr11866783wrm.23.1677514106652; Mon, 27 Feb 2023 08:08:26 -0800 (PST) Received: from [192.168.1.201] ([139.47.42.170]) by smtp.gmail.com with ESMTPSA id g14-20020a7bc4ce000000b003eb20d4d4a8sm9243150wmk.44.2023.02.27.08.08.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 27 Feb 2023 08:08:26 -0800 (PST) Message-ID: <98102d74-cb30-0190-af66-86800e0721e4@redhat.com> Date: Mon, 27 Feb 2023 17:08:25 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 From: Aldy Hernandez Subject: Re: [PATCH] Fixup possible VR_ANTI_RANGE value_range issues To: Richard Biener , gcc-patches@gcc.gnu.org References: <09219.123022708581201997@us-mta-652.us.mimecast.lan> Cc: Andrew MacLeod In-Reply-To: <09219.123022708581201997@us-mta-652.us.mimecast.lan> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-10.2 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,RCVD_IN_BARRACUDACENTRAL,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 2/27/23 14:58, Richard Biener wrote: > After fixing PR107561 the following avoids looking at VR_ANTI_RANGE > ranges where it doesn't seem obvious the code does the correct > thing here (lower_bound and upper_bound do not work as expected). I do realize there's some confusion here, and some of it is my fault. This has become obvious in my upcoming work removing all of legacy. What's going on is that ultimately min/max are broken with (non-legacy) iranges. Or at the very least inconsistent between legacy and non-legacy. These are left over from the legacy world, and have been marked DEPRECATED for a few releases, but the middle end warnings continued to use them and even added new uses after they were obsoleted. min/max have different meanings depending on kind(), which is also deprecated, btw. They are the underlying min/max fields from the legacy implementation, and thus somewhat leak the implementation details. Unfortunately, they are being called from non-legacy code which is ignoring the kind() field. In retrospect I should've converted everything away from min/max/kind years ago, or at the very least converted min/max to work with non-legacy more consistently. For the record: enum value_range_kind kind () const; // DEPRECATED tree min () const; // DEPRECATED tree max () const; // DEPRECATED bool symbolic_p () const; // DEPRECATED bool constant_p () const; // DEPRECATED void normalize_symbolics (); // DEPRECATED void normalize_addresses (); // DEPRECATED bool may_contain_p (tree) const; // DEPRECATED bool legacy_verbose_union_ (const class irange *); // DEPRECATED bool legacy_verbose_intersect (const irange *); // DEPRECATED In my local branch I tried converting all the middle-end legacy API uses to the new API, but a bunch of tests started failing, and lots more false positives started showing up in correct code. I suspect that's part of the reason legacy continued to be used in these passes :-/. As you point out in the PR, the tests seem designed to test the current (at times broken) implementation. That being said, the 5 fixes in your patch are not wrong to begin with, because all uses guard lower_bound() and upper_bound() which work correctly. They return the lowest and uppermost possible bound for the range (ignoring the underlying implementation). So, the lower bound of a signed non-zero is -INF because ~[0,0] is really [-INF,-1][1,+INF]. In the min/max legacy code, min() of signed non-zero (~[0,0]) is 0. The new implementation has no concept of anti-ranges, and we don't leak any of that out. Any uses of min/max without looking at kind() are definitely broken. OTOH uses of lower/upper_bound are fine and should work with both legacy and non-legacy. Unrelated, but one place where I haven't been able to convince myself that the use is correct is bounds_of_var_in_loop: > /* Check if init + nit * step overflows. Though we checked > scev {init, step}_loop doesn't wrap, it is not enough > because the loop may exit immediately. Overflow could > happen in the plus expression in this case. */ > if ((dir == EV_DIR_DECREASES > && compare_values (maxvr.min (), initvr.min ()) != -1) > || (dir == EV_DIR_GROWS > && compare_values (maxvr.max (), initvr.max ()) != 1)) Ughh...this is all slated to go away, and I have patches removing all of legacy and the old API. Does this help? Do you still think lower and upper bound are not working as expected? Aldy > > Bootstrapped and tested on x86_64-unknown-linux-gnu. > > OK? > > Thanks, > Richard. > > * gimple-ssa-sprintf.cc (get_int_range): Avoid VR_ANTI_RANGE > by using range_int_cst_p. > (format_integer): Likewise. > (handle_printf_call): Guard against VR_ANTI_RANGE. > * graphite-sese-to-poly.cc (add_param_constraints): Likewise. > * tree-ssa-strlen.cc (set_strlen_range): Likewise. > --- > gcc/gimple-ssa-sprintf.cc | 6 +++--- > gcc/graphite-sese-to-poly.cc | 2 +- > gcc/tree-ssa-strlen.cc | 2 +- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/gcc/gimple-ssa-sprintf.cc b/gcc/gimple-ssa-sprintf.cc > index 18975708d2c..61974072f62 100644 > --- a/gcc/gimple-ssa-sprintf.cc > +++ b/gcc/gimple-ssa-sprintf.cc > @@ -1082,7 +1082,7 @@ get_int_range (tree arg, gimple *stmt, > value_range vr; > query->range_of_expr (vr, arg, stmt); > > - if (!vr.undefined_p () && !vr.varying_p ()) > + if (range_int_cst_p (&vr)) > { > HOST_WIDE_INT type_min > = (TYPE_UNSIGNED (argtype) > @@ -1391,7 +1391,7 @@ format_integer (const directive &dir, tree arg, pointer_query &ptr_qry) > value_range vr; > ptr_qry.rvals->range_of_expr (vr, arg, dir.info->callstmt); > > - if (!vr.varying_p () && !vr.undefined_p ()) > + if (range_int_cst_p (&vr)) > { > argmin = wide_int_to_tree (TREE_TYPE (arg), vr.lower_bound ()); > argmax = wide_int_to_tree (TREE_TYPE (arg), vr.upper_bound ()); > @@ -4623,7 +4623,7 @@ handle_printf_call (gimple_stmt_iterator *gsi, pointer_query &ptr_qry) > value_range vr; > ptr_qry.rvals->range_of_expr (vr, size, info.callstmt); > > - if (!vr.undefined_p ()) > + if (!vr.undefined_p () && vr.kind () != VR_ANTI_RANGE) > { > tree type = TREE_TYPE (size); > tree tmin = wide_int_to_tree (type, vr.lower_bound ()); > diff --git a/gcc/graphite-sese-to-poly.cc b/gcc/graphite-sese-to-poly.cc > index fbe7667380a..b89262640ac 100644 > --- a/gcc/graphite-sese-to-poly.cc > +++ b/gcc/graphite-sese-to-poly.cc > @@ -426,7 +426,7 @@ add_param_constraints (scop_p scop, graphite_dim_t p, tree parameter) > > if (INTEGRAL_TYPE_P (type) > && get_range_query (cfun)->range_of_expr (r, parameter) > - && !r.undefined_p ()) > + && range_int_cst_p (&r)) > { > min = r.lower_bound (); > max = r.upper_bound (); > diff --git a/gcc/tree-ssa-strlen.cc b/gcc/tree-ssa-strlen.cc > index 7508c1768a5..e1230522564 100644 > --- a/gcc/tree-ssa-strlen.cc > +++ b/gcc/tree-ssa-strlen.cc > @@ -1936,7 +1936,7 @@ set_strlen_range (tree lhs, wide_int min, wide_int max, > { > value_range r; > get_range_query (cfun)->range_of_expr (r, bound); > - if (!r.undefined_p ()) > + if (range_int_cst_p (&r)) > { > /* For a bound in a known range, adjust the range determined > above as necessary. For a bound in some anti-range or