diff mbox series

[RFC] tree-optimization/105646 - re-interpret always executed in uninit diag

Message ID 20220822061649.3AF011332D@imap2.suse-dmz.suse.de
State New
Headers
Series [RFC] tree-optimization/105646 - re-interpret always executed in uninit diag |

Commit Message

Richard Biener Aug. 22, 2022, 6:16 a.m. UTC 
  The following fixes PR105646, not diagnosing

int f1();
int f3(){
    auto const & a = f1();
    bool v3{v3};
    return a;
}

with optimization because the early uninit diagnostic pass only
diagnoses always executed cases.  The patch does this by
re-interpreting what always executed means and choosing to
ignore exceptional and abnormal control flow for this.  At the
same time it improves things as suggested in a comment - when
the value-numbering run done without optimizing figures there's
a fallthru path, consider blocks on it as always executed.

Bootstrapped and tested on x86_64-unknown-linux-gnu.

OK?

Thanks,
Richard.

	PR tree-optimization/105646
	* tree-ssa-uninit.cc (warn_uninitialized_vars): Pre-compute
	the set of fallthru reachable blocks from function entry
	and use that to determine wlims.always_executed.

	* g++.dg/uninit-pr105646.C: New testcase.
---
 gcc/testsuite/g++.dg/uninit-pr105646.C | 17 +++++++++
 gcc/tree-ssa-uninit.cc                 | 53 +++++++++++++++++++++-----
 2 files changed, 61 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/uninit-pr105646.C

Comments

Jeff Law Sept. 27, 2022, 3:47 p.m. UTC | #1 
On 8/22/22 00:16, Richard Biener via Gcc-patches wrote:
> The following fixes PR105646, not diagnosing
>
> int f1();
> int f3(){
>      auto const & a = f1();
>      bool v3{v3};
>      return a;
> }
>
> with optimization because the early uninit diagnostic pass only
> diagnoses always executed cases.  The patch does this by
> re-interpreting what always executed means and choosing to
> ignore exceptional and abnormal control flow for this.  At the
> same time it improves things as suggested in a comment - when
> the value-numbering run done without optimizing figures there's
> a fallthru path, consider blocks on it as always executed.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
>
> OK?
>
> Thanks,
> Richard.
>
> 	PR tree-optimization/105646
> 	* tree-ssa-uninit.cc (warn_uninitialized_vars): Pre-compute
> 	the set of fallthru reachable blocks from function entry
> 	and use that to determine wlims.always_executed.
>
> 	* g++.dg/uninit-pr105646.C: New testcase.

I'm torn on this.  On one hand, ignoring abnormal flow control in the 
early pass is almost certainly going to result in false positives but 
it's also going to result in fixing some false negatives.

I'm willing to ACK and see what the real world fallout is in the spring 
when the distros run their builds.  Your call.


Jeff
Richard Biener Sept. 29, 2022, 8:25 a.m. UTC | #2 
On Tue, Sep 27, 2022 at 5:48 PM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> On 8/22/22 00:16, Richard Biener via Gcc-patches wrote:
> > The following fixes PR105646, not diagnosing
> >
> > int f1();
> > int f3(){
> >      auto const & a = f1();
> >      bool v3{v3};
> >      return a;
> > }
> >
> > with optimization because the early uninit diagnostic pass only
> > diagnoses always executed cases.  The patch does this by
> > re-interpreting what always executed means and choosing to
> > ignore exceptional and abnormal control flow for this.  At the
> > same time it improves things as suggested in a comment - when
> > the value-numbering run done without optimizing figures there's
> > a fallthru path, consider blocks on it as always executed.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >
> > OK?
> >
> > Thanks,
> > Richard.
> >
> >       PR tree-optimization/105646
> >       * tree-ssa-uninit.cc (warn_uninitialized_vars): Pre-compute
> >       the set of fallthru reachable blocks from function entry
> >       and use that to determine wlims.always_executed.
> >
> >       * g++.dg/uninit-pr105646.C: New testcase.
>
> I'm torn on this.  On one hand, ignoring abnormal flow control in the
> early pass is almost certainly going to result in false positives but
> it's also going to result in fixing some false negatives.
>
> I'm willing to ACK and see what the real world fallout is in the spring
> when the distros run their builds.  Your call.

I have pushed this now after retesting.  Let's see if there's any bad
fallout - we can certainly reconsider.

Richard.

>
> Jeff
>
>
diff mbox series

Patch

