Mercurial > octave
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 \