From patchwork Wed Jun 2 10:32:25 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 1486575 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=sourceware.org; envelope-from=gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha256 header.s=default header.b=oKT5wjz9; dkim-atps=neutral Received: from sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4Fw5444QjVz9sCD for ; Wed, 2 Jun 2021 20:34:11 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 340573886C59 for ; Wed, 2 Jun 2021 10:34:09 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 340573886C59 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1622630049; bh=eLInCJOyqMSQL/6P5qMZJuK9AYeb0dOhGs7gVc6CN0I=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:Cc:From; b=oKT5wjz9z+f4kmYOaPDTG2WSwv/3JpmCBmse0an1BxiWc4mz4CF5rOON3sCCIPmaa EJ6Oh+MF3sZfuDAUasJUlJy+oKYYERcQs+Xxil+7niDwt2X2fbrDs2e+anADu2XmWb SMJgcgxhBVRSPh0ALCioHXb5Tzzk9OS/XERtPW9g= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id 73C423857001 for ; Wed, 2 Jun 2021 10:32:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 73C423857001 Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-233-7EkS5yPkM26gZIuPeukIHA-1; Wed, 02 Jun 2021 06:32:28 -0400 X-MC-Unique: 7EkS5yPkM26gZIuPeukIHA-1 Received: by mail-wm1-f69.google.com with SMTP id o10-20020a05600c4fcab029014ae7fdec90so456693wmq.5 for ; Wed, 02 Jun 2021 03:32:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:from:subject:cc:message-id:date:user-agent :mime-version:content-language; bh=eLInCJOyqMSQL/6P5qMZJuK9AYeb0dOhGs7gVc6CN0I=; b=tnk0lHMNML6/kHq3iuhUQYNix9CRG5149UxZgkdGmYHQJBJ/3+Scp/U7VoxVJiUsFx +3eXhZB6VsYlMEZQi/qqXmzAmWtRpanL7ekYpRJxiTVWIl6m9xIgKJgqyUcdNzDapEHL IHo9htIn4SU0i4csYpgLEcYtaBBbZCPxweX6dYFllzqWA9n13HK8evmsHGhCF1rlZwme sOBuJr2bnw4XDgUJmQVQraLGCaVHBU2J4rEPC3W1eHm3sbE0DgMnPbJMUitCHLFChqER UHyS+CN3PKeXVR+npL02wrE3gbBpS+Z9WB5OAocynM21lalyiQpIM61FXhgPJ4IMWSmp m8BQ== X-Gm-Message-State: AOAM5317XgkK/PgDiu9Ju2I/bfmcJNzrC1TgzmceIOcyO7U1mKHw4zDB +XbS6n0wbOxANSG9iRmx/crGssxEFeXwqwDzEGDvtUF0S6GwWvmaYXrRdmfPBCn7DZVE5H3VLqO hXnYzBLHyPQXvXWtOBA== X-Received: by 2002:a05:600c:358f:: with SMTP id p15mr4574333wmq.14.1622629946974; Wed, 02 Jun 2021 03:32:26 -0700 (PDT) X-Google-Smtp-Source: ABdhPJyjwff5Y2iYd7MiLN9PlXPazNP1MAnZbJIdDPW+7E8gL/dlIxio0kWM3+dTgw8XScZ/8MUY7A== X-Received: by 2002:a05:600c:358f:: with SMTP id p15mr4574314wmq.14.1622629946781; Wed, 02 Jun 2021 03:32:26 -0700 (PDT) Received: from abulafia.quesejoda.com ([95.169.237.215]) by smtp.gmail.com with ESMTPSA id s128sm2402080wme.6.2021.06.02.03.32.25 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 02 Jun 2021 03:32:26 -0700 (PDT) To: Andrew MacLeod , gcc-patches Subject: [RFC/PATCH] updating global ranges and their effect on __builtin_unreachable code Message-ID: Date: Wed, 2 Jun 2021 12:32:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US X-Spam-Status: No, score=-9.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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-Patchwork-Original-From: Aldy Hernandez via Gcc-patches From: Aldy Hernandez Reply-To: Aldy Hernandez Cc: Martin Sebor Errors-To: gcc-patches-bounces+incoming=patchwork.ozlabs.org@gcc.gnu.org Sender: "Gcc-patches" We've been having "issues" in our branch when exporting to the global space ranges that take into account previously known ranges (SSA_NAME_RANGE_INFO, etc). For the longest time we had the export feature turned off because it had the potential of removing __builtin_unreachable code early in the pipeline. This was causing one or two tests to fail. I finally got fed up, and investigated why. Take the following code: i_4 = somerandom (); if (i_4 < 0) goto ; [INV] else goto ; [INV] : __builtin_unreachable (); : It turns out that both legacy evrp and VRP have code that notices the above pattern and sets the *global* range for i_4 to [0,MAX]. That is, the range for i_4 is set, not at BB4, but at the definition site. See uses of assert_unreachable_fallthru_edge_p() for details. This global range causes subsequent passes (VRP1 in the testcase below), to remove the checks and the __builtin_unreachable code altogether. // pr80776-1.c int somerandom (void); void Foo (void) { int i = somerandom (); if (! (0 <= i)) __builtin_unreachable (); if (! (0 <= i && i <= 999999)) __builtin_unreachable (); sprintf (number, "%d", i); } This means that by the time the -Wformat-overflow warning runs, the above sprintf has been left unguarded, and a bogus warning is issued. Currently the above test does not warn, but that's because of an oversight in export_global_ranges(). This function is disregarding known global ranges (SSA_NAME_RANGE_INFO and SSA_NAME_PTR_INFO) and only setting ranges the ranger knows about. For the above test the IL is: : i_4 = somerandom (); if (i_4 < 0) goto ; [INV] else goto ; [INV] : __builtin_unreachable (); : i.0_1 = (unsigned int) i_4; if (i.0_1 > 999999) goto ; [INV] else goto ; [INV] : __builtin_unreachable (); : _7 = __builtin___sprintf_chk (&number, 1, 7, "%d", i_4); Legacy evrp has determined that the range for i_4 is [0,MAX] per my analysis above, but ranger has no known range for i_4 at the definition site. So at export_global_ranges time, ranger leaves the [0,MAX] alone. OTOH, evrp sets the global range at the definition for i.0_1 to [0,999999] per the same unreachable feature. However, ranger has correctly determined that the range for i.0_1 at the definition is [0,MAX], which it then proceeds to export. Since the current export_global_ranges (mistakenly) does not take into account previous global ranges, the ranges in the global tables end up like this: i_4: [0, MAX] i.0_1: [0, MAX] This causes the first unreachable block to be removed in VRP1, but the second one to remain. Later VRP can determine that i_4 in the sprintf call is [0,999999], and no warning is issued. But... the missing bogus warning is due to current export_global_ranges ignoring SSA_NAME_RANGE_INFO and friends, something which I'd like to fix. However, fixing this, gets us back to: i_4: [0, MAX] i.0_1: [0, 999999] Which means, we'll be back to removing the unreachable blocks and issuing a warning in pr80776-1.c (like we have been since the beginning of time). The attached patch fixes export_global_ranges to the expected behavior, and adds the previous XFAIL to pr80776-1.c, while documenting why this warning is issued in the first place. Once legacy evrp is removed, this won't be an issue, as ranges in the IL will tell the truth. However, this will mean that we will no longer remove the first __builtin_unreachable combo. But ISTM, that would be correct behavior ??. BTW, in addition to this patch we could explore removing the assert_unreachable_fallthru_edge_p() use in the evrp_analyzer, since it is no longer needed to get the warnings in the testcases in the original PR correctly (gcc.dg/pr80776-[12].c). Tested on x86-64 Linux. OK? Aldy From 36684dde843a4c9556b97bf030cabef8b9430aa4 Mon Sep 17 00:00:00 2001 From: Aldy Hernandez Date: Tue, 1 Jun 2021 17:48:30 +0200 Subject: [PATCH 2/2] Use known global ranges in export_global_ranges This patch modifies export_global_ranges to take into account current global ranges. It also handles enhances said function to export pointer global ranges as well. gcc/ChangeLog: * gimple-range.cc (gimple_ranger::export_global_ranges): Call update_global_range. * value-query.cc (update_global_range): New. * value-query.h (update_global_range): New. gcc/testsuite/ChangeLog: * gcc.dg/pr80776-1.c: XFAIL and document the reason why. --- gcc/gimple-range.cc | 26 ++++++++------------- gcc/testsuite/gcc.dg/pr80776-1.c | 12 +++++++++- gcc/value-query.cc | 39 ++++++++++++++++++++++++++++++++ gcc/value-query.h | 1 + 4 files changed, 61 insertions(+), 17 deletions(-) diff --git a/gcc/gimple-range.cc b/gcc/gimple-range.cc index ed0a0c9702b..af426207092 100644 --- a/gcc/gimple-range.cc +++ b/gcc/gimple-range.cc @@ -1115,7 +1115,7 @@ gimple_ranger::range_of_stmt (irange &r, gimple *s, tree name) } // This routine will export whatever global ranges are known to GCC -// SSA_RANGE_NAME_INFO fields. +// SSA_RANGE_NAME_INFO and SSA_NAME_PTR_INFO fields. void gimple_ranger::export_global_ranges () @@ -1136,24 +1136,18 @@ gimple_ranger::export_global_ranges () && m_cache.get_global_range (r, name) && !r.varying_p()) { - // Make sure the new range is a subset of the old range. - int_range_max old_range; - old_range = gimple_range_global (name); - old_range.intersect (r); - /* Disable this while we fix tree-ssa/pr61743-2.c. */ - //gcc_checking_assert (old_range == r); - - // WTF? Can't write non-null pointer ranges?? stupid set_range_info! - if (!POINTER_TYPE_P (TREE_TYPE (name)) && !r.undefined_p ()) + bool updated = update_global_range (r, name); + + if (updated && dump_file) { value_range vr = r; - set_range_info (name, vr); - if (dump_file) + print_generic_expr (dump_file, name , TDF_SLIM); + fprintf (dump_file, " --> "); + vr.dump (dump_file); + fprintf (dump_file, "\n"); + int_range_max same = vr; + if (same != r) { - print_generic_expr (dump_file, name , TDF_SLIM); - fprintf (dump_file, " --> "); - vr.dump (dump_file); - fprintf (dump_file, "\n"); fprintf (dump_file, " irange : "); r.dump (dump_file); fprintf (dump_file, "\n"); diff --git a/gcc/testsuite/gcc.dg/pr80776-1.c b/gcc/testsuite/gcc.dg/pr80776-1.c index f3a120b6744..eca5e805ae2 100644 --- a/gcc/testsuite/gcc.dg/pr80776-1.c +++ b/gcc/testsuite/gcc.dg/pr80776-1.c @@ -17,5 +17,15 @@ Foo (void) __builtin_unreachable (); if (! (0 <= i && i <= 999999)) __builtin_unreachable (); - sprintf (number, "%d", i); /* { dg-bogus "writing" "" } */ + + /* Legacy evrp sets the range of i to [0, MAX] *before* the first conditional, + and to [0,999999] *before* the second conditional. This is because both + evrp and VRP use trickery to set global ranges when this particular use of + a __builtin_unreachable is in play (see uses of + assert_unreachable_fallthru_edge_p). + + Setting these ranges at the definition site, causes VRP to remove the + unreachable code altogether, leaving the following sprintf unguarded. This + causes the bogus warning below. */ + sprintf (number, "%d", i); /* { dg-bogus "writing" "" { xfail *-*-* } } */ } diff --git a/gcc/value-query.cc b/gcc/value-query.cc index f8b457d362c..070d706166e 100644 --- a/gcc/value-query.cc +++ b/gcc/value-query.cc @@ -224,6 +224,45 @@ get_ssa_name_ptr_info_nonnull (const_tree name) return !pi->pt.null; } +// Update the global range for NAME into the SSA_RANGE_NAME_INFO and +// SSA_NAME_PTR_INFO fields. Return TRUE if the range for NAME was +// updated. + +bool +update_global_range (irange &r, tree name) +{ + tree type = TREE_TYPE (name); + + if (r.undefined_p () || r.varying_p ()) + return false; + + if (INTEGRAL_TYPE_P (type)) + { + // If a global range already exists, incorporate it. + if (SSA_NAME_RANGE_INFO (name)) + { + value_range glob; + get_ssa_name_range_info (glob, name); + r.intersect (glob); + } + if (r.undefined_p ()) + return false; + + value_range vr = r; + set_range_info (name, vr); + return true; + } + else if (POINTER_TYPE_P (type)) + { + if (r.nonzero_p ()) + { + set_ptr_nonnull (name); + return true; + } + } + return false; +} + // Return the legacy global range for NAME if it has one, otherwise // return VARYING. diff --git a/gcc/value-query.h b/gcc/value-query.h index 97da6637747..d0512e40c5a 100644 --- a/gcc/value-query.h +++ b/gcc/value-query.h @@ -115,5 +115,6 @@ public: extern global_range_query global_ranges; extern value_range gimple_range_global (tree name); +extern bool update_global_range (irange &r, tree name); #endif // GCC_QUERY_H -- 2.31.1