https://git.ruby-lang.org/ruby.git/commit/?id=c07545bbf8

From c07545bbf82068f4fd92a5ccfa2b09bb96b39adb Mon Sep 17 00:00:00 2001
From: Jeremy Evans <code@j...>
Date: Fri, 6 Aug 2021 08:07:56 -0700
Subject: Fix multiple bugs in partial backtrace optimization

This fixes multiple bugs found in the partial backtrace
optimization added in 3b24b79.
These bugs occurs when passing a start argument to caller where
the start argument lands on a iseq frame without a pc.

Before this commit, the following code results in the same
line being printed twice, both for the #each method.

```
def a; [1].group_by { b } end
def b; puts(caller(2, 1).first, caller(3, 1).first) end
a
```

After this commit and in Ruby 2.7, the lines are different,
with the first line being for each and the second for group_by.

Before this commit, the following code can either segfault or
result in an infinite loop:

```
def foo
  caller_locations(2, 1).inspect # segfault
  caller_locations(2, 1)[0].path # infinite loop
end

1.times.map { 1.times.map { foo } }
```

After this commit, this code works correctly.

In terms of the implementation, this correctly skips iseq frames
without pc that occur before the number of frames the caller
requested to skip.

This rewrites the algorithm used for handling the partial
backtraces.  It scans from the current frame outward to the
earliest frame, until it has found the desired number of frames.
It records that frame as the start frame.  If needed, it continues
scanning backwards until arg->prev_cfp is set, as that is needed
to set the location of the first frame. Due to the fact that arg
is a void pointer, it's not possible to check this directly, but
this calls the iter_skip function in a situation where it knows
it will set arg->prev_cfp, and then breaks out of the loop.

