changeset 32375:67945d0339cf

VM Handle mixing of non-bytecode and bytecode scripts (bug #64705) Also add left-out 'ans' in variable pushing and popping in script frames. * libinterp/corefcn/stack-frame.cc: Lookup unknown name instead of panicing in scope_flags. Pretend to be user script frame to make uncompiled scripts find the correct eval frame. Skip machine compiled frame with static_link instead of parent link. * libinterp/octave-value/ov-usr-fcn.cc: Allow calling uncompiled scripts from compiled scripts. * libinterp/parse-tree/pt-bytecode-walk.cc: Also invite 'ans' id to the script frame * test/compile/: Tests updated
author Petter T. <petter.vilhelm@gmail.com>
date Thu, 05 Oct 2023 23:57:25 +0200
parents 0f5033fb818d
children 262768aefb8b
files libinterp/corefcn/stack-frame.cc libinterp/octave-value/ov-usr-fcn.cc libinterp/parse-tree/pt-bytecode-walk.cc test/compile/bytecode.tst test/compile/bytecode_script_topscope.m test/compile/bytecode_script_topscope_assert.m test/compile/bytecode_script_topscope_setup_script.m test/compile/module.mk
diffstat 8 files changed, 153 insertions(+), 23 deletions(-) [+]
line wrap: on
line diff
--- a/libinterp/corefcn/stack-frame.cc	Sat Oct 07 16:45:22 2023 +0200
+++ b/libinterp/corefcn/stack-frame.cc	Thu Oct 05 23:57:25 2023 +0200
@@ -484,7 +484,29 @@
         if (is_pers)
           error ("VM internal error: Trying to make persistent variable global");
         if (name == "")
-          error ("VM internal error: Trying to make unnamed symbol global");
+          {
+            // If a non-bytecode script adds a symbol to the scope of an eval frame
+            // that is a bytecode function we don't have any name specified in the frame
+            // so we need to look for the name in the scope, since we need it to create the 
+            // global ref.
+
+            CHECK_PANIC (!m_fcn->is_user_script ()); // A bytecode script should never be an eval frame
+
+            // TODO: Linear search. Might be abit wasteful.
+            auto scope = get_scope ();
+            auto sym_map = scope.symbols ();
+            for (auto kv : sym_map)
+              {
+                symbol_record sr = kv.second;
+                if (sr.frame_offset () == 0 && sr.data_offset () == external_offset)
+                  {
+                    name = kv.first;
+                  }
+              }
+
+            if (name == "")
+              error ("VM internal error: Trying to make unnamed symbol global");
+          }
 
         if (ov->is_ref ())
           {
@@ -1048,6 +1070,11 @@
 
   bool is_bytecode_fcn_frame (void) const { return true; }
 
+  // E.g. script_stack_frame::get_access_link() need to think the bytecode frame is a user script frame
+  // if it executes a script.
+  bool is_user_script_frame () const { return m_lazy_data ? m_lazy_data->m_is_script : false; }
+  bool is_user_fcn_frame () const { return m_lazy_data ? !m_lazy_data->m_is_script : true; }
+
   symbol_record lookup_symbol (const std::string& name) const
   {
     int local_offset = -1;
@@ -1427,14 +1454,15 @@
     lazy_data ().m_is_script = true;
 
     auto eval_frame = access_link ();
-    auto parent_frame = parent_link ();
+    auto parent_frame = static_link ();
 
     bool caller_is_eval_frame = eval_frame == parent_frame;
     bool eval_frame_is_bytecode = eval_frame->is_bytecode_fcn_frame ();
 
-    if (!caller_is_eval_frame)
+    // If the parent frame is a bytecode frame, and not the eval frame, we need to
+    // move the parent frame's values to the eval frame
+    if (!caller_is_eval_frame && parent_frame->is_bytecode_fcn_frame ())
       {
-        CHECK_PANIC (parent_frame->is_bytecode_fcn_frame ());
         auto *parent_frame_bc = static_cast<bytecode_fcn_stack_frame*> (parent_frame.get ());
 
         // Check that there are no "extra slots" in the parent frame. Those should have been added to the eval frame
@@ -1465,7 +1493,10 @@
             CHECK_PANIC (sr_parent.is_valid () && sr_parent.frame_offset () == 0);
             octave_value &ov_parent = parent_frame->varref (sr_parent.data_offset (), false);
 
-            CHECK_PANIC (!(ov_parent.is_ref () && ov_parent.ref_rep ()->is_local_ref ()));
+            // Assert that the ov in the parent is not a local vm ref, as that would be a leak.
+            // Unless the current frame has been moved by e.g. evalin() in which case there could
+            // be local vm ref:s in the eval frame from a different call chain than the current one.
+            CHECK_PANIC (!(ov_parent.is_ref () && ov_parent.ref_rep ()->is_local_ref ()) || !stacks_in_order ());
 
             bool is_global_in_eval_frame = eval_frame->is_global (sr_eval);
             bool is_global_in_parent_frame = parent_frame->is_global (sr_parent);
@@ -1501,8 +1532,8 @@
         CHECK_PANIC (sr_current.is_valid () && sr_current.frame_offset () == 0);
         octave_value &ov_current = varref (sr_current.data_offset (), false);
 
-        CHECK_PANIC (!(ov_current.is_ref () && ov_current.ref_rep ()->is_local_ref ()));
-        CHECK_PANIC (!(ov_eval->is_ref () && ov_eval->ref_rep ()->is_local_ref ()));
+        CHECK_PANIC (!(ov_current.is_ref () && ov_current.ref_rep ()->is_local_ref ()) || !stacks_in_order ());
+        CHECK_PANIC (!(ov_eval->is_ref () && ov_eval->ref_rep ()->is_local_ref ()) || !stacks_in_order ());
 
         bool is_global_in_eval_frame = eval_frame->is_global (sr_eval);
 
@@ -1528,7 +1559,7 @@
     lazy_data ().m_is_script = true;
 
     auto eval_frame = access_link ();
-    auto parent_frame = parent_link ();
+    auto parent_frame = static_link ();
 
     bool caller_is_eval_frame = eval_frame == parent_frame;
     bool eval_frame_is_bytecode = eval_frame->is_bytecode_fcn_frame ();
@@ -1546,7 +1577,7 @@
         CHECK_PANIC (sr_current.is_valid () && sr_current.frame_offset () == 0);
         octave_value &ov_current = varref (sr_current.data_offset (), false);
 
-        CHECK_PANIC (!(ov_current.is_ref () && ov_current.ref_rep ()->is_local_ref ()));
+        CHECK_PANIC (!(ov_current.is_ref () && ov_current.ref_rep ()->is_local_ref ()) || !stacks_in_order ());
 
         bool is_global_in_eval_frame = eval_frame->is_global (sr_eval);
         bool is_global_in_current_frame = is_global (sr_current);
@@ -1563,10 +1594,10 @@
         ov_current = {};
       }
 
-    // Move all values the parent frame needs to it from the eval frame
-    if (!caller_is_eval_frame)
+    // Move all values the parent frame needs to it from the eval frame, 
+    // if the parent frame is a bytecode frame.
+    if (!caller_is_eval_frame && parent_frame->is_bytecode_fcn_frame ())
       {
-        CHECK_PANIC (parent_frame->is_bytecode_fcn_frame ());
         auto *parent_frame_bc = static_cast<bytecode_fcn_stack_frame*> (parent_frame.get ());
 
         // Check that there are no "extra slots" in the parent frame. Those should have been added to the eval frame
@@ -1587,7 +1618,7 @@
             CHECK_PANIC (sr_parent.is_valid () && sr_parent.frame_offset () == 0);
             octave_value &ov_parent = parent_frame->varref (sr_parent.data_offset (), false);
 
-            CHECK_PANIC (!(ov_eval->is_ref () && ov_eval->ref_rep ()->is_local_ref ()));
+            CHECK_PANIC (!(ov_eval->is_ref () && ov_eval->ref_rep ()->is_local_ref ()) || !stacks_in_order ());
 
             bool is_global_in_eval_frame = eval_frame->is_global (sr_eval);
             bool is_global_in_parent_frame = parent_frame->is_global (sr_parent);
@@ -1649,6 +1680,26 @@
   lazy_data_struct *m_lazy_data = nullptr;
 
 private:
+
+  // Returns true of the stack frames under this frame are in order, i.e.
+  // there is no active evalin(), dbupdown or similar.
+  bool stacks_in_order ()
+  {
+    auto frame = parent_link ();
+    unsigned expected_idx = index ();
+    
+    while (frame)
+      {
+        if (frame->index () != expected_idx)
+          return false;
+
+        frame = frame->parent_link ();
+        expected_idx--;
+      }
+
+    return true;
+  }
+
   octave_user_code *m_fcn;
 
   unwind_data *m_unwind_data;
--- a/libinterp/octave-value/ov-usr-fcn.cc	Sat Oct 07 16:45:22 2023 +0200
+++ b/libinterp/octave-value/ov-usr-fcn.cc	Thu Oct 05 23:57:25 2023 +0200
@@ -200,12 +200,28 @@
 {
   if (octave::vm::maybe_compile_or_compiled (this))
     {
-      auto frame = tw.get_current_stack_frame ();
-      if (frame->is_scope_frame () || frame->is_bytecode_fcn_frame ())
+      // Check that either:
+      //     * There is an eval frame and that it is the top scope or a bytecode frame
+      //     * The caller is top scope or a bytecode frame
+      // , to allow executing the script in the VM. I.e. don't execute scripts in the VM
+      // if the caller is an user function that is not compiled.
+  
+      // TODO: "octave_value varval (std::size_t data_offset) const" and "varref" would need to
+      // follow ref_rep() like scope_stack_frame::varref() does for having un-compiled functions
+      // as an eval frame, but that might maybe degrade performance somewhat of the evaluator.
+
+      auto frame = tw.current_user_frame ();
+      auto access_frame = frame->access_link ();
+
+      bool access_frame_is_vm_or_top = access_frame && (access_frame->is_scope_frame () || access_frame->is_bytecode_fcn_frame ());
+      bool caller_is_vm_or_top = frame->is_scope_frame () || frame->is_bytecode_fcn_frame ();
+
+      if (access_frame_is_vm_or_top || caller_is_vm_or_top)
         return octave::vm::call (tw, nargout, args, this);
+      else
+        warning ("Executing compiled scripts in the VM from an un-compiled function is not supported yet");
     }
 
-
   tw.push_stack_frame (this);
 
   octave::unwind_action act ([&tw] () { tw.pop_stack_frame (); });
--- a/libinterp/parse-tree/pt-bytecode-walk.cc	Sat Oct 07 16:45:22 2023 +0200
+++ b/libinterp/parse-tree/pt-bytecode-walk.cc	Thu Oct 05 23:57:25 2023 +0200
@@ -2209,6 +2209,9 @@
 
   // We always need the magic id "ans"
   add_id_to_table ("ans");
+  // m_set_user_locals_names keeps track of what user symbols to borrow from and return back to
+  // the eval_frame when the script frame is pushed and popped.
+  m_code.m_unwind_data.m_set_user_locals_names.insert ("ans");
 
   // We add all identifiers in the body to the id-table. We also
   // make a map mapping the interpreters frame offset of a id
--- a/test/compile/bytecode.tst	Sat Oct 07 16:45:22 2023 +0200
+++ b/test/compile/bytecode.tst	Thu Oct 05 23:57:25 2023 +0200
@@ -712,13 +712,15 @@
 %! global bytecode_script_topscope_call_self
 %! bytecode_script_topscope_call_self = false;
 %!
-%! bytecode_script_topscope_setup; % Does some setups of globals and locals in base frame
-%! evalin ("base", "bytecode_script_topscope");
+%! bytecode_script_topscope_setup; % Does some setups of globals and locals in base frame, from a function
+%! evalin ("base", "bytecode_script_topscope_setup_script"); % Also does some setups, but from a script
+%! evalin ("base", "bytecode_script_topscope"); % A script that does things a does some asserts
 %!
 %! bytecode_script_topscope_assert; % Does some asserts and cleans up the globals and locals added
 %!
 %! __vm_enable__ (1, "local");
 %! bytecode_script_topscope_setup;
+%! evalin ("base", "bytecode_script_topscope_setup_script");
 %! assert (__vm_compile__ ("bytecode_script_topscope"));
 %! evalin ("base", "bytecode_script_topscope");
 %! bytecode_script_topscope_assert;
@@ -727,6 +729,7 @@
 %! bytecode_script_topscope_call_self = true;
 %!
 %! bytecode_script_topscope_setup;
+%! evalin ("base", "bytecode_script_topscope_setup_script");
 %! assert (__vm_compile__ ("bytecode_script_topscope"));
 %! evalin ("base", "bytecode_script_topscope");
 %! bytecode_script_topscope_assert;
@@ -736,12 +739,50 @@
 %! %% Redo the test, but in a command line function instead of in the top scope.
 %! bytecode_script_topscope_place = "caller"; % Governs some evalin() in nbytecode_script_topscope. Switch to "caller"
 %!
-%! eval ("function bytecode_script_topscope_cli_fn ()\nbytecode_script_topscope_setup ('caller');\nbytecode_script_topscope;\nbytecode_script_topscope_assert ('caller');\nend");
+%! eval ("function bytecode_script_topscope_cli_fn ()\nbytecode_script_topscope_setup ('caller');\nbytecode_script_topscope_setup_script;\nbytecode_script_topscope;\nbytecode_script_topscope_assert ('caller');\nend");
 %!
 %! bytecode_script_topscope_cli_fn ();
 %! bytecode_script_topscope_call_self = true;
 %! bytecode_script_topscope_cli_fn ();
 %!
+%! %% Redo the test, but test all 15 different combinations of compiled and uncompiled functions and scripts
+%!
+%! __vm_enable__ (0, "local");
+%! clear all
+%!
+%! global bytecode_script_topscope_place % An assert in nbytecode_script_topscope need to know whether to check in caller or base
+%! global bytecode_script_topscope_call_self; % Whether nbytecode_script_topscope should call it self to add another frame
+%!
+%! names = {"bytecode_script_topscope_cli_fn", "bytecode_script_topscope_setup", "bytecode_script_topscope", "bytecode_script_topscope_assert", "bytecode_script_topscope_setup_script"};
+%!
+%! for k = 0:5
+%!   choices = nchoosek (names, k);
+%!   for choise = choices'
+%!     clear bytecode_script_topscope_cli_fn bytecode_script_topscope_setup nbytecode_script_topscope nbytecode_script_topscope_assert bytecode_script_topscope_setup_script
+%!
+%!     eval ("function bytecode_script_topscope_cli_fn ()\nbytecode_script_topscope_setup ('caller');\nbytecode_script_topscope_setup_script;\nbytecode_script_topscope;\nbytecode_script_topscope_assert ('caller');\nend");
+%!
+%!     for fn = choise'
+%!       assert (__vm_compile__ (fn{1}))
+%!     end
+%!
+%!     % Check if it works in the CLI function
+%!     bytecode_script_topscope_place = "caller";
+%!     bytecode_script_topscope_call_self = false;
+%!     bytecode_script_topscope_cli_fn ();
+%!     bytecode_script_topscope_call_self = true;
+%!     bytecode_script_topscope_cli_fn ();
+%!
+%!     % Check if it works in base
+%!     bytecode_script_topscope_place = "base";
+%!     bytecode_script_topscope_call_self = false;
+%!     evalin ("base", "bytecode_script_topscope_setup ('base');\nbytecode_script_topscope_setup_script;\nbytecode_script_topscope;\nbytecode_script_topscope_assert ('base');")
+%!     bytecode_script_topscope_call_self = true;
+%!     evalin ("base", "bytecode_script_topscope_setup ('base');\nbytecode_script_topscope_setup_script;\nbytecode_script_topscope;\nbytecode_script_topscope_assert ('base');")
+%!   end
+%! end
+%!
+%! %% Cleanup after the tests
 %! clear global bytecode_script_topscope_call_self
 %! clear global bytecode_script_topscope_place
 %! clear bytecode_script_topscope_cli_fn
\ No newline at end of file
--- a/test/compile/bytecode_script_topscope.m	Sat Oct 07 16:45:22 2023 +0200
+++ b/test/compile/bytecode_script_topscope.m	Thu Oct 05 23:57:25 2023 +0200
@@ -13,12 +13,20 @@
   return
 end
 
+global bytecode_script_topscope_place % "caller" or "base", set in the test script
+
+assert (ans == 1234); % Last calculation in bytecode_script_topscope_setup_script
+4321; % Change ans to 4321
+evalin (bytecode_script_topscope_place, "assert (ans == 4321)"); % Check that ans changed in "caller" or "base" frame
+evalin ("caller", "assert (ans == 4321)"); % Should always change in caller
+
 % These should allready be global
 assert (isglobal ("glb_a"))
 assert (isglobal ("glb_b"))
 assert (isglobal ("glb_c"))
 assert (isglobal ("glb_d"))
 assert (isglobal ("glb_f"))
+assert (isglobal ("glb_g"))
 
 global glb_a % "re-global"
 global glb_b % "re-global"
@@ -26,6 +34,7 @@
 global glb_d = 1234; % re-global, init should not run due to being defined in top scope allready
 global glb_e = 6; % Not defined in top scope
 eval ("global glb_f"); % "re-global", dynamic frame
+global glb_g % "re-global"
 
 assert (glb_a == 2)
 assert (glb_b == 3)
@@ -33,6 +42,7 @@
 assert (glb_d == 5)
 assert (glb_e == 6)
 eval ("assert (glb_f == 7)")
+assert (glb_g == 8)
 
 assert (local_b == 103)
 
@@ -58,8 +68,6 @@
 local_e = 123;
 clear local_e;
 
-global bytecode_script_topscope_place % "caller" or "base", set in the test script
-
 % Not added in top scope. Clear from top scope ...
 local_f = 123;
 evalin (bytecode_script_topscope_place, "clear local_f");
--- a/test/compile/bytecode_script_topscope_assert.m	Sat Oct 07 16:45:22 2023 +0200
+++ b/test/compile/bytecode_script_topscope_assert.m	Thu Oct 05 23:57:25 2023 +0200
@@ -4,11 +4,13 @@
  evalin (place, "assert (!isglobal ('glb_c'))"); % Unglobalized in script
  evalin (place, "assert (isglobal ('glb_d'))");
  evalin (place, "assert (isglobal ('glb_e'))");
+ evalin (place, "assert (isglobal ('glb_g'))");
  evalin (place, "assert (glb_a == 2)");
  evalin (place, "assert (glb_b == 33)");
  evalin (place, "assert (!exist ('glb_c'))");
  evalin (place, "assert (glb_d == 55)");
  evalin (place, "assert (glb_e == 6)"); % Added in the script
+ evalin (place, "assert (glb_g == 8)");
  evalin (place, "assert (local_a == 102)"); % Local added in script
  evalin (place, "assert (local_b == 113)");
  evalin (place, "assert (!exist ('local_c'))"); % Cleared in script
@@ -16,7 +18,8 @@
  evalin (place, "assert (!exist ('local_e'))"); % Cleared in script
  evalin (place, "assert (!exist ('local_f'))"); % Cleared in script
  evalin (place, "assert (!exist ('local_g'))"); % Cleared in script
+ evalin (place, "assert (local_h == 456)");
 
- evalin (place, "clear global glb_a glb_b glb_c glb_d glb_e glb_f");
- evalin (place, "clear local_a local_b local_c local_d local_e local_f local_g");
+ evalin (place, "clear global glb_a glb_b glb_c glb_d glb_e glb_f glb_g");
+ evalin (place, "clear local_a local_b local_c local_d local_e local_f local_g local_h");
 endfunction
\ No newline at end of file
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/compile/bytecode_script_topscope_setup_script.m	Thu Oct 05 23:57:25 2023 +0200
@@ -0,0 +1,7 @@
+% This script is a complement to bytecode_script_topscope_setup, but is a
+% script instead of a function.
+
+local_h = 456;
+global glb_g;
+glb_g = 8;
+1234; % Should assign 1234 to ans
\ No newline at end of file
--- a/test/compile/module.mk	Sat Oct 07 16:45:22 2023 +0200
+++ b/test/compile/module.mk	Thu Oct 05 23:57:25 2023 +0200
@@ -29,6 +29,7 @@
   %reldir%/bytecode_script_topscope.m \
   %reldir%/bytecode_script_topscope_assert.m \
   %reldir%/bytecode_script_topscope_setup.m \
+  %reldir%/bytecode_script_topscope_setup_script.m \
   %reldir%/bytecode_struct.m \
   %reldir%/bytecode_subfuncs.m \
   %reldir%/bytecode_subsasgn.m \