[GR-18163] Separate rb_set_errinfo/rb_errinfo and $!

PullRequest: truffleruby/4312

Author: andrykonchin

CHANGELOG.md

@@ -40,6 +40,8 @@ Compatibility:
 * Fix formatting in `Exception#full_message` when `RuntimeError` is not handled and `highlight` option is specified (@andrykonchin).
 * Fix `String#encode` and convert fallback values into String using `#to_str` (@andrykonchin).
 * Fix `Kernel.warn` and don't call `Warning#warn` if a specified category is disabled (@andrykonchin).
+* Fix `$!` global variable and make it fiber-local (@andrykonchin).
+* Fix `rb_set_errinfo` and `rb_errinfo` and store an error separately from `$!` (#2890, @andrykonchin).
 
 Performance:
 

lib/truffle/truffle/cext.rb

@@ -721,7 +721,7 @@ def rb_reg_compile(pattern, options)
                nil
              end
     if err
-      Primitive.thread_set_exception(err)
+      Primitive.fiber_set_error_info(err)
       nil
     else
       result
@@ -1239,7 +1239,7 @@ def rb_protect(function, arg, write_status, status)
     unless Primitive.nil?(e)
       store_exception(e)
       pos = extract_tag(e)
-      Primitive.thread_set_exception(extract_ruby_exception(e))
+      Primitive.fiber_set_error_info(extract_ruby_exception(e))
     end
 
     Truffle::Interop.execute_without_conversion(write_status, status, pos)
@@ -1252,7 +1252,7 @@ def rb_jump_tag(pos)
       e = retrieve_exception
       tag = extract_tag(e)
       raise RuntimeError, 'mismatch between jump tag and captured exception' unless pos == tag
-      Primitive.thread_set_exception(nil)
+      Primitive.fiber_set_error_info(nil)
       raise_exception(e)
     end
   end
@@ -1307,7 +1307,7 @@ def rb_exc_raise(exception)
 
   def rb_set_errinfo(error)
     if Primitive.nil?(error) || Primitive.is_a?(error, Exception)
-      Primitive.thread_set_exception(error)
+      Primitive.fiber_set_error_info(error)
     else
       raise TypeError, 'assigning non-exception to ?!'
     end
@@ -1318,7 +1318,7 @@ def rb_make_exception(args)
   end
 
   def rb_errinfo
-    $!
+    Primitive.fiber_get_error_info
   end
 
   def rb_arity_error_string(arg_count, min, max)
@@ -1494,7 +1494,7 @@ def rb_get_alloc_func(ruby_class)
     begin
       allocate_method = ruby_class.method(:__allocate__).owner
     rescue NameError
-      nil
+      nil # it's fine to call this on a class that doesn't have an allocator
     else
       Primitive.object_hidden_var_get(allocate_method, ALLOCATOR_FUNC)
     end
@@ -1837,19 +1837,31 @@ def rb_ensure(b_proc, data1, e_proc, data2)
     begin
       Primitive.interop_execute(POINTER_TO_POINTER_WRAPPER, [b_proc, data1])
     ensure
-      Primitive.interop_execute(POINTER_TO_POINTER_WRAPPER, [e_proc, data2])
+      errinfo = Primitive.fiber_get_error_info
+      Primitive.fiber_set_error_info($!)
+      begin
+        Primitive.interop_execute(POINTER_TO_POINTER_WRAPPER, [e_proc, data2])
+      ensure
+        Primitive.fiber_set_error_info(errinfo)
+      end
     end
   end
   Truffle::Graal.always_split instance_method(:rb_ensure)
 
   def rb_rescue(b_proc, data1, r_proc, data2)
     begin
       Primitive.interop_execute(POINTER_TO_POINTER_WRAPPER, [b_proc, data1])
-    rescue StandardError => e
+    rescue StandardError => exc
       if Truffle::Interop.null?(r_proc)
         Primitive.cext_wrap(nil)
       else
-        Primitive.interop_execute(POINTER2_TO_POINTER_WRAPPER, [r_proc, data2, Primitive.cext_wrap(e)])
+        errinfo = Primitive.fiber_get_error_info
+        Primitive.fiber_set_error_info(exc)
+        begin
+          Primitive.interop_execute(POINTER2_TO_POINTER_WRAPPER, [r_proc, data2, Primitive.cext_wrap(exc)])
+        ensure
+          Primitive.fiber_set_error_info(errinfo)
+        end
       end
     end
   end
@@ -1858,8 +1870,14 @@ def rb_rescue(b_proc, data1, r_proc, data2)
   def rb_rescue2(b_proc, data1, r_proc, data2, rescued)
     begin
       Primitive.interop_execute(POINTER_TO_POINTER_WRAPPER, [b_proc, data1])
-    rescue *rescued => e
-      Primitive.interop_execute(POINTER2_TO_POINTER_WRAPPER, [r_proc, data2, Primitive.cext_wrap(e)])
+    rescue *rescued => exc
+      errinfo = Primitive.fiber_get_error_info
+      Primitive.fiber_set_error_info(exc)
+      begin
+        Primitive.interop_execute(POINTER2_TO_POINTER_WRAPPER, [r_proc, data2, Primitive.cext_wrap(exc)])
+      ensure
+        Primitive.fiber_set_error_info(errinfo)
+      end
     end
   end
   Truffle::Graal.always_split instance_method(:rb_rescue2)

lib/truffle/truffle/cext_ruby.rb

@@ -33,13 +33,9 @@ def rb_define_method(mod, name, function, argc)
         args = [function, Primitive.cext_wrap(self), *args.map! { |arg| Primitive.cext_wrap(arg) }]
       end
 
-      exc = $!
-      Primitive.thread_set_exception(nil)
       # We must set block argument if given here so that the
       # `rb_block_*` functions will be able to find it by walking the stack.
-      res = Primitive.call_with_c_mutex_and_frame_and_unwrap(wrapper, args, Primitive.caller_special_variables_if_available, block)
-      Primitive.thread_set_exception(exc)
-      res
+      Primitive.call_with_c_mutex_and_frame_and_unwrap(wrapper, args, Primitive.caller_special_variables_if_available, block)
     end
 
     # Even if the argc is -2, the arity number

spec/ruby/core/kernel/raise_spec.rb

@@ -46,6 +46,22 @@
     cause = StandardError.new
     -> { raise(cause: cause) }.should raise_error(ArgumentError)
   end
+
+  it "re-raises a rescued exception" do
+    -> do
+      begin
+        raise StandardError, "aaa"
+      rescue Exception
+        begin
+          raise ArgumentError
+        rescue ArgumentError
+        end
+
+        # should raise StandardError "aaa"
+        raise
+      end
+    end.should raise_error(StandardError, "aaa")
+  end
 end
 
 describe "Kernel#raise" do

spec/ruby/language/predefined_spec.rb

@@ -243,6 +243,16 @@ def test(arg)
 end
 
 describe "Predefined global $!" do
+  it "is Fiber-local" do
+    Fiber.new do
+      raise "hi"
+    rescue
+      Fiber.yield
+    end.resume
+
+    $!.should == nil
+  end
+
   # See http://jira.codehaus.org/browse/JRUBY-5550
   it "remains nil after a failed core class \"checked\" coercion against a class that defines method_missing" do
     $!.should == nil

spec/ruby/optional/capi/ext/kernel_spec.c

@@ -7,16 +7,12 @@
 extern "C" {
 #endif
 
-VALUE kernel_spec_call_proc(VALUE arg_array) {
+static VALUE kernel_spec_call_proc(VALUE arg_array) {
   VALUE arg = rb_ary_pop(arg_array);
   VALUE proc = rb_ary_pop(arg_array);
   return rb_funcall(proc, rb_intern("call"), 1, arg);
 }
 
-VALUE kernel_spec_call_proc_raise(VALUE arg_array, VALUE raised_exc) {
-  return kernel_spec_call_proc(arg_array);
-}
-
 static VALUE kernel_spec_rb_block_given_p(VALUE self) {
   return rb_block_given_p() ? Qtrue : Qfalse;
 }
@@ -134,7 +130,16 @@ VALUE kernel_spec_rb_throw_obj(VALUE self, VALUE obj, VALUE result) {
   return ID2SYM(rb_intern("rb_throw_failed"));
 }
 
-VALUE kernel_spec_call_proc_with_raised_exc(VALUE arg_array, VALUE raised_exc) {
+VALUE kernel_spec_rb_errinfo(VALUE self) {
+  return rb_errinfo();
+}
+
+VALUE kernel_spec_rb_set_errinfo(VALUE self, VALUE exc) {
+  rb_set_errinfo(exc);
+  return Qnil;
+}
+
+static VALUE kernel_spec_call_proc_with_raised_exc(VALUE arg_array, VALUE raised_exc) {
   VALUE argv[2];
   int argc;
 
@@ -181,7 +186,7 @@ VALUE kernel_spec_rb_rescue2(int argc, VALUE *args, VALUE self) {
   rb_ary_push(raise_array, args[3]);
 
   return rb_rescue2(kernel_spec_call_proc, main_array,
-      kernel_spec_call_proc_raise, raise_array, args[4], args[5], (VALUE)0);
+      kernel_spec_call_proc_with_raised_exc, raise_array, args[4], args[5], (VALUE)0);
 }
 
 static VALUE kernel_spec_rb_protect_yield(VALUE self, VALUE obj, VALUE ary) {
@@ -195,7 +200,7 @@ static VALUE kernel_spec_rb_protect_yield(VALUE self, VALUE obj, VALUE ary) {
   return res;
 }
 
-static VALUE kernel_spec_rb_protect_errinfo(VALUE self, VALUE obj, VALUE ary) {
+static VALUE kernel_spec_rb_protect_ignore_status(VALUE self, VALUE obj, VALUE ary) {
   int status = 0;
   VALUE res = rb_protect(rb_yield, obj, &status);
   rb_ary_store(ary, 0, INT2NUM(23));
@@ -382,10 +387,13 @@ void Init_kernel_spec(void) {
   rb_define_method(cls, "rb_raise", kernel_spec_rb_raise, 1);
   rb_define_method(cls, "rb_throw", kernel_spec_rb_throw, 1);
   rb_define_method(cls, "rb_throw_obj", kernel_spec_rb_throw_obj, 2);
+  rb_define_method(cls, "rb_errinfo", kernel_spec_rb_errinfo, 0);
+  rb_define_method(cls, "rb_set_errinfo", kernel_spec_rb_set_errinfo, 1);
+  rb_define_method(cls, "rb_rescue", kernel_spec_rb_rescue, 4);
   rb_define_method(cls, "rb_rescue", kernel_spec_rb_rescue, 4);
   rb_define_method(cls, "rb_rescue2", kernel_spec_rb_rescue2, -1);
   rb_define_method(cls, "rb_protect_yield", kernel_spec_rb_protect_yield, 2);
-  rb_define_method(cls, "rb_protect_errinfo", kernel_spec_rb_protect_errinfo, 2);
+  rb_define_method(cls, "rb_protect_ignore_status", kernel_spec_rb_protect_ignore_status, 2);
   rb_define_method(cls, "rb_protect_null_status", kernel_spec_rb_protect_null_status, 1);
   rb_define_method(cls, "rb_eval_string_protect", kernel_spec_rb_eval_string_protect, 2);
   rb_define_method(cls, "rb_catch", kernel_spec_rb_catch, 2);