Illegal instruction on 1.8.5 with varargs #50518
Assignees
Comments
|
On apple silicon, w/ Julia 1.8 and 1.9, this just seems to hang rather than crash. |
|
Also confirmed in
|
|
There's a couple of separate issues here:
1a. For example, what should the following do?
|
Keno
added a commit
that referenced
this issue
Jul 14, 2023
Fixes the segfault in #50518 and turns it into a proper error at both the syntax level (to catch lowering generating bad slot references) as well as at the codegen level (to catch e.g. bad generated functions and opaque closures). However, note that the latter case is technically undefined behavior, because we do not model the possibility that an otherwise-defined argument could throw at access time. Of course, throwing an error is allowable as undefined behavior and preferable to a segfault.
|
#50556 for issue 3. |
Keno
added a commit
that referenced
this issue
Jul 14, 2023
Fixes the segfault in #50518 and turns it into a proper error at both the syntax level (to catch lowering generating bad slot references) as well as at the codegen level (to catch e.g. bad generated functions and opaque closures). However, note that the latter case is technically undefined behavior, because we do not model the possibility that an otherwise-defined argument could throw at access time. Of course, throwing an error is allowable as undefined behavior and preferable to a segfault.
Keno
added a commit
that referenced
this issue
Jul 14, 2023
Fixes the case from #50518, but we actually have two test cases in the tests that also hit this (e.g. this one: ``` f40964(xs::Int...=1; k = 2) = (xs, k) ```), but just happened not to hit the bad codegen path. #50556, once merged would have complained on those definitions as well, without this fix.
|
#50559 for issue 2. |
github-merge-queue bot
pushed a commit
that referenced
this issue
Jul 15, 2023
Fixes the case from #50518, but we actually have two test cases in the tests that also hit this (e.g. this one: ``` f40964(xs::Int...=1; k = 2) = (xs, k) ``` ), but just happened not to hit the bad codegen path. #50556, once merged would have complained on those definitions as well, without this fix.
Keno
added a commit
that referenced
this issue
Jul 15, 2023
Pop quiz: Do you know what the following will do?
```
julia> function g1(a=(1,2)..., b...=3)
b
end
julia> g1()
julia> function g2(a=(1,2)..., b=3, c=4)
(b, c)
end
julia> g2()
julia> function g3(a=(1,2)..., b=3, c...=4)
(b, c)
end
julia> g3()
julia> g3(1)
```
I don't either and I don't think it's particularly well defined.
Splatting a default argument makes sense on the last argument,
which can be a vararg, and it is desirable to be able to specify
the default for the whole varargs tuple at once (although arguably
that should just be the non-`...` behavior, but that'd be too
breaking a change). Ref #50518. However, for other arguments,
there isn't really a sensible semantic meaning. This PR disallows
this in lowering. This is technically a minor change, but I doubt
anybody is using this. Splatting in default values wasn't really
ever supposed to work anyway, it just happened to fall out of
our lowering.
|
#50563 for issue 1. |
Keno
added a commit
that referenced
this issue
Jul 15, 2023
Pop quiz: Do you know what the following will do?
```
julia> function g1(a=(1,2)..., b...=3)
b
end
julia> g1()
julia> function g2(a=(1,2)..., b=3, c=4)
(b, c)
end
julia> g2()
julia> function g3(a=(1,2)..., b=3, c...=4)
(b, c)
end
julia> g3()
julia> g3(1)
```
I don't either and I don't think it's particularly well defined.
Splatting a default argument makes sense on the last argument,
which can be a vararg, and it is desirable to be able to specify
the default for the whole varargs tuple at once (although arguably
that should just be the non-`...` behavior, but that'd be too
breaking a change). Ref #50518. However, for other arguments,
there isn't really a sensible semantic meaning. This PR disallows
this in lowering. This is technically a minor change, but I doubt
anybody is using this. Splatting in default values wasn't really
ever supposed to work anyway, it just happened to fall out of
our lowering.
Keno
added a commit
that referenced
this issue
Jul 15, 2023
Pop quiz: Do you know what the following will do?
```
julia> function g1(a=(1,2)..., b...=3)
b
end
julia> g1()
julia> function g2(a=(1,2)..., b=3, c=4)
(b, c)
end
julia> g2()
julia> function g3(a=(1,2)..., b=3, c...=4)
(b, c)
end
julia> g3()
julia> g3(1)
```
I don't either and I don't think it's particularly well defined.
Splatting a default argument makes sense on the last argument,
which can be a vararg, and it is desirable to be able to specify
the default for the whole varargs tuple at once (although arguably
that should just be the non-`...` behavior, but that'd be too
breaking a change). Ref #50518. However, for other arguments,
there isn't really a sensible semantic meaning. This PR disallows
this in lowering. This is technically a minor change, but I doubt
anybody is using this. Splatting in default values wasn't really
ever supposed to work anyway, it just happened to fall out of
our lowering.
github-merge-queue bot
pushed a commit
that referenced
this issue
Jul 16, 2023
Fixes the segfault in #50518 and turns it into a proper error at both the syntax level (to catch lowering generating bad slot references) as well as at the codegen level (to catch e.g. bad generated functions and opaque closures). However, note that the latter case is technically undefined behavior, because we do not model the possibility that an otherwise-defined argument could throw at access time. Of course, throwing an error is allowable as undefined behavior and preferable to a segfault.
KristofferC
pushed a commit
that referenced
this issue
Jul 17, 2023
Fixes the case from #50518, but we actually have two test cases in the tests that also hit this (e.g. this one: ``` f40964(xs::Int...=1; k = 2) = (xs, k) ```), but just happened not to hit the bad codegen path. #50556, once merged would have complained on those definitions as well, without this fix. (cherry picked from commit c272236)
Keno
added a commit
that referenced
this issue
Jul 18, 2023
Pop quiz: Do you know what the following will do?
```
julia> function g1(a=(1,2)..., b...=3)
b
end
julia> g1()
julia> function g2(a=(1,2)..., b=3, c=4)
(b, c)
end
julia> g2()
julia> function g3(a=(1,2)..., b=3, c...=4)
(b, c)
end
julia> g3()
julia> g3(1)
```
I don't either and I don't think it's particularly well defined.
Splatting a default argument makes sense on the last argument, which can
be a vararg, and it is desirable to be able to specify the default for
the whole varargs tuple at once (although arguably that should just be
the non-`...` behavior, but that'd be too breaking a change). Ref
#50518. However, for other arguments, there isn't really a sensible
semantic meaning. This PR disallows this in lowering. This is
technically a minor change, but I doubt anybody is using this. Splatting
in default values wasn't really ever supposed to work anyway, it just
happened to fall out of our lowering.
---------
Co-authored-by: Jeff Bezanson <[email protected]>
KristofferC
pushed a commit
that referenced
this issue
Aug 10, 2023
Fixes the case from #50518, but we actually have two test cases in the tests that also hit this (e.g. this one: ``` f40964(xs::Int...=1; k = 2) = (xs, k) ```), but just happened not to hit the bad codegen path. #50556, once merged would have complained on those definitions as well, without this fix. (cherry picked from commit c272236)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
on
The text was updated successfully, but these errors were encountered: