z, ?	toggle help (this)
space, →	next slide
shift-space, ←	previous slide
d	toggle debug mode
## <ret>	go to slide #
r	reload slides
n	toggle notes

loading presentation...

Hello everyone. In this presentation, I will be discussing what was Ruby’s second oldest open bug, and how we fixed the bug in Ruby 3.3.

The

Second

Oldest

Bug

RubyKaigi 2023

My name is Jeremy Evans. I am a Ruby committer who focuses on fixing bugs in Ruby.

GitHub: jeremyevans

Twitter: @jeremyevans0

I am also the author of Polished Ruby Programming, which was published in English a couple years ago, and was published in Japanese last month. This book is aimed at intermediate Ruby programmers and focuses on teaching principles of Ruby programming, as well as trade-offs to consider when making implementation decisions.

So, what is the second oldest bug that I am discussing?

The

Second

Oldest

Bug

I am referring to bug 4040, which has the subject "SystemStackError with Hash[*a] for Large _a_"

https://bugs.ruby-lang.org/issues/4040

SystemStackError with Hash[*a] for Large _a_

The reproduction code given in the bug report was this simple Ruby code. This calls the array reference method on Hash with a splat of a very large array with over a million entries.|It turns out that this bug is not specific to that Hash method. At the time the bug was reported, this issue applied to any method, when called with a splatted array, if the number of arguments passed to the method is large enough.

Hash[*1380888.times.map{ 1 }]

You could reproduce the bug when calling the Kernel puts method.

puts(*1380888.times)

You could reproduce the bug when calling a method defined in Ruby.

def a(*ary)
end

a(*1380888.times)

You could even reproduce the bug with a method that does not accept any arguments, such as the Kernel object_id method. This is because the error occurs while setting up the arguments for the method call, before actually calling the method.

object_id(*1380888.times)

Ruby’s behavior when passing a very large array splat to a method has changed over time.

Historical

Behavior

Back in Ruby 1.8, passing too many arguments to a method would result in the Ruby interpreter crashing and dumping core, both for methods defined in C and for methods defined in Ruby.

$ ruby18 -ve 'object_id(*1380888.times)'
ruby 1.8.7 (2013-06-27 patchlevel 374) [x86_64-openbsd]
Illegal instruction (core dumped)

$ ruby18 -e 'def a(*a) end; a(*1380888.times); p 1'
Illegal instruction (core dumped)

In Ruby 1.9, this condition was recognized, and a SystemStackError was raised instead for both cases. Ruby 2.0 and 2.1 kept the same behavior as Ruby 1.9.

$ ruby19 -ve 'object_id(*1380888.times)'
ruby 1.9.3p551 (2014-11-13 revision 48407) [x86_64-openbsd]
-e:1: stack level too deep (SystemStackError)

$ ruby19 -e 'def a(*a) end; a(*1380888.times); p 1'
-e:1: stack level too deep (SystemStackError)

In Ruby 2.2, Sasada-san fixed the issue for methods defined in Ruby.

$ ruby22 -ve 'object_id(*1380888.times)'
ruby 2.2.10p489 (2018-03-28 revision 63023) [x86_64-openbsd]
-e:1: stack level too deep (SystemStackError)

$ ruby22 -e 'def a(*a) end; a(*1380888.times); p 1'
1

This same behavior remains in Ruby 3.2.

$ ruby32 -ve 'object_id(*1380888.times)'
ruby 3.2.1 (2023-02-08 revision 31819e82c8) [x86_64-openbsd]
-e:1: stack level too deep (SystemStackError)

$ ruby32 -e 'def a(*a) end; a(*1380888.times); p 1'
1

As you probably can guess, we fixed this issue in Ruby 3.3. So now calling the object_id method with a large array splat correctly results in an ArgumentError.

$ ruby33 -ve 'object_id(*1380888.times)'
ruby 3.3.0dev (2023-02-09T20:02:26Z master d620855101) [x86_64-openbsd]
-e:1:in `object_id': wrong number of arguments (given 1380888, expected 0) (ArgumentError)

object_id(*1380888.times)
          ^^^^^^^^^^^^^^
        from -e:1:in `<main>'

$ ruby33 -e 'def a(*a) end; a(*1380888.times); p 1'
1

If you switch to the example code given in the initial bug report, you can see that it works correctly, and returns a hash with a single entry.

$ ruby33 -e 'p Hash[*1380888.times.map{ 1 }]'
{1=>1}

To start work on fixing this bug, I needed to find where the bug is occurring.

Finding

Bugs

When debugging in Ruby, I usually use gdb,

gdb

To get a general idea of how method calling works, I run ruby through gdb, and tell Ruby to execute the itself method,

$ gdb --args ruby -e itself
(gdb)

then set a breakpoint on the underlying C function that will be called.

$ gdb --args ruby -e itself
(gdb) break rb_obj_itself

After the breakpoint is hit, I can have gdb print the backtrace. Here is the simplified backtrace output. We can split this backtrace into 4 basic sections.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

The bottom section is basically everything Ruby runs before calling the itself method.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

This section is generic method calling code, regardless of how the method was defined.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

This section is code specific to calling methods defined in C.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

At the very top, we have code specific to the itself method.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

Since this bug occurs when calling methods defined in C, it seems likely that the problem is in one of these three functions, or one of the functions they call.|While this is helpful, it would be best if we could see exactly where the SystemStackError is being raised.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

We can start by grepping to see where SystemStackError is defined.

$ git grep \"SystemStackError\"

It is always nice when we get only a single result back.

$ git grep \"SystemStackError\"
proc.c:    rb_eSysStackError = rb_define_class("SystemStackError", rb_eException);

After a few more greps to gather information, we end up inside the virtual machine code, looking at the line highlighted. There is one large indication that this is what we are looking for.

NORETURN(static void ec_stack_overflow(rb_execution_context_t *ec, int));
static void
ec_stack_overflow(rb_execution_context_t *ec, int setup)
{
    VALUE mesg = rb_ec_vm_ptr(ec)->special_exceptions[ruby_error_sysstack];
    ec->raised_flag = RAISED_STACKOVERFLOW;
    if (setup) {
        VALUE at = rb_ec_backtrace_object(ec);
        mesg = ruby_vm_special_exception_copy(mesg);
        rb_ivar_set(mesg, idBt, at);
        rb_ivar_set(mesg, idBt_locations, at);
    }
    ec->errinfo = mesg;
    EC_JUMP_TAG(ec, TAG_RAISE);
}

And that is the function name, which contains the words stack overflow. It looks like this is the function called if the stack would overflow, to raise an exception instead of crashing.

NORETURN(static void ec_stack_overflow(rb_execution_context_t *ec, int));
static void
ec_stack_overflow(rb_execution_context_t *ec, int setup)
{
    VALUE mesg = rb_ec_vm_ptr(ec)->special_exceptions[ruby_error_sysstack];
    ec->raised_flag = RAISED_STACKOVERFLOW;
    if (setup) {
        VALUE at = rb_ec_backtrace_object(ec);
        mesg = ruby_vm_special_exception_copy(mesg);
        rb_ivar_set(mesg, idBt, at);
        rb_ivar_set(mesg, idBt_locations, at);
    }
    ec->errinfo = mesg;
    EC_JUMP_TAG(ec, TAG_RAISE);
}

We can use gdb to see if that is correct. In this case, we will pass code that should trigger the stack overflow, then set a breakpoint on the ec_stack_overflow function.

$ gdb --args ruby -e 'itself(*1380888.times)'
(gdb) break ec_stack_overflow

Thankfully, we guessed correctly. We can use the backtrace command to see how we got there.

(gdb) backtrace
#0  ec_stack_overflow at ./vm_insnhelper.c:61
#1  vm_stackoverflow at ./vm_insnhelper.c:81
#2  vm_caller_setup_arg_splat at ./vm_args.c:795
#3  CALLER_SETUP_ARG at ./vm_insnhelper.c:2545
#4  vm_call_cfunc at ./vm_insnhelper.c:3286
#5  vm_call_method_each_type at ./vm_insnhelper.c:3920
#6  vm_call_method at ./vm_insnhelper.c:4044
#7  vm_call_general at ./vm_insnhelper.c:4088
#8  vm_sendish at ./vm_insnhelper.c:5080
#9  vm_exec_core at insns.def:820
#10 rb_vm_exec at vm.c:2374
#11 rb_iseq_eval_main at vm.c:2633
#12 rb_ec_exec_node at eval.c:289
#13 ruby_run_node at eval.c:330
#14 rb_main at ./main.c:38
#15 main at ./main.c:57

