-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Regression: Bug with nospecialize and world age #50091
Comments
narrowed down so far to this weird little recursion gadget: julia> module ExceptionUnwrapping
@nospecialize
unwrap_exception(@nospecialize(e)) = e
unwrap_exception(e::Base.TaskFailedException) = e.task.exception
@noinline function _summarize_task_exceptions(io::IO, exc, prefix = nothing)
_summarize_exception((;prefix,), io, exc)
nothing
end
@noinline function _summarize_exception(kws, io::IO, e::TaskFailedException)
_summarize_task_exceptions(io, e.task, kws.prefix)
end
# This is the overload that prints the actual exception that occurred.
@noinline function _summarize_exception(kws, io::IO, @nospecialize(exc))
@show unwrap_exception(exc) === exc
if unwrap_exception(exc) !== exc # something uninferrable
return _summarize_exception(kws, io, unwrap_exception(exc))
end
end
struct X; x; end
end;
julia> let e = ExceptionUnwrapping.X(nothing)
@show ExceptionUnwrapping.unwrap_exception(e) === e
ExceptionUnwrapping._summarize_task_exceptions(stdout, e)
end
ExceptionUnwrapping.unwrap_exception(e) === e = true
unwrap_exception(exc) === exc = true
julia> ExceptionUnwrapping.unwrap_exception(e::ExceptionUnwrapping.X) = e.x
julia> let e = ExceptionUnwrapping.X(nothing)
@show ExceptionUnwrapping.unwrap_exception(e) === e
ExceptionUnwrapping._summarize_task_exceptions(stdout, e)
end
ExceptionUnwrapping.unwrap_exception(e) === e = false
unwrap_exception(exc) === exc = true If we look closely, we see it decided that |
Is that enough to look into? I guess this is a bug that will start showing up in 1.10. |
That likely doesn't tell you much other than inlining has a bug, but we already knew that. |
I guess that PR just exposes it then? |
Whatever is broken in this example only started breaking on 1.10 though; it works fine on 1.9. Are you saying that it might have just coincidentally exposed a bug that was already occurring even before 1.10 then? 👀 |
We need 2 edges: one for the lookup (which uses the call signature) and one for the invoke (which uses the invoke signature). It is hard to make a small example for this, but the test case demonstrated this issue, particularly if inspected by `SnoopCompile.@snoopr`. Additionally, we can do some easy optimizations on the invoke invalidation, since in most cases we know from subtyping transativity that it is only invalid if the method callee target is actually deleted, and otherwise it cannot ever be partially replaced. Fixes: #50091
We need 2 edges: one for the lookup (which uses the call signature) and one for the invoke (which uses the invoke signature). It is hard to make a small example for this, but the test case demonstrated this issue, particularly if inspected by `SnoopCompile.@snoopr`. Additionally, we can do some easy optimizations on the invoke invalidation, since in most cases we know from subtyping transativity that it is only invalid if the method callee target is actually deleted, and otherwise it cannot ever be partially replaced. Fixes: #50091 Likely introduced by #49404, so marking for v1.10 backport only
Thanks for the fix! Great find. 👍👍 |
We need 2 edges: one for the lookup (which uses the call signature) and one for the invoke (which uses the invoke signature). It is hard to make a small example for this, but the test case demonstrated this issue, particularly if inspected by `SnoopCompile.@snoopr`. Additionally, we can do some easy optimizations on the invoke invalidation, since in most cases we know from subtyping transativity that it is only invalid if the method callee target is actually deleted, and otherwise it cannot ever be partially replaced. Fixes: #50091 Likely introduced by #49404, so marking for v1.10 backport only (cherry picked from commit 6097140)
We need 2 edges: one for the lookup (which uses the call signature) and one for the invoke (which uses the invoke signature). It is hard to make a small example for this, but the test case demonstrated this issue, particularly if inspected by `SnoopCompile.@snoopr`. Additionally, we can do some easy optimizations on the invoke invalidation, since in most cases we know from subtyping transativity that it is only invalid if the method callee target is actually deleted, and otherwise it cannot ever be partially replaced. Fixes: #50091 Likely introduced by #49404, so marking for v1.10 backport only (cherry picked from commit 6097140)
:/ This is a bit of a complicated MRE... It appears that there has been some sort of codegen regression introduced in julia master, where applying heavy
@nospecialize
can cause a function to not pick up method invalidation, and/or to run in the wrong world age.There should be a dynamic dispatch in this test, and it should pick up a method added by the test.
I can't reproduce it with a smaller example, but you can see it in the tests for this PR:
JuliaServices/ExceptionUnwrapping.jl@043150c from JuliaServices/ExceptionUnwrapping.jl#14.
The tests are failing on julia master, but not in the other versions. And I can reproduce it locally.
I added some printlns which seem to show the issue, although it's still hard to explain.
I added this diff:
and now you can see that the call to
is_wrapped_exception
inside the function behaves differently than the call at the top level. If i define a new type, at firstis_wrapped_exception(e)
returns false. But then, when i defineunwrap_exception(e::X) = e.x
, now is_wrapped_exception should return true. And it does at the top-level, but it doesn't inside the function:summarize_current_exceptions
seems to somehow not be picking up the newly defined method.If i remove all the
@nospecalize
s in the package, everything works again.Thanks, sorry this is pretty specific.
The text was updated successfully, but these errors were encountered: