Fix #41975 - Dropped typecheck in GotoIfNot #41994
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, on option would be to create a new expr
(e.g. `boolcheck`) that works like `:throw_undef_if_not`.
However, on second thought, I figured we might as well
just re-use GotoIfNot for this, since we'd otherwise
have to duplicate all the checks for side effects and
add codegen, etc., so this does the following:
The `dest` label in GotoIfNot becomes optional. If set
to 0, the only thing the GotoIfNot does is to perform
the boolcheck. It is also no longer considered a terminator
for SSA construction, either. The regular code for
determining side-effects of GotoIfNot will determine
the bool-ness of the condition and delete the GotoIfNot
if applicable.
This also aligns with another PR I'm working on, which
will add an optional `else` label to GotoIfNot (in
the direction of #41476). Hopefully with this, the
two paths will then be symmetric.
vtjnash
reviewed
Aug 25, 2021
| ctx.builder.CreateCondBr(isfalse, ifnot, ifso); | ||
| if (lname == 0) { | ||
| ctx.builder.CreateBr(ifso); | ||
| } else { |
There was a problem hiding this comment.
Suggested change
| } else { | |
| } | |
| else { |
Why not use the existing one? Expr(:call, GlobalRef(Core, :typeassert), %%, GlobalRef(:Core, :Bool)) |
I didn't think they were actually the same error, but I guess they are - not sure we want that to be true in the fullness of time, but let me see if that works. |
Keno
added a commit
that referenced
this pull request
Aug 25, 2021
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.
|
Superseded by #42010 |
|
Oh, I guess they are a different error, they just printed the same: julia> try; if nothing; end; catch ex; ex; end
TypeError(:if, "", Bool, nothing)
julia> typeassert(nothing, Bool)
ERROR: TypeError: non-boolean (Nothing) used in boolean context
Stacktrace:
[1] top-level scope
@ REPL[10]:1
julia> try; typeassert(nothing, Bool); catch ex; ex; end
TypeError(:typeassert, "", Bool, nothing) |
|
Should we add an optional third argument to typeassert (i.e. |
aviatesk
pushed a commit
that referenced
this pull request
Aug 26, 2021
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.
KristofferC
pushed a commit
that referenced
this pull request
Aug 27, 2021
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.
(cherry picked from commit 2445000)
KristofferC
pushed a commit
that referenced
this pull request
Aug 27, 2021
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.
(cherry picked from commit 2445000)
KristofferC
pushed a commit
that referenced
this pull request
Aug 31, 2021
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.
(cherry picked from commit 2445000)
KristofferC
pushed a commit
that referenced
this pull request
Sep 3, 2021
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.
(cherry picked from commit 2445000)
LilithHafner
pushed a commit
to LilithHafner/julia
that referenced
this pull request
Feb 22, 2022
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 JuliaLang#41994.
LilithHafner
pushed a commit
to LilithHafner/julia
that referenced
this pull request
Mar 8, 2022
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 JuliaLang#41994.
staticfloat
pushed a commit
that referenced
this pull request
Dec 23, 2022
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.
(cherry picked from commit 2445000)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recall the reproducer from the issue:
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, on option would be to create a new expr
(e.g.
boolcheck) that works like:throw_undef_if_not.However, on second thought, I figured we might as well
just re-use GotoIfNot for this, since we'd otherwise
have to duplicate all the checks for side effects and
add codegen, etc., so this does the following:
The
destlabel in GotoIfNot becomes optional. If setto 0, the only thing the GotoIfNot does is to perform
the boolcheck. It is also no longer considered a terminator
for SSA construction, either. The regular code for
determining side-effects of GotoIfNot will determine
the bool-ness of the condition and delete the GotoIfNot
if applicable.
This also aligns with another PR I'm working on, which
will add an optional
elselabel to GotoIfNot (inthe direction of #41476). Hopefully with this, the
two paths will then be symmetric.