Fixes [Bug #18053]
---
 test/ruby/test_backtrace.rb | 28 +++++++++++++++++
 vm_backtrace.c              | 73 ++++++++++++++++++++++-----------------------
 2 files changed, 64 insertions(+), 37 deletions(-)

diff --git a/test/ruby/test_backtrace.rb b/test/ruby/test_backtrace.rb
index 742463a..aa79db2 100644
--- a/test/ruby/test_backtrace.rb
+++ b/test/ruby/test_backtrace.rb
@@ -170,6 +170,34 @@ class TestBacktrace < Test::Unit::TestCase https://github.com/ruby/ruby/blob/trunk/test/ruby/test_backtrace.rb#L170
     end
   end
 
+  def test_caller_limit_cfunc_iseq_no_pc
+    def self.a; [1].group_by { b } end
+    def self.b
+      [
+        caller_locations(2, 1).first.base_label,
+        caller_locations(3, 1).first.base_label
+      ]
+    end
+    assert_equal({["each", "group_by"]=>[1]}, a)
+  end
+
+  def test_caller_location_inspect_cfunc_iseq_no_pc
+    def self.foo
+      @res = caller_locations(2, 1).inspect
+    end
+    @line = __LINE__ + 1
+    1.times.map { 1.times.map { foo } }
+    assert_equal("[\"#{__FILE__}:#{@line}:in `times'\"]", @res)
+  end
+
+  def test_caller_location_path_cfunc_iseq_no_pc
+    def self.foo
+      @res = caller_locations(2, 1)[0].path
+    end
+    1.times.map { 1.times.map { foo } }
+    assert_equal(__FILE__, @res)
+  end
+
   def test_caller_locations
     cs = caller(0); locs = caller_locations(0).map{|loc|
       loc.to_s
diff --git a/vm_backtrace.c b/vm_backtrace.c
index 93871c9..70c2ab7 100644
--- a/vm_backtrace.c
+++ b/vm_backtrace.c
@@ -502,6 +502,9 @@ backtrace_size(const rb_execution_context_t *ec) https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L502
     return start_cfp - last_cfp + 1;
 }
 
+static bool is_internal_location(const rb_control_frame_t *cfp);
+static void bt_iter_skip_skip_internal(void *ptr, const rb_control_frame_t *cfp);
+
 static int
 backtrace_each(const rb_execution_context_t *ec,
                ptrdiff_t from_last,
@@ -544,6 +547,9 @@ backtrace_each(const rb_execution_context_t *ec, https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L547
     else {
         /* Ensure we don't look at frames beyond the ones requested */
         for(; from_last > 0 && start_cfp >= last_cfp; from_last--) {
+            if (last_cfp->iseq && !last_cfp->pc) {
+                from_last++;
+            }
             last_cfp = RUBY_VM_PREVIOUS_CONTROL_FRAME(last_cfp);
         }
 
@@ -573,47 +579,40 @@ backtrace_each(const rb_execution_context_t *ec, https://github.com/ruby/ruby/blob/trunk/vm_backtrace.c#L579
     init(arg, size);
 
     /* If a limited number of frames is requested, scan the VM stack for
-     * ignored frames (iseq without pc).  Then adjust the start for the
-     * backtrace to account for skipped frames.
+     * from the current frame (after skipping the number of frames requested above)
+     * towards the earliest frame (start_cfp).  Track the total number of frames
+     * and the number of frames that will be part of the backtrace.  Start the
+     * scan at the oldest frame that should still be part of the backtrace.
+     *
+     * If the last frame in the backtrace is a cfunc frame, continue scanning
+     * till earliest frame to find the first iseq frame with pc, so that the
+     * location can be used for the trailing cfunc frames in the backtrace.
      */
     if (start > 0 && num_frames >= 0 && num_frames < real_size) {
-        ptrdiff_t ignored_frames;
-        bool ignored_frames_before_start = false;
-        for (i=0, j=0, cfp = start_cfp; i<last && j<real_size; i++, j++, cfp = RUBY_VM_NEXT_CONTROL_FRAME(cfp)) {
-            if (cfp->iseq && !cfp->pc) {
-                if (j < start)
-                    ignored_frames_before_start = true;
-                else
-                    i--;
+        int found_frames = 0, total_frames = 0;
+        bool last_frame_cfunc = FALSE;
+        const rb_control_frame_t *new_start_cfp;
+
+        for (cfp = last_cfp; found_frames < num_frames && start_cfp >= cfp; cfp = RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp), total_frames++) {
+            if ((cfp->iseq && cfp->pc) || RUBYVM_CFUNC_FRAME_P(cfp)) {
+                last_frame_cfunc = RUBYVM_CFUNC_FRAME_P(cfp);
+                found_frames++;
             }
-        }
-        ignored_frames = j - i;
-
-        if (ignored_frames) {
-            if (ignored_frames_before_start) {
-                /* There were ignored frames before start.  So just decrementing
-                 * start for ignored frames could still result in not all desired
-                 * frames being captured.
-                 *
-                 * First, scan to the CFP of the desired start frame.
-                 *
-                 * Then scan backwards to previous frames, skipping the number of
-                 * frames ignored after start and any additional ones before start,
-                 * so the number of desired frames will be correctly captured.
-                 */
-                for (i=0, j=0, cfp = start_cfp; i<last && j<real_size && j < start; i++, j++, cfp = RUBY_VM_NEXT_CONTROL_FRAME(cfp)) {
-                    /* nothing */
-                }
-                for (; start > 0 && ignored_frames > 0 && j > 0; j--, ignored_frames--, start--, cfp = RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp)) {
-                    if (cfp->iseq && !cfp->pc) {
-                        ignored_frames++;
-                    }
+         }
+         new_start_cfp = RUBY_VM_NEXT_CONTROL_FRAME(cfp);
+         if (iter_skip && (last_frame_cfunc || iter_skip == bt_iter_skip_skip_internal)) {
+             for (; start_cfp >= cfp; cfp = RUBY_VM_PREVIOUS_CONTROL_FRAME(cfp)) {
+                if (cfp->iseq && cfp->pc && (iter_skip != bt_iter_skip_skip_internal || !is_internal_location(cfp))) {
+                    iter_skip(arg, cfp);
+                    break;
                 }
-            } else {
-                /* No ignored frames before start frame, just decrement start */
-                start -= ignored_frames;
-            }
-        }
+             }
+         }
+
+         last = found_frames;
+         real_size = total_frames;
+         start = 0;
+         start_cfp = new_start_cfp;
     }
 
     for (i=0, j=0, cfp = start_cfp; i<last && j<real_size; i++, j++, cfp = RUBY_VM_NEXT_CONTROL_FRAME(cfp)) {
-- 
cgit v1.1


--
ML: ruby-changes@q...
Info: http://www.atdot.net/~ko1/quickml/