diff --git a/gcc/testsuite/g++.dg/uninit-pr105646.C b/gcc/testsuite/g++.dg/uninit-pr105646.C
new file mode 100644
index 00000000000..48ceb986ec6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/uninit-pr105646.C
@@ -0,0 +1,17 @@ 
+// { dg-do compile }
+// { dg-require-effective-target c++11 }
+// { dg-options "-O2 -Wuninitialized" }
+
+int f1();
+int f2(){
+    bool v2{v2}; // { dg-warning "is used uninitialized" }
+    auto const & a = f1();
+    return a;
+}
+int f3(){
+    auto const & a = f1();
+    // Diagnose the following when optimizing and as unconditional
+    // uninitialized use despite f1 possibly throwing
+    bool v3{v3}; // { dg-warning "is used uninitialized" }
+    return a;
+}
diff --git a/gcc/tree-ssa-uninit.cc b/gcc/tree-ssa-uninit.cc
index 7074c9117b2..b687e24a7e6 100644
--- a/gcc/tree-ssa-uninit.cc
+++ b/gcc/tree-ssa-uninit.cc
@@ -987,10 +987,49 @@  warn_uninitialized_vars (bool wmaybe_uninit)
   wlimits wlims = { };
   wlims.wmaybe_uninit = wmaybe_uninit;
 
-  gimple_stmt_iterator gsi;
-  basic_block bb;
+  auto_bb_flag ft_reachable (cfun);
+
+  /* Mark blocks that are always executed when we ignore provably
+     not executed and EH and abnormal edges.  */
+  basic_block bb = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
+  while (!(bb->flags & ft_reachable))
+    {
+      bb->flags |= ft_reachable;
+      edge e = find_fallthru_edge (bb->succs);
+      if (e && e->flags & EDGE_EXECUTABLE)
+	{
+	  bb = e->dest;
+	  continue;
+	}
+      /* Find a single executable edge.  */
+      edge_iterator ei;
+      edge ee = NULL;
+      FOR_EACH_EDGE (e, ei, bb->succs)
+	if (e->flags & EDGE_EXECUTABLE)
+	  {
+	    if (!ee)
+	      ee = e;
+	    else
+	      {
+		ee = NULL;
+		break;
+	      }
+	  }
+      if (ee)
+	bb = ee->dest;
+      else
+	{
+	  bb = get_immediate_dominator (CDI_POST_DOMINATORS, bb);
+	  if (!bb || bb->index == EXIT_BLOCK)
+	    break;
+	}
+    }
+
   FOR_EACH_BB_FN (bb, cfun)
     {
+      wlims.always_executed = (bb->flags & ft_reachable);
+      bb->flags &= ~ft_reachable;
+
       edge_iterator ei;
       edge e;
       FOR_EACH_EDGE (e, ei, bb->preds)
@@ -1001,14 +1040,10 @@  warn_uninitialized_vars (bool wmaybe_uninit)
       if (!e)
 	continue;
 
-      basic_block succ = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
-      /* ???  This could be improved when we use a greedy walk and have
-	 some edges marked as not executable.  */
-      wlims.always_executed = dominated_by_p (CDI_POST_DOMINATORS, succ, bb);
-
       if (wlims.always_executed)
 	warn_uninit_phi_uses (bb);
 
+      gimple_stmt_iterator gsi;
       for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
 	{
 	  gimple *stmt = gsi_stmt (gsi);
@@ -1029,7 +1064,7 @@  warn_uninitialized_vars (bool wmaybe_uninit)
 	  FOR_EACH_SSA_USE_OPERAND (use_p, stmt, op_iter, SSA_OP_USE)
 	    {
 	      /* BIT_INSERT_EXPR first operand should not be considered
-	         a use for the purpose of uninit warnings.  */
+		 a use for the purpose of uninit warnings.  */
 	      if (gassign *ass = dyn_cast <gassign *> (stmt))
 		{
 		  if (gimple_assign_rhs_code (ass) == BIT_INSERT_EXPR
@@ -1040,7 +1075,7 @@  warn_uninitialized_vars (bool wmaybe_uninit)
 	      if (wlims.always_executed)
 		warn_uninit (OPT_Wuninitialized, use,
 			     SSA_NAME_VAR (use), stmt);
-	      else if (wmaybe_uninit)
+	      else if (wlims.wmaybe_uninit)
 		warn_uninit (OPT_Wmaybe_uninitialized, use,
 			     SSA_NAME_VAR (use), stmt);
 	    }