From 6097140fdd7ed3523b1a80dd78a6c6a0a81c99ed Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Sat, 26 Aug 2023 13:00:49 -0400 Subject: [PATCH] add missing invoke edge for nospecialize targets (#51036) 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 --- base/compiler/ssair/inlining.jl | 5 ++-- src/gf.c | 5 +++- src/staticdata_utils.c | 44 +++++++++++++++++++-------------- test/worlds.jl | 37 +++++++++++++++++++++++++++ 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index d7b996bdc2bfd..5343daa2646c1 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -805,8 +805,8 @@ end function compileable_specialization(mi::MethodInstance, effects::Effects, et::InliningEdgeTracker, @nospecialize(info::CallInfo); compilesig_invokes::Bool=true) mi_invoke = mi + method, atype, sparams = mi.def::Method, mi.specTypes, mi.sparam_vals if compilesig_invokes - method, atype, sparams = mi.def::Method, mi.specTypes, mi.sparam_vals new_atype = get_compileable_sig(method, atype, sparams) new_atype === nothing && return nothing if atype !== new_atype @@ -824,7 +824,8 @@ function compileable_specialization(mi::MethodInstance, effects::Effects, return nothing end end - add_inlining_backedge!(et, mi) + add_inlining_backedge!(et, mi) # to the dispatch lookup + push!(et.edges, method.sig, mi_invoke) # add_inlining_backedge to the invoke call return InvokeCase(mi_invoke, effects, info) end diff --git a/src/gf.c b/src/gf.c index 1e119a1054aa3..e8cdb493026b0 100644 --- a/src/gf.c +++ b/src/gf.c @@ -2130,7 +2130,10 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method int replaced_edge; if (invokeTypes) { // n.b. normally we must have mi.specTypes <: invokeTypes <: m.sig (though it might not strictly hold), so we only need to check the other subtypes - replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec); + if (jl_egal(invokeTypes, caller->def.method->sig)) + replaced_edge = 0; // if invokeTypes == m.sig, then the only way to change this invoke is to replace the method itself + else + replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec); } else { replaced_edge = replaced_dispatch; diff --git a/src/staticdata_utils.c b/src/staticdata_utils.c index bf1a830b608de..ed80a1f6278f4 100644 --- a/src/staticdata_utils.c +++ b/src/staticdata_utils.c @@ -505,19 +505,17 @@ static void jl_collect_edges(jl_array_t *edges, jl_array_t *ext_targets, jl_arra size_t max_valid = ~(size_t)0; if (invokeTypes) { assert(jl_is_method_instance(callee)); - jl_methtable_t *mt = jl_method_get_table(((jl_method_instance_t*)callee)->def.method); - if ((jl_value_t*)mt == jl_nothing) { - callee_ids = NULL; // invalid - break; - } - else { + jl_method_t *m = ((jl_method_instance_t*)callee)->def.method; + matches = (jl_value_t*)m; // valid because there is no method replacement permitted +#ifndef NDEBUG + jl_methtable_t *mt = jl_method_get_table(m); + if ((jl_value_t*)mt != jl_nothing) { matches = jl_gf_invoke_lookup_worlds(invokeTypes, (jl_value_t*)mt, world, &min_valid, &max_valid); - if (matches == jl_nothing) { - callee_ids = NULL; // invalid - break; + if (matches != jl_nothing) { + assert(m == ((jl_method_match_t*)matches)->method); } - matches = (jl_value_t*)((jl_method_match_t*)matches)->method; } +#endif } else { if (jl_is_method_instance(callee)) { @@ -855,19 +853,27 @@ static jl_array_t *jl_verify_edges(jl_array_t *targets, size_t minworld) size_t max_valid = ~(size_t)0; if (invokesig) { assert(callee && "unsupported edge"); - jl_methtable_t *mt = jl_method_get_table(((jl_method_instance_t*)callee)->def.method); - if ((jl_value_t*)mt == jl_nothing) { - max_valid = 0; + jl_method_t *m = ((jl_method_instance_t*)callee)->def.method; + if (jl_egal(invokesig, m->sig)) { + // the invoke match is `m` for `m->sig`, unless `m` is invalid + if (m->deleted_world < max_valid) + max_valid = 0; } else { - matches = jl_gf_invoke_lookup_worlds(invokesig, (jl_value_t*)mt, minworld, &min_valid, &max_valid); - if (matches == jl_nothing) { - max_valid = 0; + jl_methtable_t *mt = jl_method_get_table(m); + if ((jl_value_t*)mt == jl_nothing) { + max_valid = 0; } else { - matches = (jl_value_t*)((jl_method_match_t*)matches)->method; - if (matches != expected) { - max_valid = 0; + matches = jl_gf_invoke_lookup_worlds(invokesig, (jl_value_t*)mt, minworld, &min_valid, &max_valid); + if (matches == jl_nothing) { + max_valid = 0; + } + else { + matches = (jl_value_t*)((jl_method_match_t*)matches)->method; + if (matches != expected) { + max_valid = 0; + } } } } diff --git a/test/worlds.jl b/test/worlds.jl index b5a8f1c5449ac..8e820bdab88df 100644 --- a/test/worlds.jl +++ b/test/worlds.jl @@ -419,3 +419,40 @@ ccall(:jl_debug_method_invalidation, Any, (Cint,), 0) which(mc48954, (AbstractFloat, Int)), "jl_method_table_insert" ] + +# issue #50091 -- missing invoke edge affecting nospecialized dispatch +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. +result = Bool[] +@noinline function _summarize_exception(kws, io::IO, @nospecialize(exc)) + global result + push!(result, 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 +let e = ExceptionUnwrapping.X(nothing) + @test ExceptionUnwrapping.unwrap_exception(e) === e + ExceptionUnwrapping._summarize_task_exceptions(devnull, e) + @test ExceptionUnwrapping.result == [true] + empty!(ExceptionUnwrapping.result) +end +ExceptionUnwrapping.unwrap_exception(e::ExceptionUnwrapping.X) = e.x +let e = ExceptionUnwrapping.X(nothing) + @test !(ExceptionUnwrapping.unwrap_exception(e) === e) + ExceptionUnwrapping._summarize_task_exceptions(devnull, e) + @test ExceptionUnwrapping.result == [false, true] + empty!(ExceptionUnwrapping.result) +end