patchGNU Octave - Patches: patch #10366, Same destructor order for...

 
 

patch #10366: Same destructor order for octave_value:s in frame in std::vector for libstdc++ and libc++

Submitter:  Petter <petter>
Submitted:  Sun 25 Jun 2023 12:18:46 AM UTC
   
 
Category:  None Priority:  5 - Normal
Status:  Ready For Test Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sat 15 Jul 2023 08:56:43 AM UTC, comment #9: 

Erasing the elements in the destructor is probably better than overwriting them with new (short-lived) objects that are probably deleted immediately after they've been created. So, I pushed the change that I proposed in comment #8 to the default branch here:
https://hg.savannah.gnu.org/hgweb/octave/rev/b9cabe8a9fb0

Marking as ready for test again.

Markus Mützel <mmuetzel>
Group administrator
Tue 11 Jul 2023 06:21:10 PM UTC, comment #8: 

Maybe something like this?

diff -r 829a32a4171c libinterp/corefcn/stack-frame.cc
--- a/libinterp/corefcn/stack-frame.cc        Tue Jul 11 19:43:03 2023 +0200
+++ b/libinterp/corefcn/stack-frame.cc        Tue Jul 11 20:20:19 2023 +0200
@@ -1197,11 +1197,13 @@

     // Member dtor order is last to first.  So, m_auto_vars before m_values.

-    for (auto& auto_var : m_auto_vars)
-      auto_var = octave_value ();
-
-    for (auto& value : m_values)
-      value = octave_value ();
+    for (auto auto_vars_iter = m_auto_vars.begin ();
+         auto_vars_iter != m_auto_vars.end ();)
+      auto_vars_iter = m_auto_vars.erase (auto_vars_iter);
+
+    for (auto values_iter = m_values.begin ();
+         values_iter != m_values.end ();)
+      values_iter = m_values.erase (values_iter);
   }

   std::size_t size () const


Markus Mützel <mmuetzel>
Group administrator
Tue 11 Jul 2023 06:06:29 PM UTC, comment #7: 

I pushed the following change to use the non-default destructor unconditionally:
https://hg.savannah.gnu.org/hgweb/octave/rev/829a32a4171c

I also took the opportunity to use C++11 idioms for iterating over all loop elements.

Is it really necessary to overwrite each element with a new `octave_value`? Or could we just erase all elements in a given order?

Markus Mützel <mmuetzel>
Group administrator
Tue 11 Jul 2023 05:06:14 PM UTC, comment #6: 

It seems reasonable to me to ensure that the classdef destructors called in the same order for all versions of Octave regardless of what a particular standard library implementation does to destroy vector elements.  In that case, we should not assume that libstdc++ will always behave the way it does now and we should enforce the behavior we want unconditionally.

John W. Eaton <jwe>
Group administrator
Tue 11 Jul 2023 04:58:43 PM UTC, comment #5: 

There doesn't seem to be a guarantee in which order the elements of a `std::vector` are destroyed. See also some citations to different parts of the standard in this discussion: [1]

I don't know if there are any compatibility reasons why we should guarantee a certain order. It just felt a bit "odd" if destructors are called in a different order if Octave is linking against libstdc++ or against libc++. But I don't have any strong feelings about it.
I agree that the explicit destructor should not be conditional on HAVE_LLVM_LIBCXX. It should be used unconditionally in case we'd like to keep this.

If we'd like to revert this change, could we please wait until changes to the BISTs are in place that make them resilient against the order in which elements are destroyed?

[1]: https://stackoverflow.com/questions/6169125/order-of-destruction-of-elements-of-an-stdvector

Markus Mützel <mmuetzel>
Group administrator
Wed 05 Jul 2023 07:03:09 PM UTC, comment #4: 

If there is no C++ standard guarantee of the order that elements of an array will be deleted, then I don't think this change should be conditional on HAVE_LLVM_LIBCXX.

Is there any compatibility reason we need to guarantee the order of execution for classdef destructors?  If not, then I don't see the point in Octave forcing a particular order either and in that case, I'd opt to fix the tests instead.

John W. Eaton <jwe>
Group administrator
Wed 05 Jul 2023 05:38:13 PM UTC, comment #3: 

Thank you for clarifying.

Your patch fixes the failing test for me. So, I pushed it with some minor changes to the default branch here:
https://hg.savannah.gnu.org/hgweb/octave/rev/01ab19a0e2b9

Marking as ready for test.

Btw: Now that the main parts of the VM have landed on the default branch, it might be better to post any new changes on top of that on the bug tracker (instead of the patch tracker). The bug tracker sees a bit more traffic by developers.

Markus Mützel <mmuetzel>
Group administrator
Thu 29 Jun 2023 09:35:25 PM UTC, comment #2: 

From the point of view of the VM it does not matter. It does the libstdc++ order (first to last element of the variables) and does not use the code effected by the patch.

I guess the destructor execution order matter if a user put side effects in a classdef dtor? I guess the specific order is not that important, but it might be confusing of different builds of Octave of the same version are not deterministic in their execution?

I think I'll do a patch an alternative patch for the test that accepts any dtor order anyways.

Petter <petter>
Wed 28 Jun 2023 10:18:36 AM UTC, comment #1: 

If the order in which the destructors are executed really doesn't matter, it might be better (from a maintenance and code simplicity point of view) to adapt the BIST instead imho.

However, if there are cases where the order of destructor execution might matter, it might be better to have the same order with the tree based evaluator and with the VM.

Do you have any hints on that?

Markus Mützel <mmuetzel>
Group administrator
Sun 25 Jun 2023 12:18:46 AM UTC, original submission:  

"
Same destructor order for octave_value:s in frame in std::vector for libstdc++ and libc++

  • libinterp/corefcn/stack-frame.cc:

Explicitly overwrite octave_value:s in libstdc++ order for libc++ users
"

The current behaviour might be correct.

clang with libc++ Octave builds fails in the bytecode_cdef_use.m test due to base_value_stack_frame using std::vector to store octave_value:s in the frame. The test is dependent on that the tree_evaluator has the same classdef dtor order as the VM.

The patch writes nil values to the std::vector:s for libc++ to get the same dtor order as libstdc++.

I remember reading some documentation that classdef dtor order is not defined for Octave so maybe the test should be updated to accept both orders instead?

Petter <petter>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #54872:  octave_32274.patch added by petter (1KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by petter (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 4 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-07-15 mmuetzel StatusIn Progress Ready For Test
    2023-07-11 mmuetzel StatusReady For Test In Progress
    2023-07-05 mmuetzel StatusNone Ready For Test
    2023-06-25 petter Attached File- Added octave_32274.patch, #54872

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code