Skip to content

Commit

Permalink
Fix #41975 - Dropped typecheck in GotoIfNot (#42010)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Keno authored Aug 26, 2021
1 parent afc504b commit 2445000
Show file tree
Hide file tree
Showing 3 changed files with 6 additions and 2 deletions.
2 changes: 1 addition & 1 deletion base/compiler/ssair/slot2ssa.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/typeinfer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,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
Expand Down
4 changes: 4 additions & 0 deletions test/compiler/ssair.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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()

0 comments on commit 2445000

Please sign in to comment.