bugGNU Octave - Bugs: bug #62308, use of octave::feval causes error:...

 
 

bug #62308: use of octave::feval causes error: function '...' not found

Submitter:  Axul Zwar <nalaz177>
Submitted:  Wed 13 Apr 2022 12:18:31 PM UTC
 
Category:  Interpreter Severity:  5 - Blocker
Priority:  7 - High Item Group:  Unexpected Error or Warning
Status:  Fixed Assigned to:  None
Originator Name:  nAlAz117 Open/Closed:  * Closed
Release:  * 7.1.0 Operating System:  * Any
* Mandatory Fields

Add a New Comment (Rich Markup)
   

Comment Type & Canned Response:
       

 

( Jump to the original submission )

Tue 05 Jul 2022 04:05:38 PM UTC, comment #20: 

The changeset has been in the code tree one month.  Marking bug as Fixed and closing report.

Rik <rik5>
Project Administrator
Tue 07 Jun 2022 12:35:51 PM UTC, comment #19: 

@Markus: If you haven't seen any crashes for any of the cases you listed in comment #14, then maybe it is OK to conditionally call shutdown from the destructor.  I don't remember all the details but I probably removed the shutdown call there because I thought calling it from the destructor was too late for it to perform all of its actions.

John W. Eaton <jwe>
Project Administrator
Sat 28 May 2022 12:24:58 PM UTC, comment #18: 

I added a commit message to the patch proposed in comment #12 and pushed it here:
https://hg.savannah.gnu.org/hgweb/octave/rev/ecb867dfc8eb

I hope any possible issues can be caught with or before the release candidate for Octave 7.2.0.

This still leaves the question whether we should make interpreter::shutdown a public function again. That might not be necessary if calling it from the interpreter destructor doesn't cause issues.

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Fri 29 Apr 2022 04:44:30 PM UTC, comment #17: 

No need to be sorry. It's also my impression that startup/shutdown is complex with many different sequences of events and use cases to have in mind. I very much appreciate the work you did in that area.

Do you think the change proposed in comment #12 is ok?
Would it be ok to make `interpreter::shutdown` a public function again for Octave 7.2?

Markus Mützel <mmuetzel>
Project Member
Thu 28 Apr 2022 02:24:25 PM UTC, comment #16: 

@Markus: I also see the crash when using --experimental-terminal-widget without --gui and attempting plotting.  I'm not sure why that is happening, but I'll try to check it out.  I thought that did work at some point and it certainly should.

If I recall correctly, startup/shutdown is complicated by the current GUI design and different ways of starting the interpreter depending on whether the GUI uses the experimental or original terminal widget.  It's still a work in progress.  Sorry for the trouble.

John W. Eaton <jwe>
Project Administrator
Sun 24 Apr 2022 06:18:14 AM UTC, comment #15: 

I wrote 8 interfaces to base mathematical packages including Octave. And all SDK's provide a very similar way to work with it.
1. Maple - StartMaple(...) + StopMaple(...)
2. Mathematica - MLInitialize(...) + MLDeinitialize(...)
3. Matlab - engOpen() + engClose(...)
4. Python - Py_Initialize() + Py_Finalize() (some specific)
5. R - Rf_initEmbeddedR(...) + Rf_endEmbeddedR(...)
6. Ruby - ruby_init() + ruby_cleanup(...)
7. Scilab - StartScilab(...) + TerminateScilab()
I think that a solution in 6.4.0 with execute() + shutdown() is a better choice and more logical. 

Axul Zwar <nalaz177>
Sat 23 Apr 2022 01:29:40 PM UTC, comment #14: 

@jwe: Is calling `interpreter::shutdown` from the destructor problematic? If it is, we'd probably need to call it from somewhere else. Where could that be?

Alternatively, we could advise users of the interpreter class to call shutdown manually before quitting their programs. In that case, `shutdown` would need to be a public member function of the `interpreter` class. Was there a reason to make it a private member function? Would making it public again now constitute an API change?

I am using the changes proposed in comment #12 for a while now. And I'm not seeing any issues with random crashes at exit (neither on Ubuntu nor Windows). But I couldn't reproduce the crashes in bug #61994 either.
It would be nice if someone else could check if this is working for them, too.

