Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Compiler/test/inference.jl
Original file line number Diff line number Diff line change
Expand Up @@ -6428,4 +6428,21 @@ global invalid_setglobal!_exct_modeling::Int
setglobal!(@__MODULE__, :invalid_setglobal!_exct_modeling, x)
end == ErrorException

# Issue #58257 - Hang in inference during BindingPartition resolution
module A58257
module B58257
using ..A58257
# World age here is N
end
using .B58257
# World age here is N+1
@eval f() = $(GlobalRef(B58257, :get!))
end

## The sequence of events is critical here.
A58257.get! # Creates binding partition in A, N+1:∞
A58257.B58257.get! # Creates binding partition in A.B, N+1:∞
Base.invoke_in_world(UInt(38678), getglobal, A58257, :get!) # Expands binding partition in A through <N
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you using random numbers as worlds? This will break very badly someday...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was supposed to be captured earlier, just forgot to commit the change. will fix.

@test Base.infer_return_type(A58257.f) == typeof(Base.get!) # Attempt to lookup A.B in world age N hangs

end # module inference
20 changes: 11 additions & 9 deletions src/module.c
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,18 @@ static jl_binding_partition_t *jl_implicit_import_resolved(jl_binding_t *b, stru
if (jl_is_binding_partition(gap.parent)) {
// Check if we can merge this into the previous binding partition
jl_binding_partition_t *prev = (jl_binding_partition_t *)gap.parent;
assert(new_max_world != ~(size_t)0); // It is inconsistent to have a gap with `gap.parent` set, but max_world == ~(size_t)0
size_t expected_prev_min_world = new_max_world + 1;
if (prev->restriction == resolution.binding_or_const && prev->kind == new_kind) {
retry:
if (!jl_atomic_cmpswap(&prev->min_world, &expected_prev_min_world, new_min_world)) {
if (expected_prev_min_world <= new_min_world) {
return prev;
}
else if (expected_prev_min_world <= new_max_world) {
// Concurrent modification by another thread - bail.
return NULL;
// Concurrent modification of the partition. However, our lookup is still valid,
// so we should still be able to extend the partition.
goto retry;
}
// There remains a gap - proceed
} else {
Expand All @@ -154,7 +157,7 @@ static jl_binding_partition_t *jl_implicit_import_resolved(jl_binding_t *b, stru
for (;;) {
// We've updated the previous partition - check if we've closed a gap
size_t next_max_world = jl_atomic_load_relaxed(&next->max_world);
if (next_max_world == expected_prev_min_world-1 && next->kind == new_kind && next->restriction == resolution.binding_or_const) {
if (next_max_world >= expected_prev_min_world-1 && next->kind == new_kind && next->restriction == resolution.binding_or_const) {
if (jl_atomic_cmpswap(&prev->min_world, &expected_prev_min_world, next_min_world)) {
jl_binding_partition_t *nextnext = jl_atomic_load_relaxed(&next->next);
if (!jl_atomic_cmpswap(&prev->next, &next, nextnext)) {
Expand Down Expand Up @@ -370,15 +373,15 @@ JL_DLLEXPORT void jl_update_loaded_bpart(jl_binding_t *b, jl_binding_partition_t
bpart->kind = resolution.ultimate_kind;
}

STATIC_INLINE jl_binding_partition_t *jl_get_binding_partition_(jl_binding_t *b JL_PROPAGATES_ROOT, jl_value_t *parent, _Atomic(jl_binding_partition_t *)*insert, size_t world, modstack_t *st) JL_GLOBALLY_ROOTED
STATIC_INLINE jl_binding_partition_t *jl_get_binding_partition_(jl_binding_t *b JL_PROPAGATES_ROOT, jl_value_t *parent, _Atomic(jl_binding_partition_t *)*insert, size_t world, size_t max_world, modstack_t *st) JL_GLOBALLY_ROOTED
{
assert(jl_is_binding(b));
struct implicit_search_gap gap;
gap.parent = parent;
gap.insert = insert;
gap.inherited_flags = 0;
gap.min_world = 0;
gap.max_world = ~(size_t)0;
gap.max_world = max_world;
while (1) {
gap.replace = jl_atomic_load_relaxed(gap.insert);
jl_binding_partition_t *bpart = jl_get_binding_partition__(b, world, &gap);
Expand All @@ -395,15 +398,14 @@ jl_binding_partition_t *jl_get_binding_partition(jl_binding_t *b, size_t world)
if (!b)
return NULL;
// Duplicate the code for the entry frame for branch prediction
return jl_get_binding_partition_(b, (jl_value_t*)b, &b->partitions, world, NULL);
return jl_get_binding_partition_(b, (jl_value_t*)b, &b->partitions, world, ~(size_t)0, NULL);
}

jl_binding_partition_t *jl_get_binding_partition_with_hint(jl_binding_t *b, jl_binding_partition_t *prev, size_t world) JL_GLOBALLY_ROOTED {
// Helper for getting a binding partition for an older world after we've already looked up the partition for a newer world
assert(b);
// TODO: Is it possible for a concurrent lookup to have expanded this bpart, making this false?
assert(jl_atomic_load_relaxed(&prev->min_world) > world);
return jl_get_binding_partition_(b, (jl_value_t*)prev, &prev->next, world, NULL);
size_t prev_min_world = jl_atomic_load_relaxed(&prev->min_world);
return jl_get_binding_partition_(b, (jl_value_t*)prev, &prev->next, world, prev_min_world-1, NULL);
}

jl_binding_partition_t *jl_get_binding_partition_all(jl_binding_t *b, size_t min_world, size_t max_world) {
Expand Down