Most of the backtrace is the same as the previous one. I have highlighted the new lines.

(gdb) backtrace
#0  ec_stack_overflow at ./vm_insnhelper.c:61
#1  vm_stackoverflow at ./vm_insnhelper.c:81
#2  vm_caller_setup_arg_splat at ./vm_args.c:795
#3  CALLER_SETUP_ARG at ./vm_insnhelper.c:2545
#4  vm_call_cfunc at ./vm_insnhelper.c:3286
#5  vm_call_method_each_type at ./vm_insnhelper.c:3920
#6  vm_call_method at ./vm_insnhelper.c:4044
#7  vm_call_general at ./vm_insnhelper.c:4088
#8  vm_sendish at ./vm_insnhelper.c:5080
#9  vm_exec_core at insns.def:820
#10 rb_vm_exec at vm.c:2374
#11 rb_iseq_eval_main at vm.c:2633
#12 rb_ec_exec_node at eval.c:289
#13 ruby_run_node at eval.c:330
#14 rb_main at ./main.c:38
#15 main at ./main.c:57

The top two lines are both for functions with stack overflow in the name, so these are likely called when a potential stack overflow has been detected, and not the cause of the stack overflow.

(gdb) backtrace
#0  ec_stack_overflow at ./vm_insnhelper.c:61
#1  vm_stackoverflow at ./vm_insnhelper.c:81
#2  vm_caller_setup_arg_splat at ./vm_args.c:795
#3  CALLER_SETUP_ARG at ./vm_insnhelper.c:2545
#4  vm_call_cfunc at ./vm_insnhelper.c:3286
#5  vm_call_method_each_type at ./vm_insnhelper.c:3920
#6  vm_call_method at ./vm_insnhelper.c:4044
#7  vm_call_general at ./vm_insnhelper.c:4088
#8  vm_sendish at ./vm_insnhelper.c:5080
#9  vm_exec_core at insns.def:820
#10 rb_vm_exec at vm.c:2374
#11 rb_iseq_eval_main at vm.c:2633
#12 rb_ec_exec_node at eval.c:289
#13 ruby_run_node at eval.c:330
#14 rb_main at ./main.c:38
#15 main at ./main.c:57

The function directly before those lines, named vm_caller_setup_arg_splat, is the probable cause.

(gdb) backtrace
#0  ec_stack_overflow at ./vm_insnhelper.c:61
#1  vm_stackoverflow at ./vm_insnhelper.c:81
#2  vm_caller_setup_arg_splat at ./vm_args.c:795
#3  CALLER_SETUP_ARG at ./vm_insnhelper.c:2545
#4  vm_call_cfunc at ./vm_insnhelper.c:3286
#5  vm_call_method_each_type at ./vm_insnhelper.c:3920
#6  vm_call_method at ./vm_insnhelper.c:4044
#7  vm_call_general at ./vm_insnhelper.c:4088
#8  vm_sendish at ./vm_insnhelper.c:5080
#9  vm_exec_core at insns.def:820
#10 rb_vm_exec at vm.c:2374
#11 rb_iseq_eval_main at vm.c:2633
#12 rb_ec_exec_node at eval.c:289
#13 ruby_run_node at eval.c:330
#14 rb_main at ./main.c:38
#15 main at ./main.c:57

Here is the definition of that function.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

It determines which object is being splatted.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

If the object being splatted is not nil, we assume it is an array.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

We then determine of the length of the array to splat.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

In the normal case, we copy all objects from the array to the VM stack.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

Then we increase the number of arguments the method is called with with the length of the splatted array, subtracting one for the array itself.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

However, before doing any copying, we check whether copying all objects to the VM stack would overflow the stack, so that a SystemStackError can be raised instead of the program crashing. This is the check that is being hit when splatting a large array.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

Unfortunately, there is not a way to avoid this issue as long as you take the approach of copying all arguments from the splatted array to the stack. The only way to avoid the approach would be to not pass the arguments on the stack.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

If we go back to the initial backtrace, when calling itself without any arguments.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

We see that the function calling rb_obj_itself is named ractor_safe_call_cfunc_0.

(gdb) backtrace
#0  rb_obj_itself at object.c:564
#1  ractor_safe_call_cfunc_0 at ./vm_insnhelper.c:3085
#2  vm_call_cfunc_with_frame at ./vm_insnhelper.c:3268
#3  vm_call_cfunc at ./vm_insnhelper.c:3289
#4  vm_call_method_each_type at ./vm_insnhelper.c:3920
#5  vm_call_method at ./vm_insnhelper.c:4044
#6  vm_call_general at ./vm_insnhelper.c:4088
#7  vm_sendish at ./vm_insnhelper.c:5080
#8  vm_exec_core at insns.def:820
#9  rb_vm_exec at vm.c:2374
#10 rb_iseq_eval_main at vm.c:2633
#11 rb_ec_exec_node at eval.c:289
#12 ruby_run_node at eval.c:330
#13 rb_main at ./main.c:38
#14 main at ./main.c:57

The prototype for that function looks like this.


ractor_safe_call_cfunc_0(VALUE recv, int argc, const VALUE *argv,
                         VALUE (*func)(ANYARGS))

It takes the receiver of the Ruby method,


ractor_safe_call_cfunc_0(VALUE recv, int argc, const VALUE *argv,
                         VALUE (*func)(ANYARGS))

the number of arguments the Ruby method was called with,


ractor_safe_call_cfunc_0(VALUE recv, int argc, const VALUE *argv,
                         VALUE (*func)(ANYARGS))

a pointer to the first argument for the method.


ractor_safe_call_cfunc_0(VALUE recv, int argc, const VALUE *argv,
                         VALUE (*func)(ANYARGS))

and a function pointer for the C function to call that implements the Ruby method.


ractor_safe_call_cfunc_0(VALUE recv, int argc, const VALUE *argv,
                         VALUE (*func)(ANYARGS))

The function calling ractor_safe_call_cfunc_0 is named vm_call_cfunc_with_frame. This function is large, so I will focus on the line calling ractor_safe_call_cfunc_0.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    // ...
}

The important part here is the value it is passing as the pointer to the first argument. In this case, it is always passing a pointer to the stack. Maybe if we could change this call to pass a pointer to somewhere else in memory, such as the heap, calling a method defined in C with a large array splat would work.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    // ...
}

We can start that process back in the vm_caller_setup_arg_splat function.

static inline void
vm_caller_setup_arg_splat(rb_control_frame_t *cfp, struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];

    vm_check_canary(GET_EC(), cfp->sp);
    cfp->sp--;

    if (!NIL_P(ary)) {
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;
    }
}

We can reduce the code to focus on the case where there is an array to splat.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        CHECK_VM_STACK_OVERFLOW(cfp, len);

        for (i = 0; i < len; i++) {
            *cfp->sp++ = ptr[i];
        }
        calling->argc += i - 1;

If some condition is true, we should use an approach that does not copy arguments to the stack.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (some_condition) {
            // do not use stack
        }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

One simple condition is if the number of arguments being passed to the method plus the length of the splatted array is over some high number, say 1000, then do not use the stack for argument passing.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            // do not use stack
        }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

How should we pass the arguments if we are not going to pass them on the stack? One of the easiest ways to do so is to create a temporary Ruby array for the arguments, and then instead of passing a pointer to the stack, we can pass a pointer to the first element of that array.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            // do not use stack
        }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

Here is code that implements that approach.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

We start off by creating a Ruby array with the expected capacity.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

We then hide the array. This makes sure the array cannot be accessed via ObjectSpace.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

We copy the method arguments before the splatted array to the temporary array.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

Then we copy the elements of the splatted array to the temporary array.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

We still pass the pointer to the temporary array of arguments on the VM stack, so we need to adjust sp, the stack pointer, to reflect that.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

Since there is only one argument on the stack, we adjust the calling information to show only one argument. This is correct from a stack perspective, but incorrect from a method calling perspective.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

Finally, we set a new flag in the calling information named heap_argv to flag that the arguments are being passed on the heap in a temporary array, instead of on the stack.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

Back to the function for calling methods defined in C, now that we have setup the temporary array for arguments on the heap, we need to make changes to use the pointer to the first element of the array, instead of using a pointer to the stack.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    // ...
}

We need to recognize when to use the temporary array for arguments, which we can do by checking for the heap_argv flag.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        // Use arguments from Ruby array
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

