From fc7a88197f543c5a1712a66815dbf00fa1202999 Mon Sep 17 00:00:00 2001
From: Cody Tapscott <topolarity@tapscott.me>
Date: Wed, 28 Feb 2024 08:44:17 -0500
Subject: [PATCH] Fix condition for `unreachable` code in IRCode conversion

This is a partial back-port of #50924, where we discovered that the
optimizer would ignore:
  1. must-throw `%XX = SlotNumber(_)` statements
  2. must-throw `goto #bb if not %x` statements
when re-building the CFG during IRCode conversion.

This is mostly harmless, except that in the case of (1) we can accidentally
fall through the statically deleted (`Const()`-wrapped) code from inference
and observe a control-flow edge that never should have existed, such as an
edge into a catch block. Such an edge is invalid semantically and breaks our
SSA conversion.

This one-line change fixes (1) but not (2), which is enough for IR validity.
We should follow-up with a tweak to `compute_basic_blocks` to enforce this
requirement.

The test added here is very brittle, but it's better than nothing until we
have utilities to provide hand-written (pre-optimizer) IR and pass it through
part of our pipeline.
---
 base/compiler/optimize.jl  |  2 +-
 test/compiler/inference.jl | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/base/compiler/optimize.jl b/base/compiler/optimize.jl
index 3a8de06811cc2..7c8413ceaf9af 100644
--- a/base/compiler/optimize.jl
+++ b/base/compiler/optimize.jl
@@ -561,7 +561,7 @@ function convert_to_ircode(ci::CodeInfo, sv::OptimizationState)
             idx += 1
             prevloc = codeloc
         end
-        if code[idx] isa Expr && ssavaluetypes[idx] === Union{}
+        if ssavaluetypes[idx] === Union{} && code[idx] !== nothing && !(code[idx] isa Core.Const)
             if !(idx < length(code) && isa(code[idx + 1], ReturnNode) && !isdefined((code[idx + 1]::ReturnNode), :val))
                 # insert unreachable in the same basic block after the current instruction (splitting it)
                 insert!(code, idx + 1, ReturnNode())
diff --git a/test/compiler/inference.jl b/test/compiler/inference.jl
index 106abc695247e..223794e74fff0 100644
--- a/test/compiler/inference.jl
+++ b/test/compiler/inference.jl
@@ -5161,3 +5161,27 @@ let x = 1, _Any = Any
     foo27031() = bar27031((x, 1.0), Val{_Any})
     @test foo27031() == "OK"
 end
+
+# Issue #53366
+#
+# FIXME: This test is quite brittle, since it relies on lowering making a particular
+# (and unnecessary) decision to embed a `SlotNumber` in statement position.
+#
+# This should be re-written to have the bad IR directly, then run type inference +
+# SSA conversion. It should also avoid running `compact!` afterward, since that can
+# mask bugs by cleaning up unused ϕ nodes that should never have existed.
+function issue53366(sc::Threads.Condition)
+    @lock sc begin
+        try
+            if Core.Compiler.inferencebarrier(true)
+                return nothing
+            end
+            return nothing
+        finally
+        end
+    end
+end
+
+let (ir, rt) = only(Base.code_ircode(issue53366, (Threads.Condition,)))
+    Core.Compiler.verify_ir(ir)
+end