Skip to content
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

Closed
NHDaly opened this issue Jun 6, 2023 · 7 comments · Fixed by #51036
Closed

Regression: Bug with nospecialize and world age #50091

NHDaly opened this issue Jun 6, 2023 · 7 comments · Fixed by #51036
Labels
bug Indicates an unexpected problem or unintended behavior compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) regression Regression in behavior compared to a previous version
Milestone

Comments

@NHDaly
Copy link
Member

NHDaly commented Jun 6, 2023

:/ 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:

 #     Cons: possibly hiding intermediate exceptions that might have been helpful to see.
@@ -110,7 +112,10 @@ function _summarize_exception(io::IO, e::CompositeException, stack; prefix 
= not
     end
 end
 # This is the overload that prints the actual exception that occurred.
function _summarize_exception(io::IO, exc, stack; prefix = nothing)
+    @show exc
+    @show is_wrapped_exception(exc)
+    global EXC = exc
     # First, check that this exception isn't some other kind of user-defined
     # wrapped exception. We want to unwrap this layer as well, so that we are
     # printing just the true exceptions in the summary, not any exception

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 first is_wrapped_exception(e) returns false. But then, when i define unwrap_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:

julia> e = try try
           @assert false
       catch e
           rethrow(X(e))
       end catch e; 
           @show e
           @show is_wrapped_exception(e) 
           ExceptionUnwrapping.summarize_current_exceptions(stdout)
       end
e = X(AssertionError("false"))
is_wrapped_exception(e) = false
=== EXCEPTION SUMMARY ===

exc = X(AssertionError("false"))
is_wrapped_exception(exc) = false
X(AssertionError("false"))
 [1] top-level scope
   @ REPL[23]:2

julia> ExceptionUnwrapping.unwrap_exception(e::X) = e.x

julia> e = try try
           @assert false
       catch e
           rethrow(X(e))
       end catch e; 
           @show e
           @show is_wrapped_exception(e) 
           ExceptionUnwrapping.summarize_current_exceptions(stdout)
       end
e = X(AssertionError("false"))
is_wrapped_exception(e) = true
=== EXCEPTION SUMMARY ===

exc = X(AssertionError("false"))
is_wrapped_exception(exc) = false
X(AssertionError("false"))
 [1] top-level scope
   @ REPL[25]:2

julia> is_wrapped_exception(ExceptionUnwrapping.EXC)
true

summarize_current_exceptions seems to somehow not be picking up the newly defined method.

If i remove all the @nospecalizes in the package, everything works again.

Thanks, sorry this is pretty specific.

@NHDaly NHDaly added bug Indicates an unexpected problem or unintended behavior regression Regression in behavior compared to a previous version labels Jun 6, 2023
@NHDaly NHDaly changed the title Regression: Codegen bug with nospecialize and world age Regression: Bug with nospecialize and world age Jun 6, 2023
@vtjnash
Copy link
Member

vtjnash commented Jun 8, 2023

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 _summarize_task_exceptions(::IO, ::Any, ::Any) does not need to track any edges, even though the inlining pass clearly should have added them (to invoke _summarize_exception).

@NHDaly NHDaly added the compiler:inference Type inference label Jun 27, 2023
@NHDaly
Copy link
Member Author

NHDaly commented Jun 27, 2023

Is that enough to look into? I guess this is a bug that will start showing up in 1.10.

@vtjnash vtjnash added this to the 1.10 milestone Jun 27, 2023
@gbaraldi
Copy link
Member

gbaraldi commented Aug 7, 2023

#49404 @vtjnash this got bisected to that PR.

@vtjnash
Copy link
Member

vtjnash commented Aug 8, 2023

That likely doesn't tell you much other than inlining has a bug, but we already knew that.

@gbaraldi
Copy link
Member

gbaraldi commented Aug 8, 2023

I guess that PR just exposes it then?

@NHDaly
Copy link
Member Author

NHDaly commented Aug 10, 2023

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? 👀

@JeffBezanson JeffBezanson added compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) and removed compiler:inference Type inference labels Aug 15, 2023
vtjnash added a commit that referenced this issue Aug 24, 2023
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
vtjnash added a commit that referenced this issue Aug 26, 2023
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
@NHDaly
Copy link
Member Author

NHDaly commented Aug 29, 2023

Thanks for the fix! Great find. 👍👍

KristofferC pushed a commit that referenced this issue Sep 15, 2023
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)
nalimilan pushed a commit that referenced this issue Nov 5, 2023
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants