[Bug #19016] re-order error handling at cleanup

Build and store the error message with `#detailed_message` before
terminating all Ractors, then show the message later.

eval.c

@@ -100,8 +100,10 @@ ruby_init(void)
 {
     int state = ruby_setup();
     if (state) {
-        if (RTEST(ruby_debug))
+        if (RTEST(ruby_debug)) {
-            error_print(GET_EC());
+            rb_execution_context_t *ec = GET_EC();
+            rb_ec_error_print(ec, ec->errinfo);
+        }
         exit(EXIT_FAILURE);
     }
 }
@@ -120,9 +122,9 @@ ruby_options(int argc, char **argv)
     }
     else {
         rb_ec_clear_current_thread_trace_func(ec);
-        state = error_handle(ec, state);
+        int exitcode = error_handle(ec, ec->errinfo, state);
         ec->errinfo = Qnil; /* just been handled */
-        iseq = (void *)INT2FIX(state);
+        iseq = (void *)INT2FIX(exitcode);
     }
     EC_POP_TAG();
     return iseq;
@@ -138,7 +140,7 @@ rb_ec_fiber_scheduler_finalize(rb_execution_context_t *ec)
         rb_fiber_scheduler_set(Qnil);
     }
     else {
-        state = error_handle(ec, state);
+        state = error_handle(ec, ec->errinfo, state);
     }
     EC_POP_TAG();
 }
@@ -181,16 +183,17 @@ ruby_cleanup(int ex)
 }
 
 static int
-rb_ec_cleanup(rb_execution_context_t *ec, enum ruby_tag_type ex0)
+rb_ec_cleanup(rb_execution_context_t *ec, enum ruby_tag_type ex)
 {
     int state;
-    volatile VALUE errs[2] = { Qundef, Qundef };
+    volatile VALUE save_error = Qundef;
-    int nerr;
+    volatile int sysex = EXIT_SUCCESS;
+    volatile int signaled = 0;
     rb_thread_t *th = rb_ec_thread_ptr(ec);
     rb_thread_t *const volatile th0 = th;
-    volatile int sysex = EXIT_SUCCESS;
     volatile int step = 0;
-    volatile enum ruby_tag_type ex = ex0;
+    volatile VALUE message = Qnil;
+    VALUE buf;
 
     rb_threadptr_interrupt(th);
     rb_threadptr_check_signal(th);
@@ -200,56 +203,57 @@ rb_ec_cleanup(rb_execution_context_t *ec, enum ruby_tag_type ex0)
         SAVE_ROOT_JMPBUF(th, { RUBY_VM_CHECK_INTS(ec); });
 
       step_0: step++;
-        errs[1] = ec->errinfo;
+        save_error = ec->errinfo;
         if (THROW_DATA_P(ec->errinfo)) ec->errinfo = Qnil;
-        ruby_init_stack(&errs[STACK_UPPER(errs, 0, 1)]);
+        ruby_init_stack(&message);
 
+        /* exits with failure but silently when an exception raised
+         * here */
         SAVE_ROOT_JMPBUF(th, rb_ec_teardown(ec));
 
       step_1: step++;
+        VALUE err = ec->errinfo;
+        int mode0 = 0, mode1 = 0;
+        if (err != save_error && !NIL_P(err)) {
+            mode0 = exiting_split(err, &sysex, &signaled);
+        }
+
+        /* exceptions after here will be ignored */
+
+        /* build error message including causes */
+        err = ATOMIC_VALUE_EXCHANGE(save_error, Qnil);
+
+        if (!NIL_P(err) && !THROW_DATA_P(err)) {
+            mode1 = exiting_split(err, (mode0 & EXITING_WITH_STATUS) ? NULL : &sysex, &signaled);
+            if (mode1 & EXITING_WITH_MESSAGE) {
+                buf = rb_str_new(NULL, 0);
+                SAVE_ROOT_JMPBUF(th, rb_ec_error_print_detailed(ec, err, buf, Qundef));
+                message = buf;
+            }
+        }
+
+      step_2: step++;
         /* protect from Thread#raise */
         th->status = THREAD_KILLED;
 
-        errs[0] = ec->errinfo;
         SAVE_ROOT_JMPBUF(th, rb_ractor_terminate_all());
+
+      step_3: step++;
+        if (!NIL_P(buf = message)) {
+            warn_print_str(buf);
+        }
+        else if (!NIL_OR_UNDEF_P(err = save_error) ||
+                 (ex != TAG_NONE && !((mode0|mode1) & EXITING_WITH_STATUS))) {
+            sysex = error_handle(ec, err, ex);
+        }
     }
     else {
         th = th0;
         switch (step) {
           case 0: goto step_0;
           case 1: goto step_1;
-        }
+          case 2: goto step_2;
-        if (ex == 0) ex = state;
+          case 3: goto step_3;
-    }
-    ec->errinfo = errs[1];
-    sysex = error_handle(ec, ex);
-
-    state = 0;
-    for (nerr = 0; nerr < numberof(errs); ++nerr) {
-        VALUE err = ATOMIC_VALUE_EXCHANGE(errs[nerr], Qnil);
-        VALUE sig;
-
-        if (!RTEST(err)) continue;
-
-        /* ec->errinfo contains a NODE while break'ing */
-        if (THROW_DATA_P(err)) continue;
-
-        if (rb_obj_is_kind_of(err, rb_eSystemExit)) {
-            sysex = sysexit_status(err);
-            break;
-        }
-        else if (rb_obj_is_kind_of(err, rb_eSignal)) {
-            VALUE sig = rb_ivar_get(err, id_signo);
-            state = NUM2INT(sig);
-            break;
-        }
-        else if (rb_obj_is_kind_of(err, rb_eSystemCallError) &&
-                 FIXNUM_P(sig = rb_attr_get(err, id_signo))) {
-            state = NUM2INT(sig);
-            break;
-        }
-        else if (sysex == EXIT_SUCCESS) {
-            sysex = EXIT_FAILURE;
         }
     }
 