Here is the code for using the temporary array for arguments.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        VALUE argv_array = reg_cfp->sp[-1];
        VALUE argv = RARRAY_PTR(argv_array);
        argc = (int)RARRAY_LEN(argv_array);
        val = (*cfunc->invoker)(recv, argc, argv, cfunc->func);
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

We get the pointer to the temporary array containing the arguments, which is located on the stack.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        VALUE argv_array = reg_cfp->sp[-1];
        VALUE argv = RARRAY_PTR(argv_array);
        argc = (int)RARRAY_LEN(argv_array);
        val = (*cfunc->invoker)(recv, argc, argv, cfunc->func);
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

We get the pointer to the first element of the temporary array, which is located on the heap.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        VALUE argv_array = reg_cfp->sp[-1];
        VALUE argv = RARRAY_PTR(argv_array);
        argc = (int)RARRAY_LEN(argv_array);
        val = (*cfunc->invoker)(recv, argc, argv, cfunc->func);
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

We use the length of the temporary array as the number of method arguments.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        VALUE argv_array = reg_cfp->sp[-1];
        VALUE argv = RARRAY_PTR(argv_array);
        argc = (int)RARRAY_LEN(argv_array);
        val = (*cfunc->invoker)(recv, argc, argv, cfunc->func);
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

Finally, we call the C function using the correct number of arguments and pointer to the first element of the temporary array.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        VALUE argv_array = reg_cfp->sp[-1];
        VALUE argv = RARRAY_PTR(argv_array);
        argc = (int)RARRAY_LEN(argv_array);
        val = (*cfunc->invoker)(recv, argc, argv, cfunc->func);
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

While the changes that I went over are the most important changes, there were a few changes needed in other functions,

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        VALUE argv_array = reg_cfp->sp[-1];
        VALUE argv = RARRAY_PTR(argv_array);
        argc = (int)RARRAY_LEN(argv_array);
        val = (*cfunc->invoker)(recv, argc, argv, cfunc->func);
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

mostly to check if the heap argv flag was set, and adjusting argument handling in that case. I do not have time to discuss the other places that needed to be changed, but it required adding about 5 different checks for the heap argv flag.

static VALUE
vm_call_cfunc_with_frame(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
                         struct rb_calling_info *calling)
{
    // ...
    if (calling->heap_argv) {
        VALUE argv_array = reg_cfp->sp[-1];
        VALUE argv = RARRAY_PTR(argv_array);
        argc = (int)RARRAY_LEN(argv_array);
        val = (*cfunc->invoker)(recv, argc, argv, cfunc->func);
    }
    else {
        val = (*cfunc->invoker)(recv, argc, reg_cfp->sp + 1, cfunc->func);
    }
    // ...
}

After making those changes, it did not take much more work to get Ruby’s test suite passing with this change.

Passes

Tests!

Unfortunately, while the tests passed, this change resulted in a minor slowdown. The additional checks slowed down method calling microbenchmarks by up to seven percent. Considering that few users are affected by this bug, slowing down method calling to such a degree was not acceptable.

Minor

Slowdown!!

I tried a couple different approaches to speed up the implementation. Ultimately, I was able to get the performance difference to be small enough that even the microbenchmark did not show it as slower.

Speedup

Attempt

However, my approach was a little invasive, making multiple changes to a couple methods that were used in the generic method dispatch code. Since the issue had already been fixed for many years for methods defined in Ruby, and we were trying to fix it for methods defined in C, it would be nice if the changes could be made only to the dispatch code for methods defined in C.

Invasive

In discussions with Sasada-san, I mentioned that we could switch to that approach, but it would result in more duplication. Sasada-san took it upon himself to implement that noninvasive approach.

Noninvasive

It turns out I was right about the duplication. Here is the vm_call_cfunc function before the changes.

static VALUE
vm_call_cfunc(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    const struct rb_callinfo *ci = calling->ci;
    RB_DEBUG_COUNTER_INC(ccf_cfunc);

    CALLER_SETUP_ARG(reg_cfp, calling, ci);
    CALLER_REMOVE_EMPTY_KW_SPLAT(reg_cfp, calling, ci);
    CC_SET_FASTPATH(calling->cc, vm_call_cfunc_with_frame,
                    !rb_splat_or_kwargs_p(ci) && !calling->kw_splat);
    return vm_call_cfunc_with_frame(ec, reg_cfp, calling);
}

To give the correct scope for the amount of changes, I will shrink the font size. Now we can review the function after the changes.

static VALUE
vm_call_cfunc(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    const struct rb_callinfo *ci = calling->ci;
    RB_DEBUG_COUNTER_INC(ccf_cfunc);

    CALLER_SETUP_ARG(reg_cfp, calling, ci);
    CALLER_REMOVE_EMPTY_KW_SPLAT(reg_cfp, calling, ci);
    CC_SET_FASTPATH(calling->cc, vm_call_cfunc_with_frame,
                    !rb_splat_or_kwargs_p(ci) && !calling->kw_splat);
    return vm_call_cfunc_with_frame(ec, reg_cfp, calling);
}

As you can see, this is quite a large increase in lines.

static VALUE
vm_call_cfunc_setup_argv_ary(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];
    long len = RARRAY_LEN(ary);

    if (UNLIKELY(len + argc > VM_ARGC_STACK_MAX)) {
        vm_check_canary(ec, cfp->sp);
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        VALUE argv_ary = rb_ary_new_capa(len + argc - 1);
        rb_obj_hide(argv_ary);
        rb_ary_cat(argv_ary, argv, argc-1);
        rb_ary_cat(argv_ary, ptr, len);
        cfp->sp -= argc - 1;
        cfp->sp[-1] = argv_ary;
        calling->argc = 1;

        return argv_ary;
    }
    else {
        return Qfalse;
    }
}

static VALUE
vm_call_cfunc(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    const struct rb_callinfo *ci = calling->ci;
    RB_DEBUG_COUNTER_INC(ccf_cfunc);

    VALUE argv_ary;

    if (UNLIKELY(IS_ARGS_SPLAT(ci)) && (argv_ary = vm_call_cfunc_setup_argv_ary(ec, reg_cfp, calling))) {
        // special case of CALLER_SETUP_ARG
        if (!IS_ARGS_KW_OR_KW_SPLAT(ci)) {
            long hash_idx = RARRAY_LEN(argv_ary) - 1;
            VALUE final_hash = RARRAY_AREF(argv_ary, hash_idx);

            if (RB_TYPE_P(final_hash, T_HASH) &&
                (((struct RHash *)final_hash)->basic.flags & RHASH_PASS_AS_KEYWORDS)) {
                if (RHASH_EMPTY_P(final_hash)) {
                    rb_ary_pop(argv_ary);
                }
                else {
                    final_hash = rb_hash_dup(final_hash);
                    RARRAY_ASET(argv_ary, hash_idx, final_hash);
                    calling->kw_splat = 1;
                }
            }
        }

        if (UNLIKELY(IS_ARGS_KW_OR_KW_SPLAT(ci))) {
            VM_ASSERT(!IS_ARGS_KEYWORD(ci)); // should be KW_SPLAT

            long hash_idx = RARRAY_LEN(argv_ary) - 1;
            VALUE keyword_hash = RARRAY_AREF(argv_ary, hash_idx);

            if (!RB_TYPE_P(keyword_hash, T_HASH)) {
                /* Convert a non-hash keyword splat to a new hash */
                RARRAY_ASET(argv_ary, hash_idx, rb_hash_dup(rb_to_hash_type(keyword_hash)));
            }
            else if (!IS_ARGS_KW_SPLAT_MUT(ci)) {
                /* Convert a hash keyword splat to a new hash unless
                 * a mutable keyword splat was passed.
                 */
                RARRAY_ASET(argv_ary, hash_idx, rb_hash_dup(keyword_hash));
            }
        }

        // special case of CALLER_REMOVE_EMPTY_KW_SPLAT()
        if (UNLIKELY(calling->kw_splat)) {
            VALUE kw_hash = RARRAY_AREF(argv_ary, RARRAY_LEN(argv_ary)-1);
            if (RHASH_EMPTY_P(kw_hash)) {
                rb_ary_pop(argv_ary);
                calling->kw_splat = false;
            }
        }

        int argc = RARRAY_LENINT(argv_ary);
        VALUE *argv = (void *)RARRAY_CONST_PTR_TRANSIENT(argv_ary);
        VALUE *stack_bottom = reg_cfp->sp - 2;

        VM_ASSERT(calling->argc == 1);
        VM_ASSERT(RB_TYPE_P(argv_ary, T_ARRAY));
        VM_ASSERT(RBASIC_CLASS(argv_ary) == 0); // hidden ary

        return vm_call_cfunc_with_frame_(ec, reg_cfp, calling, argc, argv, stack_bottom);
    }
    else {
        CALLER_SETUP_ARG(reg_cfp, calling, ci);
        CALLER_REMOVE_EMPTY_KW_SPLAT(reg_cfp, calling, ci);
        CC_SET_FASTPATH(calling->cc, vm_call_cfunc_with_frame,
                        !rb_splat_or_kwargs_p(ci) && !calling->kw_splat);

        return vm_call_cfunc_with_frame(ec, reg_cfp, calling);
    }
}