IIUC, the "critical" scenarios are e.g.:
- having an oct-file class hanging around when exiting Octave:

a = audioplayer([1:100], 44000)
exit

- closing Octave with an open figure:

sombrero()
exit

- both with and without `--gui`

- gui with and without `--experimental-terminal-widget`

- rebuild the documentation

- others?

(Unrelated to this bug: When I execute `./run-octave --experimental-terminal-widget` and run the command `sombrero`, Octave crashes for me. Don't know if this is a known issue.)

Markus Mützel <mmuetzel>
Project Member
Fri 15 Apr 2022 06:34:34 AM UTC, comment #13: 

I think a possibility to use `shutdown` function should be accessible for an user. To make it as private member is too restrictive.

In 6.4.0 I did following
// Global variable
octave::interpreter *interpreter=NULL;

Init event - separate procedure:
// Start interpreter first
   interpreter = new octave::interpreter;

   if(interpreter)
   {
      interpreter->execute();

      // All other staff
      return true;
   }
   return false;

// Finish (shutdown) event - separate procedure

   interpreter->shutdown();

   delete interpreter;

Remark: To use Octave facilities I have built a DLL-Variant of interface and not standalone solution. Therefore it is possible to split init and exit events in main core application.

Axul Zwar <nalaz177>
Thu 14 Apr 2022 04:14:35 PM UTC, comment #12: 

This all is pretty intricate.

IIUC, the shutdown needs to happen inside `execute` if the interpreter was constructed with app context. It is probably also a good idea for it to happen if an exception is caught (during initialization or any other step).
It should not happen inside that function if the interpreter is embedded like in the example here.
Does that mean it should never shutdown inside that function if the interpreter was constructed without app context?

In that case, the interpreter needs to be shutdown somewhere else (to avoid a segfault at exit). The `shutdown` function is private. So, I assume that it needs to be called somewhat automatically. My best guess was that this could be inside its destructor.

diff -r f67d2ce5d6d4 libinterp/corefcn/interpreter.cc
--- a/libinterp/corefcn/interpreter.cc        Tue Apr 12 17:07:01 2022 +0200
+++ b/libinterp/corefcn/interpreter.cc        Thu Apr 14 18:08:12 2022 +0200
@@ -654,6 +654,9 @@

   interpreter::~interpreter (void)
   {
+    if (! m_app_context)
+      shutdown ();
+
     delete m_gh_manager;
   }

@@ -906,15 +909,17 @@
               }
             else
               exit_status = main_loop ();
+
+            shutdown ();
           }
       }
     catch (const exit_exception& xe)
       {
         exit_status = xe.exit_status ();
+
+        shutdown ();
       }

-    shutdown ();
-
     return exit_status;
   }

However, jwe removed `shutdown` from the destructor for bug #61994:
https://hg.savannah.gnu.org/hgweb/octave/rev/dadbfe6fddd6

So, moving it back there could potentially cause other issues. It fixes the example here for me though...

Markus Mützel <mmuetzel>
Project Member
Thu 14 Apr 2022 01:25:51 PM UTC, comment #11: 

A small workaround. It is enough to patch LIBOCTINTERP-10.DLL to remove wrong call to shutdown() on exit. Then all will be as usual :-)
0083C506: E8 90
0083C507: 65 90
0083C508: 92 90
0083C509: FF 90
0083C50A: FF 90

Axul Zwar <nalaz177>
Thu 14 Apr 2022 12:53:08 PM UTC, comment #10: 

"Do I understand correctly that you also need to avoid calling `interpreter::execute()` completely?"
Yes. The reason is probably "calling `shutdown()` unconditionally before returning"

Axul Zwar <nalaz177>
Thu 14 Apr 2022 12:46:26 PM UTC, comment #9: 

Thanks for tracking this down.

If calling `interpreter::initialize()` is mandatory now, the example code should be adapted:
https://hg.savannah.gnu.org/hgweb/octave/file/d7f799e6d836/examples/code/embedded.cc

Better yet would be to understand why that step is no longer optional. (Or is it?)

Do I understand correctly that you also need to avoid calling `interpreter::execute()` completely?

It is indeed surprising that it is calling `shutdown()` unconditionally before returning:
https://hg.savannah.gnu.org/hgweb/octave/file/d7f799e6d836/libinterp/corefcn/interpreter.cc#l921