@@ -266,7 +270,8 @@ rb_ec_cleanup(rb_execution_context_t *ec, enum ruby_tag_type ex0)
     ruby_vm_destruct(th->vm);
     // For YJIT, call this after ruby_vm_destruct() frees jit_cont for the root fiber.
     rb_jit_cont_finish();
-    if (state) ruby_default_signal(state);
+
+    if (signaled) ruby_default_signal(signaled);
 
     return sysex;
 }

eval_error.c

@@ -73,12 +73,6 @@ set_backtrace(VALUE info, VALUE bt)
     rb_check_funcall(info, set_backtrace, 1, &bt);
 }
 
-static void
-error_print(rb_execution_context_t *ec)
-{
-    rb_ec_error_print(ec, ec->errinfo);
-}
-
 #define CSI_BEGIN "\033["
 #define CSI_SGR "m"
 
@@ -338,12 +332,11 @@ rb_error_write(VALUE errinfo, VALUE emesg, VALUE errat, VALUE str, VALUE opt, VA
     }
 }
 
-void
+static void
-rb_ec_error_print(rb_execution_context_t * volatile ec, volatile VALUE errinfo)
+rb_ec_error_print_detailed(rb_execution_context_t *volatile ec, volatile VALUE errinfo, VALUE str, volatile VALUE emesg)
 {
     volatile uint8_t raised_flag = ec->raised_flag;
     volatile VALUE errat = Qundef;
-    volatile VALUE emesg = Qundef;
     volatile bool written = false;
 
     VALUE opt = rb_hash_new();
@@ -365,7 +358,7 @@ rb_ec_error_print(rb_execution_context_t * volatile ec, volatile VALUE errinfo)
 
     if (!written) {
         written = true;
-        rb_error_write(errinfo, emesg, errat, Qnil, opt, highlight, Qfalse);
+        rb_error_write(errinfo, emesg, errat, str, opt, highlight, Qfalse);
     }
 
     EC_POP_TAG();
@@ -373,6 +366,12 @@ rb_ec_error_print(rb_execution_context_t * volatile ec, volatile VALUE errinfo)
     rb_ec_raised_set(ec, raised_flag);
 }
 
+void
+rb_ec_error_print(rb_execution_context_t *volatile ec, volatile VALUE errinfo)
+{
+    rb_ec_error_print_detailed(ec, errinfo, Qnil, Qundef);
+}
+
 #define undef_mesg_for(v, k) rb_fstring_lit("undefined"v" method `%1$s' for "k" `%2$s'")
 #define undef_mesg(v) ( \
         is_mod ? \
@@ -429,11 +428,58 @@ sysexit_status(VALUE err)
     return NUM2INT(st);
 }
 
