Move the PC regardless of the leaf flag (ruby#8232)

Co-authored-by: Alan Wu <alansi.xingwu@shopify.com>

Author: k0kubun

compile.c

@@ -10217,12 +10217,6 @@ dump_disasm_list_with_cursor(const LINK_ELEMENT *link, const LINK_ELEMENT *curr,
     fflush(stdout);
 }
 
-bool
-rb_insns_leaf_p(int i)
-{
-    return insn_leaf_p(i);
-}
-
 int
 rb_insn_len(VALUE insn)
 {

gc.c

@@ -2423,42 +2423,7 @@ static void
 gc_event_hook_body(rb_execution_context_t *ec, rb_objspace_t *objspace, const rb_event_flag_t event, VALUE data)
 {
     if (UNLIKELY(!ec->cfp)) return;
-    const VALUE *pc = ec->cfp->pc;
-    if (pc && VM_FRAME_RUBYFRAME_P(ec->cfp)) {
-        int prev_opcode = rb_vm_insn_addr2opcode((void *)*ec->cfp->iseq->body->iseq_encoded);
-        for (const VALUE *insn = ec->cfp->iseq->body->iseq_encoded; insn < pc; insn += rb_insn_len(prev_opcode)) {
-            prev_opcode = rb_vm_insn_addr2opcode((void *)*insn);
-        }
-
-        /* If the previous instruction is a leaf instruction, then the PC is
-         * the currently executing instruction. We should increment the PC
-         * because the source line is calculated with PC-1 in calc_pos.
-         *
-         * If the previous instruction is not a leaf instruction and the
-         * current instruction is not a leaf instruction, then the PC was
-         * incremented before the instruction was ran (meaning the currently
-         * executing instruction is actually the previous instruction), so we
-         * should not increment the PC otherwise we will calculate the source
-         * line for the next instruction.
-         *
-         * However, this implementation still has a bug. Consider the
-         * following situation:
-         *
-         *   non-leaf
-         *   leaf <-
-         *
-         * Where the PC currently points to a leaf instruction. We don't know
-         * which instruction we really are at since we could be at the non-leaf
-         * instruction (since it incremented the PC before executing the
-         * instruction). We could also be at the leaf instruction since the PC
-         * doesn't get incremented until the instruction finishes.
-         */
-        if (rb_insns_leaf_p(prev_opcode)) {
-            ec->cfp->pc++;
-        }
-    }
     EXEC_EVENT_HOOK(ec, event, ec->cfp->self, 0, 0, 0, data);
-    ec->cfp->pc = pc;
 }
 
 #define gc_event_hook_available_p(objspace) ((objspace)->flags.has_hook)

internal/compile.h

@@ -17,7 +17,6 @@ struct rb_iseq_struct;          /* in vm_core.h */
 /* compile.c */
 int rb_dvar_defined(ID, const struct rb_iseq_struct *);
 int rb_local_defined(ID, const struct rb_iseq_struct *);
-bool rb_insns_leaf_p(int i);
 int rb_insn_len(VALUE insn);
 const char *rb_insns_name(int i);
 VALUE rb_insns_name_array(void);

test/objspace/test_objspace.rb

@@ -225,6 +225,13 @@ def test_trace_object_allocations
       assert_equal(__FILE__, ObjectSpace.allocation_sourcefile(o4))
       assert_equal(line4, ObjectSpace.allocation_sourceline(o4))
 
+      # The line number should be based on newarray instead of getinstancevariable.
+      line5 = __LINE__; o5 = [ # newarray (leaf)
+        @ivar, # getinstancevariable (not leaf)
+      ]
+      assert_equal(__FILE__, ObjectSpace.allocation_sourcefile(o5))
+      assert_equal(line5, ObjectSpace.allocation_sourceline(o5))
+
       # [Bug #19482]
       EnvUtil.under_gc_stress do
         100.times do

tool/ruby_vm/views/_insn_entry.erb

@@ -24,7 +24,7 @@ INSN_ENTRY(<%= insn.name %>)
     <%= ope[:decl] %> = (<%= ope[:type] %>)GET_OPERAND(<%= i + 1 %>);
 % end
 #   define INSN_ATTR(x) <%= insn.call_attribute(' ## x ## ') %>
-    const bool leaf = INSN_ATTR(leaf);
+    const bool MAYBE_UNUSED(leaf) = INSN_ATTR(leaf);
 % insn.pops.reverse_each.with_index.reverse_each do |pop, i|
     <%= pop[:decl] %> = <%= insn.cast_from_VALUE pop, "TOPN(#{i})"%>;
 % end
@@ -35,7 +35,7 @@ INSN_ENTRY(<%= insn.name %>)
 % end
 
     /* ### Instruction preambles. ### */
-    if (! leaf) ADD_PC(INSN_ATTR(width));
+    ADD_PC(INSN_ATTR(width));
 % if insn.handles_sp?
     POPN(INSN_ATTR(popn));
 % end
@@ -68,7 +68,6 @@ INSN_ENTRY(<%= insn.name %>)
     VM_ASSERT(!RB_TYPE_P(TOPN(<%= i %>), T_MOVED));
 %   end
 % end
-    if (leaf) ADD_PC(INSN_ATTR(width));
 #   undef INSN_ATTR
 
     /* ### Leave the instruction. ### */

tool/ruby_vm/views/_leaf_helpers.erb

@@ -10,31 +10,6 @@
 
 #include "iseq.h"
 
-extern const bool rb_vm_insn_leaf_p[];
-
-#ifdef RUBY_VM_INSNS_INFO
-const bool rb_vm_insn_leaf_p[] = {
-% RubyVM::Instructions.each_slice(20) do |insns|
-    <%= insns.map do |insn|
-        if insn.is_a?(RubyVM::BareInstructions)
-          insn.always_leaf? ? '1' : '0'
-        else
-          '0'
-        end
-      end.join(', ')
-    %>,
-% end
-};
-#endif
-
-CONSTFUNC(MAYBE_UNUSED(static bool insn_leaf_p(VALUE insn)));
-
-bool
-insn_leaf_p(VALUE insn)
-{
-    return rb_vm_insn_leaf_p[insn];
-}
-
 // This is used to tell RJIT that this insn would be leaf if CHECK_INTS didn't exist.
 // It should be used only when RUBY_VM_CHECK_INTS is directly written in insns.def.
 static bool leafness_of_check_ints = false;

vm_insnhelper.h

@@ -181,10 +181,8 @@ CC_SET_FASTPATH(const struct rb_callcache *cc, vm_call_handler func, bool enable
 /**********************************************************/
 
 #define CALL_SIMPLE_METHOD() do { \
-    rb_snum_t x = leaf ? INSN_ATTR(width) : 0; \
-    rb_snum_t y = attr_width_opt_send_without_block(0); \
-    rb_snum_t z = x - y; \
-    ADD_PC(z); \
+    rb_snum_t insn_width = attr_width_opt_send_without_block(0); \
+    ADD_PC(-insn_width); \
     DISPATCH_ORIGINAL_INSN(opt_send_without_block); \
 } while (0)