These two lines in the normal case, when you are not passing a large argument splat, use the standard argument setup functions.

static VALUE
vm_call_cfunc_setup_argv_ary(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];
    long len = RARRAY_LEN(ary);

    if (UNLIKELY(len + argc > VM_ARGC_STACK_MAX)) {
        vm_check_canary(ec, cfp->sp);
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        VALUE argv_ary = rb_ary_new_capa(len + argc - 1);
        rb_obj_hide(argv_ary);
        rb_ary_cat(argv_ary, argv, argc-1);
        rb_ary_cat(argv_ary, ptr, len);
        cfp->sp -= argc - 1;
        cfp->sp[-1] = argv_ary;
        calling->argc = 1;

        return argv_ary;
    }
    else {
        return Qfalse;
    }
}

static VALUE
vm_call_cfunc(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    const struct rb_callinfo *ci = calling->ci;
    RB_DEBUG_COUNTER_INC(ccf_cfunc);

    VALUE argv_ary;

    if (UNLIKELY(IS_ARGS_SPLAT(ci)) && (argv_ary = vm_call_cfunc_setup_argv_ary(ec, reg_cfp, calling))) {
        // special case of CALLER_SETUP_ARG
        if (!IS_ARGS_KW_OR_KW_SPLAT(ci)) {
            long hash_idx = RARRAY_LEN(argv_ary) - 1;
            VALUE final_hash = RARRAY_AREF(argv_ary, hash_idx);

            if (RB_TYPE_P(final_hash, T_HASH) &&
                (((struct RHash *)final_hash)->basic.flags & RHASH_PASS_AS_KEYWORDS)) {
                if (RHASH_EMPTY_P(final_hash)) {
                    rb_ary_pop(argv_ary);
                }
                else {
                    final_hash = rb_hash_dup(final_hash);
                    RARRAY_ASET(argv_ary, hash_idx, final_hash);
                    calling->kw_splat = 1;
                }
            }
        }

        if (UNLIKELY(IS_ARGS_KW_OR_KW_SPLAT(ci))) {
            VM_ASSERT(!IS_ARGS_KEYWORD(ci)); // should be KW_SPLAT

            long hash_idx = RARRAY_LEN(argv_ary) - 1;
            VALUE keyword_hash = RARRAY_AREF(argv_ary, hash_idx);

            if (!RB_TYPE_P(keyword_hash, T_HASH)) {
                /* Convert a non-hash keyword splat to a new hash */
                RARRAY_ASET(argv_ary, hash_idx, rb_hash_dup(rb_to_hash_type(keyword_hash)));
            }
            else if (!IS_ARGS_KW_SPLAT_MUT(ci)) {
                /* Convert a hash keyword splat to a new hash unless
                 * a mutable keyword splat was passed.
                 */
                RARRAY_ASET(argv_ary, hash_idx, rb_hash_dup(keyword_hash));
            }
        }

        // special case of CALLER_REMOVE_EMPTY_KW_SPLAT()
        if (UNLIKELY(calling->kw_splat)) {
            VALUE kw_hash = RARRAY_AREF(argv_ary, RARRAY_LEN(argv_ary)-1);
            if (RHASH_EMPTY_P(kw_hash)) {
                rb_ary_pop(argv_ary);
                calling->kw_splat = false;
            }
        }

        int argc = RARRAY_LENINT(argv_ary);
        VALUE *argv = (void *)RARRAY_CONST_PTR_TRANSIENT(argv_ary);
        VALUE *stack_bottom = reg_cfp->sp - 2;

        VM_ASSERT(calling->argc == 1);
        VM_ASSERT(RB_TYPE_P(argv_ary, T_ARRAY));
        VM_ASSERT(RBASIC_CLASS(argv_ary) == 0); // hidden ary

        return vm_call_cfunc_with_frame_(ec, reg_cfp, calling, argc, argv, stack_bottom);
    }
    else {
        CALLER_SETUP_ARG(reg_cfp, calling, ci);
        CALLER_REMOVE_EMPTY_KW_SPLAT(reg_cfp, calling, ci);
        CC_SET_FASTPATH(calling->cc, vm_call_cfunc_with_frame,
                        !rb_splat_or_kwargs_p(ci) && !calling->kw_splat);

        return vm_call_cfunc_with_frame(ec, reg_cfp, calling);
    }
}

Those two lines are replaced with expanded and customized versions in the case where you are splatting a large array.

static VALUE
vm_call_cfunc_setup_argv_ary(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    int argc = calling->argc;
    VALUE *argv = cfp->sp - argc;
    VALUE ary = argv[argc-1];
    long len = RARRAY_LEN(ary);

    if (UNLIKELY(len + argc > VM_ARGC_STACK_MAX)) {
        vm_check_canary(ec, cfp->sp);
        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        VALUE argv_ary = rb_ary_new_capa(len + argc - 1);
        rb_obj_hide(argv_ary);
        rb_ary_cat(argv_ary, argv, argc-1);
        rb_ary_cat(argv_ary, ptr, len);
        cfp->sp -= argc - 1;
        cfp->sp[-1] = argv_ary;
        calling->argc = 1;

        return argv_ary;
    }
    else {
        return Qfalse;
    }
}

static VALUE
vm_call_cfunc(rb_execution_context_t *ec, rb_control_frame_t *reg_cfp,
              struct rb_calling_info *calling)
{
    const struct rb_callinfo *ci = calling->ci;
    RB_DEBUG_COUNTER_INC(ccf_cfunc);

    VALUE argv_ary;

    if (UNLIKELY(IS_ARGS_SPLAT(ci)) && (argv_ary = vm_call_cfunc_setup_argv_ary(ec, reg_cfp, calling))) {
        // special case of CALLER_SETUP_ARG
        if (!IS_ARGS_KW_OR_KW_SPLAT(ci)) {
            long hash_idx = RARRAY_LEN(argv_ary) - 1;
            VALUE final_hash = RARRAY_AREF(argv_ary, hash_idx);

            if (RB_TYPE_P(final_hash, T_HASH) &&
                (((struct RHash *)final_hash)->basic.flags & RHASH_PASS_AS_KEYWORDS)) {
                if (RHASH_EMPTY_P(final_hash)) {
                    rb_ary_pop(argv_ary);
                }
                else {
                    final_hash = rb_hash_dup(final_hash);
                    RARRAY_ASET(argv_ary, hash_idx, final_hash);
                    calling->kw_splat = 1;
                }
            }
        }

        if (UNLIKELY(IS_ARGS_KW_OR_KW_SPLAT(ci))) {
            VM_ASSERT(!IS_ARGS_KEYWORD(ci)); // should be KW_SPLAT

            long hash_idx = RARRAY_LEN(argv_ary) - 1;
            VALUE keyword_hash = RARRAY_AREF(argv_ary, hash_idx);

            if (!RB_TYPE_P(keyword_hash, T_HASH)) {
                /* Convert a non-hash keyword splat to a new hash */
                RARRAY_ASET(argv_ary, hash_idx, rb_hash_dup(rb_to_hash_type(keyword_hash)));
            }
            else if (!IS_ARGS_KW_SPLAT_MUT(ci)) {
                /* Convert a hash keyword splat to a new hash unless
                 * a mutable keyword splat was passed.
                 */
                RARRAY_ASET(argv_ary, hash_idx, rb_hash_dup(keyword_hash));
            }
        }

        // special case of CALLER_REMOVE_EMPTY_KW_SPLAT()
        if (UNLIKELY(calling->kw_splat)) {
            VALUE kw_hash = RARRAY_AREF(argv_ary, RARRAY_LEN(argv_ary)-1);
            if (RHASH_EMPTY_P(kw_hash)) {
                rb_ary_pop(argv_ary);
                calling->kw_splat = false;
            }
        }

        int argc = RARRAY_LENINT(argv_ary);
        VALUE *argv = (void *)RARRAY_CONST_PTR_TRANSIENT(argv_ary);
        VALUE *stack_bottom = reg_cfp->sp - 2;

        VM_ASSERT(calling->argc == 1);
        VM_ASSERT(RB_TYPE_P(argv_ary, T_ARRAY));
        VM_ASSERT(RBASIC_CLASS(argv_ary) == 0); // hidden ary

        return vm_call_cfunc_with_frame_(ec, reg_cfp, calling, argc, argv, stack_bottom);
    }
    else {
        CALLER_SETUP_ARG(reg_cfp, calling, ci);
        CALLER_REMOVE_EMPTY_KW_SPLAT(reg_cfp, calling, ci);
        CC_SET_FASTPATH(calling->cc, vm_call_cfunc_with_frame,
                        !rb_splat_or_kwargs_p(ci) && !calling->kw_splat);

        return vm_call_cfunc_with_frame(ec, reg_cfp, calling);
    }
}

