Skip to content

Commit

Permalink
Add GC write barrier in jl_reserve_excstack (#51096)
Browse files Browse the repository at this point in the history
`jl_push_excstack` and `jl_reserve_excstack` take an address to the
field `excstack` in `jl_task_t`, and may update the field. In such a
case, a GC write barrier is needed. This PR adds the missing write
barrier.

Note: I am not sure if this affects the correctness of Julia GC or not.
This could be special cased in the GC code, as the task is always
rooted. However, generally if the task is in the old generation, it does
not have to be scanned even if it is a root. A write barrier is needed
to make sure the GC is aware of the update.
  • Loading branch information
qinsoon authored Aug 30, 2023
1 parent f9792b4 commit 197180d
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 6 deletions.
2 changes: 1 addition & 1 deletion src/julia_internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -1219,7 +1219,7 @@ STATIC_INLINE size_t jl_excstack_next(jl_excstack_t *stack, size_t itr) JL_NOTSA
return itr-2 - jl_excstack_bt_size(stack, itr);
}
// Exception stack manipulation
void jl_push_excstack(jl_excstack_t **stack JL_REQUIRE_ROOTED_SLOT JL_ROOTING_ARGUMENT,
void jl_push_excstack(jl_task_t* task, jl_excstack_t **stack JL_REQUIRE_ROOTED_SLOT JL_ROOTING_ARGUMENT,
jl_value_t *exception JL_ROOTED_ARGUMENT,
jl_bt_element_t *bt_data, size_t bt_size);

Expand Down
7 changes: 4 additions & 3 deletions src/rtutils.c
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ static void jl_copy_excstack(jl_excstack_t *dest, jl_excstack_t *src) JL_NOTSAFE
dest->top = src->top;
}

static void jl_reserve_excstack(jl_excstack_t **stack JL_REQUIRE_ROOTED_SLOT,
static void jl_reserve_excstack(jl_task_t* task, jl_excstack_t **stack JL_REQUIRE_ROOTED_SLOT,
size_t reserved_size)
{
jl_excstack_t *s = *stack;
Expand All @@ -339,13 +339,14 @@ static void jl_reserve_excstack(jl_excstack_t **stack JL_REQUIRE_ROOTED_SLOT,
if (s)
jl_copy_excstack(new_s, s);
*stack = new_s;
jl_gc_wb(task, new_s);
}

void jl_push_excstack(jl_excstack_t **stack JL_REQUIRE_ROOTED_SLOT JL_ROOTING_ARGUMENT,
void jl_push_excstack(jl_task_t* task, jl_excstack_t **stack JL_REQUIRE_ROOTED_SLOT JL_ROOTING_ARGUMENT,
jl_value_t *exception JL_ROOTED_ARGUMENT,
jl_bt_element_t *bt_data, size_t bt_size)
{
jl_reserve_excstack(stack, (*stack ? (*stack)->top : 0) + bt_size + 2);
jl_reserve_excstack(task, stack, (*stack ? (*stack)->top : 0) + bt_size + 2);
jl_excstack_t *s = *stack;
jl_bt_element_t *rawstack = jl_excstack_raw(s);
memcpy(rawstack + s->top, bt_data, sizeof(jl_bt_element_t)*bt_size);
Expand Down
4 changes: 2 additions & 2 deletions src/task.c
Original file line number Diff line number Diff line change
Expand Up @@ -721,7 +721,7 @@ JL_DLLEXPORT JL_NORETURN void jl_no_exc_handler(jl_value_t *e, jl_task_t *ct)
/* The temporary ptls->bt_data is rooted by special purpose code in the\
GC. This exists only for the purpose of preserving bt_data until we \
set ptls->bt_size=0 below. */ \
jl_push_excstack(&ct->excstack, exception, \
jl_push_excstack(ct, &ct->excstack, exception, \
ptls->bt_data, ptls->bt_size); \
ptls->bt_size = 0; \
} \
Expand Down Expand Up @@ -1224,7 +1224,7 @@ CFI_NORETURN
jl_timing_block_task_enter(ct, ptls, NULL);
if (jl_atomic_load_relaxed(&ct->_isexception)) {
record_backtrace(ptls, 0);
jl_push_excstack(&ct->excstack, ct->result,
jl_push_excstack(ct, &ct->excstack, ct->result,
ptls->bt_data, ptls->bt_size);
res = ct->result;
}
Expand Down

2 comments on commit 197180d

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

Please sign in to comment.