fix jl_active_task_stack to return correct values #56477

vtjnash · 2024-11-06T18:39:26Z

Store the activefp at every point where it might be observed to have changed. Seems potentially slightly costly to need to generate these extra writes for every safepoint transition to keep track of the current stack location, but that is potentially an unavoidable cost to doing conservative stack scanning for gc.

Replace #54923

fingolfin · 2024-11-21T22:15:44Z

This would be really nice to have :-)

fingolfin

Looks good to me to the extent that I can evaluate it. I really hope people like @JeffBezanson @d-netto @gbaraldi @udesou might find time to have a look so it can be merged

fingolfin · 2025-01-20T16:46:30Z

src/julia_threads.h

+typedef struct _jl_excstack_t jl_excstack_t;
+typedef struct _jl_handler_t jl_handler_t;
+
+typedef struct _jl_task_t {


A major chunk of this PR seems to be about moving _jl_task_t from julia.h to here. Is there a functional reason why this is necessary for this PR? Perhaps something got added to _jl_task_t -- but it's difficult to say because everything is in a single commit

Made PR #57117 to take care of this

fingolfin · 2025-01-20T16:46:50Z

src/gc-stock.c

@@ -3335,6 +3335,7 @@ JL_DLLEXPORT void jl_gc_collect(jl_gc_collection_t collection)
    }
    jl_gc_debug_print();

+    ct->ctx.activefp = (char*)jl_get_frame_addr();


OK, that one seems simple enough :-)

fingolfin · 2025-01-20T16:48:59Z

src/julia_internal.h

@@ -1720,18 +1720,6 @@ JL_DLLEXPORT unsigned jl_special_vector_alignment(size_t nfields, jl_value_t *fi
 void register_eh_frames(uint8_t *Addr, size_t Size) JL_NOTSAFEPOINT;
 void deregister_eh_frames(uint8_t *Addr, size_t Size) JL_NOTSAFEPOINT;

-STATIC_INLINE void *jl_get_frame_addr(void) JL_NOTSAFEPOINT


Moved to julia_threads.h so it can be called there, OK

fingolfin · 2025-01-20T16:54:39Z

src/julia_threads.h

@@ -270,6 +345,9 @@ STATIC_INLINE int8_t jl_gc_state_set(jl_ptls_t ptls, int8_t state,
 {
    assert(old_state != JL_GC_PARALLEL_COLLECTOR_THREAD);
    assert(old_state != JL_GC_CONCURRENT_COLLECTOR_THREAD);
+    if (__builtin_constant_p(state) ? state : // required if !old_state && state, otherwise optional
+        __builtin_constant_p(old_state) ? !old_state : 1)
+        jl_atomic_load_relaxed(&ptls->current_task)->ctx.activefp = (char*)jl_get_frame_addr();


I have no idea what the semantics for jl_gc_state_set are, when it is supposed to be called, etc., so I can't judge whether this is sensible or effective.

This seems to store the current frame pointer into the task. Presumably that means this function is being called from the task as it is about to switch out? At least that would make sense.

My personal dream is that the wonderful people adding MMTk and doing other GC restructuring work, like @gbaraldi @d-netto @udesou), add some more comments describing such things as they figure this stuff out for themselves ;-).

Not sure if we should be updating the activefp in jl_gc_state_set?

Makes more sense to me to update this in the segv_handler or jl_set_gc_and_wait.

Also, I believe we should fetch the frame pointer from the context the thread was running with before it got trapped in the signal handler -- note that we run the signal handlers with a different stack.

This is more or less what I started doing in #55003.

Not sure if we should be updating the activefp in jl_gc_state_set? ... Also, I believe we should fetch the frame pointer from the context the thread was running with before it got trapped in the signal handler -- note that we run the signal handlers with a different stack.

It ends up being mandatory, since we don't stop all threads when reading their pgcstack, although that does mean it may be often unreliable / unsafe to conservatively scan the stack of the other threads. The signal handler issue is a good point. We would need to pass through the ucontext_t to jl_set_gc_and_wait, like we do on Apple, to get a stack pointer from there (plus redzone).

Even with those changed though, we know now that in v1.11+ the llvm optimizations have reached a point where we know many cases that will necessarily cause the GC to segfault when doing conservative stack scanning, so there isn't any safe way to do this. I am going to go ahead and close out this PR, though someone could revive it under a build flag if they really care about it, and don't care if using conservative GC makes their code segfault sometimes or are willing to run with -O0.

Well, we already do use conservative GC in Julia 1.11 and master... though we do it by catching the "segfaults", though really the only ones we had so far were due to incorrect values for the stack start being reported causing us to read into guard pages.

Though perhaps we'd be crashing more if we used tasks extensively? I don't know enough about what the problem is right now / how "wrong" the currently reported values are? I thought they are just "too big" as in the stack is overestimated? But maybe that's not true anymore?

The observation now is that values won't be allocated correctly once they are provably garbage (during optimization), which may lead to the GC trying to access memory that is not valid to access. So you need to either turn off optimizations (-O0) or use a precise stack scan, or just hope to be lucky that it doesn't crash often.

I still don't quite understand what that means. Anyway based on the bits I do understand I hope we won't be affected because we don't scan for general Julia objects, just for those of a specific "foreign" type. Since these are allocated via the C ABI from C code I don't think the LLVM optimizer has any chance of optimizing them away in any way...

fingolfin · 2025-01-20T16:58:48Z

src/llvm-codegen-shared.h

+        Value *currentfp = builder.CreateConstInBoundsGEP1_32(T_int8, task, offset, "gc_state");
+        Value *fp = builder.CreateIntrinsic(Intrinsic::frameaddress, {builder.getPtrTy()}, {builder.getInt32(0)});
+        builder.CreateAlignedStore(fp, currentfp, Align(sizeof(void*)));
+    }


