aboutsummaryrefslogtreecommitdiffstats
path: root/bootstraptest
diff options
context:
space:
mode:
authorJeremy Evans <code@jeremyevans.net>2023-03-06 15:58:58 -0800
committerJeremy Evans <code@jeremyevans.net>2023-04-25 08:06:16 -0700
commit99c6d19e502b5fdadbd367ae4b6bb3fab850fddc (patch)
tree241e8c41259147f7eb310fff63c2b84e918baebf /bootstraptest
parente7cdce83e8c8797c481ccb54c260c0db1e1afa7c (diff)
downloadruby-99c6d19e502b5fdadbd367ae4b6bb3fab850fddc.tar.gz
Generalize cfunc large array splat fix to fix many additional cases raising SystemStackError
Originally, when 2e7bceb34ea858649e1f975a934ce1894d1f06a6 fixed cfuncs to no longer use the VM stack for large array splats, it was thought to have fully fixed Bug #4040, since the issue was fixed for methods defined in Ruby (iseqs) back in Ruby 2.2. After additional research, I determined that same issue affects almost all types of method calls, not just iseq and cfunc calls. There were two main types of remaining issues, important cases (where large array splat should work) and pedantic cases (where large array splat raised SystemStackError instead of ArgumentError). Important cases: ```ruby define_method(:a){|*a|} a(*1380888.times) def b(*a); end send(:b, *1380888.times) :b.to_proc.call(self, *1380888.times) def d; yield(*1380888.times) end d(&method(:b)) def self.method_missing(*a); end not_a_method(*1380888.times) ``` Pedantic cases: ```ruby def a; end a(*1380888.times) def b(_); end b(*1380888.times) def c(_=nil); end c(*1380888.times) c = Class.new do attr_accessor :a alias b a= end.new c.a(*1380888.times) c.b(*1380888.times) c = Struct.new(:a) do alias b a= end.new c.a(*1380888.times) c.b(*1380888.times) ``` This patch fixes all usage of CALLER_SETUP_ARG with splatting a large number of arguments, and required similar fixes to use a temporary hidden array in three other cases where the VM would use the VM stack for handling a large number of arguments. However, it is possible there may be additional cases where splatting a large number of arguments still causes a SystemStackError. This has a measurable performance impact, as it requires additional checks for a large number of arguments in many additional cases. This change is fairly invasive, as there were many different VM functions that needed to be modified to support this. To avoid too much API change, I modified struct rb_calling_info to add a heap_argv member for storing the array, so I would not have to thread it through many functions. This struct is always stack allocated, which helps ensure sure GC doesn't collect it early. Because of how invasive the changes are, and how rarely large arrays are actually splatted in Ruby code, the existing test/spec suites are not great at testing for correct behavior. To try to find and fix all issues, I tested this in CI with VM_ARGC_STACK_MAX to -1, ensuring that a temporary array is used for all array splat method calls. This was very helpful in finding breaking cases, especially ones involving flagged keyword hashes. Fixes [Bug #4040] Co-authored-by: Jimmy Miller <jimmy.miller@shopify.com>
Diffstat (limited to 'bootstraptest')
-rw-r--r--bootstraptest/test_yjit.rb16
1 files changed, 16 insertions, 0 deletions
diff --git a/bootstraptest/test_yjit.rb b/bootstraptest/test_yjit.rb
index 69e93cabee..a6525e7af9 100644
--- a/bootstraptest/test_yjit.rb
+++ b/bootstraptest/test_yjit.rb
@@ -3947,3 +3947,19 @@ assert_equal 'true', %q{
calling_my_func
}
+
+# Fix failed case for large splat
+assert_equal 'true', %q{
+ def d(a, b=:b)
+ end
+
+ def calling_func
+ ary = 1380888.times;
+ d(*ary)
+ end
+ begin
+ calling_func
+ rescue ArgumentError
+ true
+ end
+} unless defined?(RubyVM::RJIT) && RubyVM::RJIT.enabled? # Not yet working on RJIT