CC'ing jwe because he'll probably know how this should be working.

Markus Mützel <mmuetzel>
Project Member
Thu 14 Apr 2022 12:19:54 PM UTC, comment #8: 

Finally got it working!
Simply instead of interpreter.execute() use interpreter.initialize()
All my staff runs perfectly in 7.1.0 with no exceptions

On my opinion a problem placed in \libinterp\corefcn\interpreter.cc in -> int interpreter::execute (void)
In comparison with 6.4.0 you use shutdown() on several places. But why? First initialize() and then on exit shutdown()?

Axul Zwar <nalaz177>
Thu 14 Apr 2022 09:35:46 AM UTC, comment #7: 

Raising priority and setting this as a blocker for the next minor release.

Markus Mützel <mmuetzel>
Project Member
Thu 14 Apr 2022 08:59:07 AM UTC, comment #6: 

For me in Octave 6.4.0 was enough to define
octave::interpreter interpreter
+
interpreter.execute()
and after that everything was working!

Axul Zwar <nalaz177>
Thu 14 Apr 2022 08:49:36 AM UTC, comment #5: 

Problem is not completely solved!
1. Removing of interpreter.execute() causes clean execution but for 'path' is nothing on output :roll_eyes:
2. It seems that interpreter.execute() initializes path to function files over OCTAVE_HOME. If it is not called i can't use lot of functions, for example 'setdiff' or 'repmat'

Axul Zwar <nalaz177>
Thu 14 Apr 2022 08:47:20 AM UTC, comment #4: 

It's probably not a good sign that nothing is shown in the load path and the program is segfaulting instead.

Which commands are needed to initialize the interpreter in standalone programs correctly? Why is `interpreter.execute()` not working?

Markus Mützel <mmuetzel>
Project Member
Thu 14 Apr 2022 07:39:43 AM UTC, comment #3: 

You are right! The problem lies in call of interpreter.execute() !
If this statement will be omitted then all works nicely.

Axul Zwar <nalaz177>
Thu 14 Apr 2022 07:25:35 AM UTC, comment #2: 

If I reduce the program to this:

#include <iostream>
#include <octave/oct.h>
#include <octave/octave.h>
#include <octave/parse.h>
#include <octave/interpreter.h>

int
main (void)
{
  // Create interpreter.

  octave::interpreter interpreter;

  // Don't load init (local or global) files
  interpreter.read_init_files (false);
  interpreter.read_site_files (false);

  octave::feval ("path");
  return 0;
}

it does find and execute the path function and outputs:

Octave's search path contains the following directories:

Segmentation fault

A.R. Burgers <arb>
Wed 13 Apr 2022 12:40:32 PM UTC, comment #1: 

I can confirm on Windows 11 and Ubuntu 21.10 with Octave 7.1.0 or the current stable branch (hg id f67d2ce5d6d4), respectively.

I'm attaching an example that reproduces this for me. Any (built-in?) function seems to cause the same error.

To reproduce build the attached file with:

mkoctfile --link-stand-alone bug62308.cc -o bug62308

Output:

$ ./bug62308
terminate called after throwing an instance of 'octave::execution_exception'
  what():  feval: function 'path' not found

Unfortunately, this snuck it's way into the release. It would be nice if we had some BISTs for this use-case.

(file #53085)

Markus Mützel <mmuetzel>
Project Member
Wed 13 Apr 2022 12:18:31 PM UTC, original submission:  

Any use of octave::feval, for example octave::feval (“path”) brings always error
terminate called after throwing an instance of ‘octave::execution_exception’
what(): feval: function ‘path’ not found

Axul Zwar <nalaz177>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #53085:  bug62308.cc added by mmuetzel (701B - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by arb (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by nalaz177 (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 project members can vote.

     

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2022-07-05 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2022-05-28 mmuetzel StatusConfirmed Ready For Test
    2022-04-14 mmuetzel Carbon-Copy- Added jwe
    2022-04-14 mmuetzel Severity3 - Normal 5 - Blocker
        Priority5 - Normal 7 - High
    2022-04-13 mmuetzel Attached File- Added bug62308.cc, #53085
        StatusNone Confirmed
        Operating SystemMicrosoft Windows Any

    Back to the top


    Powered by Savane 3.9