I have no idea how this code is supposed to work, so I can't review it other than on a superficial level: My guess would be that this function is a code-emitting counterpart to jl_gc_state_set, and so a similar modification as there is applied here, to update activefp to contain the current framepointer. The inserted code looks plausibly like it might be doing that

fingolfin · 2025-01-20T16:59:45Z

src/task.c

-    *size = task->ctx.bufsz - off;
-    return (void *)((char *)task->ctx.stkbuf + off);
-}
-
 JL_DLLEXPORT void jl_active_task_stack(jl_task_t *task,


THe changes in jl_active_task_stack look plausible to me

This was removed previously in PR #54527 but had to be reverted in PR #54559 as one usage remained (more by accident then by design). This has since then been resolved.

Seems potentially slightly costly to need to generate these extra writes for every safepoint transition to keep track of the current stack location, but that is potentially an unavoidable cost to doing conservative stack scanning for gc.

This was removed previously in PR #54527 but had to be reverted in PR #54559 as one usage remained (more by accident then by design). This has since then been resolved. This is also part of PR #56477 but that seems stalled right now, and in fact merging parts of it now may make the review of it easier later on...

This is a subset of PR #56477 . I hope that it will be easier to get this merged first, as it just moves things around, afterwards we can update #56477 and it will be considerably smaller, and will thus hopefully break less often (and in less bad ways) over time. (I can perform the required update of that PR, too). See also PR #57116 which contains another independent part of that PR.

This was removed previously in PR JuliaLang#54527 but had to be reverted in PR JuliaLang#54559 as one usage remained (more by accident then by design). This has since then been resolved. This is also part of PR JuliaLang#56477 but that seems stalled right now, and in fact merging parts of it now may make the review of it easier later on...

This is a subset of PR JuliaLang#56477 . I hope that it will be easier to get this merged first, as it just moves things around, afterwards we can update JuliaLang#56477 and it will be considerably smaller, and will thus hopefully break less often (and in less bad ways) over time. (I can perform the required update of that PR, too). See also PR JuliaLang#57116 which contains another independent part of that PR.

vtjnash added the GC Garbage collector label Nov 6, 2024

vtjnash requested a review from JeffBezanson November 6, 2024 18:39

fingolfin approved these changes Jan 20, 2025

View reviewed changes

IanButterworth mentioned this pull request Jan 20, 2025

Default to 1 interactive thread #57087

Merged

6 tasks

fingolfin mentioned this pull request Jan 21, 2025

Remove jl_task_stack_buffer (again) #57116

Merged

Remove jl_task_stack_buffer

9d2210c

This was removed previously in PR #54527 but had to be reverted in PR #54559 as one usage remained (more by accident then by design). This has since then been resolved.

fingolfin mentioned this pull request Jan 21, 2025

Move jl_task_t to julia_threads.h #57117

Merged

fingolfin force-pushed the jn/jl_active_task_stack branch from 557fcb7 to f1b0405 Compare January 21, 2025 14:33

fingolfin and others added 2 commits January 21, 2025 15:34

Move jl_task_t to julia_threads.h

be7b44d

fingolfin force-pushed the jn/jl_active_task_stack branch from f1b0405 to df68965 Compare January 21, 2025 14:37

vtjnash closed this Jan 22, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix jl_active_task_stack to return correct values #56477

fix jl_active_task_stack to return correct values #56477

vtjnash commented Nov 6, 2024 •

edited

Loading

fingolfin commented Nov 21, 2024

fingolfin left a comment

fingolfin Jan 20, 2025

fingolfin Jan 21, 2025

fingolfin Jan 20, 2025

fingolfin Jan 20, 2025

fingolfin Jan 20, 2025

d-netto Jan 21, 2025

vtjnash Jan 22, 2025

fingolfin Jan 22, 2025

vtjnash Jan 22, 2025 •

edited

Loading

fingolfin Jan 22, 2025

fingolfin Jan 20, 2025

fingolfin Jan 20, 2025

fix jl_active_task_stack to return correct values #56477

fix jl_active_task_stack to return correct values #56477

Conversation

vtjnash commented Nov 6, 2024 • edited Loading

fingolfin commented Nov 21, 2024

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash commented Nov 6, 2024 •

edited

Loading

vtjnash Jan 22, 2025 •

edited

Loading