[gcc r12-7251] analyzer: fix uninit false +ve due to optimized conditionals [PR102692]

[gcc r12-7251] analyzer: fix uninit false +ve due to optimized conditionals [PR102692]

[David Malcolm]

https://gcc.gnu.org/g:1e2fe6715a949f80c1204ae244baad3cd80ffaf0

commit r12-7251-g1e2fe6715a949f80c1204ae244baad3cd80ffaf0
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Fri Feb 11 16:43:21 2022 -0500

    analyzer: fix uninit false +ve due to optimized conditionals [PR102692]
    
    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 <D.1986>; else goto <D.1988>;
       6   │   <D.1988>:
       7   │   if (end < prev) goto <D.1986>; else goto <D.1989>;
       8   │   <D.1989>:
       9   │   _1 = tail->next;
      10   │   if (_1 == 0B) goto <D.1986>; else goto <D.1987>;
      11   │   <D.1986>:
    
    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 <D.1986>; else goto <D.1988>;
       9   │   <D.1988>:
      10   │   _4 = tail->next;
      11   │   if (_4 == 0B) goto <D.1986>; else goto <D.1987>;
    
    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 <dmalcolm@redhat.com>

Diff:
---
 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(-)

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..69e8fa7d1e3 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 <const gassign *> (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 <const gassign *> (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
 	 '<unknown>'.
 	 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;