From 375a73ceb62b38abf0e6b81cd0a669ed6bec2230 Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Wed, 25 Aug 2021 18:21:01 -0400 Subject: [PATCH] Fix #41975 - Dropped typecheck in GotoIfNot Recall the reproducer from the issue: ``` julia> f() = (if nothing; end; unsafe_load(Ptr{Int}(0))) f (generic function with 1 method) julia> f() Unreachable reached at 0x7fb33bb50090 signal (4): Illegal instruction in expression starting at REPL[13]:1 unsafe_load at ./pointer.jl:105 [inlined] unsafe_load at ./pointer.jl:105 [inlined] ``` There were actually two places where we were dropping the GotoIfNot, one in type annotation after inference, one in SSA conversion. The one in SSA conversion was structural: When both branches target the same jump destination, the GotoIfNot would be dropped. This was fine in general, except that as shown above, GotoIfNot can actually itself have a side effect, namely throwing a type error if the condition is not a boolean. Thus in order to actually drop the node we need to prove that the error check does not fire. The reason we want to drop the GotoIfNot node here is that IRCode has an invariant that every basic block is in the predecessor list only once (otherwise PhiNodes would have to carry extra state regarding which branch they refer to). To fix this, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like #41994. --- base/compiler/ssair/slot2ssa.jl | 2 +- base/compiler/typeinfer.jl | 2 +- test/compiler/ssair.jl | 4 ++++ 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/base/compiler/ssair/slot2ssa.jl b/base/compiler/ssair/slot2ssa.jl index 6ca17be2f0a83..777e7e2bb0af7 100644 --- a/base/compiler/ssair/slot2ssa.jl +++ b/base/compiler/ssair/slot2ssa.jl @@ -825,7 +825,7 @@ function construct_ssa!(ci::CodeInfo, ir::IRCode, domtree::DomTree, defuse, new_dest = block_for_inst(cfg, stmt.dest) if new_dest == bb+1 # Drop this node - it's a noop - new_code[idx] = stmt.cond + new_code[idx] = Expr(:call, GlobalRef(Core, :typeassert), stmt.cond, GlobalRef(Core, :Bool)) else new_code[idx] = GotoIfNot(stmt.cond, new_dest) end diff --git a/base/compiler/typeinfer.jl b/base/compiler/typeinfer.jl index 1d2559cf10c3a..ef6e5a161864a 100644 --- a/base/compiler/typeinfer.jl +++ b/base/compiler/typeinfer.jl @@ -639,7 +639,7 @@ function type_annotate!(sv::InferenceState, run_optimizer::Bool) expr = body[i] if isa(expr, GotoIfNot) if !isa(states[expr.dest], VarTable) - body[i] = expr.cond + body[i] = Expr(:call, GlobalRef(Core, :typeassert), expr.cond, GlobalRef(Core, :Bool)) end end end diff --git a/test/compiler/ssair.jl b/test/compiler/ssair.jl index f90bb71e291d0..17a0753eddc64 100644 --- a/test/compiler/ssair.jl +++ b/test/compiler/ssair.jl @@ -310,3 +310,7 @@ let cfg = CFG(BasicBlock[ Compiler.domtree_insert_edge!(domtree, cfg.blocks, 1, 3) @test domtree.idoms_bb == Compiler.naive_idoms(cfg.blocks) == [0, 1, 1, 3, 1, 4] end + +# Issue #41975 - SSA conversion drops type check +f_if_typecheck() = (if nothing; end; unsafe_load(Ptr{Int}(0))) +@test_throws TypeError f_if_typecheck()