Preserve the scope across the exception handler

[vchuravy]

Fixes
#60620 (comment)
previously we were preserving the new scope across the exception
handler, instead of the scope that was actually stored in the eh
context.

The additionaly write_barriers are probably not necessary (the task
should always be young.

Alternative to #60620

[Keno]

LGTM on the root change, but NAK on the unnecessary write barriers as inline commented (and similar for the ones in the runtime).

[vchuravy]

@Keno I added comments for the wbs. Does that look okay, or do you want to use something like jl_gc_wb_current_task?

[Keno]

@Keno I added comments for the wbs. Does that look okay, or do you want to use something like jl_gc_wb_current_task?

If you're gonna add a comment anyway, might as well make it executable, so that it's standardized.

[vchuravy]

If you're gonna add a comment anyway, might as well make it executable, so that it's standardized

I was looking for a previous example of using marker functions for this, but didn't find one. Do you have an example?

[NHDaly]

Why doesn't this one need to be backported to 1.11 and 1.13 as well?

[vchuravy]

Technically this needs to be back ported to v1.11 but I don't expect a new release there.

Why doesn't this one need to be backported to 1.11 and 1.13 as well?

Because I was looking at the wrong PR when I added the labels ;)

[vchuravy]

I wanted to add a LLVM code generation test, but looking at a slightly simplified version:

julia> using Base.ScopedValues: ScopedValue, with
julia> const sv = ScopedValue([])

julia> @noinline function inner()
           val = with(sv=>Any[2]) do
                   # Inside this new scope, when we perform GC, the parent scope can be freed.
                   # The fix for this issue made sure that the first scope in this task remains
                   # rooted.
                   GC.gc()
                   GC.gc()
                   sv[]
           end
           @show val
            # Finally, we've returned to the original scope, but that could be a dangling
               # pointer if the scope itself was freed by the above GCs. So these GCs could crash:
               GC.gc()
               GC.gc()
           end
julia> @noinline function outer()
           # Set a new Scope- this is the scope that could be freed.
               @inline with(sv=>Any[1]) do
                   return inner()
               end
           end

On 1.11:

julia> @code_llvm outer()
L52:                                              ; preds = %guard_exit16, %L25
   %value_phi = phi ptr [ %"box::Scope", %L25 ], [ %"box::Scope32", %guard_exit16 ]
   %current_scope = getelementptr inbounds ptr, ptr %tls_pgcstack52, i64 -9
   %9 = load ptr, ptr %current_scope, align 8
   store ptr %value_phi, ptr %current_scope, align 8
   %gc_slot_addr_0 = getelementptr inbounds ptr, ptr %gcframe1, i64 2
   store ptr %9, ptr %gc_slot_addr_0, align 16
   %10 = call i64 @ijl_excstack_state(ptr nonnull %current_task1)
   call void @llvm.lifetime.start.p0(i64 256, ptr nonnull %0)
   call void @ijl_enter_handler(ptr nonnull %current_task1, ptr nonnull %0)
   %11 = call i32 @__sigsetjmp(ptr nonnull %0, i32 0) #10
   %12 = icmp eq i32 %11, 0
   br i1 %12, label %try, label %catch_pop
try:                                              ; preds = %L52
; │┌ @ REPL[4]:4 within `#3`
    call void @j_inner_1330()
; │└
   call void @ijl_pop_handler_noexcept(ptr nonnull %current_task1, i32 1)
   call void @llvm.lifetime.end.p0(i64 256, ptr nonnull %0)
   store ptr %9, ptr %current_scope, align 8
   %frame.prev51 = load ptr, ptr %frame.prev, align 8
   store ptr %frame.prev51, ptr %tls_pgcstack52, align 8
   ret void

catch_pop:                                        ; preds = %L52
   call void @ijl_pop_handler(ptr nonnull %current_task1, i32 1)
   call void @llvm.lifetime.end.p0(i64 256, ptr nonnull %0)
   store ptr %9, ptr %current_scope, align 8
   call void @j_rethrow_1332() #8
   unreachable

So %9 contains the old scope, and it is rooted through the gc_frame. We set the new scope before we call jl_enter_handler
So in 1.11 this seems to be all correct. And indeed reading through #52309 there we do indeed save the old scope correctly.

It is only in #55907 when the scope is moved into the _eh_frame that we are preserving the "wrong one", and indeed

L51:                                              ; preds = %guard_exit, %L24
   %value_phi = phi ptr [ %"box::Scope", %L24 ], [ %"box::Scope30", %guard_exit ]
   %gc_slot_addr_0 = getelementptr inbounds ptr, ptr %gcframe1, i64 2
   store ptr %value_phi, ptr %gc_slot_addr_0, align 8
   %6 = call i64 @ijl_excstack_state(ptr nonnull %current_task)
   call void @llvm.lifetime.start.p0(i64 264, ptr nonnull %0)
   call void @ijl_enter_handler(ptr nonnull %current_task, ptr nonnull %0)
   %7 = call i32 @__sigsetjmp(ptr nonnull %0, i32 0) #8
   %8 = icmp eq i32 %7, 0
   br i1 %8, label %try, label %catch_pop

So we don't need to backport this to 1.11

[NHDaly]

Thanks again Valentin! 🎉

[NHDaly]

Thanks again Valentin! 🎉

[DilumAluthge]

[This doesn't backport cleanly to 1.12. I've asked Valentin if he can put together a manual backport to backports-release-1.12.]