There is a maintainability tradeoff here.

Tradeoff

One option is to add features to generic functions, making the functions more complicated. This can make the generic functions slower, and it make it more difficult to debug code that does not benefit from the features. On the plus side, this can make maintenance easier, especially if future code can also benefit from the features added to the generic functions. My approach choose this tradeoff.

Complicate

Another option is to duplicate the parts of the generic functions you need, and modify them for the specific use case. This can result in faster code, and it localizes the change. However, it can make maintenance more challenging if you need to make the same change in multiple places. Sasada-san’s approach chose this tradeoff.

Duplicate

Sasada-san’s patch was merged in January, ensuring that the second oldest bug

The

Second

Oldest

Bug

will be fixed in Ruby 3.3.

The

Second

Oldest

Bug

Fixed!

At least, that is what I thought when I started working on this presentation. It turns out, this bug is not a single bug, but a general class of bugs, and my initial patch and Sasada-san’s committed patch only fixed a single instance of this class of bugs.

The

Second

Oldest

Bug

Fixed?

So the bug was not fixed, or at least not completely fixed. I will go over some code that still raised SystemStackError for large argument splats after the patch was committed.

The

Second

Oldest

Bug

Fixed

I mentioned earlier that Sasada-san made changes in Ruby 2.2 to allow this code to work.


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

However, if you replace the normal singleton method definition


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

with the equivalent define_singleton_method call


define_singleton_method(:a) do |*ary|
  ary.length
end

a(*1380888.times)
# => 1380888

it raised a SystemStackError.


define_singleton_method(:a) do |*ary|
  ary.length
end

a(*1380888.times)
# SystemStackError

To understand why the normal method definition approach worked but the block based approach failed, you need to understand that Ruby has multiple method types, each of which is handled differently internally.

Method

Types

Ruby has a C function named vm_call_method_each_type, which handles all of the different method types that Ruby supports. A simplified version of this method is shown here.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

This function uses a C switch statement on the type of the method, similar to a Ruby case expression.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Normal ruby methods defined with def use the iseq method type.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Logic specific to calling iseq methods is in vm_call_iseq_setup. This is the code path that Sasada-san fixed in Ruby 2.2 to allow normal methods to accept large argument splats.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Ruby methods defined by C functions use the cfunc method type.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Logic specific to calling cfunc methods is in vm_call_cfunc. This is the code path that was fixed by my initial patch and Sasada-san’s patch.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Ruby methods defined with define_method or define_singleton_method use the bmethod type. I think bmethod is short for block method, since these are methods defined by blocks.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Logic specific to calling bmethods is in vm_call_bmethod. This is one code path that had not been fixed, which is why calling a bmethod with a large argument splat raised SystemStackError even after Sasada-san’s patch was merged.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Unrelated to the current bug, but useful to know, is that Ruby uses a call cache to improve performance. The first time you call a method, the call cache directs you to the slower generic method dispatch function.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

However, once Ruby figures out a more specific method handler, it updates the call cache, so that future method calls at the same callsite can jump directly to the specific method handler.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Going back to the vm_call_bmethod function, we need to determine why it fails.

static VALUE
vm_call_method_each_type(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                         struct rb_calling_info *calling)
{
    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);

    switch (cme->def->type) {
      case VM_METHOD_TYPE_ISEQ:
        CC_SET_FASTPATH(cc, vm_call_iseq_setup, TRUE);
        return vm_call_iseq_setup(ec, cfp, calling);

      case VM_METHOD_TYPE_CFUNC:
        CC_SET_FASTPATH(cc, vm_call_cfunc, TRUE);
        return vm_call_cfunc(ec, cfp, calling);

      case VM_METHOD_TYPE_BMETHOD:
        CC_SET_FASTPATH(cc, vm_call_bmethod, TRUE);
        return vm_call_bmethod(ec, cfp, calling);
      // ...

Here is the definition of the vm_call_bmethod function. There is one similarity here with the vm_call_cfunc function before we fixed the bug for cfunc methods.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci);
    argc = calling->argc;
    argv = ALLOCA_N(VALUE, argc);
    MEMCPY(argv, cfp->sp - argc, VALUE, argc);
    cfp->sp += - argc - 1;

    return vm_call_bmethod_body(ec, calling, argv);
}

That similarity is in the use of the CALLER_SETUP_ARG function.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci);
    argc = calling->argc;
    argv = ALLOCA_N(VALUE, argc);
    MEMCPY(argv, cfp->sp - argc, VALUE, argc);
    cfp->sp += - argc - 1;

    return vm_call_bmethod_body(ec, calling, argv);
}

Looking back at the backtrace for the stack overflow for cfunc methods, we see that the stack overflow happens during a call to the CALLER_SETUP_ARG function. It turns out that all callers of this function are vulnerable to this issue. In Ruby 3.2, all method types other than iseq use CALLER_SETUP_ARG, which is why they were all vulnerable to this issue.

(gdb) backtrace
#0  ec_stack_overflow at ./vm_insnhelper.c:61
#1  vm_stackoverflow at ./vm_insnhelper.c:81
#2  vm_caller_setup_arg_splat at ./vm_args.c:795
#3  CALLER_SETUP_ARG at ./vm_insnhelper.c:2545
#4  vm_call_cfunc at ./vm_insnhelper.c:3286
#5  vm_call_method_each_type at ./vm_insnhelper.c:3920
#6  vm_call_method at ./vm_insnhelper.c:4044
#7  vm_call_general at ./vm_insnhelper.c:4088
#8  vm_sendish at ./vm_insnhelper.c:5080
#9  vm_exec_core at insns.def:820
#10 rb_vm_exec at vm.c:2374
#11 rb_iseq_eval_main at vm.c:2633
#12 rb_ec_exec_node at eval.c:289
#13 ruby_run_node at eval.c:330
#14 rb_main at ./main.c:38
#15 main at ./main.c:57

I mentioned earlier that my initial patch to fix this issue made the generic argument setup code more complicated,

Complicate

while Sasada-san’s patch was less invasive because it only modified the logic for cfunc methods. The duplication approach may be preferable when you only have a problem in a specific case. However, because all usage of CALLER_SETUP_ARG is vulnernable to this issue, the duplication approach is not maintainable.

Duplicate

The only maintainable approach that fixes the issue for all cases requires complicating the generic code.

Complicate

