From patchwork Wed Mar 4 15:57:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 1249089 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=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-520633-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.a=rsa-sha1 header.s=default header.b=Qo0F4LLM; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=QWSLEUdP; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 48Xdnh512bz9sR4 for ; Thu, 5 Mar 2020 02:58:00 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:mime-version:content-type :content-transfer-encoding; q=dns; s=default; b=ovj6UVr/xi9K6J9z GMvbcJDOQHTROKk2MhgFxqTLUevPZPGburScfryZw2tdTgEVtdAinwsmQv1L8Te6 JzwxKFdyn6qytMwm3/qNAML4ejfM2tRT/1Q0eORmq4CgRfAsXtMYXEcff2ym1Lxr /YvxozcgpNEZjWpyEpCLTa46Smk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:mime-version:content-type :content-transfer-encoding; s=default; bh=1HxXJLF7cC94xZv4bc1h7N 3/oXk=; b=Qo0F4LLMZN+8Tu1oS/Rp442A2IO+kXtoXKh4gY5YfAoTG6+voMGK5l R1ZaxNeOItVYfrgPpJ9W/b++sm7tQsXHHgwQnWycTnosOYsW9dBiolVmjm8YwKZz I//FqdCPJ9YqDqdsOaB/NKzABfKvHLssUfRCRGysOlXrEw/BEX1A4= Received: (qmail 105003 invoked by alias); 4 Mar 2020 15:57:48 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 104986 invoked by uid 89); 4 Mar 2020 15:57:48 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-23.4 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE autolearn=unavailable version=3.3.1 spammy=logger, sid X-HELO: us-smtp-1.mimecast.com Received: from us-smtp-delivery-1.mimecast.com (HELO us-smtp-1.mimecast.com) (207.211.31.120) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 04 Mar 2020 15:57:46 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1583337464; 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; bh=wawGHWmz5/QfGZGjGSTbl3tQCEk6LaEBrZrKpaTkvs4=; b=QWSLEUdPoXDfaVkxt+thuOAPKJ23TINdhL2cuJs/kDQ7Zn+ERYxk9nTcvXD9uUqGW8+pL+ BLYcVVR+63jSFB/8DNL4cmVEXBCnLns+WwMOZO7AUY4HTb1aSEOkkL/2Pq7WuzbagFuTDP ddAlBVBX9sxYZNsLtq//6dia2wIsPxQ= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-465-sguH6qwvM1i-GsnNbH2GZA-1; Wed, 04 Mar 2020 10:57:42 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 83A388C474F; Wed, 4 Mar 2020 15:57:41 +0000 (UTC) Received: from t470.redhat.com (ovpn-116-56.phx2.redhat.com [10.3.116.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id E7D165DA7B; Wed, 4 Mar 2020 15:57:40 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org Cc: David Malcolm Subject: [committed] analyzer: fix ICE on non-lvalue in prune_for_sm_diagnostic [PR93993] Date: Wed, 4 Mar 2020 10:57:36 -0500 Message-Id: <20200304155736.4946-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-IsSubscribed: yes PR analyzer/93993 reports another ICE within diagnostic_manager::prune_for_sm_diagnostic in which the expression of interest becomes a non-lvalue (similar to PR 93544, PR 93647, and PR 93950), due to attempting to get an lvalue for a non-lvalue with a NULL context, leading to an ICE when the failure is reported to make_region_for_unexpected_tree_code. The tree in question is an ADDR_EXPR of a VAR_DECL, due to: event 11: switching var of interest from ‘tm’ in callee to ‘&qb’ in caller This patch adds more bulletproofing to the routine by introducing a tentative_region_model_context class that can be passed in such circumstances which records that an error occurred, and then checking to see if an error was recorded, thus avoiding the ICE. This is papering over the problem, but a better solution seems more like stage 1 material. The patch also refactors the error-checking for CONSTANT_CLASS_P. The testcase pr93993.f90 has a false positive: pr93993.f90:19:0: 19 | allocate (tm) ! { dg-warning "dereference of possibly-NULL" } | Warning: dereference of possibly-NULL ‘_6’ [CWE-690] [-Wanalyzer-possible-null-dereference] which appears to be a pre-existing bug affecting any allocate call in Fortran, which I will fix in a followup. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to master as r10-7023-g3d66e153b40ed000af30a9e569a05f34d5d576aa. gcc/analyzer/ChangeLog: PR analyzer/93993 * checker-path.h (state_change_event::get_lvalue): Add ctxt param and pass it to region_model::get_value call. * diagnostic-manager.cc (get_any_origin): Pass a tentative_region_model_context to the calls to get_lvalue and reject the comparison if errors occur. (can_be_expr_of_interest_p): New function. (diagnostic_manager::prune_for_sm_diagnostic): Replace checks for CONSTANT_CLASS_P with calls to update_for_unsuitable_sm_exprs. Pass a tentative_region_model_context to the calls to state_change_event::get_lvalue and reject the comparison if errors occur. (diagnostic_manager::update_for_unsuitable_sm_exprs): New. * diagnostic-manager.h (diagnostic_manager::update_for_unsuitable_sm_exprs): New decl. * region-model.h (class tentative_region_model_context): New class. gcc/testsuite/ChangeLog: PR analyzer/93993 * gfortran.dg/analyzer/pr93993.f90: New test. --- gcc/analyzer/checker-path.h | 4 +- gcc/analyzer/diagnostic-manager.cc | 90 ++++++++++++------- gcc/analyzer/diagnostic-manager.h | 1 + gcc/analyzer/region-model.h | 48 ++++++++++ .../gfortran.dg/analyzer/pr93993.f90 | 33 +++++++ 5 files changed, 142 insertions(+), 34 deletions(-) create mode 100644 gcc/testsuite/gfortran.dg/analyzer/pr93993.f90 diff --git a/gcc/analyzer/checker-path.h b/gcc/analyzer/checker-path.h index 30cb43c13ba..2eead25f058 100644 --- a/gcc/analyzer/checker-path.h +++ b/gcc/analyzer/checker-path.h @@ -201,9 +201,9 @@ public: label_text get_desc (bool can_colorize) const FINAL OVERRIDE; - region_id get_lvalue (tree expr) const + region_id get_lvalue (tree expr, region_model_context *ctxt) const { - return m_dst_state.m_region_model->get_lvalue (expr, NULL); + return m_dst_state.m_region_model->get_lvalue (expr, ctxt); } const supernode *m_node; diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index 7435092e2d7..1b2c3ce68fa 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -574,9 +574,14 @@ get_any_origin (const gimple *stmt, if (const gassign *assign = dyn_cast (stmt)) { tree lhs = gimple_assign_lhs (assign); - /* Use region IDs to compare lhs with DST_REP. */ - if (dst_state.m_region_model->get_lvalue (lhs, NULL) - == dst_state.m_region_model->get_lvalue (dst_rep, NULL)) + /* Use region IDs to compare lhs with DST_REP, bulletproofing against + cases where they can't have lvalues by using + tentative_region_model_context. */ + tentative_region_model_context ctxt; + region_id lhs_rid = dst_state.m_region_model->get_lvalue (lhs, &ctxt); + region_id dst_rep_rid + = dst_state.m_region_model->get_lvalue (dst_rep, &ctxt); + if (lhs_rid == dst_rep_rid && !ctxt.had_errors_p ()) { tree rhs1 = gimple_assign_rhs1 (assign); enum tree_code op = gimple_assign_rhs_code (assign); @@ -1059,6 +1064,25 @@ diagnostic_manager::prune_path (checker_path *path, path->maybe_log (get_logger (), "pruned"); } +/* A cheap test to determine if EXPR can be the expression of interest in + an sm-diagnostic, so that we can reject cases where we have a non-lvalue. + We don't have always have a model when calling this, so we can't use + tentative_region_model_context, so there can be false positives. */ + +static bool +can_be_expr_of_interest_p (tree expr) +{ + if (!expr) + return false; + + /* Reject constants. */ + if (CONSTANT_CLASS_P (expr)) + return false; + + /* Otherwise assume that it can be an lvalue. */ + return true; +} + /* First pass of diagnostic_manager::prune_path: apply verbosity level, pruning unrelated state change events. @@ -1081,11 +1105,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, tree var, state_machine::state_t state) const { - /* If we have a constant (such as NULL), assume its state is also - constant, so as not to attempt to get its lvalue whilst tracking the - origin of the state. */ - if (var && CONSTANT_CLASS_P (var)) - var = NULL_TREE; + update_for_unsuitable_sm_exprs (&var); int idx = path->num_events () - 1; while (idx >= 0 && idx < (signed)path->num_events ()) @@ -1105,7 +1125,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, else log ("considering event %i", idx); } - gcc_assert (var == NULL || !CONSTANT_CLASS_P (var)); + gcc_assert (var == NULL || can_be_expr_of_interest_p (var)); switch (base_event->m_kind) { default: @@ -1157,19 +1177,21 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, case EK_STATE_CHANGE: { state_change_event *state_change = (state_change_event *)base_event; - if (state_change->get_lvalue (state_change->m_var) - == state_change->get_lvalue (var)) + /* Use region IDs to compare var with the state_change's m_var, + bulletproofing against cases where they can't have lvalues by + using tentative_region_model_context. */ + tentative_region_model_context ctxt; + region_id state_var_rid + = state_change->get_lvalue (state_change->m_var, &ctxt); + region_id var_rid = state_change->get_lvalue (var, &ctxt); + if (state_var_rid == var_rid && !ctxt.had_errors_p ()) { if (state_change->m_origin) { log ("event %i: switching var of interest from %qE to %qE", idx, var, state_change->m_origin); var = state_change->m_origin; - if (var && CONSTANT_CLASS_P (var)) - { - log ("new var is a constant; setting var to NULL"); - var = NULL_TREE; - } + update_for_unsuitable_sm_exprs (&var); } log ("event %i: switching state of interest from %qs to %qs", idx, sm->get_state_name (state_change->m_to), @@ -1185,6 +1207,8 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, else log ("filtering event %i: state change to %qE", idx, state_change->m_var); + if (ctxt.had_errors_p ()) + log ("context had errors"); path->delete_event (idx); } } @@ -1218,12 +1242,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, /* If we've chosen a bad exploded_path, then the phi arg might be a constant. Fail gracefully for this case. */ - if (CONSTANT_CLASS_P (var)) - { - log ("new var is a constant (bad path?);" - " setting var to NULL"); - var = NULL; - } + update_for_unsuitable_sm_exprs (&var); } } @@ -1266,11 +1285,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, var = caller_var; if (expr.param_p ()) event->record_critical_state (var, state); - if (var && CONSTANT_CLASS_P (var)) - { - log ("new var is a constant; setting var to NULL"); - var = NULL_TREE; - } + update_for_unsuitable_sm_exprs (&var); } } break; @@ -1296,11 +1311,7 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, var = callee_var; if (expr.return_value_p ()) event->record_critical_state (var, state); - if (var && CONSTANT_CLASS_P (var)) - { - log ("new var is a constant; setting var to NULL"); - var = NULL_TREE; - } + update_for_unsuitable_sm_exprs (&var); } } } @@ -1321,6 +1332,21 @@ diagnostic_manager::prune_for_sm_diagnostic (checker_path *path, } } +/* Subroutine of diagnostic_manager::prune_for_sm_diagnostic. + If *EXPR is not suitable to be the expression of interest in + an sm-diagnostic, set *EXPR to NULL and log. */ + +void +diagnostic_manager::update_for_unsuitable_sm_exprs (tree *expr) const +{ + gcc_assert (expr); + if (*expr && !can_be_expr_of_interest_p (*expr)) + { + log ("new var %qE is unsuitable; setting var to NULL", *expr); + *expr = NULL_TREE; + } +} + /* Second pass of diagnostic_manager::prune_path: remove redundant interprocedural information. diff --git a/gcc/analyzer/diagnostic-manager.h b/gcc/analyzer/diagnostic-manager.h index f32ca75e279..1c7bc7462af 100644 --- a/gcc/analyzer/diagnostic-manager.h +++ b/gcc/analyzer/diagnostic-manager.h @@ -126,6 +126,7 @@ private: const state_machine *sm, tree var, state_machine::state_t state) const; + void update_for_unsuitable_sm_exprs (tree *expr) const; void prune_interproc_events (checker_path *path) const; void finish_pruning (checker_path *path) const; diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index c185eb18d0b..f3cf45566d1 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -1955,6 +1955,54 @@ class region_model_context const dump_location_t &loc) = 0; }; +/* A subclass of region_model_context for determining if operations fail + e.g. "can we generate a region for the lvalue of EXPR?". */ + +class tentative_region_model_context : public region_model_context +{ +public: + tentative_region_model_context () : m_num_unexpected_codes (0) {} + + void warn (pending_diagnostic *) FINAL OVERRIDE {} + void remap_svalue_ids (const svalue_id_map &) FINAL OVERRIDE {} + int on_svalue_purge (svalue_id, const svalue_id_map &) FINAL OVERRIDE + { + return 0; + } + logger *get_logger () FINAL OVERRIDE { return NULL; } + void on_inherited_svalue (svalue_id parent_sid ATTRIBUTE_UNUSED, + svalue_id child_sid ATTRIBUTE_UNUSED) + FINAL OVERRIDE + { + } + void on_cast (svalue_id src_sid ATTRIBUTE_UNUSED, + svalue_id dst_sid ATTRIBUTE_UNUSED) FINAL OVERRIDE + { + } + void on_condition (tree lhs ATTRIBUTE_UNUSED, + enum tree_code op ATTRIBUTE_UNUSED, + tree rhs ATTRIBUTE_UNUSED) FINAL OVERRIDE + { + } + void on_unknown_change (svalue_id sid ATTRIBUTE_UNUSED) FINAL OVERRIDE + { + } + void on_phi (const gphi *phi ATTRIBUTE_UNUSED, + tree rhs ATTRIBUTE_UNUSED) FINAL OVERRIDE + { + } + void on_unexpected_tree_code (tree, const dump_location_t &) + FINAL OVERRIDE + { + m_num_unexpected_codes++; + } + + bool had_errors_p () const { return m_num_unexpected_codes > 0; } + +private: + int m_num_unexpected_codes; +}; + /* A bundle of data for use when attempting to merge two region_model instances to make a third. */ diff --git a/gcc/testsuite/gfortran.dg/analyzer/pr93993.f90 b/gcc/testsuite/gfortran.dg/analyzer/pr93993.f90 new file mode 100644 index 00000000000..8d5261c0eb9 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/analyzer/pr93993.f90 @@ -0,0 +1,33 @@ +module np + implicit none + integer, parameter :: za = selected_real_kind(15, 307) +end module np + +module gg + use np + + type et(real_kind) + integer, kind :: real_kind + end type et + +contains + + function hv (tm) result(ce) + type (et(real_kind=za)), allocatable, target :: tm + type (et(real_kind=za)), pointer :: ce + + allocate (tm) ! { dg-bogus "dereference of possibly-NULL" "" { xfail *-*-* } } + ce => tm + end function hv ! { dg-warning "leak of 'tm'" } + +end module gg + +program a5 + use np + use gg + implicit none + type (et(real_kind=za)), allocatable :: qb + type (et(real_kind=za)), pointer :: vt + + vt => hv (qb) +end program a5