From 7082b0528d639e6bc1850d031b00ec05b25d8dd6 Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Sun, 16 Jun 2024 16:53:01 +0200 Subject: [PATCH] [consider merging] Ruby binding bugfix: 'return' from block was behaving like 'break' The problem was that Ruby uses internal exceptions to implement 'return', 'break' and other types. These were mapped to a single tl::CancelException, rendering the effect of 'break' and 'return' the same. --- src/rba/rba/rba.cc | 32 +++++++++++++++++--------------- src/rba/rba/rbaInternal.h | 20 ++++++++++++++++++++ src/rba/rba/rbaUtils.cc | 23 ++++++++++------------- src/rba/rba/rbaUtils.h | 4 ++-- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/src/rba/rba/rba.cc b/src/rba/rba/rba.cc index e6ab2a357..e51abfeb4 100644 --- a/src/rba/rba/rba.cc +++ b/src/rba/rba/rba.cc @@ -869,6 +869,12 @@ handle_exception (const std::string & /*where*/, rba::RubyError &ex) handle_exception (ex.exc (), ex.first_chance ()); } +static void +handle_exception (rba::RubyContinueException &ex) +{ + rb_jump_tag (ex.state ()); +} + static void handle_exception (const std::string &where, tl::Exception &ex) { @@ -897,6 +903,8 @@ handle_exception (const std::string &where) handle_exception ((where), ex); \ } catch (tl::ExitException &ex) { \ handle_exception ((where), ex); \ + } catch (rba::RubyContinueException &ex) { \ + handle_exception (ex); \ } catch (rba::RubyError &ex) { \ handle_exception ((where), ex); \ } catch (tl::Exception &ex) { \ @@ -1381,23 +1389,17 @@ method_adaptor (int mid, int argc, VALUE *argv, VALUE self, bool ctor) std::unique_ptr iter ((gsi::IterAdaptorAbstractBase *) retlist.read (heap)); if (iter.get ()) { - try { + gsi::SerialArgs rr (iter->serial_size ()); + while (! iter->at_end ()) { - gsi::SerialArgs rr (iter->serial_size ()); - while (! iter->at_end ()) { + rr.reset (); + iter->get (rr); - rr.reset (); - iter->get (rr); + VALUE value = pull_arg (meth->ret_type (), p, rr, heap); + rba_yield_checked (value); - VALUE value = pull_arg (meth->ret_type (), p, rr, heap); - rba_yield_checked (value); + iter->inc (); - iter->inc (); - - } - - } catch (tl::CancelException &) { - // break encountered } } @@ -2402,7 +2404,7 @@ RubyInterpreter::require (const std::string &filename_utf8) RUBY_END_EXEC if (error) { - rba_check_error (); + rba_check_error (error); } } @@ -2423,7 +2425,7 @@ RubyInterpreter::load_file (const std::string &filename_utf8) RUBY_END_EXEC if (error) { - rba_check_error (); + rba_check_error (error); } } diff --git a/src/rba/rba/rbaInternal.h b/src/rba/rba/rbaInternal.h index 0a1ccd1ac..58a4fc7a8 100644 --- a/src/rba/rba/rbaInternal.h +++ b/src/rba/rba/rbaInternal.h @@ -65,6 +65,26 @@ private: VALUE m_exc; }; +/** + * @brief A class responsible for forwarding exceptions raised from "break", "return" and other flow control functions + */ +class RubyContinueException + : public tl::CancelException +{ +public: + RubyContinueException (int state) + : tl::CancelException (), m_state (state) + { } + + int state () const + { + return m_state; + } + +private: + int m_state; +}; + /** * @brief The proxy object that represents the C++ object on the Ruby side */ diff --git a/src/rba/rba/rbaUtils.cc b/src/rba/rba/rbaUtils.cc index f11463965..87f592593 100644 --- a/src/rba/rba/rbaUtils.cc +++ b/src/rba/rba/rbaUtils.cc @@ -127,26 +127,23 @@ exceptions_blocked () } void -rba_check_error () +rba_check_error (int state) { VALUE lasterr = rb_errinfo (); - // NOTE: this seems to be required to avoid segfaults on Ruby 2.3.1 after - // a break was encountered. - rb_set_errinfo (Qnil); - - // Ruby employs this pseudo-exception to indicate a "break" of a loop + // Ruby employs this pseudo-exception to indicate a "break" or "return" of a loop. + // As this is an opaque condition, we continue Ruby execution later through a "RubyContinueException". #if HAVE_RUBY_VERSION_CODE < 10900 if (lasterr == Qnil) { - throw tl::CancelException (); + throw RubyContinueException (state); } #elif HAVE_RUBY_VERSION_CODE < 20300 if (TYPE (lasterr) == T_NODE) { - throw tl::CancelException (); + throw RubyContinueException (state); } #else if (TYPE (lasterr) == T_IMEMO) { - throw tl::CancelException (); + throw RubyContinueException (state); } #endif @@ -405,7 +402,7 @@ rba_class_new_instance_checked (int argc, VALUE *argv, VALUE klass) RUBY_END_EXEC if (error) { - rba_check_error (); + rba_check_error (error); } return ret; } @@ -458,7 +455,7 @@ VALUE rba_funcall2_checked (VALUE obj, ID id, int argc, VALUE *args) RUBY_END_EXEC if (error) { - rba_check_error (); + rba_check_error (error); } return ret; } @@ -497,7 +494,7 @@ rba_f_eval_checked (int argc, VALUE *argv, VALUE self) RUBY_END_EXEC if (error) { - rba_check_error (); + rba_check_error (error); } return ret; } @@ -513,7 +510,7 @@ rba_yield_checked (VALUE value) RUBY_END_EXEC if (error) { - rba_check_error (); + rba_check_error (error); } } diff --git a/src/rba/rba/rbaUtils.h b/src/rba/rba/rbaUtils.h index 1f9e6b5d8..6c06e2005 100644 --- a/src/rba/rba/rbaUtils.h +++ b/src/rba/rba/rbaUtils.h @@ -190,7 +190,7 @@ namespace rba tl::BacktraceElement rba_split_bt_information (const char *m, size_t l); void rba_get_backtrace_from_array (VALUE backtrace, std::vector &bt, unsigned int skip); -void rba_check_error (); +void rba_check_error (int state); VALUE rba_string_value_f (VALUE obj); VALUE rba_safe_string_value (VALUE obj); VALUE rba_safe_obj_as_string (VALUE obj); @@ -265,7 +265,7 @@ R rba_safe_func (R (*f) (A), A arg) RUBY_END_EXEC if (error) { - rba_check_error (); + rba_check_error (error); } return cp.r; }