Here is the initial code I discussed earlier to avoid the SystemStackError. Between when I originally submitted my pull request and when I started working on this presentation and found the additional bugs, Sasada-san had modified the related code to significantly improve peformance.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE argv_array = rb_ary_new_capa(len + argc - 1);
            rb_obj_hide(argv_array);
            rb_ary_cat(argv_array, argv, argc-1);
            rb_ary_cat(argv_array, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_array;
            calling->argc = 1;
            calling->heap_argv = 1;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

I had to make some changes to my original patch to adjust for Sasada-san’s changes. One of the changes is at the point this code is called, the VM stack has already removed the array. Another of the changes is that any keyword splat is passed as a separate argument and not as the last element of the array.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE *argv = cfp->sp - argc;
            VALUE argv_ary = rb_ary_hidden_new(len + argc + 1);
            rb_ary_cat(argv_ary, argv, argc);
            rb_ary_cat(argv_ary, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_ary;
            calling->argc = 1;
            calling->heap_argv = argv_ary;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

Instead of separate rb_ary_new_capa and rb_obj_hide functions, I found there was an rb_ary_hidden_new function that combined the two features, so I switched to calling that.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE *argv = cfp->sp - argc;
            VALUE argv_ary = rb_ary_hidden_new(len + argc + 1);
            rb_ary_cat(argv_ary, argv, argc);
            rb_ary_cat(argv_ary, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_ary;
            calling->argc = 1;
            calling->heap_argv = argv_ary;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

The capacity of the temporary array is set to the number of arguments before the splat, plus the number of arguments in the array being splatted, with one extra for a possible keyword hash.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE *argv = cfp->sp - argc;
            VALUE argv_ary = rb_ary_hidden_new(len + argc + 1);
            rb_ary_cat(argv_ary, argv, argc);
            rb_ary_cat(argv_ary, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_ary;
            calling->argc = 1;
            calling->heap_argv = argv_ary;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

The other important change is that I switched to storing a pointer to the temporary array in the calling structure, instead of only storing the pointer on the VM stack. This was mostly for simplicitly so I could easily reference it later.


        const VALUE *ptr = RARRAY_CONST_PTR_TRANSIENT(ary);
        long len = RARRAY_LEN(ary), i;

        if (argc + len > 1000) {
            VALUE *argv = cfp->sp - argc;
            VALUE argv_ary = rb_ary_hidden_new(len + argc + 1);
            rb_ary_cat(argv_ary, argv, argc);
            rb_ary_cat(argv_ary, ptr, len);
            cfp->sp -= argc - 1;
            cfp->sp[-1] = argv_ary;
            calling->argc = 1;
            calling->heap_argv = argv_ary;
       }
        else {
            CHECK_VM_STACK_OVERFLOW(cfp, len);

            for (i = 0; i < len; i++) {
                *cfp->sp++ = ptr[i];
            }
            calling->argc += i - 1;
        }

If we go back to the vm_call_bmethod function, we can modify it to use this new approach.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci);
    argc = calling->argc;
    argv = ALLOCA_N(VALUE, argc);
    MEMCPY(argv, cfp->sp - argc, VALUE, argc);
    cfp->sp += - argc - 1;

    return vm_call_bmethod_body(ec, calling, argv);
}

Here are the changes needed.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

The first change is the addition of an argument to CALLER_SETUP_ARG for whether it is safe to use a temporary array for the arguments. In this case, it is.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

If there was temporary array created to handle a large argument splat,

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

then we get a pointer to the first element in the array.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

This is instead of allocating space on the C stack, and copying the arguments from the VM stack to the C stack.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

We need to decrement the stack pointer by 2, since the array is considered one argument.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

This is to match the stack pointer adjustment in the normal case. Failure to adjust the stack pointer correctly results in sp/bp mismatch errors or failing cfp consistency checks, both of which are time consuming to debug.|Unfortunately, this change did not fix the stack overflow issue for bmethods. That is because we have a similar issue later in the VM.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

3 nested function calls after calling vm_call_bmethod_body,

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

the VM calls a function named invoke_iseq_block_from_c

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, kw_splat, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

which also copies all arguments to the VM stack.

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, kw_splat, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

First raising a SystemStackError if copying the arguments would overflow the stack.

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, kw_splat, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

I worked around this issue using the same approach used in the CALLER_SETUP_ARG code, by creating a temporary hidden Ruby array to store the arguments.

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...
    int flags = (kw_splat ? VM_CALL_KW_SPLAT : 0);
    VALUE *use_argv = (VALUE *)argv;
    VALUE av[2];

    if (UNLIKELY(argc > VM_ARGC_STACK_MAX)) {
        use_argv = vm_argv_ruby_array(av, argv, &flags, &argc, kw_splat);
    }

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = use_argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, flags, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

If the number of arguments provided is over the limit,

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...
    int flags = (kw_splat ? VM_CALL_KW_SPLAT : 0);
    VALUE *use_argv = (VALUE *)argv;
    VALUE av[2];

    if (UNLIKELY(argc > VM_ARGC_STACK_MAX)) {
        use_argv = vm_argv_ruby_array(av, argv, &flags, &argc, kw_splat);
    }

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = use_argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, flags, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

we call a newly added vm_argv_ruby_array function to create a temporary Ruby array, copy the arguments from argv into it, and then return a pointer to the Ruby array.

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...
    int flags = (kw_splat ? VM_CALL_KW_SPLAT : 0);
    VALUE *use_argv = (VALUE *)argv;
    VALUE av[2];

    if (UNLIKELY(argc > VM_ARGC_STACK_MAX)) {
        use_argv = vm_argv_ruby_array(av, argv, &flags, &argc, kw_splat);
    }

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = use_argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, flags, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

We actually always pass an array of two arguments in this case. The first argument is that temporary array. The second argument is always a keyword hash.

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...
    int flags = (kw_splat ? VM_CALL_KW_SPLAT : 0);
    VALUE *use_argv = (VALUE *)argv;
    VALUE av[2];

    if (UNLIKELY(argc > VM_ARGC_STACK_MAX)) {
        use_argv = vm_argv_ruby_array(av, argv, &flags, &argc, kw_splat);
    }

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = use_argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, flags, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

If kw_splat is true, that means the last argument of the array is keyword arguments, so it is removed from the array and stored as a separate argument. If kw_splat is false, we add an empty hash for keyword arguments, so that Ruby will not treat a flagged keyword hash passed as a regular argument as a keyword splat.

 static inline VALUE
invoke_iseq_block_from_c(rb_execution_context_t *ec, const struct rb_captured_block *captured,
                         VALUE self, int argc, const VALUE *argv, int kw_splat, VALUE passed_block_handler,
                         const rb_cref_t *cref, int is_lambda, const rb_callable_method_entry_t *me)
{
    // ...
    int flags = (kw_splat ? VM_CALL_KW_SPLAT : 0);
    VALUE *use_argv = (VALUE *)argv;
    VALUE av[2];

    if (UNLIKELY(argc > VM_ARGC_STACK_MAX)) {
        use_argv = vm_argv_ruby_array(av, argv, &flags, &argc, kw_splat);
    }

    CHECK_VM_STACK_OVERFLOW(cfp, argc);
    vm_check_canary(ec, sp);
    cfp->sp = sp + argc;
    for (i=0; i<argc; i++) {
        sp[i] = use_argv[i];
    }

    opt_pc = vm_yield_setup_args(ec, iseq, argc, sp, flags, passed_block_handler,
                                 (is_lambda ? arg_setup_method : arg_setup_block));

    // ...
}

With those changes, you can now call methods defined with define_method or define_singleton_method with large array splats. However, that was not the only case that previously failed, there were other cases that failed as well.


define_singleton_method(:a) do |*ary|
  ary.length
end

a(*1380888.times)
# => 1380888

While passing a large array splat worked with normal method definition starting working in Ruby 2.2,


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

If you called the same method using send,


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

send(:a, *1380888.times)

you got a SystemStackError.


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

send(:a, *1380888.times)
# SystemStackError

If you used Symbol#to_proc and called the resulting proc with a large array splat,


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

:a.to_proc.(self, *1380888.times)

you got a SystemStackError.


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

:a.to_proc.(self, *1380888.times)
# SystemStackError

If you created a Method object for the method, and called that with a large array splat,


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

method(:a).(*1380888.times)

you got a SystemStackError.


def self.a(*ary)
  ary.length
end

a(*1380888.times)
# => 1380888

method(:a).(*1380888.times)
# SystemStackError

If you defined method_missing,


def self.method_missing(method, *ary)
  ary.length
end

a(*1380888.times)

then called a method that did not exist with a large array splat,


def self.method_missing(method, *ary)
  ary.length
end

a(*1380888.times)

you got a SystemStackError. Additionally, passing a large number of arguments in a C extension using rb_yield_block also resulted in a SystemStackError. Those were the most important cases that needed to be handled where it could be useful to pass a large number of arguments.|There were also some cases where the method type only accepts 0 or 1 argument, where passing a large array splat resulted in a SystemStackError instead of an ArgumentError.


def self.method_missing(method, *ary)
  ary.length
end

a(*1380888.times)
# SystemStackError

If we go back to the normal method definition,


def self.a(*ary)
  ary.length
end

a(*1380888.times)

and change the argument splat,


def self.a(*ary)
  ary.length
end

a(*1380888.times)

to a regular argument,


def self.a(ary)
  ary.length
end

a(*1380888.times)

when calling the method with a large array splat


def self.a(ary)
  ary.length
end

a(*1380888.times)

It would be best to have an ArgumentError raised,


def self.a(ary)
  ary.length
end

a(*1380888.times)
# ArgumentError

but Ruby would raise a SystemStackError in that case.


def self.a(ary)
  ary.length
end

a(*1380888.times)
# SystemStackError

Similarly, if you have a class that uses attr_reader


class A
  attr_reader :a
end

calling the method defined with a large array splat


class A
  attr_reader :a
end

A.new.a(*1380888.times)

raises a SystemStackError instead of an ArgumentError. The same issue is true for attr_writer.


class A
  attr_reader :a
end

A.new.a(*1380888.times)
# SystemStackError

If you create a Struct class,


A = Struct.new(:a)

Calling the member reader method for an instance of the Struct with a large array splat


A = Struct.new(:a)

A.new.a(*1380888.times)

raises a SystemStackError instead of an ArgumentError. The same issue is true for the member setter method.


A = Struct.new(:a)

A.new.a(*1380888.times)
# SystemStackError

Squashing this bug, both for the method types that could accept a large number of arguments, and to get the correct exception raised for method types that only accept 0 or 1 argument, took quite a long time.

Squashing

the

Bug

One reason for this is this issue is not really a single bug, but a whole class of related bugs.

Squashing

the

Bugs

One thing to be aware of is that almost no code in Ruby’s test suite passes a large array splat. So running the test suites that come with Ruby would be unlikely to catch issues with the changes I was making.

Testing

Rare

Cases

To get a higher level of confidence that the bug fixes were actually working, I did most of my debugging by forcing temporary array usage for all method calls with splats, instead of the default behavior of only using a temporary array for splats of large arrays.

Forcing

Temporary

Arrays

Eventually, I was able to get the entire test suite passing, fixing all cases I was aware of where passing a large array splat raised SystemStackError. However, just because all bugs have been fixed does not mean that the fixes should necessarily be committed.

All

Bugs

Fixed!

When deciding whether to commit anything, you need to have an understanding of the costs and benefits of committing, and you should feel that the benefits are worth the costs.

Costs

vs.

Benefits

In this case, the benefit is you can splat an array when calling any Ruby method, and you should get the expected behavior regardless of the size of the array. I think that is a significant benefit, but the actual need to pass a large array splat is rare, and it is almost always better to pass a large array of arguments as a normal argument, instead of splatting the array.

Benefits

There are significant costs associated with the fixes.

Costs

For one, the fixes are invasive, much more so than my initial attempt to fix the issue for cfunc methods.

Invasive

For another, there is a minor slowdown associated with this change. With all of the work to improve Ruby’s performance, commiting this patch can feel like a step backward.|I thought it would be unlikely that committers would accept a minor slowdown to fix these corner cases. Thankfully, during my work fixing these bugs, I learned a lot about Ruby’s internals.

Minor

Slowdown

With what I learned, I developed a series of patches to optimize method calling in certain cases. My goal with this series of patches was to offset the minor slowdown introduced by the bug fixes.

Method

Calling

Optimizations

One of the optimizations was to bmethod calling. Here is the code for vm_call_bmethod again.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

One thing to notice is that this function always calls CALLER_SETUP_ARG.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

The reason it does this is so it can flatten the arguments into a C array, and pass a pointer to the first argument to vm_call_bmethod_body.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

I determined that this call to CALLER_SETUP_ARG is not actually needed in the common case where the block for the bmethod is defined in Ruby. However, it is needed in other cases where the block for the bmethod is defined in C or was created by Symbol#to_proc.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    VALUE *argv;
    int argc;
    const struct rb_callinfo *ci = calling->ci;

    CALLER_SETUP_ARG(cfp, calling, ci, ALLOW_HEAP_ARGV);
    if (UNLIKELY(calling->heap_argv)) {
        argv = RARRAY_PTR(calling->heap_argv);
        cfp->sp -= 2;
    }
    else {
      argc = calling->argc;
      argv = ALLOCA_N(VALUE, argc);
      MEMCPY(argv, cfp->sp - argc, VALUE, argc);
      cfp->sp += - argc - 1;
    }

    return vm_call_bmethod_body(ec, calling, argv);
}

So I decided to split bmethod call handling into two paths, one to handle the case where the block is defined in Ruby, and another for other blocks.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);
    VALUE procv = cme->def->body.bmethod.proc;
    rb_proc_t *proc;
    GetProcPtr(procv, proc);
    const struct rb_block *block = &proc->block;

    while (vm_block_type(block) == block_type_proc) {
        block = vm_proc_block(block->as.proc);
    }
    if (vm_block_type(block) == block_type_iseq) {
        CC_SET_FASTPATH(cc, vm_call_iseq_bmethod, TRUE);
        return vm_call_iseq_bmethod(ec, cfp, calling);
    }

    CC_SET_FASTPATH(cc, vm_call_noniseq_bmethod, TRUE);
    return vm_call_noniseq_bmethod(ec, cfp, calling);
}

