From 1e2fe6715a949f80c1204ae244baad3cd80ffaf0 Mon Sep 17 00:00:00 2001 From: David Malcolm Date: Fri, 11 Feb 2022 16:43:21 -0500 Subject: [PATCH] analyzer: fix uninit false +ve due to optimized conditionals [PR102692] MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit There is 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. 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 | 111 +++++++++++++++++++++ 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, 142 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 1854193..1f52725 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 e659cf0..69e8fa7 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -68,6 +68,10 @@ 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" #if ENABLE_ANALYZER @@ -829,6 +833,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 +958,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 46cf37e..c2c89a2 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 0000000..c72fde2 --- /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 c8993c8..a6c6bc4 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.7.4