correctly encode and validate fully_covers as edges (#58861)

This is a bugfix and code cleanup, since it wasn't properly encoding and
checking for newly "missing"-type backedges `fully_covering` during
verification.

Co-authored-by: Claude <[email protected]>
(cherry picked from commit 6efd218)

Author: 2 people

Compiler/src/stmtinfo.jl

@@ -60,7 +60,7 @@ function _add_edges_impl(edges::Vector{Any}, info::MethodMatchInfo, mi_edge::Boo
         end
     end
     nmatches = length(info.results)
-    if nmatches == length(info.edges) == 1
+    if nmatches == length(info.edges) == 1 && fully_covering(info)
         # try the optimized format for the representation, if possible and applicable
         # if this doesn't succeed, the backedge will be less precise,
         # but the forward edge will maintain the precision
@@ -78,13 +78,15 @@ function _add_edges_impl(edges::Vector{Any}, info::MethodMatchInfo, mi_edge::Boo
         end
     end
     # add check for whether this lookup already existed in the edges list
+    # encode nmatches as negative if fully_covers is false
+    encoded_nmatches = fully_covering(info) ? nmatches : -nmatches
     for i in 1:length(edges)
-        if edges[i] === nmatches && edges[i+1] == info.atype
+        if edges[i] === encoded_nmatches && edges[i+1] == info.atype
             # TODO: must also verify the CodeInstance match too
             return nothing
         end
     end
-    push!(edges, nmatches, info.atype)
+    push!(edges, encoded_nmatches, info.atype)
     for i = 1:nmatches
         edge = info.edges[i]
         m = info.results[i]
@@ -101,7 +103,7 @@ function add_one_edge!(edges::Vector{Any}, edge::MethodInstance)
     i = 1
     while i <= length(edges)
         edgeᵢ = edges[i]
-        edgeᵢ isa Int && (i += 2 + edgeᵢ; continue)
+        edgeᵢ isa Int && (i += 2 + abs(edgeᵢ); continue)
         edgeᵢ isa CodeInstance && (edgeᵢ = get_ci_mi(edgeᵢ))
         edgeᵢ isa MethodInstance || (i += 1; continue)
         if edgeᵢ === edge && !(i > 1 && edges[i-1] isa Type)
@@ -116,7 +118,7 @@ function add_one_edge!(edges::Vector{Any}, edge::CodeInstance)
     i = 1
     while i <= length(edges)
         edgeᵢ_orig = edgeᵢ = edges[i]
-        edgeᵢ isa Int && (i += 2 + edgeᵢ; continue)
+        edgeᵢ isa Int && (i += 2 + abs(edgeᵢ); continue)
         edgeᵢ isa CodeInstance && (edgeᵢ = get_ci_mi(edgeᵢ))
         edgeᵢ isa MethodInstance || (i += 1; continue)
         if edgeᵢ === edge.def && !(i > 1 && edges[i-1] isa Type)

base/staticdata.jl

@@ -177,12 +177,15 @@ function verify_method(codeinst::CodeInstance, stack::Vector{CodeInstance}, visi
             end
             if edge isa MethodInstance
                 sig = edge.specTypes
-                min_valid2, max_valid2, matches = verify_call(sig, callees, j, 1, world)
+                min_valid2, max_valid2, matches = verify_call(sig, callees, j, 1, world, true)
                 j += 1
             elseif edge isa Int
                 sig = callees[j+1]
-                min_valid2, max_valid2, matches = verify_call(sig, callees, j+2, edge, world)
-                j += 2 + edge
+                # Handle negative counts (fully_covers=false)
+                nmatches = abs(edge)
+                fully_covers = edge > 0
+                min_valid2, max_valid2, matches = verify_call(sig, callees, j+2, nmatches, world, fully_covers)
+                j += 2 + nmatches
                 edge = sig
             elseif edge isa Core.Binding
                 j += 1
@@ -288,7 +291,7 @@ function verify_method(codeinst::CodeInstance, stack::Vector{CodeInstance}, visi
     return 0, minworld, maxworld
 end
 
-function verify_call(@nospecialize(sig), expecteds::Core.SimpleVector, i::Int, n::Int, world::UInt)
+function verify_call(@nospecialize(sig), expecteds::Core.SimpleVector, i::Int, n::Int, world::UInt, fully_covers::Bool)
     # verify that these edges intersect with the same methods as before
     mi = nothing
     if n == 1
@@ -304,20 +307,25 @@ function verify_call(@nospecialize(sig), expecteds::Core.SimpleVector, i::Int, n
                     mi = t::MethodInstance
                 end
                 meth = mi.def::Method
-                if !iszero(mi.dispatch_status & METHOD_SIG_LATEST_ONLY)
+                # Fast path is legal when fully_covers=true OR when METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC is unset
+                if (fully_covers || iszero(meth.dispatch_status & METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC)) &&
+                   !iszero(mi.dispatch_status & METHOD_SIG_LATEST_ONLY)
                     minworld = meth.primary_world
                     @assert minworld ≤ world
                     maxworld = typemax(UInt)
                     result = Any[] # result is unused
                     return minworld, maxworld, result
                 end
             end
-            if !iszero(meth.dispatch_status & METHOD_SIG_LATEST_ONLY)
-                minworld = meth.primary_world
-                @assert minworld ≤ world
-                maxworld = typemax(UInt)
-                result = Any[] # result is unused
-                return minworld, maxworld, result
+            # Fast path is legal when fully_covers=true OR when METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC is unset
+            if fully_covers || iszero(meth.dispatch_status & METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC)
+                if !iszero(meth.dispatch_status & METHOD_SIG_LATEST_ONLY)
+                    minworld = meth.primary_world
+                    @assert minworld ≤ world
+                    maxworld = typemax(UInt)
+                    result = Any[] # result is unused
+                    return minworld, maxworld, result
+                end
             end
         end
     end
@@ -382,6 +390,8 @@ end
 const METHOD_SIG_LATEST_WHICH = 0x1
 # true indicates this method would be returned as the only result from `methods` when calling `method.sig` in the current latest world
 const METHOD_SIG_LATEST_ONLY = 0x2
+# true indicates there exists some other method that is not more specific than this one in the current latest world (which might be more fully covering)
+const METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC = 0x8
 
 function verify_invokesig(@nospecialize(invokesig), expected::Method, world::UInt)
     @assert invokesig isa Type

src/gf.c

@@ -2140,12 +2140,6 @@ static int jl_type_intersection2(jl_value_t *t1, jl_value_t *t2, jl_value_t **is
 }
 
 
-enum morespec_options {
-    morespec_unknown,
-    morespec_isnot,
-    morespec_is
-};
-
 // check if `type` is replacing `m` with an ambiguity here, given other methods in `d` that already match it
 static int is_replacing(char ambig, jl_value_t *type, jl_method_t *m, jl_method_t *const *d, size_t n, jl_value_t *isect, jl_value_t *isect2, char *morespec)
 {
@@ -2155,17 +2149,15 @@ static int is_replacing(char ambig, jl_value_t *type, jl_method_t *m, jl_method_
         // see if m2 also fully covered this intersection
         if (m == m2 || !(jl_subtype(isect, m2->sig) || (isect2 && jl_subtype(isect2, m2->sig))))
             continue;
-        if (morespec[k] == (char)morespec_unknown)
-            morespec[k] = (char)(jl_type_morespecific(m2->sig, type) ? morespec_is : morespec_isnot);
-        if (morespec[k] == (char)morespec_is)
+        if (morespec[k])
             // not actually shadowing this--m2 will still be better
             return 0;
         // if type is not more specific than m (thus now dominating it)
         // then there is a new ambiguity here,
         // since m2 was also a previous match over isect,
         // see if m was previously dominant over all m2
         // or if this was already ambiguous before
-        if (ambig == morespec_is && !jl_type_morespecific(m->sig, m2->sig)) {
+        if (ambig && !jl_type_morespecific(m->sig, m2->sig)) {
             // m and m2 were previously ambiguous over the full intersection of mi with type, and will still be ambiguous with addition of type
             return 0;
         }
@@ -2659,17 +2651,27 @@ void jl_method_table_activate(jl_typemap_entry_t *newentry)
     oldvalue = get_intersect_matches(jl_atomic_load_relaxed(&mt->defs), newentry, &replaced, max_world);
 
     int invalidated = 0;
-    int only = !(jl_atomic_load_relaxed(&method->dispatch_status) & METHOD_SIG_PRECOMPILE_MANY); // will compute if this will be currently the only result that would returned from `ml_matches` given `sig`
+    int dispatch_bits = METHOD_SIG_LATEST_WHICH; // Always set LATEST_WHICH
+    // Check precompiled dispatch status bits
+    int precompiled_status = jl_atomic_load_relaxed(&method->dispatch_status);
+    if (!(precompiled_status & METHOD_SIG_PRECOMPILE_MANY))
+        dispatch_bits |= METHOD_SIG_LATEST_ONLY; // Tentatively set, will be cleared if not applicable
+    if (precompiled_status & METHOD_SIG_PRECOMPILE_HAS_NOTMORESPECIFIC)
+        dispatch_bits |= METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC;
     if (replaced) {
         oldvalue = (jl_value_t*)replaced;
         jl_method_t *m = replaced->func.method;
         invalidated = 1;
         method_overwrite(newentry, m);
-        // this is an optimized version of below, given we know the type-intersection is exact
+        // This is an optimized version of below, given we know the type-intersection is exact
         jl_method_table_invalidate(m, max_world);
         int m_dispatch = jl_atomic_load_relaxed(&m->dispatch_status);
-        jl_atomic_store_relaxed(&m->dispatch_status, 0);
-        only = m_dispatch & METHOD_SIG_LATEST_ONLY;
+        // Clear METHOD_SIG_LATEST_ONLY and METHOD_SIG_LATEST_WHICH bits, only keeping NOTMORESPECIFIC
+        jl_atomic_store_relaxed(&m->dispatch_status, m_dispatch & METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC);
+        // Edge case: don't set dispatch_bits |= METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC unconditionally since `m` is not an visible method for invalidations
+        dispatch_bits |= (m_dispatch & METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC);
+        if (!(m_dispatch & METHOD_SIG_LATEST_ONLY))
+            dispatch_bits &= ~METHOD_SIG_LATEST_ONLY;
     }
     else {
         jl_method_t *const *d;
@@ -2685,13 +2687,28 @@ void jl_method_table_activate(jl_typemap_entry_t *newentry)
 
             oldmi = jl_alloc_vec_any(0);
             char *morespec = (char*)alloca(n);
-            memset(morespec, morespec_unknown, n);
+            // Compute all morespec values upfront
+            for (j = 0; j < n; j++)
+                morespec[j] = (char)jl_type_morespecific(d[j]->sig, type);
             for (j = 0; j < n; j++) {
                 jl_method_t *m = d[j];
-                if (morespec[j] == (char)morespec_is) {
-                    only = 0;
-                    continue;
+                // Compute ambig state: is there an ambiguity between new method and old m?
+                char ambig = !morespec[j] && !jl_type_morespecific(type, m->sig);
+                // Compute updates to the dispatch state bits
+                int m_dispatch = jl_atomic_load_relaxed(&m->dispatch_status);
+                if (morespec[j] || ambig) {
+                    // !morespecific(new, old)
+                    dispatch_bits &= ~METHOD_SIG_LATEST_ONLY;
+                    m_dispatch |= METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC;
+                }
+                if (!morespec[j]) {
+                    // !morespecific(old, new)
+                    dispatch_bits |= METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC;
+                    m_dispatch &= ~METHOD_SIG_LATEST_ONLY;
                 }
+                jl_atomic_store_relaxed(&m->dispatch_status, m_dispatch);
+                if (morespec[j])
+                    continue;
                 loctag = jl_atomic_load_relaxed(&m->specializations); // use loctag for a gcroot
                 _Atomic(jl_method_instance_t*) *data;
                 size_t l;
@@ -2703,27 +2720,19 @@ void jl_method_table_activate(jl_typemap_entry_t *newentry)
                     data = (_Atomic(jl_method_instance_t*)*) &loctag;
                     l = 1;
                 }
-                enum morespec_options ambig = morespec_unknown;
                 for (size_t i = 0; i < l; i++) {
                     jl_method_instance_t *mi = jl_atomic_load_relaxed(&data[i]);
                     if ((jl_value_t*)mi == jl_nothing)
                         continue;
                     isect3 = jl_type_intersection(m->sig, (jl_value_t*)mi->specTypes);
                     if (jl_type_intersection2(type, isect3, &isect, &isect2)) {
+                        // Replacing a method--see if this really was the selected method previously
+                        // over the intersection (not ambiguous) and the new method will be selected now (morespec_is).
                         // TODO: this only checks pair-wise for ambiguities, but the ambiguities could arise from the interaction of multiple methods
                         // and thus might miss a case where we introduce an ambiguity between two existing methods
                         // We could instead work to sort this into 3 groups `morespecific .. ambiguous .. lesspecific`, with `type` in ambiguous,
                         // such that everything in `morespecific` dominates everything in `ambiguous`, and everything in `ambiguous` dominates everything in `lessspecific`
                         // And then compute where each isect falls, and whether it changed group--necessitating invalidation--or not.
-                        if (morespec[j] == (char)morespec_unknown)
-                            morespec[j] = (char)(jl_type_morespecific(m->sig, type) ? morespec_is : morespec_isnot);
-                        if (morespec[j] == (char)morespec_is)
-                            // not actually shadowing--the existing method is still better
-                            break;
-                        if (ambig == morespec_unknown)
-                            ambig = jl_type_morespecific(type, m->sig) ? morespec_isnot : morespec_is;
-                        // replacing a method--see if this really was the selected method previously
-                        // over the intersection (not ambiguous) and the new method will be selected now (morespec_is)
                         int replaced_dispatch = is_replacing(ambig, type, m, d, n, isect, isect2, morespec);
                         // found that this specialization dispatch got replaced by m
                         // call invalidate_backedges(mi, max_world, "jl_method_table_insert");
@@ -2740,20 +2749,6 @@ void jl_method_table_activate(jl_typemap_entry_t *newentry)
                         invalidated |= invalidatedmi;
                     }
                 }
-                // now compute and store updates to METHOD_SIG_LATEST_ONLY
-                int m_dispatch = jl_atomic_load_relaxed(&m->dispatch_status);
-                if (m_dispatch & METHOD_SIG_LATEST_ONLY) {
-                    if (morespec[j] == (char)morespec_unknown)
-                        morespec[j] = (char)(jl_type_morespecific(m->sig, type) ? morespec_is : morespec_isnot);
-                    if (morespec[j] == (char)morespec_isnot)
-                        jl_atomic_store_relaxed(&m->dispatch_status, ~METHOD_SIG_LATEST_ONLY & m_dispatch);
-                }
-                if (only) {
-                    if (morespec[j] == (char)morespec_is || ambig == morespec_is ||
-                        (ambig == morespec_unknown && !jl_type_morespecific(type, m->sig))) {
-                        only = 0;
-                    }
-                }
             }
         }
 
@@ -2802,7 +2797,7 @@ void jl_method_table_activate(jl_typemap_entry_t *newentry)
         jl_array_ptr_1d_push(_jl_debug_method_invalidation, loctag);
     }
     jl_atomic_store_relaxed(&newentry->max_world, ~(size_t)0);
-    jl_atomic_store_relaxed(&method->dispatch_status, METHOD_SIG_LATEST_WHICH | (only ? METHOD_SIG_LATEST_ONLY : 0)); // TODO: this should be sequenced fully after the world counter store
+    jl_atomic_store_relaxed(&method->dispatch_status, dispatch_bits); // TODO: this should be sequenced fully after the world counter store
     JL_GC_POP();
 }
 

src/julia_internal.h

@@ -677,6 +677,8 @@ typedef union {
 #define METHOD_SIG_LATEST_WHICH             0b0001
 #define METHOD_SIG_LATEST_ONLY              0b0010
 #define METHOD_SIG_PRECOMPILE_MANY          0b0100
+#define METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC 0b1000  // indicates there exists some other method that is not more specific than this one
+#define METHOD_SIG_PRECOMPILE_HAS_NOTMORESPECIFIC 0b10000  // precompiled version of METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC
 
 JL_DLLEXPORT jl_code_instance_t *jl_engine_reserve(jl_method_instance_t *m, jl_value_t *owner);
 JL_DLLEXPORT void jl_engine_fulfill(jl_code_instance_t *ci, jl_code_info_t *src);

src/staticdata.c

@@ -1834,7 +1834,12 @@ static void jl_write_values(jl_serializer_state *s) JL_GC_DISABLED
                     if (jl_atomic_load_relaxed(&newm->primary_world) > 1) {
                         jl_atomic_store_relaxed(&newm->primary_world, ~(size_t)0); // min-world
                         int dispatch_status = jl_atomic_load_relaxed(&newm->dispatch_status);
-                        jl_atomic_store_relaxed(&newm->dispatch_status, dispatch_status & METHOD_SIG_LATEST_ONLY ? 0 : METHOD_SIG_PRECOMPILE_MANY);
+                        int new_dispatch_status = 0;
+                        if (!(dispatch_status & METHOD_SIG_LATEST_ONLY))
+                            new_dispatch_status |= METHOD_SIG_PRECOMPILE_MANY;
+                        if (dispatch_status & METHOD_SIG_LATEST_HAS_NOTMORESPECIFIC)
+                            new_dispatch_status |= METHOD_SIG_PRECOMPILE_HAS_NOTMORESPECIFIC;
+                        jl_atomic_store_relaxed(&newm->dispatch_status, new_dispatch_status);
                         arraylist_push(&s->fixup_objs, (void*)reloc_offset);
                     }
                 }