staticdata: make hookup of code instances correct (#48751)

Previously we would double-account for many of these, leading to
occasional chaos. Try to avoid serializing code that does not belong to
this incremental compilation session package.

Refs: #48723

src/method.c

@@ -610,6 +610,8 @@ JL_DLLEXPORT jl_code_info_t *jl_code_for_staged(jl_method_instance_t *linfo)
         for (int i = 0; i < jl_array_len(func->code); ++i) {
             jl_value_t *stmt = jl_array_ptr_ref(func->code, i);
             if (jl_is_expr(stmt) && ((jl_expr_t*)stmt)->head == jl_new_opaque_closure_sym) {
+                if (jl_options.incremental && jl_generating_output())
+                    jl_error("Impossible to correctly handle OpaqueClosure inside @generated returned during precompile process.");
                 jl_value_t *uninferred = jl_copy_ast((jl_value_t*)func);
                 jl_value_t *old = NULL;
                 if (jl_atomic_cmpswap(&linfo->uninferred, &old, uninferred)) {

src/staticdata.c

@@ -600,10 +600,10 @@ static uintptr_t jl_fptr_id(void *fptr)
 
 // `jl_queue_for_serialization` adds items to `serialization_order`
 #define jl_queue_for_serialization(s, v) jl_queue_for_serialization_((s), (jl_value_t*)(v), 1, 0)
-static void jl_queue_for_serialization_(jl_serializer_state *s, jl_value_t *v, int recursive, int immediate);
+static void jl_queue_for_serialization_(jl_serializer_state *s, jl_value_t *v, int recursive, int immediate) JL_GC_DISABLED;
 
 
-static void jl_queue_module_for_serialization(jl_serializer_state *s, jl_module_t *m)
+static void jl_queue_module_for_serialization(jl_serializer_state *s, jl_module_t *m) JL_GC_DISABLED
 {
     jl_queue_for_serialization(s, m->name);
     jl_queue_for_serialization(s, m->parent);
@@ -634,7 +634,7 @@ static void jl_queue_module_for_serialization(jl_serializer_state *s, jl_module_
 // you want to handle uniquing of `Dict{String,Float64}` before you tackle `Vector{Dict{String,Float64}}`.
 // Uniquing is done in `serialization_order`, so the very first mention of such an object must
 // be the "source" rather than merely a cross-reference.
-static void jl_insert_into_serialization_queue(jl_serializer_state *s, jl_value_t *v, int recursive, int immediate)
+static void jl_insert_into_serialization_queue(jl_serializer_state *s, jl_value_t *v, int recursive, int immediate) JL_GC_DISABLED
 {
     jl_datatype_t *t = (jl_datatype_t*)jl_typeof(v);
     jl_queue_for_serialization_(s, (jl_value_t*)t, 1, immediate);
@@ -659,6 +659,7 @@ static void jl_insert_into_serialization_queue(jl_serializer_state *s, jl_value_
     }
     if (s->incremental && jl_is_method_instance(v)) {
         jl_method_instance_t *mi = (jl_method_instance_t*)v;
+        jl_value_t *def = mi->def.value;
         if (needs_uniquing(v)) {
             // we only need 3 specific fields of this (the rest are not used)
             jl_queue_for_serialization(s, mi->def.value);
@@ -667,13 +668,24 @@ static void jl_insert_into_serialization_queue(jl_serializer_state *s, jl_value_
             recursive = 0;
             goto done_fields;
         }
-        else if (needs_recaching(v)) {
+        else if (jl_is_method(def) && jl_object_in_image(def)) {
             // we only need 3 specific fields of this (the rest are restored afterward, if valid)
+            // in particular, cache is repopulated by jl_mi_cache_insert for all foreign function,
+            // so must not be present here
             record_field_change((jl_value_t**)&mi->uninferred, NULL);
             record_field_change((jl_value_t**)&mi->backedges, NULL);
             record_field_change((jl_value_t**)&mi->callbacks, NULL);
             record_field_change((jl_value_t**)&mi->cache, NULL);
         }
+        else {
+            assert(!needs_recaching(v));
+        }
+        // n.b. opaque closures cannot be inspected and relied upon like a
+        // normal method since they can get improperly introduced by generated
+        // functions, so if they appeared at all, we will probably serialize
+        // them wrong and segfault. The jl_code_for_staged function should
+        // prevent this from happening, so we do not need to detect that user
+        // error now.
     }
     if (s->incremental && jl_is_globalref(v)) {
         jl_globalref_t *gr = (jl_globalref_t*)v;
@@ -691,6 +703,15 @@ static void jl_insert_into_serialization_queue(jl_serializer_state *s, jl_value_
             assert(!jl_object_in_image((jl_value_t*)tn->wrapper));
         }
     }
+    if (s->incremental && jl_is_code_instance(v)) {
+        jl_code_instance_t *ci = (jl_code_instance_t*)v;
+        // make sure we don't serialize other reachable cache entries of foreign methods
+        if (jl_object_in_image((jl_value_t*)ci->def->def.value)) {
+            // TODO: if (ci in ci->defs->cache)
+            record_field_change((jl_value_t**)&ci->next, NULL);
+        }
+    }
+
 
     if (immediate) // must be things that can be recursively handled, and valid as type parameters
         assert(jl_is_immutable(t) || jl_is_typevar(v) || jl_is_symbol(v) || jl_is_svec(v));
@@ -769,7 +790,7 @@ done_fields: ;
     *bp = (void*)((char*)HT_NOTFOUND + 1 + idx);
 }
 
-static void jl_queue_for_serialization_(jl_serializer_state *s, jl_value_t *v, int recursive, int immediate)
+static void jl_queue_for_serialization_(jl_serializer_state *s, jl_value_t *v, int recursive, int immediate) JL_GC_DISABLED
 {
     if (!jl_needs_serialization(s, v))
         return;
@@ -812,7 +833,7 @@ static void jl_queue_for_serialization_(jl_serializer_state *s, jl_value_t *v, i
 // Do a pre-order traversal of the to-serialize worklist, in the identical order
 // to the calls to jl_queue_for_serialization would occur in a purely recursive
 // implementation, but without potentially running out of stack.
-static void jl_serialize_reachable(jl_serializer_state *s)
+static void jl_serialize_reachable(jl_serializer_state *s) JL_GC_DISABLED
 {
     size_t i, prevlen = 0;
     while (object_worklist.len) {
@@ -2813,10 +2834,11 @@ static void jl_restore_system_image_from_stream_(ios_t *f, jl_image_t *image, jl
         *method_roots_list = (jl_array_t*)jl_delayed_reloc(&s, offset_method_roots_list);
         *ext_targets = (jl_array_t*)jl_delayed_reloc(&s, offset_ext_targets);
         *edges = (jl_array_t*)jl_delayed_reloc(&s, offset_edges);
+        if (!*new_specializations)
+            *new_specializations = jl_alloc_vec_any(0);
     }
     s.s = NULL;
 
-
     // step 3: apply relocations
     assert(!ios_eof(f));
     jl_read_symbols(&s);
@@ -3071,19 +3093,8 @@ static void jl_restore_system_image_from_stream_(ios_t *f, jl_image_t *image, jl
             jl_code_instance_t *ci = (jl_code_instance_t*)obj;
             assert(s.incremental);
             ci->min_world = world;
-            if (ci->max_world == 1) { // sentinel value: has edges to external callables
-                ptrhash_put(&new_code_instance_validate, ci, (void*)(~(uintptr_t)HT_NOTFOUND));   // "HT_FOUND"
-            }
-            else if (ci->max_world) {
-                // It's valid, but it may not be connected
-                if (!jl_atomic_load_relaxed(&ci->def->cache))
-                    jl_atomic_store_release(&ci->def->cache, ci);
-            }
-            else {
-                // Ensure this code instance is not connected
-                if (jl_atomic_load_relaxed(&ci->def->cache) == ci)
-                    jl_atomic_store_release(&ci->def->cache, NULL);
-            }
+            if (ci->max_world != 0)
+                jl_array_ptr_1d_push(*new_specializations, (jl_value_t*)ci);
         }
         else if (jl_is_globalref(obj)) {
             continue; // wait until all the module binding tables have been initialized
@@ -3249,7 +3260,6 @@ static jl_value_t *jl_restore_package_image_from_stream(ios_t *f, jl_image_t *im
         else {
             ios_close(f);
             ios_static_buffer(f, sysimg, len);
-            htable_new(&new_code_instance_validate, 0);
             pkgcachesizes cachesizes;
             jl_restore_system_image_from_stream_(f, image, depmods, checksum, (jl_array_t**)&restored, &init_order, &extext_methods, &new_specializations, &method_roots_list, &ext_targets, &edges, &base, &ccallable_list, &cachesizes);
             JL_SIGATOMIC_END();
@@ -3261,12 +3271,9 @@ static jl_value_t *jl_restore_package_image_from_stream(ios_t *f, jl_image_t *im
             jl_copy_roots(method_roots_list, jl_worklist_key((jl_array_t*)restored));
             // Handle edges
             jl_insert_backedges((jl_array_t*)edges, (jl_array_t*)ext_targets, (jl_array_t*)new_specializations); // restore external backedges (needs to be last)
-            // check new CodeInstances and validate any that lack external backedges
-            validate_new_code_instances();
             // reinit ccallables
             jl_reinit_ccallable(&ccallable_list, base, NULL);
             arraylist_free(&ccallable_list);
-            htable_free(&new_code_instance_validate);
             if (complete) {
                 cachesizes_sv = jl_alloc_svec_uninit(7);
                 jl_svec_data(cachesizes_sv)[0] = jl_box_long(cachesizes.sysdata);

src/staticdata_utils.c

@@ -1,6 +1,3 @@
-static htable_t new_code_instance_validate;
-
-
 // inverse of backedges graph (caller=>callees hash)
 jl_array_t *edges_map JL_GLOBALLY_ROOTED = NULL; // rooted for the duration of our uses of this
 
@@ -172,48 +169,50 @@ static int has_backedge_to_worklist(jl_method_instance_t *mi, htable_t *visited,
     // HT_NOTFOUND: not yet analyzed
     // HT_NOTFOUND + 1: no link back
     // HT_NOTFOUND + 2: does link back
-    // HT_NOTFOUND + 3 + depth: in-progress
+    // HT_NOTFOUND + 3: does link back, and included in new_specializations already
+    // HT_NOTFOUND + 4 + depth: in-progress
     int found = (char*)*bp - (char*)HT_NOTFOUND;
     if (found)
         return found - 1;
     arraylist_push(stack, (void*)mi);
     int depth = stack->len;
-    *bp = (void*)((char*)HT_NOTFOUND + 3 + depth); // preliminarily mark as in-progress
+    *bp = (void*)((char*)HT_NOTFOUND + 4 + depth); // preliminarily mark as in-progress
     size_t i = 0, n = jl_array_len(mi->backedges);
     int cycle = 0;
     while (i < n) {
         jl_method_instance_t *be;
         i = get_next_edge(mi->backedges, i, NULL, &be);
         int child_found = has_backedge_to_worklist(be, visited, stack);
-        if (child_found == 1) {
+        if (child_found == 1 || child_found == 2) {
             // found what we were looking for, so terminate early
             found = 1;
             break;
         }
-        else if (child_found >= 2 && child_found - 2 < cycle) {
+        else if (child_found >= 3 && child_found - 3 < cycle) {
             // record the cycle will resolve at depth "cycle"
-            cycle = child_found - 2;
+            cycle = child_found - 3;
             assert(cycle);
         }
     }
     if (!found && cycle && cycle != depth)
-        return cycle + 2;
+        return cycle + 3;
     // If we are the top of the current cycle, now mark all other parts of
     // our cycle with what we found.
     // Or if we found a backedge, also mark all of the other parts of the
     // cycle as also having an backedge.
     while (stack->len >= depth) {
         void *mi = arraylist_pop(stack);
         bp = ptrhash_bp(visited, mi);
-        assert((char*)*bp - (char*)HT_NOTFOUND == 4 + stack->len);
+        assert((char*)*bp - (char*)HT_NOTFOUND == 5 + stack->len);
         *bp = (void*)((char*)HT_NOTFOUND + 1 + found);
     }
     return found;
 }
 
 // Given the list of CodeInstances that were inferred during the build, select
-// those that are (1) external, (2) still valid, and (3) are inferred to be
-// called from the worklist or explicitly added by a `precompile` statement.
+// those that are (1) external, (2) still valid, (3) are inferred to be called
+// from the worklist or explicitly added by a `precompile` statement, and
+// (4) are the most recently computed result for that method.
 // These will be preserved in the image.
 static jl_array_t *queue_external_cis(jl_array_t *list)
 {
@@ -228,23 +227,35 @@ static jl_array_t *queue_external_cis(jl_array_t *list)
     arraylist_new(&stack, 0);
     jl_array_t *new_specializations = jl_alloc_vec_any(0);
     JL_GC_PUSH1(&new_specializations);
-    for (i = 0; i < n0; i++) {
+    for (i = n0; i-- > 0; ) {
         jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(list, i);
         assert(jl_is_code_instance(ci));
         jl_method_instance_t *mi = ci->def;
         jl_method_t *m = mi->def.method;
-        if (jl_is_method(m) && jl_object_in_image((jl_value_t*)m->module)) {
+        if (ci->inferred && jl_is_method(m) && jl_object_in_image((jl_value_t*)m->module)) {
             int found = has_backedge_to_worklist(mi, &visited, &stack);
-            assert(found == 0 || found == 1);
+            assert(found == 0 || found == 1 || found == 2);
             assert(stack.len == 0);
             if (found == 1 && ci->max_world == ~(size_t)0) {
-                jl_array_ptr_1d_push(new_specializations, (jl_value_t*)ci);
+                void **bp = ptrhash_bp(&visited, mi);
+                if (*bp != (void*)((char*)HT_NOTFOUND + 3)) {
+                    *bp = (void*)((char*)HT_NOTFOUND + 3);
+                    jl_array_ptr_1d_push(new_specializations, (jl_value_t*)ci);
+                }
             }
         }
     }
     htable_free(&visited);
     arraylist_free(&stack);
     JL_GC_POP();
+    // reverse new_specializations
+    n0 = jl_array_len(new_specializations);
+    jl_value_t **news = (jl_value_t**)jl_array_data(new_specializations);
+    for (i = 0; i < n0; i++) {
+        jl_value_t *temp = news[i];
+        news[i] = news[n0 - i - 1];
+        news[n0 - i - 1] = temp;
+    }
     return new_specializations;
 }
 
@@ -810,11 +821,6 @@ static void jl_copy_roots(jl_array_t *method_roots_list, uint64_t key)
     }
 }
 
-static int remove_code_instance_from_validation(jl_code_instance_t *codeinst)
-{
-    return ptrhash_remove(&new_code_instance_validate, codeinst);
-}
-
 // verify that these edges intersect with the same methods as before
 static jl_array_t *jl_verify_edges(jl_array_t *targets)
 {
@@ -1045,45 +1051,30 @@ static void jl_insert_backedges(jl_array_t *edges, jl_array_t *ext_targets, jl_a
     htable_new(&visited, 0);
     jl_verify_methods(edges, valids, &visited); // consumes valids, creates visited
     valids = jl_verify_graph(edges, &visited); // consumes visited, creates valids
-    size_t i, l = jl_array_len(edges) / 2;
+    size_t i, l;
 
     // next build a map from external MethodInstances to their CodeInstance for insertion
-    if (ci_list == NULL) {
-        htable_reset(&visited, 0);
-    }
-    else {
-        size_t i, l = jl_array_len(ci_list);
-        htable_reset(&visited, l);
-        for (i = 0; i < l; i++) {
-            jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(ci_list, i);
-            assert(ci->max_world == 1 || ci->max_world == ~(size_t)0);
-            assert(ptrhash_get(&visited, (void*)ci->def) == HT_NOTFOUND);   // check that we don't have multiple cis for same mi
-            ptrhash_put(&visited, (void*)ci->def, (void*)ci);
-        }
-    }
-
-    // next disable any invalid codes, so we do not try to enable them
+    l = jl_array_len(ci_list);
+    htable_reset(&visited, l);
     for (i = 0; i < l; i++) {
-        jl_method_instance_t *caller = (jl_method_instance_t*)jl_array_ptr_ref(edges, 2 * i);
-        assert(jl_is_method_instance(caller) && jl_is_method(caller->def.method));
-        int valid = jl_array_uint8_ref(valids, i);
-        if (valid)
-            continue;
-        void *ci = ptrhash_get(&visited, (void*)caller);
-        if (ci != HT_NOTFOUND) {
-            assert(jl_is_code_instance(ci));
-            remove_code_instance_from_validation((jl_code_instance_t*)ci); // mark it as handled
+        jl_code_instance_t *ci = (jl_code_instance_t*)jl_array_ptr_ref(ci_list, i);
+        assert(ci->min_world == world);
+        if (ci->max_world == 1) { // sentinel value: has edges to external callables
+            ptrhash_put(&visited, (void*)ci->def, (void*)ci);
         }
         else {
-            jl_code_instance_t *codeinst = jl_atomic_load_relaxed(&caller->cache);
-            while (codeinst) {
-                remove_code_instance_from_validation(codeinst); // should be left invalid
-                codeinst = jl_atomic_load_relaxed(&codeinst->next);
+            assert(ci->max_world == ~(size_t)0);
+            jl_method_instance_t *caller = ci->def;
+            if (ci->inferred && jl_rettype_inferred(caller, world, ~(size_t)0) == jl_nothing) {
+                jl_mi_cache_insert(caller, ci);
             }
+            //jl_static_show((jl_stream*)ios_stderr, (jl_value_t*)caller);
+            //ios_puts("free\n", ios_stderr);
         }
     }
 
-    // finally enable any applicable new codes
+    // next enable any applicable new codes
+    l = jl_array_len(edges) / 2;
     for (i = 0; i < l; i++) {
         jl_method_instance_t *caller = (jl_method_instance_t*)jl_array_ptr_ref(edges, 2 * i);
         int valid = jl_array_uint8_ref(valids, i);
@@ -1110,29 +1101,19 @@ static void jl_insert_backedges(jl_array_t *edges, jl_array_t *ext_targets, jl_a
                     jl_method_table_add_backedge(mt, sig, (jl_value_t*)caller);
             }
         }
-        // then enable it
+        // then enable any methods associated with it
         void *ci = ptrhash_get(&visited, (void*)caller);
+        //assert(ci != HT_NOTFOUND);
         if (ci != HT_NOTFOUND) {
             // have some new external code to use
             assert(jl_is_code_instance(ci));
             jl_code_instance_t *codeinst = (jl_code_instance_t*)ci;
-            remove_code_instance_from_validation(codeinst); // mark it as handled
             assert(codeinst->min_world == world && codeinst->inferred);
             codeinst->max_world = ~(size_t)0;
             if (jl_rettype_inferred(caller, world, ~(size_t)0) == jl_nothing) {
                 jl_mi_cache_insert(caller, codeinst);
             }
         }
-        else {
-            jl_code_instance_t *codeinst = jl_atomic_load_relaxed(&caller->cache);
-            while (codeinst) {
-                if (remove_code_instance_from_validation(codeinst)) { // mark it as handled
-                    assert(codeinst->min_world >= world && codeinst->inferred);
-                    codeinst->max_world = ~(size_t)0;
-                }
-                codeinst = jl_atomic_load_relaxed(&codeinst->next);
-            }
-        }
     }
 
     htable_free(&visited);
@@ -1148,27 +1129,6 @@ static void classify_callers(htable_t *callers_with_edges, jl_array_t *edges)
     }
 }
 
-static void validate_new_code_instances(void)
-{
-    size_t world = jl_atomic_load_acquire(&jl_world_counter);
-    size_t i;
-    for (i = 0; i < new_code_instance_validate.size; i += 2) {
-        if (new_code_instance_validate.table[i+1] != HT_NOTFOUND) {
-            //assert(0 && "unexpected unprocessed CodeInstance found");
-            jl_code_instance_t *ci = (jl_code_instance_t*)new_code_instance_validate.table[i];
-            JL_GC_PROMISE_ROOTED(ci); // TODO: this needs a root (or restructuring to avoid it)
-            assert(ci->min_world == world && ci->inferred);
-            assert(ci->max_world == ~(size_t)0);
-            jl_method_instance_t *caller = ci->def;
-            if (jl_rettype_inferred(caller, world, ~(size_t)0) == jl_nothing) {
-                jl_mi_cache_insert(caller, ci);
-            }
-            //jl_static_show((JL_STREAM*)ios_stderr, (jl_value_t*)caller);
-            //ios_puts("FREE\n", ios_stderr);
-        }
-    }
-}
-
 static jl_value_t *read_verify_mod_list(ios_t *s, jl_array_t *depmods)
 {
     if (!jl_main_module->build_id.lo) {