Fix nothrow behavior for intrinsics #34539
Merged
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
vtjnash
reviewed
Jan 27, 2020
Keno
force-pushed
the
kf/intrinsicnothrow
branch
from
891fada
to
79ce8b9
Compare
vtjnash
reviewed
Feb 19, 2020
base/compiler/tfuncs.jl
Outdated
| if f === Intrinsics.checked_udiv_int || f === Intrinsics.checked_urem_int || f === Intrinsics.checked_srem_int || f === Intrinsics.checked_sdiv_int | ||
| # Nothrow as long as the second argument is guaranteed not to be zero | ||
| isa(argtypes[2], Const) || return false | ||
| den_val = argtypes[2].val | ||
| den_val !== zero(typeof(den_val)) || return false | ||
| return true |
There was a problem hiding this comment.
Feel like these should have isprimitivetype tests added too? (and checked_sdiv_int below)
| @@ -1169,8 +1169,7 @@ function early_inline_special_case(ir::IRCode, s::Signature, e::Expr, params::Pa | |||
| val = etype.val | |||
| is_inlineable_constant(val) || return nothing | |||
| if isa(f, IntrinsicFunction) | |||
| if is_pure_intrinsic_infer(f) && | |||
| (intrinsic_nothrow(f) || intrinsic_nothrow(f, atypes[2:end])) | |||
| if is_pure_intrinsic_infer(f) && intrinsic_nothrow(f, atypes[2:end]) | |||
There was a problem hiding this comment.
Do we need to handle Vararg early in this function and bail?
There was a problem hiding this comment.
No, vararg is handled earlier in inlining and it refuses to construct a Signature object for such cases.
base/compiler/tfuncs.jl
Outdated
| if f in (Intrinsics.sext_int, Intrinsics.zext_int, Intrinsics.trunc_int, | ||
| Intrinsics.fptoui, Intrinsics.fptosi, Intrinsics.uitofp, | ||
| Intrinsics.sitofp, Intrinsics.fptrunc, Intrinsics.fpext) | ||
| ty = instanceof_tfunc(argtypes[1])[1] |
There was a problem hiding this comment.
IIUC, if the second return value is false, then ty may include the value Union{} (e.g. the call Intrinsics.sext_int(Union{}, 1))
base/compiler/tfuncs.jl
Outdated
| Intrinsics.sitofp, Intrinsics.fptrunc, Intrinsics.fpext) | ||
| ty = instanceof_tfunc(argtypes[1])[1] | ||
| xty = widenconst(argtypes[2]) | ||
| return isprimitivetype(ty) && isprimitivetype(xty) |
There was a problem hiding this comment.
Separately, we may need to also review what happens if the relation between ty.size ≏ xty.size is not what LLVM expects for these
There was a problem hiding this comment.
Yes, but I'll leave that for future work.
base/compiler/tfuncs.jl
Outdated
| return isprimitivetype(ty) && isprimitivetype(xty) | ||
| end | ||
| # The remaining intrinsics are math/bits intrinsics. They work on all | ||
| # primitive types. |
There was a problem hiding this comment.
"...all primitive types of the same type" ?
There was a problem hiding this comment.
In theory yes, but I don't think we check this in the runtime implementation.
There was a problem hiding this comment.
easy to check...
julia> Core.Intrinsics.add_int(Int64(1), UInt64(1))
ERROR: add_int: types of a and b must match
Stacktrace:
[1] top-level scope at REPL[1]:1
|
Looking good! Just thought of a few more cases we may want to handle, while we're at it. |
This fixes the case in #34482 and adds some more robustness for similar cases, though I wouldn't be surprised if there were more dragons hiding here. Intrinsics are a bit special and from the start, we sort of expected them to only ever be called correctly under pain of segfaults or other undefined behavior. We've been gradually making these more robust, but fundamentally, they were never intended to be used by users directly, only through the type-validating wrappers in Base. What has changed to cause the recent influx of issues in this area is that people now like to do compiler transforms that happily recurse through these wrappers and perform transforms that are not always legal on Intrinsics. This should help catch a number of them, but this interface is still not very thoroughly validated, and I would be surprised to see crashes or other errors stemming from incorrect usage here.
Keno
force-pushed
the
kf/intrinsicnothrow
branch
from
9dbdb92
to
91ecdd6
Compare
|
Good to go? |
|
Sure. I was just realizing that the test we do is insufficient though: |
|
Are there cases where our intrinsics throw when they see the |
ravibitsgoa
pushed a commit
to ravibitsgoa/julia
that referenced
this pull request
Apr 9, 2020
This fixes the case in JuliaLang#34482 and adds some more robustness for similar cases, though I wouldn't be surprised if there were more dragons hiding here. Intrinsics are a bit special and from the start, we sort of expected them to only ever be called correctly under pain of segfaults or other undefined behavior. We've been gradually making these more robust, but fundamentally, they were never intended to be used by users directly, only through the type-validating wrappers in Base. What has changed to cause the recent influx of issues in this area is that people now like to do compiler transforms that happily recurse through these wrappers and perform transforms that are not always legal on Intrinsics. This should help catch a number of them, but this interface is still not very thoroughly validated, and I would be surprised to see crashes or other errors stemming from incorrect usage here.
KristofferC
pushed a commit
that referenced
this pull request
Apr 11, 2020
This fixes the case in #34482 and adds some more robustness for similar cases, though I wouldn't be surprised if there were more dragons hiding here. Intrinsics are a bit special and from the start, we sort of expected them to only ever be called correctly under pain of segfaults or other undefined behavior. We've been gradually making these more robust, but fundamentally, they were never intended to be used by users directly, only through the type-validating wrappers in Base. What has changed to cause the recent influx of issues in this area is that people now like to do compiler transforms that happily recurse through these wrappers and perform transforms that are not always legal on Intrinsics. This should help catch a number of them, but this interface is still not very thoroughly validated, and I would be surprised to see crashes or other errors stemming from incorrect usage here.
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.
This fixes the case in #34482 and adds some more robustness for similar
cases, though I wouldn't be surprised if there were more dragons hiding
here. Intrinsics are a bit special and from the start, we sort of expected
them to only ever be called correctly under pain of segfaults or other
undefined behavior. We've been gradually making these more robust,
but fundamentally, they were never intended to be used by users directly,
only through the type-validating wrappers in Base. What has changed
to cause the recent influx of issues in this area is that people now
like to do compiler transforms that happily recurse through these wrappers
and perform transforms that are not always legal on Intrinsics. This should
help catch a number of them, but this interface is still not very thoroughly
validated, and I would be surprised to see crashes or other errors stemming
from incorrect usage here.