From 77c44e6c556ccff1be9484a97a0f59ff6fbac570 Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Fri, 12 May 2023 21:49:34 +0000
Subject: [PATCH] irinterp: Don't introduce invalid CFGs

This is yet another followup to #49692 and #49750.
With the introduced change, we kill the CFG edge
from the basic block with the discovered error to
its successors. However, we have an invariant in
the verifier that the CFG should always match the
IR. Turns out this is for good reason, as we assume
in a number of places (including, ironically in the
irinterp) that a GotoNode/GotoIfNot terminator means
that the BB has the corresponding number of successors
in the IR. Fix all this by killing the rest of the basic
block when we discover that it is unreachable and if
possible introducing an unreachable node at the end.
However, of course if the erroring statement is the
fallthrough terminator itself, there is no space for
an unreachable node. We fix this by tweaking the
verification to allow this case, as its really
no worse than the other problems with fall-through
terminators (#41476), but of course it would be good
to address that as part of a more general IR refactor.
---
 base/compiler/ssair/irinterp.jl | 15 ++++++++++-----
 base/compiler/ssair/verify.jl   | 10 ++++++++--
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/base/compiler/ssair/irinterp.jl b/base/compiler/ssair/irinterp.jl
index 4aed080e57bb4..ba2587173c089 100644
--- a/base/compiler/ssair/irinterp.jl
+++ b/base/compiler/ssair/irinterp.jl
@@ -253,14 +253,19 @@ function _ir_abstract_constant_propagation(interp::AbstractInterpreter, irsv::IR
                 inst = ir.stmts[idx][:inst]
                 typ = ir.stmts[idx][:type]
             end
-            if idx == lstmt
-                process_terminator!(ir, inst, idx, bb, all_rets, bb_ip) && @goto residual_scan
-                (isa(inst, GotoNode) || isa(inst, GotoIfNot) || isa(inst, ReturnNode) || isexpr(inst, :enter)) && continue
-            end
-            if typ === Bottom && !isa(inst, PhiNode)
+            if typ === Bottom && !(isa(inst, PhiNode) || isa(inst, GotoNode) || isa(inst, GotoIfNot) || isa(inst, ReturnNode) || isexpr(inst, :enter))
                 kill_terminator_edges!(irsv, lstmt, bb)
+                if idx != lstmt
+                    for idx2 in (idx+1:lstmt-1)
+                        ir[SSAValue(idx2)] = nothing
+                    end
+                    ir[SSAValue(lstmt)][:inst] = ReturnNode()
+                end
                 break
             end
+            if idx == lstmt
+                process_terminator!(ir, inst, idx, bb, all_rets, bb_ip) && @goto residual_scan
+            end
         end
     end
     @goto compute_rt
diff --git a/base/compiler/ssair/verify.jl b/base/compiler/ssair/verify.jl
index c281aa4b7786f..baa0e6e2235a6 100644
--- a/base/compiler/ssair/verify.jl
+++ b/base/compiler/ssair/verify.jl
@@ -174,8 +174,14 @@ function verify_ir(ir::IRCode, print::Bool=true,
                     end
                     isa(stmt, PhiNode) || break
                 end
-                @verify_error "Block $idx successors ($(block.succs)), does not match fall-through terminator ($terminator)"
-                error("")
+                if isempty(block.succs) && ir.stmts[idx][:type] == Union{}
+                    # Allow fallthrough terminators that are known to error to
+                    # be removed from the CFG. Ideally we'd add an unreachable
+                    # here, but that isn't always possible.
+                else
+                    @verify_error "Block $idx successors ($(block.succs)), does not match fall-through terminator ($terminator)"
+                    error("")
+                end
             end
         end
     end