ruby-core

thank you for you patch.
but it is not acceptable because "set only main thread" is highly 
restricted. stackprof doesn't work well.

I'm thinking that making interrupt_flag global.

I think it is okay to remain this issue over ruby 2.6.

On 2018/10/28 8:38, Eric Wong wrote:
> @hsbt: I post here on ruby-core because I hit errors with
> https://bugs.ruby-lang.org/ just now
> 
> -------8<-----
> Subject: [PATCH] vm_trace.c (postponed_job_register): only hit main thread
> 
> Since postponed_job_register may be called in a signal handler,
> only the main thread is safe to touch as other threads may
> become invalid.  Furthermore, the problem with trap interrupt
> being lost during ec_switch [Bug #14939] also applies to the
> postponed job and timer interrupts, so we need to preserve all
> three interrupts in ec_switch.
> 
> Note: A minor problem is a possible crash during/after
>    ruby_vm_destruct if postponed jobs are registered.
> 
>    The correct and performant fix would be to leak memory at exit
>    for `vm' and `vm->main_thread'.  free(3) slows down short-lived
>    scripts, as does unregistering signal handlers.
> 
> * vm_trace.c (postponed_job_register): only hit main thread
> * cont.c (ec_switch): preserve postponed and timer interrupt flags, too
> ---
>   cont.c     | 30 +++++++++++++++++++++++-------
>   vm_trace.c | 16 +++++++---------
>   2 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/cont.c b/cont.c
> index 95f2de9b87..5b73f5eac4 100644
> --- a/cont.c
> +++ b/cont.c
> @@ -261,14 +261,30 @@ ec_switch(rb_thread_t *th, rb_fiber_t *fib)
>   {
>       rb_execution_context_t *ec = &fib->cont.saved_ec;
>   
> -    ruby_current_execution_context_ptr = th->ec = ec;
> +    if (th->vm->main_thread == th) {
> +        /*
> +         * Other thread may set interrupt on previous th->ec at any time;
> +         * ensure we do not delay (or lose) the trap interrupt handling.
> +         */
> +        rb_atomic_t old_fl, new_fl;
> +        rb_execution_context_t *prev_ec = ruby_current_execution_context_ptr;
>   
> -    /*
> -     * timer-thread may set trap interrupt on previous th->ec at any time;
> -     * ensure we do not delay (or lose) the trap interrupt handling.
> -     */
> -    if (th->vm->main_thread == th && rb_signal_buff_size() > 0) {
> -        RUBY_VM_SET_TRAP_INTERRUPT(ec);
> +        ruby_current_execution_context_ptr = th->ec = ec;
> +        old_fl = ATOMIC_EXCHANGE(prev_ec->interrupt_flag, 0);
> +
> +        /* migrate interrupts from the previous EC to the current EC */
> +        new_fl = old_fl & (TIMER_INTERRUPT_MASK |
> +                           POSTPONED_JOB_INTERRUPT_MASK |
> +                           TRAP_INTERRUPT_MASK);
> +
> +        if (new_fl) ATOMIC_OR(ec->interrupt_flag, new_fl);
> +
> +        /* Pending-interrupts stay with the previous EC */
> +        old_fl &= PENDING_INTERRUPT_MASK;
> +        if (old_fl) ATOMIC_OR(prev_ec->interrupt_flag, old_fl);
> +    }
> +    else {
> +        ruby_current_execution_context_ptr = th->ec = ec;
>       }
>   
>       VM_ASSERT(ec->fiber_ptr->cont.self == 0 || ec->vm_stack != NULL);
> diff --git a/vm_trace.c b/vm_trace.c
> index 54aa34f8d2..4e2ecf79ab 100644
> --- a/vm_trace.c
> +++ b/vm_trace.c
> @@ -1596,7 +1596,7 @@ enum postponed_job_register_result {
>   
>   /* Async-signal-safe */
>   static enum postponed_job_register_result
> -postponed_job_register(rb_execution_context_t *ec, rb_vm_t *vm,
> +postponed_job_register(rb_vm_t *vm,
>                          unsigned int flags, rb_postponed_job_func_t func, void *data, int max, int expected_index)
>   {
>       rb_postponed_job_t *pjob;
> @@ -1614,7 +1614,7 @@ postponed_job_register(rb_execution_context_t *ec, rb_vm_t *vm,
>       pjob->func = func;
>       pjob->data = data;
>   
> -    RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(ec);
> +    RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(vm->main_thread->ec);
>   
>       return PJRR_SUCCESS;
>   }
> @@ -1626,11 +1626,10 @@ postponed_job_register(rb_execution_context_t *ec, rb_vm_t *vm,
>   int
>   rb_postponed_job_register(unsigned int flags, rb_postponed_job_func_t func, void *data)
>   {
> -    rb_execution_context_t *ec = GET_EC();
> -    rb_vm_t *vm = rb_ec_vm_ptr(ec);
> +    rb_vm_t *vm = GET_VM();
>   
>     begin:
> -    switch (postponed_job_register(ec, vm, flags, func, data, MAX_POSTPONED_JOB, vm->postponed_job_index)) {
> +    switch (postponed_job_register(vm, flags, func, data, MAX_POSTPONED_JOB, vm->postponed_job_index)) {
>         case PJRR_SUCCESS    : return 1;
>         case PJRR_FULL       : return 0;
>         case PJRR_INTERRUPTED: goto begin;
> @@ -1645,8 +1644,7 @@ rb_postponed_job_register(unsigned int flags, rb_postponed_job_func_t func, void
>   int
>   rb_postponed_job_register_one(unsigned int flags, rb_postponed_job_func_t func, void *data)
>   {
> -    rb_execution_context_t *ec = GET_EC();
> -    rb_vm_t *vm = rb_ec_vm_ptr(ec);
> +    rb_vm_t *vm = GET_VM();
>       rb_postponed_job_t *pjob;
>       int i, index;
>   
> @@ -1655,11 +1653,11 @@ rb_postponed_job_register_one(unsigned int flags, rb_postponed_job_func_t func,
>       for (i=0; i<index; i++) {
>           pjob = &vm->postponed_job_buffer[i];
>           if (pjob->func == func) {
> -            RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(ec);
> +            RUBY_VM_SET_POSTPONED_JOB_INTERRUPT(vm->main_thread->ec);
>               return 2;
>           }
>       }
> -    switch (postponed_job_register(ec, vm, flags, func, data, MAX_POSTPONED_JOB + MAX_POSTPONED_JOB_SPECIAL_ADDITION, index)) {
> +    switch (postponed_job_register(vm, flags, func, data, MAX_POSTPONED_JOB + MAX_POSTPONED_JOB_SPECIAL_ADDITION, index)) {
>         case PJRR_SUCCESS    : return 1;
>         case PJRR_FULL       : return 0;
>         case PJRR_INTERRUPTED: goto begin;
> 

-- 
// SASADA Koichi at atdot dot net

Unsubscribe: <mailto:[email protected]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>