Skip to content

Commit 210fb22

Browse files
vtjnashKristofferC
authored andcommitted
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 (cherry picked from commit 6097140)
1 parent aa11508 commit 210fb22

File tree

4 files changed

+69
-22
lines changed

4 files changed

+69
-22
lines changed

base/compiler/ssair/inlining.jl

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -817,8 +817,8 @@ end
817817
function compileable_specialization(mi::MethodInstance, effects::Effects,
818818
et::InliningEdgeTracker, @nospecialize(info::CallInfo); compilesig_invokes::Bool=true)
819819
mi_invoke = mi
820+
method, atype, sparams = mi.def::Method, mi.specTypes, mi.sparam_vals
820821
if compilesig_invokes
821-
method, atype, sparams = mi.def::Method, mi.specTypes, mi.sparam_vals
822822
new_atype = get_compileable_sig(method, atype, sparams)
823823
new_atype === nothing && return nothing
824824
if atype !== new_atype
@@ -836,7 +836,8 @@ function compileable_specialization(mi::MethodInstance, effects::Effects,
836836
return nothing
837837
end
838838
end
839-
add_inlining_backedge!(et, mi)
839+
add_inlining_backedge!(et, mi) # to the dispatch lookup
840+
push!(et.edges, method.sig, mi_invoke) # add_inlining_backedge to the invoke call
840841
return InvokeCase(mi_invoke, effects, info)
841842
end
842843

src/gf.c

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2130,7 +2130,10 @@ JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method
21302130
int replaced_edge;
21312131
if (invokeTypes) {
21322132
// 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
2133-
replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec);
2133+
if (jl_egal(invokeTypes, caller->def.method->sig))
2134+
replaced_edge = 0; // if invokeTypes == m.sig, then the only way to change this invoke is to replace the method itself
2135+
else
2136+
replaced_edge = jl_subtype(invokeTypes, type) && is_replacing(ambig, type, m, d, n, invokeTypes, NULL, morespec);
21342137
}
21352138
else {
21362139
replaced_edge = replaced_dispatch;

src/staticdata_utils.c

Lines changed: 25 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -505,19 +505,17 @@ static void jl_collect_edges(jl_array_t *edges, jl_array_t *ext_targets, jl_arra
505505
size_t max_valid = ~(size_t)0;
506506
if (invokeTypes) {
507507
assert(jl_is_method_instance(callee));
508-
jl_methtable_t *mt = jl_method_get_table(((jl_method_instance_t*)callee)->def.method);
509-
if ((jl_value_t*)mt == jl_nothing) {
510-
callee_ids = NULL; // invalid
511-
break;
512-
}
513-
else {
508+
jl_method_t *m = ((jl_method_instance_t*)callee)->def.method;
509+
matches = (jl_value_t*)m; // valid because there is no method replacement permitted
510+
#ifndef NDEBUG
511+
jl_methtable_t *mt = jl_method_get_table(m);
512+
if ((jl_value_t*)mt != jl_nothing) {
514513
matches = jl_gf_invoke_lookup_worlds(invokeTypes, (jl_value_t*)mt, world, &min_valid, &max_valid);
515-
if (matches == jl_nothing) {
516-
callee_ids = NULL; // invalid
517-
break;
514+
if (matches != jl_nothing) {
515+
assert(m == ((jl_method_match_t*)matches)->method);
518516
}
519-
matches = (jl_value_t*)((jl_method_match_t*)matches)->method;
520517
}
518+
#endif
521519
}
522520
else {
523521
if (jl_is_method_instance(callee)) {
@@ -855,19 +853,27 @@ static jl_array_t *jl_verify_edges(jl_array_t *targets, size_t minworld)
855853
size_t max_valid = ~(size_t)0;
856854
if (invokesig) {
857855
assert(callee && "unsupported edge");
858-
jl_methtable_t *mt = jl_method_get_table(((jl_method_instance_t*)callee)->def.method);
859-
if ((jl_value_t*)mt == jl_nothing) {
860-
max_valid = 0;
856+
jl_method_t *m = ((jl_method_instance_t*)callee)->def.method;
857+
if (jl_egal(invokesig, m->sig)) {
858+
// the invoke match is `m` for `m->sig`, unless `m` is invalid
859+
if (m->deleted_world < max_valid)
860+
max_valid = 0;
861861
}
862862
else {
863-
matches = jl_gf_invoke_lookup_worlds(invokesig, (jl_value_t*)mt, minworld, &min_valid, &max_valid);
864-
if (matches == jl_nothing) {
865-
max_valid = 0;
863+
jl_methtable_t *mt = jl_method_get_table(m);
864+
if ((jl_value_t*)mt == jl_nothing) {
865+
max_valid = 0;
866866
}
867867
else {
868-
matches = (jl_value_t*)((jl_method_match_t*)matches)->method;
869-
if (matches != expected) {
870-
max_valid = 0;
868+
matches = jl_gf_invoke_lookup_worlds(invokesig, (jl_value_t*)mt, minworld, &min_valid, &max_valid);
869+
if (matches == jl_nothing) {
870+
max_valid = 0;
871+
}
872+
else {
873+
matches = (jl_value_t*)((jl_method_match_t*)matches)->method;
874+
if (matches != expected) {
875+
max_valid = 0;
876+
}
871877
}
872878
}
873879
}

test/worlds.jl

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,3 +419,40 @@ ccall(:jl_debug_method_invalidation, Any, (Cint,), 0)
419419
which(mc48954, (AbstractFloat, Int)),
420420
"jl_method_table_insert"
421421
]
422+
423+
# issue #50091 -- missing invoke edge affecting nospecialized dispatch
424+
module ExceptionUnwrapping
425+
@nospecialize
426+
unwrap_exception(@nospecialize(e)) = e
427+
unwrap_exception(e::Base.TaskFailedException) = e.task.exception
428+
@noinline function _summarize_task_exceptions(io::IO, exc, prefix = nothing)
429+
_summarize_exception((;prefix,), io, exc)
430+
nothing
431+
end
432+
@noinline function _summarize_exception(kws, io::IO, e::TaskFailedException)
433+
_summarize_task_exceptions(io, e.task, kws.prefix)
434+
end
435+
# This is the overload that prints the actual exception that occurred.
436+
result = Bool[]
437+
@noinline function _summarize_exception(kws, io::IO, @nospecialize(exc))
438+
global result
439+
push!(result, unwrap_exception(exc) === exc)
440+
if unwrap_exception(exc) !== exc # something uninferrable
441+
return _summarize_exception(kws, io, unwrap_exception(exc))
442+
end
443+
end
444+
struct X; x; end
445+
end
446+
let e = ExceptionUnwrapping.X(nothing)
447+
@test ExceptionUnwrapping.unwrap_exception(e) === e
448+
ExceptionUnwrapping._summarize_task_exceptions(devnull, e)
449+
@test ExceptionUnwrapping.result == [true]
450+
empty!(ExceptionUnwrapping.result)
451+
end
452+
ExceptionUnwrapping.unwrap_exception(e::ExceptionUnwrapping.X) = e.x
453+
let e = ExceptionUnwrapping.X(nothing)
454+
@test !(ExceptionUnwrapping.unwrap_exception(e) === e)
455+
ExceptionUnwrapping._summarize_task_exceptions(devnull, e)
456+
@test ExceptionUnwrapping.result == [false, true]
457+
empty!(ExceptionUnwrapping.result)
458+
end

0 commit comments

Comments
 (0)