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 69CCD3858024 for ; Mon, 14 Feb 2022 15:58:13 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 69CCD3858024 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-189-AnyZFXLHP4CWdb2wuhMoFg-1; Mon, 14 Feb 2022 10:58:10 -0500 X-MC-Unique: AnyZFXLHP4CWdb2wuhMoFg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 015912F25 for ; Mon, 14 Feb 2022 15:58:10 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.16.48]) by smtp.corp.redhat.com (Postfix) with ESMTP id 43F1C7CD68; Mon, 14 Feb 2022 15:57:59 +0000 (UTC) From: David Malcolm To: gcc@gcc.gnu.org Cc: Mark Wielaard , David Malcolm Subject: Uninit warnings due to optimizing short-circuit conditionals Date: Mon, 14 Feb 2022 10:57:57 -0500 Message-Id: <20220214155757.861877-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.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_DNSWL_LOW, SPF_HELO_NONE, SPF_NONE, 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@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Feb 2022 15:58:15 -0000 [CCing Mark in the hopes of insight from the valgrind side of things] There is a false positive from -Wanalyzer-use-of-uninitialized-value on gcc.dg/analyzer/pr102692.c here: ‘fix_overlays_before’: events 1-3 | | 75 | while (tail | | ~~~~ | 76 | && (tem = make_lisp_ptr (tail, 5), | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (1) following ‘false’ branch (when ‘tail’ is NULL)... | 77 | (end = marker_position (XOVERLAY (tem)->end)) >= pos)) | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |...... | 82 | if (!tail || end < prev || !tail->next) | | ~~~~~ ~~~~~~~~~~ | | | | | | | (3) use of uninitialized value ‘end’ here | | (2) ...to here | The issue is that inner || of the conditionals have been folded within the frontend from a chain of control flow: 5 │ if (tail == 0B) goto ; else goto ; 6 │ : 7 │ if (end < prev) goto ; else goto ; 8 │ : 9 │ _1 = tail->next; 10 │ if (_1 == 0B) goto ; else goto ; 11 │ : to an OR expr (and then to a bitwise-or by the gimplifier): 5 │ _1 = tail == 0B; 6 │ _2 = end < prev; 7 │ _3 = _1 | _2; 8 │ if (_3 != 0) goto ; else goto ; 9 │ : 10 │ _4 = tail->next; 11 │ if (_4 == 0B) goto ; else goto ; This happens for sufficiently simple conditionals in fold_truth_andor. In particular, the (end < prev) is short-circuited without optimization, but is evaluated with optimization, leading to the false positive. Given how early this folding occurs, it seems the simplest fix is to try to detect places where this optimization appears to have happened, and suppress uninit warnings within the statement that would have been short-circuited (and thus e.g. ignoring them when evaluating _2 above for the case where _1 is known to be true at the (_1 | _2) , and thus _2 being redundant). Attached is a patch that implements this. There are some more details in the patch, but I'm wondering if this is a known problem, and how e.g. valgrind copes with such code. My patch feels like something of a hack, but I'm not sure of any other way around it given that the conditional is folded directly within the frontend. I've successfully bootstrapped & regrtested the patch on x86_64-pc-linux-gnu; it fixes the false positive. Thoughts? Thanks for reading Dave analyzer: fix uninit false +ve due to optimized conditionals [PR102692] gcc/analyzer/ChangeLog: PR analyzer/102692 * exploded-graph.h (impl_region_model_context::get_stmt): New. * region-model.cc: Include "gimple-ssa.h", "tree-phinodes.h", "tree-ssa-operands.h", and "ssa-iterators.h". (within_short_circuited_stmt_p): New. (region_model::check_for_poison): Don't warn about uninit values if within_short_circuited_stmt_p. * region-model.h (region_model_context::get_stmt): New vfunc. (noop_region_model_context::get_stmt): New. gcc/testsuite/ChangeLog: PR analyzer/102692 * gcc.dg/analyzer/pr102692-2.c: New test. * gcc.dg/analyzer/pr102692.c: Remove xfail. Remove -O2 from options and move to... * gcc.dg/analyzer/torture/pr102692.c: ...here. Signed-off-by: David Malcolm --- gcc/analyzer/exploded-graph.h | 2 + gcc/analyzer/region-model.cc | 112 ++++++++++++++++++ gcc/analyzer/region-model.h | 5 + gcc/testsuite/gcc.dg/analyzer/pr102692-2.c | 22 ++++ .../gcc.dg/analyzer/{ => torture}/pr102692.c | 4 +- 5 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr102692-2.c rename gcc/testsuite/gcc.dg/analyzer/{ => torture}/pr102692.c (94%) diff --git a/gcc/analyzer/exploded-graph.h b/gcc/analyzer/exploded-graph.h index 1854193c65b..1f52725dc98 100644 --- a/gcc/analyzer/exploded-graph.h +++ b/gcc/analyzer/exploded-graph.h @@ -90,6 +90,8 @@ class impl_region_model_context : public region_model_context const state_machine **out_sm, unsigned *out_sm_idx) FINAL OVERRIDE; + const gimple *get_stmt () const OVERRIDE { return m_stmt; } + exploded_graph *m_eg; log_user m_logger; exploded_node *m_enode_for_diag; diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index e659cf03a86..8a509c44178 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -68,6 +68,11 @@ along with GCC; see the file COPYING3. If not see #include "stor-layout.h" #include "attribs.h" #include "tree-object-size.h" +#include "gimple-ssa.h" +#include "tree-phinodes.h" +#include "tree-ssa-operands.h" +#include "ssa-iterators.h" +#include "gimple-pretty-print.h" #if ENABLE_ANALYZER @@ -829,6 +834,108 @@ region_model::get_gassign_result (const gassign *assign, } } +/* Workaround for discarding certain false positives from + -Wanalyzer-use-of-uninitialized-value + of the form: + ((A OR-IF B) OR-IF C) + and: + ((A AND-IF B) AND-IF C) + where evaluating B is redundant, but could involve simple accesses of + uninitialized locals. + + When optimization is turned on the FE can immediately fold compound + conditionals. Specifically, c_parser_condition parses this condition: + ((A OR-IF B) OR-IF C) + and calls c_fully_fold on the condition. + Within c_fully_fold, fold_truth_andor is called, which bails when + optimization is off, but if any optimization is turned on can convert the + ((A OR-IF B) OR-IF C) + into: + ((A OR B) OR_IF C) + for sufficiently simple B + i.e. the inner OR-IF becomes an OR. + At gimplification time the inner OR becomes BIT_IOR_EXPR (in gimplify_expr), + giving this for the inner condition: + tmp = A | B; + if (tmp) + thus effectively synthesizing a redundant access of B when optimization + is turned on, when compared to: + if (A) goto L1; else goto L4; + L1: if (B) goto L2; else goto L4; + L2: if (C) goto L3; else goto L4; + for the unoptimized case. + + Return true if CTXT appears to be handling such a short-circuitable stmt, + such as the def-stmt for B for the: + tmp = A | B; + case above, for the case where A is true and thus B would have been + short-circuited without optimization, using MODEL for the value of A. */ + +static bool +within_short_circuited_stmt_p (const region_model *model, + region_model_context *ctxt) +{ + gcc_assert (ctxt); + const gimple *curr_stmt = ctxt->get_stmt (); + if (curr_stmt == NULL) + return false; + + /* We must have an assignment to a temporary of _Bool type. */ + const gassign *assign_stmt = dyn_cast (curr_stmt); + if (!assign_stmt) + return false; + tree lhs = gimple_assign_lhs (assign_stmt); + if (TREE_TYPE (lhs) != boolean_type_node) + return false; + if (TREE_CODE (lhs) != SSA_NAME) + return false; + if (SSA_NAME_VAR (lhs) != NULL_TREE) + return false; + + /* The temporary bool must be used exactly once: as the second arg of + a BIT_IOR_EXPR or BIT_AND_EXPR. */ + use_operand_p use_op; + gimple *use_stmt; + if (!single_imm_use (lhs, &use_op, &use_stmt)) + return false; + const gassign *use_assign = dyn_cast (use_stmt); + if (!use_assign) + return false; + enum tree_code op = gimple_assign_rhs_code (use_assign); + if (!(op == BIT_IOR_EXPR ||op == BIT_AND_EXPR)) + return false; + if (!(gimple_assign_rhs1 (use_assign) != lhs + && gimple_assign_rhs2 (use_assign) == lhs)) + return false; + + /* The first arg of the bitwise stmt must have a known value in MODEL + that implies that the value of the second arg doesn't matter, i.e. + 1 for bitwise or, 0 for bitwise and. */ + tree other_arg = gimple_assign_rhs1 (use_assign); + /* Use a NULL ctxt here to avoid generating warnings. */ + const svalue *other_arg_sval = model->get_rvalue (other_arg, NULL); + tree other_arg_cst = other_arg_sval->maybe_get_constant (); + if (!other_arg_cst) + return false; + switch (op) + { + default: + gcc_unreachable (); + case BIT_IOR_EXPR: + if (zerop (other_arg_cst)) + return false; + break; + case BIT_AND_EXPR: + if (!zerop (other_arg_cst)) + return false; + break; + } + + /* All tests passed. We appear to be in a stmt that generates a boolean + temporary with a value that won't matter. */ + return true; +} + /* Check for SVAL being poisoned, adding a warning to CTXT. Return SVAL, or, if a warning is added, another value, to avoid repeatedly complaining about the same poisoned value in followup code. */ @@ -852,6 +959,11 @@ region_model::check_for_poison (const svalue *sval, && is_empty_type (sval->get_type ())) return sval; + /* Special case to avoid certain false positives. */ + if (pkind == POISON_KIND_UNINIT + && within_short_circuited_stmt_p (this, ctxt)) + return sval; + /* If we have an SSA name for a temporary, we don't want to print ''. Poisoned values are shared by type, and so we can't reconstruct diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 46cf37e6b26..c2c89a20d50 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -930,6 +930,9 @@ class region_model_context virtual bool get_taint_map (sm_state_map **out_smap, const state_machine **out_sm, unsigned *out_sm_idx) = 0; + + /* Get the current statement, if any. */ + virtual const gimple *get_stmt () const = 0; }; /* A "do nothing" subclass of region_model_context. */ @@ -980,6 +983,8 @@ public: { return false; } + + const gimple *get_stmt () const OVERRIDE { return NULL; } }; /* A subclass of region_model_context for determining if operations fail diff --git a/gcc/testsuite/gcc.dg/analyzer/pr102692-2.c b/gcc/testsuite/gcc.dg/analyzer/pr102692-2.c new file mode 100644 index 00000000000..c72fde21744 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/pr102692-2.c @@ -0,0 +1,22 @@ +/* { dg-additional-options "-O1" } */ + +struct Lisp_Overlay +{ + struct Lisp_Overlay *next; +}; + +void +test_1 (struct Lisp_Overlay *tail, long prev) +{ + long end; + if (!tail || end < prev || !tail->next) /* { dg-warning "use of uninitialized value 'end'" } */ + return; +} + +void +test_2 (struct Lisp_Overlay *tail, long prev) +{ + long end; + if (tail && end < prev && !tail->next) /* { dg-warning "use of uninitialized value 'end'" } */ + return; +} diff --git a/gcc/testsuite/gcc.dg/analyzer/pr102692.c b/gcc/testsuite/gcc.dg/analyzer/torture/pr102692.c similarity index 94% rename from gcc/testsuite/gcc.dg/analyzer/pr102692.c rename to gcc/testsuite/gcc.dg/analyzer/torture/pr102692.c index c8993c82980..a6c6bc47896 100644 --- a/gcc/testsuite/gcc.dg/analyzer/pr102692.c +++ b/gcc/testsuite/gcc.dg/analyzer/torture/pr102692.c @@ -1,4 +1,4 @@ -/* { dg-additional-options "-O2 -Wno-analyzer-too-complex" } */ +/* { dg-additional-options "-Wno-analyzer-too-complex" } */ /* TODO: remove the need for -Wno-analyzer-too-complex. */ struct lisp; @@ -73,7 +73,7 @@ fix_overlays_before (struct buffer *bp, long prev, long pos) parent = tail; tail = tail->next; } - if (!tail || end < prev || !tail->next) /* { dg-bogus "use of uninitialized value 'end'" "uninit" { xfail *-*-* } } */ + if (!tail || end < prev || !tail->next) /* { dg-bogus "use of uninitialized value 'end'" "uninit" } */ /* { dg-bogus "dereference of NULL 'tail'" "null deref" { target *-*-* } .-1 } */ return; right_pair = parent; -- 2.26.3