Mercurial > octave
changeset 32328:dfcbe2dcd8e2
VM Fix '[]' as rhs in chained subassigns (bug #64704)
Using the constant '[]' as rhs in e.g. 'a(ii) = b(ii) = []' made
'[]' lose its deleting abilities on 'a' due to call to storable().
* libinterp/parse-tree/pt-bytecode-vm.cc: New opcode DUP_MOVE
* libinterp/parse-tree/pt-bytecode-walk.cc: Use DUP_MOVE instead of slots
and FORCE_ASSIGN
* libinterp/parse-tree/pt-bytecode.h: Opcode enum
* test/compile/bytecode_subsasgn.m: Update tests
| author | Petter T. <petter.vilhelm@gmail.com> |
|---|---|
| date | Sat, 23 Sep 2023 00:45:01 +0200 |
| parents | 55ea70c8d00a |
| children | 0c32e838e522 |
| files | libinterp/parse-tree/pt-bytecode-vm.cc libinterp/parse-tree/pt-bytecode-walk.cc libinterp/parse-tree/pt-bytecode.h test/compile/bytecode_subsasgn.m |
| diffstat | 4 files changed, 82 insertions(+), 48 deletions(-) [+] |
line wrap: on
line diff
--- a/libinterp/parse-tree/pt-bytecode-vm.cc Sat Sep 23 08:03:06 2023 -0400 +++ b/libinterp/parse-tree/pt-bytecode-vm.cc Sat Sep 23 00:45:01 2023 +0200 @@ -279,6 +279,7 @@ CASE_START (LOAD_CST_ALT4) PCHAR () CASE_END () CASE_START (LOAD_2_CST) PCHAR () CASE_END () CASE_START (POP_N_INTS) PCHAR () CASE_END () + CASE_START (DUP_MOVE) PCHAR () CASE_END () CASE_START (ASSIGN) PSLOT() CASE_END () CASE_START (BIND_ANS) PSLOT() CASE_END () @@ -864,6 +865,7 @@ &&anon_maybe_set_ignore_output, &&enter_nested_frame, &&install_function, + &&dup_move, }; if (OCTAVE_UNLIKELY (m_profiler_enabled)) @@ -6211,6 +6213,16 @@ goto *instr [opcode]; // Execute the next opcode } + dup_move: + { + // Copy the top of the stack and move it n positions down the stack. + int n = arg0; + + octave_value ov = sp[-1].ov; + sp[-1 - n].ov = ov; + } + DISPATCH (); + enter_script_frame: { auto fp = m_tw->get_current_stack_frame ();
--- a/libinterp/parse-tree/pt-bytecode-walk.cc Sat Sep 23 08:03:06 2023 -0400 +++ b/libinterp/parse-tree/pt-bytecode-walk.cc Sat Sep 23 00:45:01 2023 +0200 @@ -3589,6 +3589,11 @@ tree_argument_list *arg = *arg_lists.begin (); + // rhs will be copied to this stack position if assigns are chained. + // (Chained as in b(1) = c(2) = 3) + if (DEPTH () != 1) + PUSH_CODE (INSTR::PUSH_NIL); + int nargs = 0; if (arg) { @@ -3612,19 +3617,19 @@ // The value of rhs is on the operand stack now // If the assignment is not at root we want to keep the - // value on the stack, e.g. + // rhs value on the stack, e.g. // a = b(1) = 3; // Gives: a == 3 - // We use a slot to store the rhs in. - std::string rhs_copy_nm = "%rhs_" + std::to_string (CODE_SIZE ()); - int slot_cpy = -1; + // + // If that is the case, we pushed a nil earlier, so we + // copy the top of the stack (rhs) to the nils place in the stack. + // The copy will then be in place to be rhs again. if (DEPTH () != 1) { - slot_cpy = add_id_to_table (rhs_copy_nm); - PUSH_CODE (INSTR::DUP); - MAYBE_PUSH_WIDE_OPEXT (slot_cpy); - PUSH_CODE (INSTR::FORCE_ASSIGN); - PUSH_SLOT (slot_cpy); + PUSH_CODE (INSTR::DUP_MOVE); + // There is rhs and n args on the stack, over the nil + // we want to copy rhs (the top of the stack) to. + PUSH_CODE (nargs + 1); } int slot = SLOT (name); @@ -3633,13 +3638,6 @@ PUSH_SLOT (slot); PUSH_CODE (nargs); - if (DEPTH () != 1) - { - MAYBE_PUSH_WIDE_OPEXT (slot_cpy); - PUSH_CODE (INSTR::PUSH_SLOT_INDEXED); - PUSH_SLOT (slot_cpy); - } - maybe_emit_push_and_disp_id (expr, name); } else if (type == '.') @@ -3670,15 +3668,19 @@ rhs->accept (*this); // The value of rhs is on the operand stack now - std::string rhs_copy_nm = "%rhs_" + std::to_string (CODE_SIZE ()); - int slot_cpy = -1; - if (DEPTH () != 1) // Chained assignments? + // If the assignment is not at root we want to keep the + // rhs value on the stack, e.g. + // a = b(1) = 3; + // Gives: a == 3 + // + // If that is the case, dup rhs on the stack. + // The copy will then be in place to be rhs again. + if (DEPTH () != 1) { - slot_cpy = add_id_to_table (rhs_copy_nm); + // There is 1 rhs and no args on the stack, so just do a dup, + // not a DUP_MOVE like for SUBASSIGN_ID and SUBASSIGN_CELL that + // got args on the stack that need to be moved around. PUSH_CODE (INSTR::DUP); - MAYBE_PUSH_WIDE_OPEXT (slot_cpy); - PUSH_CODE (INSTR::FORCE_ASSIGN); - PUSH_SLOT (slot_cpy); } int slot = SLOT (name); @@ -3687,13 +3689,6 @@ PUSH_SLOT (slot); PUSH_WSLOT (slot_field); - if (DEPTH () != 1) - { - MAYBE_PUSH_WIDE_OPEXT (slot_cpy); - PUSH_CODE (INSTR::PUSH_SLOT_INDEXED); - PUSH_SLOT (slot_cpy); - } - maybe_emit_push_and_disp_id (expr, name); } else if (is_dynamic_field && is_id) @@ -3831,6 +3826,11 @@ CHECK (arg_lists.size ()); tree_argument_list *arg = *arg_lists.begin (); + // rhs will be copied to this stack position if assigns are chained. + // (Chained as in b(1) = c(2) = 3) + if (DEPTH () != 1) + PUSH_CODE (INSTR::PUSH_NIL); + int nargs = 0; if (arg) { @@ -3854,19 +3854,19 @@ // The value of rhs is on the operand stack now // If the assignment is not at root we want to keep the - // value on the stack, e.g. - // a = b(1) = 3; - // Gives: a == 3 - // We use a slot to store the rhs in. - std::string rhs_copy_nm = "%rhs_" + std::to_string (CODE_SIZE ()); - int slot_cpy = -1; + // rhs value on the stack, e.g. + // a = b(1) = 3; + // Gives: a == 3 + // + // If that is the case, we pushed a nil earlier, so we + // copy the top of the stack (rhs) to the nils place in the stack. + // The copy will then be in place to be rhs again. if (DEPTH () != 1) { - slot_cpy = add_id_to_table (rhs_copy_nm); - PUSH_CODE (INSTR::DUP); - MAYBE_PUSH_WIDE_OPEXT (slot_cpy); - PUSH_CODE (INSTR::FORCE_ASSIGN); - PUSH_SLOT (slot_cpy); + PUSH_CODE (INSTR::DUP_MOVE); + // There is rhs and n args on the stack, over the nil + // we want to copy rhs (the top of the stack) to. + PUSH_CODE (nargs + 1); } int slot = SLOT (name); @@ -3875,13 +3875,6 @@ PUSH_SLOT (slot); PUSH_CODE (nargs); - if (DEPTH () != 1) - { - MAYBE_PUSH_WIDE_OPEXT (slot_cpy); - PUSH_CODE (INSTR::PUSH_SLOT_INDEXED); - PUSH_SLOT (slot_cpy); - } - maybe_emit_push_and_disp_id (expr, name); } else
--- a/libinterp/parse-tree/pt-bytecode.h Sat Sep 23 08:03:06 2023 -0400 +++ b/libinterp/parse-tree/pt-bytecode.h Sat Sep 23 00:45:01 2023 +0200 @@ -193,6 +193,7 @@ ANON_MAYBE_SET_IGNORE_OUTPUTS, ENTER_NESTED_FRAME, INSTALL_FUNCTION, + DUP_MOVE, }; enum class unwind_entry_type
--- a/test/compile/bytecode_subsasgn.m Sat Sep 23 08:03:06 2023 -0400 +++ b/test/compile/bytecode_subsasgn.m Sat Sep 23 00:45:01 2023 +0200 @@ -112,4 +112,32 @@ M = ones (2,2); M(idx{:}) *= 3; __printf_assert__ ("%d ", M); + + % SUBASSIGN_ID need to handle magic [] constant when chained (bug #64704) + M1 = [1 2 3]; + M2 = [4 5 6]; + M3 = [7 8 9]; + + M1(1) = M2(2) = M3(3) = []; + assert (M1 == [2 3]) + assert (M2 == [4 6]) + assert (M3 == [7 8]) + + M1 = {1 2 3}; + M2 = {4 5 6}; + M3 = {7 8 9}; + + M1(1) = M2(2) = M3(3) = []; + assert (cell2mat (M1) == [2 3]) + assert (cell2mat (M2) == [4 6]) + assert (cell2mat (M3) == [7 8]) + + M1 = [1 2 3; 4 5 6]; + M2 = {4 5 6; 7 8 9}; + S3.a = "zxc"; + + M1(1,2) = M2{2,1} = S3.a = 123; + assert (M1 == [1 123 3; 4 5 6]) + assert (cell2mat (M2) == [4 5 6; 123 8 9]) + assert (S3.a == 123) end