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
RubyConf 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 a couple years ago. 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 elements in the array was 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 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, Koichi Sasada, the author of the Ruby virtual machine, 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}
Some of you may be thinking, who cares about this issue? Surely nobody is splatting such a large array, right?
Who
Cares?
But do any of you have any code in your applications that does something like this, where you splat a variable when calling a method? I’m assuming most of us do.
some_method(*list)
Any chance that you have code somewhere in one of the applications you maintain, that accepts user input and assigns it to a variable? Again, most of us probably have code like this.
list = params['list']
Any chance that the variable with user input is later splatted? This becomes less likely, but I’m guessing a small percentage of the audience has code somewhere that splats user input.
list = params['list']
some_method(*list)
If so, you need to recognize that this probably allows a user to remotely trigger a SystemStackError.
list = params['list']
some_method(*list) # SystemStackError
Let’s say you know your application deals with user input, and you want to handle unexpected errors using rescue.
list = params['list']
begin
some_method(*list) # SystemStackError
rescue
cleanup_method
end
Unfortunately, this doesn’t work as you expect. If the call to some_method raises a SystemStackError, then cleanup_method will not be called.
list = params['list']
begin
some_method(*list) # SystemStackError
rescue
cleanup_method
end
That’s because this rescue does not catch SystemStackError.
list = params['list']
begin
some_method(*list) # SystemStackError
rescue
cleanup_method
end
rescue without an argument is the same as rescue StandardError.
list = params['list']
begin
some_method(*list) # SystemStackError
rescue # rescue StandardError
cleanup_method
end
And SystemStackError is a subclass of Exception, not a subclass of StandardError.
list = params['list']
begin
some_method(*list) # SystemStackError < Exception
rescue # rescue StandardError
cleanup_method
end
Let’s say you expect some_method to raise non-StandardError exceptions, and you want to do some cleanup using a couple of methods, and then reraise the exception, using this code. What is the problem here?
list = params['list']
begin
some_method(*list) # SystemStackError
rescue Exception
cleanup_method(*list)
other_cleanup_method
raise
end
The problem is this call to cleanup method also raises a SystemStackError.
list = params['list']
begin
some_method(*list) # SystemStackError
rescue Exception
cleanup_method(*list) # SystemStackError
other_cleanup_method
raise
end
Which means that the call to other cleanup method will never be reached. Hopefully these examples were helpful in demonstrating that this issue may exist in your own code and may be triggerable remotely.
list = params['list']
begin
some_method(*list) # SystemStackError
rescue Exception
cleanup_method(*list) # SystemStackError
other_cleanup_method
raise
end
I am now going to discuss details about why this bug occurred. I will focus on how arguments were handled for methods defined in C in Ruby 3.2, which will generally apply to earlier Ruby versions as well.
Why?
For background, I will first discuss how methods are defined using Ruby’s C-API.
C-API
Method
Definition
Here’s a example of how Kernel#itself is defined in Ruby.
static VALUE
rb_obj_itself(VALUE obj)
{
return obj;
}
// ...
rb_define_method(rb_mKernel, "itself", rb_obj_itself, 0);
This part is the C function for the method.
static VALUE
rb_obj_itself(VALUE obj)
{
return obj;
}
// ...
rb_define_method(rb_mKernel, "itself", rb_obj_itself, 0);
This part is how the C function is registered as a method in Ruby.
static VALUE
rb_obj_itself(VALUE obj)
{
return obj;
}
// ...
rb_define_method(rb_mKernel, "itself", rb_obj_itself, 0);
Kernel#itself does not take any arguments, so you pass 0 as the number of arguments when registering the method.
static VALUE
rb_obj_itself(VALUE obj)
{
return obj;
}
// ...
rb_define_method(rb_mKernel, "itself", rb_obj_itself, 0);
Since the Ruby method does not accept any arguments, the C function is called with a single argument, which is the receiver of the method.
static VALUE
rb_obj_itself(VALUE obj)
{
return obj;
}
// ...
rb_define_method(rb_mKernel, "itself", rb_obj_itself, 0);
VALUE here is the C type used for all Ruby objects. Both the C function argument and the C function return value must be Ruby objects.
static VALUE
rb_obj_itself(VALUE obj)
{
return obj;
}
// ...
rb_define_method(rb_mKernel, "itself", rb_obj_itself, 0);
To start work on fixing this bug, I needed to find where the bug is occurring.
Finding
Bugs
One of the tools I use most when starting out debugging is grep, or git grep.
grep
Another of the tools I use frequently when debugging C code is gdb, the GNU debugger.
gdb
To get a general idea of how method calling works, we can 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
If you are building Ruby with support for the shared library, most of Ruby is contained in a libruby shared object and not in the ruby binary itself, so you have to tell gdb that you want the breakpoint to be pending the shared library load.
$ gdb --args ruby -e itself
(gdb) break rb_obj_itself
Function "rb_obj_itself" not defined.
Make breakpoint pending on future shared library load? (y or [n])
Then gdb will set the pending breakpoint.
$ gdb --args ruby -e itself
(gdb) break rb_obj_itself
Function "rb_obj_itself" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (rb_obj_itself) pending.
(gdb)
You can then tell gdb to run the ruby program.
$ gdb --args ruby -e itself
(gdb) break rb_obj_itself
Function "rb_obj_itself" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (rb_obj_itself) pending.
(gdb) r
gdb runs ruby until it hits the breakpoint.
$ gdb --args ruby -e itself
(gdb) break rb_obj_itself
Function "rb_obj_itself" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (rb_obj_itself) pending.
(gdb) r
Starting program: /path/to/ruby -e itself
[New thread 382119]
Thread 1 hit Breakpoint 1, rb_obj_itself at object.c:562
562 {
(gdb)
At that point, you can use the gdb backtrace command to see how you ended up calling the rb_obj_itself function. This should show how Ruby calls methods defined in C.
$ gdb --args ruby -e itself
(gdb) break rb_obj_itself
Function "rb_obj_itself" not defined.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (rb_obj_itself) pending.
(gdb) r
Starting program: /path/to/ruby -e itself
[New thread 382119]
Thread 1 hit Breakpoint 1, rb_obj_itself at object.c:562
562 {
(gdb) 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);
From this line, we can see that the SystemStackError Ruby class
$ git grep \"SystemStackError\"
proc.c: rb_eSysStackError = rb_define_class("SystemStackError", rb_eException);
is stored in a C global variable named rb_eSysStackError.
$ git grep \"SystemStackError\"
proc.c: rb_eSysStackError = rb_define_class("SystemStackError", rb_eException);
If we grep for that,
$ git grep rb_eSysStackError
we get a few results, but for simplicity I will filter most of them out.
$ git grep rb_eSysStackError
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
The stack level too deep string gives a pretty good indication that we are on the right track.
$ git grep rb_eSysStackError
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
Here we see a call to rb_vm_register_special_exception. I do not know what this function does, but based on the name, it probably registers an exception.
$ git grep rb_eSysStackError
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
Since we know that rb_eSysStackError is a reference to SystemStackError
$ git grep rb_eSysStackError
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
We can probably assume that the first argument is being registered with it.
$ git grep rb_eSysStackError
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
If we grep for ruby_error_systack,
$ git grep ruby_error_sysstack
we get only four results.
$ git grep ruby_error_sysstack
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
vm_core.h: ruby_error_sysstack,
vm_core.h:#define sysstack_error GET_VM()->special_exceptions[ruby_error_sysstack]
vm_insnhelper.c: VALUE mesg = rb_ec_vm_ptr(ec)->special_exceptions[ruby_error_sysstack];
One of which we can ignore as it is the same line as the previous grep.
$ git grep ruby_error_sysstack
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
vm_core.h: ruby_error_sysstack,
vm_core.h:#define sysstack_error GET_VM()->special_exceptions[ruby_error_sysstack]
vm_insnhelper.c: VALUE mesg = rb_ec_vm_ptr(ec)->special_exceptions[ruby_error_sysstack];
This line seems interesting. What if we open up the related file and go to that line?
$ git grep ruby_error_sysstack
proc.c: rb_vm_register_special_exception(ruby_error_sysstack, rb_eSysStackError, "stack level too deep");
vm_core.h: ruby_error_sysstack,
vm_core.h:#define sysstack_error GET_VM()->special_exceptions[ruby_error_sysstack]
vm_insnhelper.c: VALUE mesg = rb_ec_vm_ptr(ec)->special_exceptions[ruby_error_sysstack];
Here’s the code around that line. 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.
$ gdb --args ruby -e 'itself(*1380888.times)'
Then set a breakpoint on the ec_stack_overflow function and then run the program.
$ gdb --args ruby -e 'itself(*1380888.times)'
(gdb) break ec_stack_overflow
(gdb) r
Thankfully, we guessed correctly, and the breakpoint is hit.
$ gdb --args ruby -e 'itself(*1380888.times)'
(gdb) break ec_stack_overflow
(gdb) r
[New thread 527494]
Thread 1 hit Breakpoint 2, ec_stack_overflow at ./vm_insnhelper.c:61
61 VALUE mesg = rb_ec_vm_ptr(ec)->special_exceptions[ruby_error_sysstack];
(gdb)
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;
}
}
Some of you are probably wondering, what is the VM stack?
VM
Stack?
VM stack here refers to the stack used by Ruby’s virtual machine. Virtual machines are generally either stack-based or register-based, and Ruby’s virtual machine is stack-based. Ruby’s compiler compiles your Ruby code into virtual machine instructions, which operate on this stack. Values on the VM stack are Ruby objects.
Virtual Machine
Stack
To understand the underlying SystemStackError issue, at this point, I think it would be helpful for me to explain how things are laid out in memory. This explanation will be oversimplified, brief, and at least partly wrong, but hopefully it will still help.
Memory
Layout
You can consider memory as continuous storage from the lowest memory address, represented here by all zeroes, to the highest memory address, represented here by all Fs.
Memory
0x00000000
0xFFFFFFFF
Ruby is written in C, and while C does not use a virtual machine, C uses a stack to store stack frames. There is generally one stack frame per C function call, and the stack frame holds the memory locations of the local variables for that function.
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
When C needs to store data in memory that is not stored on the stack, it first needs to allocate the memory from the operating system, often done using the malloc function. The section of memory where these allocations are located is referred to as the heap. Ruby uses malloc and other functions to allocate most of the memory it uses, so most of Ruby’s memory usage is on the C Heap.
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
C Heap
0x10000000
0xEFFFFFFF
One of the things Ruby stores on the C heap is its object pool. This is where Ruby objects are stored in memory. For large objects, sometimes they do not fit in the space allocated, and excess data is stored elsewhere on the C heap.
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
C Heap
0x10000000
0xEFFFFFFF
Object Pool
0x18000000
0x1FFFFFFF
Another of the things Ruby stores here is the stack for Ruby’s virtual machine, which is what this presentation refers to as the VM stack. This is where Ruby stores stack frames for Ruby method calls, and where it temporarily stores references to objects that are passed as arguments to those methods.
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
C Heap
0x10000000
0xEFFFFFFF
Object Pool
0x18000000
0x1FFFFFFF
VM Stack
0x30000000
0x30FFFFFF
If you try to pass too many arguments to a method call, or you have unbounded recursion, your program would try to write beyond the end of Ruby’s VM stack. Since Ruby 1.9, Ruby recognizes when you are about to do this, and raises a SystemStackError instead of having the program crash, as I discussed earlier.
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
C Heap
0x10000000
0xEFFFFFFF
Object Pool
0x18000000
0x1FFFFFFF
VM Stack
0x30000000
0x30FFFFFF
While in this presentation, I discuss the VM stack as if it were a single place in memory, Ruby can have multiple VM stacks. The main thread has the main VM stack,
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
C Heap
0x10000000
0xEFFFFFFF
Object Pool
0x18000000
0x1FFFFFFF
VM Stack
0x30000000
0x30FFFFFF
but each additional thread has its own thread stack. VM stacks for additional threads are generally smaller than for the main thread.
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
C Heap
0x10000000
0xEFFFFFFF
Object Pool
0x18000000
0x1FFFFFFF
VM Stack
0x30000000
0x30FFFFFF
VM Thread Stack
VM Thread Stack
Additionally, each fiber you create also has its own fiber stack. Fiber stacks are generally smaller than thread stacks. One reason that Ruby doesn’t scale well for huge number of fibers is the garbage collection time for scanning fiber stacks.|In this presentation, the term VM stack refers to the stack used by the currently executing Ruby code, which could be the main thread stack, and or one of the separate thread or fiber stacks.
0x00000000
0xFFFFFFFF
C Stack
0x01000000
0x01FFFFFF
C Heap
0x10000000
0xEFFFFFFF
Object Pool
0x18000000
0x1FFFFFFF
VM Stack
0x30000000
0x30FFFFFF
VM Thread Stack
VM Thread Stack
VM Fiber Stack
VM Fiber Stack
If we go back to the inital example, we saw that we needed over a million arguments to trigger this issue. That’s because this uses the main VM stack.
$ ruby32 -e 'p Hash[*1380888.times.map{ 1 }]'
You only need about 132 thousand arguments to trigger this in a thread stack. One thing to be aware of is the majority of request handling in Ruby web applications is using thread stacks and not the main VM stack.|If your Ruby web application accepts JSON input, and will splat the result of the JSON input, triggering a SystemStackError remotely may be possible with less than 300 kilobytes uploaded.
$ ruby32 -e "Thread.new{Hash[*132000.times.map{ 1 }]}.join"
If you run your Ruby web application using a Fiber per request, it may take only 16400 arguments to trigger a SystemStackError.
$ ruby32 -e "Fiber.new{Hash[*16400.times.map{ 1 }]}.resume"
If you tune your Ruby application to decrease stack size per fiber or thread, for better garbage collection performance and reduced memory usage, triggering the SystemStackError may be possible with just over 2000 arguments.
$ set RUBY_FIBER_VM_STACK_SIZE=16384
$ ruby32 -e "Fiber.new{Hash[*2010.times.map{ 1 }]}.resume"
With that background, let’s get back to our example. To refresh, we just discussed this section, which copies 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;
}
}
After this, 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 VM 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 that implements the Ruby method.
ractor_safe_call_cfunc_0(VALUE recv, int argc, const VALUE *argv,
VALUE (*func)(ANYARGS))
Taking a step up, the function calling ractor_safe_call_cfunc_0 is named vm_call_cfunc_with_frame.
(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 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);
// ...
}
It may be hard to tell that this is calling ractor_safe_call_cfunc_0, because that is actually stored in the invoker function pointer.
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 VM stack, and the stack is quite limited in size. Maybe if we could change this call to pass a pointer to somewhere else in memory, 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;
}
}
Let’s 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 VM 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 VM 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 VM 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 temporary 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 the 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 VM 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 VM 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;
}
You may be wondering, how does this code that copies arguments before the splat and the splat itself handle arguments after the splat?
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;
}
It turns out, you do not need to worry about that, because Ruby’s compiler implicitly converts method calls with arguments after the splat like this,
a(*ary, arg)
into a single array that is then splatted. So the method argument handler does not need to worry about arguments after the splat.
a(*ary, arg)
a(*[*ary, arg])
What if you are splatting multiple arrays, as in this code?
a(*ary1, *ary2)
Similarly, Ruby combines both into a single array that is then splatted. So the method argument handler does not need to worry about multiple splats.
a(*ary1, *ary2)
a(*[*ary1, *ary2])
However, what about keyword arguments, either literal keyword arguments or a keyword argument splat?
a(*ary, kw: 1)
a(*ary, **kw)
This is actually a single case, since Ruby compiles a literal keyword argument after an argument splat
a(*ary, kw: 1)
a(*ary, **kw)
as creating a new hash and then keyword splatting that hash.
a(*ary, kw: 1)
a(*ary, **{kw: 1})
a(*ary, **kw)
In Ruby 3.2, for all of these calls, the compiler will actually combine the argument splat and keywords into a single array, and set a special flag when calling.
a(*ary, kw: 1)
a(*ary, **{kw: 1})
a(*ary, **kw)
a(*[*ary, kw])
In Ruby 3.3, Koichi changed the compiler to not combine the keywords into the argument splat, avoiding 3 unnecessary array allocations and an unnecessary hash allocation.|That means the code in Ruby 3.3 does need to handle keyword argument splats to get correct behavior.
a(*ary, kw: 1)
a(*ary, **{kw: 1})
a(*ary, **kw)
To make sure the keyword splat is passed properly when using a temporary array to hold the arguments, after the call to vm_caller_setup_arg_splat, we push the keyword hash onto the array,
if (!RHASH_EMPTY_P(keyword_hash)) {
rb_ary_push(argv_array, keyword_hash);
}
but only if the keyword hash is not empty, because empty keyword hashes are ignored.
if (!RHASH_EMPTY_P(keyword_hash)) {
rb_ary_push(argv_array, keyword_hash);
}
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 VM 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.
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);
}
// ...
}
From that, we get the pointer to the first element of the temporary array. That pointer points to the heap, instead of the VM 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 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 handling things differently in that case. I do not have time to discuss the other places that needed to be changed, but it requiring 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 Koichi, I mentioned that we could switch to that approach, but it would result in more duplication. Koichi 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 perspective, I will shrink the font size. Now let’s see 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 can 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. Koichi’s approach chose this tradeoff.
Duplicate
Koichi’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 Koichi’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 Koichi 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 function 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 Koichi 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 Koichi’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 Koichi’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 Koichi’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, Koichi 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 Koichi’s changes. One of the changes is at the point this code is called, the array has already been removed from the VM stack. 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.|If you remember a few slides ago, the code that called this function copied arguments from the VM stack to the C stack, and this copies the same arguments from the C stack back to the VM stack. This unnecessary copying is one reason bmethod calling is slower than calling normal ruby methods.
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 splatting a large array starting working with normal method definition 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 an entire class of related bugs.
Squashing
the
Bugs
Another reason is that fixing these bugs turned into a game of bug whack-a-mole, where fixing a single instance of this bug broke other test cases. I would fix those test cases, just to have something else break.
Bug
Whack-A-Mole
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 necessarily mean that the fixes should 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.|Additionally, as I showed early in this presentation, if you don’t fix this issue, it will be possible to remotely trigger a SystemStackError in some Ruby web applications.
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, committing this patch can feel like a step backward.
Minor
Slowdown
One reason for the slowdown is that extra checks were added. For every call to a method not written in Ruby, there are extra checks for whether a temporary array was used, in some cases even if the method call did not use a splat.
Extra
Checks
The code added to handle arguments in temporary arrays also bloats the code. That can result additional instruction cache misses, because less of Ruby’s code may fit in the processor’s instruction cache.
Code
Bloat
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.
Offsetting
Minor
Slowdown
With what I learned, I developed a series of patches to optimize 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 function, 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 110% 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 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 in April 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 remaining 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!!
At least, that’s the current plan. While the fix has been merged, there is a definite possibility it will be reverted in the future if the additional complexity becomes problematic to maintain, or if it starts becoming a larger performance burden as Ruby itself becomes faster.
The
Second
Oldest
Bug
Fixed (for now)
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 50 open bugs in the bug tracker that are over 5 years old, just waiting for you to fix. We look forward to your contibutions!
50+
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.