All of this code is necessary to determine whether the block was defined in Ruby.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);
    VALUE procv = cme->def->body.bmethod.proc;
    rb_proc_t *proc;
    GetProcPtr(procv, proc);
    const struct rb_block *block = &proc->block;

    while (vm_block_type(block) == block_type_proc) {
        block = vm_proc_block(block->as.proc);
    }
    if (vm_block_type(block) == block_type_iseq) {
        CC_SET_FASTPATH(cc, vm_call_iseq_bmethod, TRUE);
        return vm_call_iseq_bmethod(ec, cfp, calling);
    }

    CC_SET_FASTPATH(cc, vm_call_noniseq_bmethod, TRUE);
    return vm_call_noniseq_bmethod(ec, cfp, calling);
}

If the block was defined in Ruby, we can call an optimized function that does not need to use CALLER_SETUP_ARG.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);
    VALUE procv = cme->def->body.bmethod.proc;
    rb_proc_t *proc;
    GetProcPtr(procv, proc);
    const struct rb_block *block = &proc->block;

    while (vm_block_type(block) == block_type_proc) {
        block = vm_proc_block(block->as.proc);
    }
    if (vm_block_type(block) == block_type_iseq) {
        CC_SET_FASTPATH(cc, vm_call_iseq_bmethod, TRUE);
        return vm_call_iseq_bmethod(ec, cfp, calling);
    }

    CC_SET_FASTPATH(cc, vm_call_noniseq_bmethod, TRUE);
    return vm_call_noniseq_bmethod(ec, cfp, calling);
}

If the block was not defined in Ruby, we call another function that uses CALLER_SETUP_ARG, which is similar to the previous implementation.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);
    VALUE procv = cme->def->body.bmethod.proc;
    rb_proc_t *proc;
    GetProcPtr(procv, proc);
    const struct rb_block *block = &proc->block;

    while (vm_block_type(block) == block_type_proc) {
        block = vm_proc_block(block->as.proc);
    }
    if (vm_block_type(block) == block_type_iseq) {
        CC_SET_FASTPATH(cc, vm_call_iseq_bmethod, TRUE);
        return vm_call_iseq_bmethod(ec, cfp, calling);
    }

    CC_SET_FASTPATH(cc, vm_call_noniseq_bmethod, TRUE);
    return vm_call_noniseq_bmethod(ec, cfp, calling);
}

In either case, before calling the function, we update the call cache.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);
    VALUE procv = cme->def->body.bmethod.proc;
    rb_proc_t *proc;
    GetProcPtr(procv, proc);
    const struct rb_block *block = &proc->block;

    while (vm_block_type(block) == block_type_proc) {
        block = vm_proc_block(block->as.proc);
    }
    if (vm_block_type(block) == block_type_iseq) {
        CC_SET_FASTPATH(cc, vm_call_iseq_bmethod, TRUE);
        return vm_call_iseq_bmethod(ec, cfp, calling);
    }

    CC_SET_FASTPATH(cc, vm_call_noniseq_bmethod, TRUE);
    return vm_call_noniseq_bmethod(ec, cfp, calling);
}

The reason we update the call cache is so future calls at the same callsite can jump directly to the optimized method, and skip all of this code.

