bugGNU Octave - Bugs: bug #55254, clear all crashes when variable...

 
 

bug #55254: clear all crashes when variable from oct file class is displayed in workspace

Submitted by:  John Donoghue <lostbard>
Submitted on:  Thu 20 Dec 2018 01:24:24 PM UTC  
 
Category:  GUI Severity:  4 - Important
Priority:  5 - Normal Item Group:  Segfault, Bus Error, etc.
Status:  In Progress Assigned to:  John W. Eaton <jwe>
Originator Name:  JohnD Open/Closed:  Open
Release:  dev Operating System:  Any

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Tue 29 Jan 2019 11:14:08 AM UTC, comment #22:

Should I create separate tickets for the forge packages that are effected by this issue ?

John Donoghue <lostbard>
Project Member
Fri 18 Jan 2019 09:22:33 PM UTC, comment #21:

Oops, I forgot to attach the file mentioned in comment #20. It should be attached to this comment instead.

(file #46016)

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Fri 18 Jan 2019 09:20:54 PM UTC, comment #20:

OK, a start at implementing total use counts for octave_value objects is attached. It sketches out what I was thinking about as far as counting, but doesn't attempt to do anything when overall use counts go to zero.

However, it also doesn't work properly as written because virtual methods called from constructors or destructors are not virtual. D'oh, I forgot about that important detail when imagining how this all would work.

So, I see that it could still work, but instead of modifying the octave_base_value constructors and destructor to manage counts for derived classes, the constructors and destructors for ALL of the derived classes would have to be modified to manage their total use counts.

Attempting to make things a bit simpler by managing total use counts inside the octave_value constructors that create the derived types is probably not a good solution because we can't guarantee that all uses will be created this way.

Is it worth the effort to try to modify the constructors and destructor for all derived types to manage their total use counts?

I think it is better to just prevent the unloading of .oct files that define data types. That will still require modifying some existing code, but it will be a simpler and I think much more reliable solution.

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Thu 17 Jan 2019 07:44:36 PM UTC, comment #19:

Another option would be to track the total number of copies for all octave_value objects, not just those that are dynamically loaded. Maybe that would be a bit simpler to do.

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Thu 17 Jan 2019 07:28:02 PM UTC, comment #18:

We can safely unload .oct files that contain simple functions when all the functions are cleared. But to unload a .oct file that defines and installs a new octave_value type, we have to be sure that no references to the dynamically created octave_value type remain and then unregister the octave_value type.

After giving it a little more thought, I can see a way to do that. It will

  • Add the cost of a call to a virtual function to copying any octave_value object.
  • Require changing any dynamically loaded octave_value object to be derived from a new base class that stores a reference to the shared library object that contains the definition of the dynamically loaded octave_value object. This new base class will track all copies of the octave_value object and attempt to unload the shared library object when the count goes to zero.

With these changes, I think reloading a .oct file that defines a new data type would be possible provided that no copies of the dynamically loaded octave_value object(s) exist when attempting to unload the .oct file. If there were, we should be able to emit an error message saying that values need to be cleared first. I don't think it will always be possible for Octave to clear them automatically since devious users may store copies in ways that Octave doesn't know about.

I think the type of crash reported here was avoided in the past because normally, all copies of dynamically loaded octave_value objects were stored in the symbol_scope object. My change to pass a different "summary" symbol_info object to the GUI instead of a symbol_scope object meant that other references might exist that were not deleted before the shared library was unloaded. Going back to the way things were before is not an option because of the pending changes to the way values are stored in the interpreter (see http://lists.gnu.org/archive/html/octave-maintainers/2019-01/msg00042.html for more info about that).

I'll see if I can come up with a draft patch to try. Then we can decide whether the change is worth doing, or whether we should just do something simpler, like prohibiting unloading .oct files that create new octave_value data types.

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Thu 17 Jan 2019 06:03:08 PM UTC, comment #17:

Wont the oct file need a way to be unlocked in order to undate it? (Assuming that is has been loaded and then unloaded) and later updated, or will there need be a more explicit error when trying to update a package that has been used ?

John Donoghue <lostbard>
Project Member
Thu 17 Jan 2019 06:00:18 PM UTC, comment #16:

I kind of think the last solution mentioned might be the best way forward. It is simple, quick to implement, and the penalty is small (the extra memory of a .oct file hanging around).

Rik <rik5>
Project Administrator
Thu 17 Jan 2019 05:55:32 PM UTC, comment #15:

Rik: That's fine as we can avoid the crash unless people go out of their way to make it happen by using munlock.

John D: Yes, a partial fix in that it is not automatic and it will require fixing any .oct files that define new octave_value data types.

However, I'm not sure what to do for a better fix. It is possible to get the current shared library object from inside a DEFMETHOD_DLD function using something like this:

From there, it would be possible to set up an association between the shared library object and any new octave_value types that are created so that the shared library may only be unloaded when all functions and value objects that it defines are cleared. This job is already done for the functions but not the values. But this seems messy and would require defining the octave_value so that they count all instantiated values. A change like this is possible, but would add some overhead and make dynamically loaded octave_value types different from the octave_value objects defined in core Octave.

Maybe it would be better to just add a symbol to .oct files that define new octave_value data types to declare that they may not be unloaded? That's a very simple change for developers to make and imposes very low overhead for Octave (a one-time cost when loading the .oct file).

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Thu 17 Jan 2019 05:37:12 PM UTC, comment #14:

The partial fix being that octave classes doesn't crash, but all class based packages do?

John Donoghue <lostbard>
Project Member
Thu 17 Jan 2019 05:04:01 PM UTC, comment #13:

I'm crossing this off the list at https://wiki.octave.org/Bug_Fix_List_-_5.0_Release#Bugs_marked_as_Crash since there is a partial fix now available.

Rik <rik5>
Project Administrator
Thu 17 Jan 2019 01:21:34 PM UTC, comment #12:

Running your changes in windows appear to work for the audiorecorder class.

I was playing around with the windows package, and if I create an additional function, _windows_pkg_lock_ and call it from PKG_ADD and PKG_DEL to lock, unlock that function, then it stops the clear all from crashing

That doesnt stop someone from unloading the package and then calling clear all though.

John Donoghue <lostbard>
Project Member
Wed 16 Jan 2019 08:43:34 PM UTC, comment #11:

For now, I pushed this changeset for the audiorecorder and audioplayer types:

http://hg.savannah.gnu.org/hgweb/octave/rev/5ace3d2b4674

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Wed 16 Jan 2019 05:47:55 PM UTC, comment #10:

I saw it initially in instrument control when doing a clear all from with a serial port object created.

I also see it if I create a zeromq socket (zmq_socket), get an object in the windows package (actxserver), both which create and return classes based on octave_base_value.

If I do a search in the forge package code repos that I have local for classes using octave_base_value, I get hits on:

communications

database

example-package/plain-package

instrument-control

mpi

parallel

video

windows

zeromq

On all ? of the new classes, we call register_type method on them, at least for the first use of that class type.

John Donoghue <lostbard>
Project Member
Wed 16 Jan 2019 05:02:33 PM UTC, comment #9:

How many other packages define new octave_value types?

And yes, something needs to be done to prevent .oct files from being unloaded while objects of the new octave_value types they create exist in the Octave interpreter.

I'm willing to try for a better fix that doesn't require modifying user code. Do you see a way to do that?

Is there some way to know that an octave_value is created inside a .oct file? Maybe we could make the association when a new data type is added to the "typeinfo" list? I suppose that counting all instances of a particular type could be done in the octave_base_value constructors and destructors, but that adds (a very small) overhead to copying all octave_value types, not just ones that are dynamically loaded. Is it worth trying to avoid that cost for built-in types by having dynamically loaded octave_values derive from an intermediate class instead of octave_base_value? Maybe that would be the simplest thing as the constructor for that new class could also handle the registration/association of the type with the shared library that is loaded?

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Wed 16 Jan 2019 04:33:47 PM UTC, comment #8:

So all forge packages that return a object need to do the same mlock change ?

John Donoghue <lostbard>
Project Member
Wed 16 Jan 2019 07:20:37 AM UTC, comment #7:

I'm not sure why the behavior changed with the changeset mentioned in comment #5, but this seems to be a problem with unloading shared libraries that define new octave_value types. This can happen because "clear all" also clears functions and when all functions that are defined in a shared library (.oct file) are cleared, Octave unloads the shared library. Then if an existing variable of that type is used, some kind of memory error is highly likely.

I'm attaching a temporary fix (mlock all functions that create values of the types that are defined in the .oct file).

Longer term, it would be better to maintain a mapping of dynamically defined data types with the .oct file that defines them and also record the number of values of each dynamically defined data type that exist. Then we could only unload shared libraries that dynamically define data types when the number of those values is zero. Maybe for Octave 6. For now, I think it is best to go with the mlock solution.

(file #45986)

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Tue 15 Jan 2019 06:08:42 PM UTC, comment #6:

Yes, I will take a look at it.

John W. Eaton <jwe>
Project AdministratorIn charge of this item.
Fri 11 Jan 2019 11:58:50 PM UTC, comment #5:

@jwe: I confirm John D.'s bisection and changeset 26117:a6df420457ac is where the problem was introduced. Can you take a look?

Rik <rik5>
Project Administrator
Sun 23 Dec 2018 05:45:19 PM UTC, comment #4:

Thanks for doing the bisection. I added jwe to this bug report since changeset 26117:a6df420457ac was his (drat!).

Rik <rik5>
Project Administrator
Fri 21 Dec 2018 08:18:53 PM UTC, comment #3:

I did a bisect on mine and it indicates changeset 26117:a6df420457ac
use symbol_info_list instead of symbol_scope to pass workspace info to gui

as where it started crashing.

John Donoghue <lostbard>
Project Member
Thu 20 Dec 2018 05:06:48 PM UTC, comment #2:

Confirmed as well on Kubuntu 18.04. As noted, this is only a problem with the GUI. If I start Octave with --no-gui-libs or --no-gui then the code sample

works.

Rik <rik5>
Project Administrator
Thu 20 Dec 2018 04:18:53 PM UTC, comment #1:

Confirmed on Debian.

Mike Miller <mtmiller>
Project Administrator
Thu 20 Dec 2018 01:24:24 PM UTC, original submission:

It doesn't appear to be limited just to audioreorder/player, but is an easy example to do.

Currently only checked in windows

Example:

> ar = audiorecorder


    • The ar value is displayed in the workspace window.

> clear all


    • crash occurs:

If I cleared that variable (even if there are others in the work space) before doing the clear all, it doesn't crash.

It also crashes if run as

> clear -c


It did not crash in 4.4.1

John Donoghue <lostbard>
Project Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #46016:  diffs.txt added by jwe (25KiB - text/plain)
file #45986:  audio-mlock-diffs.txt added by jwe (28KiB - 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 rik5 (Posted a comment)
  • -email is unavailable- added by lostbard (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only project members can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-01-18 jwe Attached File- => Added diffs.txt, #46016
    2019-01-16 jwe StatusPatch Submitted => In Progress
    2019-01-16 jwe Attached File- => Added audio-mlock-diffs.txt, #45986
        StatusConfirmed => Patch Submitted
    2018-12-23 rik5 Assigned toNone => jwe
    2018-12-20 rik5 Operating SystemMicrosoft Windows => Any
    2018-12-20 mtmiller CategoryNone => GUI
        Severity3 - Normal => 4 - Important
        StatusNone => Confirmed

    Back to the top


    Powered by Savane 3.4