Mercurial > octave
changeset 32449:36d6af1923f7
Don't mix up field and id slots when creating bytecode script frames (bug #64817)
Instead of looking up variables by name lineary in the slot name data, use
a map.
* stack-frame.cc: Use map for id lookup
* pt-bytecode-walk.cc: Populate map
* pt-bytecode.h: Change set to map
* test/compile/bytecode_struct.m: Update tests
* test/compile/module.mk: Add new test script
* test/compile/script_defines_qweqwe.m: New test script
| author | Petter T. |
|---|---|
| date | Fri, 27 Oct 2023 18:20:27 +0200 |
| parents | a7a7ac88fdc4 |
| children | 1f77c3f61aea |
| files | libinterp/corefcn/stack-frame.cc libinterp/parse-tree/pt-bytecode-walk.cc libinterp/parse-tree/pt-bytecode.h test/compile/bytecode_struct.m test/compile/module.mk test/compile/script_defines_qweqwe.m |
| diffstat | 6 files changed, 64 insertions(+), 45 deletions(-) [+] |
line wrap: on
line diff
--- a/libinterp/corefcn/stack-frame.cc Fri Oct 27 18:20:21 2023 +0200 +++ b/libinterp/corefcn/stack-frame.cc Fri Oct 27 18:20:27 2023 +0200 @@ -1079,29 +1079,14 @@ symbol_record lookup_symbol (const std::string& name) const { - int local_offset = -1; scope_flags flag = LOCAL; - for (int i = 0; i < static_cast<int> (m_size); i++) + auto it = m_unwind_data->m_map_user_locals_names_to_slot.find (name); + + if (it != m_unwind_data->m_map_user_locals_names_to_slot.end ()) { - if (m_name_data [i] == name) - { - local_offset = i; - - bool is_global = slot_is_global (local_offset); - bool is_pers = slot_is_persistent (local_offset); - - if (is_global) - flag = GLOBAL; - else if (is_pers) - flag = PERSISTENT; - - break; - } - } - - if (local_offset >= 0) - { + int local_offset = it->second; + symbol_record ret (name, flag); size_t frame_offset; @@ -1139,7 +1124,7 @@ } // Search the "scope" object of this and any nested frame - // The scope object will have e.g. variables added by scripts or eval + // The scope object will have e.g. variables added by scripts, global declares or eval const stack_frame *frame = this; std::size_t frame_cntr = 0; while (frame) @@ -1150,9 +1135,10 @@ if (sym) { - // Return symbol record with adjusted frame offset (relative to the one lookup is done on) + // Return symbol record with adjusted frame offset relative + // to the one lookup is done on, i.e. the 'this' frame. symbol_record new_sym = sym.dup (); - new_sym.set_frame_offset (frame_cntr); + new_sym.set_frame_offset (new_sym.frame_offset () + frame_cntr); return new_sym; } @@ -1460,8 +1446,9 @@ // Move all user symbol values from the parent frame to the eval frame. // Replace the values in the parent frame with a pointer-like object "octave_value_ref_vmlocal" // pointing to the eval frame. - for (std::string id_name : parent_frame_bc->m_unwind_data->m_set_user_locals_names) + for (const auto &kv : parent_frame_bc->m_unwind_data->m_map_user_locals_names_to_slot) { + const std::string &id_name = kv.first; symbol_record sr_eval = eval_frame->lookup_symbol (id_name); if (!sr_eval.is_valid ()) sr_eval = eval_frame->insert_symbol (id_name); @@ -1506,8 +1493,9 @@ // Move all user symbols from the eval frame to the current frame we are entering. // Replace the moved values in the eval frame with a pointer-like object "octave_value_ref_vmlocal" // pointing to the current frame. - for (std::string id_name : m_unwind_data->m_set_user_locals_names) + for (const auto &kv : m_unwind_data->m_map_user_locals_names_to_slot) { + const std::string &id_name = kv.first; symbol_record sr_eval = eval_frame->lookup_symbol (id_name); if (!sr_eval.is_valid ()) sr_eval = eval_frame->insert_symbol (id_name); @@ -1554,10 +1542,12 @@ bool eval_frame_is_bytecode = eval_frame->is_bytecode_fcn_frame (); // Move all user symbols from the current frame to the eval frame. - for (std::string id_name : m_unwind_data->m_set_user_locals_names) + for (const auto &kv : m_unwind_data->m_map_user_locals_names_to_slot) { + const std::string &id_name = kv.first; symbol_record sr_eval = eval_frame->lookup_symbol (id_name); CHECK_PANIC (sr_eval.is_valid ()); + octave_value *ov_eval = sr_eval.frame_offset () ? &eval_frame->varref (sr_eval, false) : &eval_frame->varref (sr_eval.data_offset (), false); @@ -1595,8 +1585,9 @@ // Move all values the parent frame needs to it from the eval frame. // In the eval frame, put a pointer-like object "octave_value_ref_vmlocal" // pointing to the parent frame - for (std::string id_name : parent_frame_bc->m_unwind_data->m_set_user_locals_names) + for (const auto &kv : parent_frame_bc->m_unwind_data->m_map_user_locals_names_to_slot) { + const std::string &id_name = kv.first; symbol_record sr_eval = eval_frame->lookup_symbol (id_name); CHECK_PANIC (sr_eval.is_valid ()); octave_value *ov_eval = sr_eval.frame_offset () ?
--- a/libinterp/parse-tree/pt-bytecode-walk.cc Fri Oct 27 18:20:21 2023 +0200 +++ b/libinterp/parse-tree/pt-bytecode-walk.cc Fri Oct 27 18:20:27 2023 +0200 @@ -2418,10 +2418,10 @@ add_id_to_table("%nargout"); // 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 + int slot_ans = add_id_to_table ("ans"); + // m_map_user_locals_names_to_slot 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"); + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({"ans", slot_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 @@ -2445,7 +2445,7 @@ m_code.m_unwind_data.m_external_frame_offset_to_internal[frame_offset][offset] = slot; if (frame_offset == 0) - m_code.m_unwind_data.m_set_user_locals_names.insert (name); + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({name, slot}); } } @@ -2630,17 +2630,19 @@ { for (auto it = returns->begin (); it != returns->end (); it++) { - std::string name = (*it)->name(); tree_identifier *id = (*it)->ident (); CHECK_NONNULL (id); - add_id_to_table (id->name ()); + std::string name = id->name (); + int slot = add_id_to_table (name); + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({name, slot}); } } + if (m_varargout) - add_id_to_table ("varargout"); // Not in the returns list. Need to be last - - // The function itself is put after the arg outs - /* add_id_to_table (fcn.name ()); */ + { + int slot = add_id_to_table ("varargout"); // Not in the returns list. Need to be last + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({"varargout", slot}); + } // Then the arguments for (std::string name : v_paras) @@ -2678,11 +2680,13 @@ continue; } - add_id_to_table (name); + int slot = add_id_to_table (name); + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({name, slot}); } // We always need the magic id "ans" - add_id_to_table ("ans"); + int slot_ans = add_id_to_table ("ans"); + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({"ans", slot_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 @@ -2705,7 +2709,7 @@ m_code.m_unwind_data.m_external_frame_offset_to_internal[frame_offset][offset] = slot; if (frame_offset == 0) - m_code.m_unwind_data.m_set_user_locals_names.insert (name); + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({name, slot}); } } // We need the arguments and return id:s in the map too. @@ -2716,8 +2720,10 @@ CHECK_NONNULL (*it); tree_identifier *id = (*it)->ident (); int offset = id->symbol ().data_offset (); - int slot = SLOT (id->name ()); + std::string name = id->name (); + int slot = SLOT (name); m_code.m_unwind_data.m_external_frame_offset_to_internal[0][offset] = slot; + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({name, slot}); // If the parameter has an init expression e.g. // "function foo (a = sin (pi))" @@ -2728,12 +2734,12 @@ auto v_names_offsets = collect_idnames_walker::collect_id_names (*init_expr); for (auto name_offset : v_names_offsets) { - std::string name = name_offset.m_name; + std::string name_i = name_offset.m_name; int offset_i = name_offset.m_offset; - add_id_to_table (name); - int slot_i = SLOT (name); + int slot_i = add_id_to_table (name_i); m_code.m_unwind_data.m_external_frame_offset_to_internal[0][offset_i] = slot_i; + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({name_i, slot_i}); } } } @@ -2791,6 +2797,8 @@ int slot = add_id_to_table (fn_name); m_code.m_unwind_data.m_external_frame_offset_to_internal[frame_offset][offset] = slot; + + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({fn_name, slot}); } // Add the parents' function names for nested functions @@ -2810,6 +2818,8 @@ int slot = add_id_to_table (parent_name); m_code.m_unwind_data.m_external_frame_offset_to_internal[frame_offset][offset] = slot; + + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({parent_name, slot}); } } @@ -2880,6 +2890,8 @@ // Increasing number for fake external offset int fake_external_offset = m_code.m_unwind_data.m_external_frame_offset_to_internal[1].size (); m_code.m_unwind_data.m_external_frame_offset_to_internal[1][fake_external_offset] = slot_added; + + m_code.m_unwind_data.m_map_user_locals_names_to_slot.insert ({name, slot_added}); } int slot = SLOT (name);
--- a/libinterp/parse-tree/pt-bytecode.h Fri Oct 27 18:20:21 2023 +0200 +++ b/libinterp/parse-tree/pt-bytecode.h Fri Oct 27 18:20:27 2023 +0200 @@ -273,7 +273,7 @@ std::map<int, tree*> m_ip_to_tree; std::vector<arg_name_entry> m_argname_entries; std::vector<std::map<int,int>> m_external_frame_offset_to_internal; - std::set<std::string> m_set_user_locals_names; + std::map<std::string, int> m_map_user_locals_names_to_slot; struct nested_var_offset { int m_depth; int m_slot_parent; int m_slot_nested; }; std::vector<nested_var_offset> m_v_nested_vars;
--- a/test/compile/bytecode_struct.m Fri Oct 27 18:20:21 2023 +0200 +++ b/test/compile/bytecode_struct.m Fri Oct 27 18:20:27 2023 +0200 @@ -17,8 +17,22 @@ % Test word command struct subref __printf_assert__ ("%d ", suby.b); + + % Bug 64817 + test_bug_64817; end function a = suby () a.b = 4; end + +function test_bug_64817 + % Field id slot collided with variable slot when making + % the script frame. + + script_defines_qweqwe; % Just does "qweqwe = 0;" + + if 0 + asd.qweqwe; % 'qweqwe' is a field here + end +end
--- a/test/compile/module.mk Fri Oct 27 18:20:21 2023 +0200 +++ b/test/compile/module.mk Fri Oct 27 18:20:27 2023 +0200 @@ -48,6 +48,7 @@ %reldir%/inputname_args.m \ %reldir%/just_call_handle_with_arg.m \ %reldir%/return_isargout.m \ + %reldir%/script_defines_qweqwe.m \ %reldir%/script1.m \ %reldir%/script11.m \ %reldir%/script2.m \