static VALUE
vm_call_bmethod(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                struct rb_calling_info *calling)
{
    RB_DEBUG_COUNTER_INC(ccf_bmethod);

    const struct rb_callcache *cc = calling->cc;
    const rb_callable_method_entry_t *cme = vm_cc_cme(cc);
    VALUE procv = cme->def->body.bmethod.proc;
    rb_proc_t *proc;
    GetProcPtr(procv, proc);
    const struct rb_block *block = &proc->block;

    while (vm_block_type(block) == block_type_proc) {
        block = vm_proc_block(block->as.proc);
    }
    if (vm_block_type(block) == block_type_iseq) {
        CC_SET_FASTPATH(cc, vm_call_iseq_bmethod, TRUE);
        return vm_call_iseq_bmethod(ec, cfp, calling);
    }

    CC_SET_FASTPATH(cc, vm_call_noniseq_bmethod, TRUE);
    return vm_call_noniseq_bmethod(ec, cfp, calling);
}

I added some benchmarks

Method

Calling

Benchmarks

and found that these changes improved bmethod calling by 40% in simple cases, and up to 180% in keyword cases.

Method

Calling

Benchmarks

bmethod:

40-180%

Another of the optimizations was to method missing calls. Here is the top of vm_call_method_missing_body before the bug fix and optimization.

static VALUE
vm_call_method_missing_body(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                            struct rb_calling_info *calling, const struct rb_callinfo *orig_ci,
                            enum method_missing_reason reason)
{
    RB_DEBUG_COUNTER_INC(ccf_method_missing);

    VALUE *argv = STACK_ADDR_FROM_TOP(calling->argc);
    unsigned int argc;

    CALLER_SETUP_ARG(reg_cfp, calling, orig_ci);
    argc = calling->argc + 1;

    unsigned int flag = VM_CALL_FCALL | VM_CALL_OPT_SEND |
                        (calling->kw_splat ? VM_CALL_KW_SPLAT : 0);
    calling->argc = argc;
    // ...

I determined that the CALLER_SETUP_ARG function call here is unnecessary, as method_missing calls do not need to modify existing arguments, they just need to add an argument before those arguments.

static VALUE
vm_call_method_missing_body(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                            struct rb_calling_info *calling, const struct rb_callinfo *orig_ci,
                            enum method_missing_reason reason)
{
    RB_DEBUG_COUNTER_INC(ccf_method_missing);

    VALUE *argv = STACK_ADDR_FROM_TOP(calling->argc);
    unsigned int argc;

    CALLER_SETUP_ARG(reg_cfp, calling, orig_ci);
    argc = calling->argc + 1;

    unsigned int flag = VM_CALL_FCALL | VM_CALL_OPT_SEND |
                        (calling->kw_splat ? VM_CALL_KW_SPLAT : 0);
    calling->argc = argc;
    // ...

So the call to CALLER_SETUP_ARG can be removed completely.

static VALUE
vm_call_method_missing_body(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                            struct rb_calling_info *calling, const struct rb_callinfo *orig_ci,
                            enum method_missing_reason reason)
{
    RB_DEBUG_COUNTER_INC(ccf_method_missing);

    VALUE *argv = STACK_ADDR_FROM_TOP(calling->argc);
    unsigned int argc;

    
    argc = calling->argc + 1;

    unsigned int flag = VM_CALL_FCALL | VM_CALL_OPT_SEND |
                        (calling->kw_splat ? VM_CALL_KW_SPLAT : 0);
    calling->argc = argc;
    // ...

However, you do need to fix the calling flags. The calling flags include information about whether the call includes an argument splat or keyword arguments. Since CALLER_SETUP_ARG was used previously, new calling flags were created.

static VALUE
vm_call_method_missing_body(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                            struct rb_calling_info *calling, const struct rb_callinfo *orig_ci,
                            enum method_missing_reason reason)
{
    RB_DEBUG_COUNTER_INC(ccf_method_missing);

    VALUE *argv = STACK_ADDR_FROM_TOP(calling->argc);
    unsigned int argc;

    
    argc = calling->argc + 1;

    unsigned int flag = VM_CALL_FCALL | VM_CALL_OPT_SEND |
                        (calling->kw_splat ? VM_CALL_KW_SPLAT : 0);
    calling->argc = argc;
    // ...

However, when removing the CALLER_SETUP_ARG function call, you can just copy the calling flags from the original method call that resulted in method_missing, and that fixes the issue.

static VALUE
vm_call_method_missing_body(rb_execution_context_t *ec, rb_control_frame_t *cfp,
                            struct rb_calling_info *calling, const struct rb_callinfo *orig_ci,
                            enum method_missing_reason reason)
{
    RB_DEBUG_COUNTER_INC(ccf_method_missing);

    VALUE *argv = STACK_ADDR_FROM_TOP(calling->argc);
    unsigned int argc;

    
    argc = calling->argc + 1;

    unsigned int flag = VM_CALL_FCALL | VM_CALL_OPT_SEND |
                        vm_ci_flag(orig_ci);
    calling->argc = argc;
    // ...

That simple change improved method missing calls by 10% for simple calls, and up to 100% for calls involving keyword arguments.

Method

Calling

Benchmarks

bmethod:

40-180%

method_missing:

10-110%

I made a similar change for calls to symbol procs, which are procs created by Symbol#to_proc. That improved performance 5% for simple calls and up to 100% for keyword argument calls.

Method

Calling

Benchmarks

bmethod:

40-180%

method_missing:

10-110%

symproc:

5-100%

I also made a similar change to method calls using send, which improved performance 5% for simple calls and up to 115% for keyword argument calls.

Method

Calling

Benchmarks

bmethod:

40-180%

method_missing:

10-110%

symproc:

5-100%

send:

5-115%

To see the overall effect of the bug fixes combined with the performance optimizations, I used yjit-bench. yjit-bench was developed by Shopify to test yjit performance, but is also very useful as a set of general benchmarks. It contains 33 separate benchmarks.

Method

Calling

Benchmarks

yjit-bench

Results showed that performance was about the same in 12 benchmarks.

Method

Calling

Benchmarks

yjit-bench

same:

12

8 benchmarks got slower in spite of the optimizations, up to 3% slower in the worst case.

Method

Calling

Benchmarks

yjit-bench

same:

12 slower:

8 (1-3%)

However, 13 benchmarks got faster, up to 10% faster, because the performance increase from the optimizations was higher than the performance decrease from the bug fixes.

Method

Calling

Benchmarks

yjit-bench

same:

12 slower:

8 (1-3%)

faster:

13 (1-10%)

After finishing those method calling optimizations, I brought the issue up as a topic last month during the monthly developer meeting. This way other committers could provide feedback, and matz could decide whether the benefits were worth the costs. There was some concern over the performance and the invasiveness of the changes.

Developer

Meeting

However, I ultimately received approval to merge the changes. There was one main issue.

Merge

Approval!

I mentioned earlier that I fixed all bugs I was aware of, but there should be a caveat added. In truth, I fixed all bugs in the virtual machine,

All

Bugs

Fixed!

but it turns out that yjit did not support the change I was making. The yjit tests on arm64 generally failed, with occasional failures on amd64. I discussed the issue with the yjit team, and thankfully they were able to fix the yjit issues. After the yjit issues were fixed, and one last round of testing,

All

Bugs

Fixed!

(except yjit)

I was able to merge the changes.

Merged!

Ensuring that the second oldest bug, in all of its incarnations, will finally be fixed in Ruby 3.3.

The

Second

Oldest

Bug

Fixed!!

Here are the lessons I learned from my experience fixing these bugs.

Lessons

Learned

First, just because a bug is old, does not mean it cannot be fixed. Splatting a very large array in a method call has been a problem since Ruby supported splatting arrays, and this issue was known for over a decade before I started work on fixing it. In general, it just takes one person with the determination to fix the bug.

Old Bugs

Can Be Fixed

Second, fixing an old bug is often a learning process that teaches you useful things you probably would not have learned otherwise. Without the experience I gained from fixing these bugs, I would not have been able to implement the performance optimizations I discussed.

Old Bugs

Teach

New Tricks

Finally, do not worry if you cannot fix any Ruby bug perfectly by yourself. As this issue showed, other committers will likely be available to help improve your bug fixes so they can be committed.

Help Is

Available!

Ruby currently has over 80 open bugs in the bug tracker that are over 5 years old, just waiting for you to fix. We look forward to your contibutions!

80+

Old

Bugs

I hope you had fun learning about Ruby’s second oldest bug, and how we fixed it in Ruby 3.3.

The

Second

Oldest

Bug

If you enjoyed this presentation, and want to read more of my thoughts on Ruby programming, please consider picking up a copy of Polished Ruby Programming.

That concludes my presentation. I would like to thank all of you for listening to me.

ありがとうございました

A special thank you to Shopify and Cookpad for sponsoring my travel to RubyKaigi. If you have any questions, please ask me during the break.