From bcc6d71260681d899bf7b64fe6747d5357723a54 Mon Sep 17 00:00:00 2001 From: Jameson Nash Date: Tue, 21 Feb 2023 16:04:20 -0500 Subject: [PATCH] staticdata: make hookup of code instances correct 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 | 2 + src/staticdata.c | 55 ++++++++++-------- src/staticdata_utils.c | 128 ++++++++++++++--------------------------- 3 files changed, 77 insertions(+), 108 deletions(-) diff --git a/src/method.c b/src/method.c index 946f9d986a1ed..d5f56a5921358 100644 --- a/src/method.c +++ b/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)) { diff --git a/src/staticdata.c b/src/staticdata.c index 179df35ff568a..c4ed2b531ae0f 100644 --- a/src/staticdata.c +++ b/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); diff --git a/src/staticdata_utils.c b/src/staticdata_utils.c index e577394c67fae..41d0bdc216fba 100644 --- a/src/staticdata_utils.c +++ b/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,32 +169,33 @@ 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 @@ -205,15 +203,16 @@ static int has_backedge_to_worklist(jl_method_instance_t *mi, htable_t *visited, 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) {