+enum {
+    EXITING_WITH_MESSAGE = 1,
+    EXITING_WITH_STATUS = 2,
+    EXITING_WITH_SIGNAL = 4
+};
+static int
+exiting_split(VALUE errinfo, volatile int *exitcode, volatile int *sigstatus)
+{
+    int ex = EXIT_SUCCESS;
+    VALUE signo;
+    int sig = 0;
+    int result = 0;
+
+    if (NIL_P(errinfo)) return 0;
+
+    if (rb_obj_is_kind_of(errinfo, rb_eSystemExit)) {
+        ex = sysexit_status(errinfo);
+        result |= EXITING_WITH_STATUS;
+    }
+    else if (rb_obj_is_kind_of(errinfo, rb_eSignal)) {
+        signo = rb_ivar_get(errinfo, id_signo);
+        sig = FIX2INT(signo);
+        result |= EXITING_WITH_SIGNAL;
+        /* no message when exiting by signal */
+        if (signo == INT2FIX(SIGSEGV) || !rb_obj_is_instance_of(errinfo, rb_eSignal))
+            /* except for SEGV and subclasses */
+            result |= EXITING_WITH_MESSAGE;
+    }
+    else if (rb_obj_is_kind_of(errinfo, rb_eSystemCallError) &&
+        FIXNUM_P(signo = rb_attr_get(errinfo, id_signo))) {
+        sig = FIX2INT(signo);
+        result |= EXITING_WITH_SIGNAL;
+        /* no message when exiting by error to be mapped to signal */
+    }
+    else {
+        ex = EXIT_FAILURE;
+        result |= EXITING_WITH_STATUS | EXITING_WITH_MESSAGE;
+    }
+
+    if (exitcode && (result & EXITING_WITH_STATUS))
+        *exitcode = ex;
+    if (sigstatus && (result & EXITING_WITH_SIGNAL))
+        *sigstatus = sig;
+
+    return result;
+}
+
 #define unknown_longjmp_status(status) \
     rb_bug("Unknown longjmp status %d", status)
 
 static int
-error_handle(rb_execution_context_t *ec, enum ruby_tag_type ex)
+error_handle(rb_execution_context_t *ec, VALUE errinfo, enum ruby_tag_type ex)
 {
     int status = EXIT_FAILURE;
 
@@ -469,26 +515,13 @@ error_handle(rb_execution_context_t *ec, enum ruby_tag_type ex)
         error_pos(Qnil);
         warn_print("unexpected throw\n");
         break;
-      case TAG_RAISE: {
+      case TAG_RAISE:
-        VALUE errinfo = ec->errinfo;
+        if (!(exiting_split(errinfo, &status, NULL) & EXITING_WITH_MESSAGE)) {
-        if (rb_obj_is_kind_of(errinfo, rb_eSystemExit)) {
+            break;
-            status = sysexit_status(errinfo);
         }
-        else if (rb_obj_is_instance_of(errinfo, rb_eSignal) &&
+        /* fallthrough */
-                 rb_ivar_get(errinfo, id_signo) != INT2FIX(SIGSEGV)) {
-            /* no message when exiting by signal */
-        }
-        else if (rb_obj_is_kind_of(errinfo, rb_eSystemCallError) &&
-                 FIXNUM_P(rb_attr_get(errinfo, id_signo))) {
-            /* no message when exiting by error to be mapped to signal */
-        }
-        else {
-            rb_ec_error_print(ec, errinfo);
-        }
-        break;
-      }
       case TAG_FATAL:
-        error_print(ec);
+        rb_ec_error_print(ec, errinfo);
         break;
       default:
         unknown_longjmp_status(ex);

eval_jump.c

@@ -121,7 +121,7 @@ rb_ec_exec_end_proc(rb_execution_context_t * ec)
     }
     else {
         EC_TMPPOP_TAG();
-        error_handle(ec, state);
+        error_handle(ec, ec->errinfo, state);
         if (!NIL_P(ec->errinfo)) errinfo = ec->errinfo;
         EC_REPUSH_TAG();
         goto again;

test/ruby/test_exception.rb

@@ -1459,19 +1459,20 @@ $stderr = $stdout; raise "\x82\xa0"') do |outs, errs, status|
   end
 
   def test_syntax_error_detailed_message
-    Tempfile.create(%w[detail .rb]) do |lib|
+    Dir.mktmpdir do |dir|
-      lib.print "#{<<~"begin;"}\n#{<<~'end;'}"
+      File.write(File.join(dir, "detail.rb"), "#{<<~"begin;"}\n#{<<~'end;'}")
       begin;
         class SyntaxError
           def detailed_message(**)
-            Thread.start {}.join
+            Thread.new {}.join
-            "#{super}\n""<#{File.basename(__FILE__)}>"
+            "<#{super}>\n""<#{File.basename(__FILE__)}>"
+          rescue ThreadError => e
+            e.message
           end
         end
       end;
-      lib.close
+      pattern = /^<detail\.rb>/
-      pattern = /^<#{Regexp.quote(File.basename(lib.path))}>/
+      assert_in_out_err(%W[-r#{dir}/detail -], "1+", [], pattern)
-      assert_in_out_err(%W[-r#{lib.path} -], "1+", [], pattern)
     end
   end
 end