bugGNU Octave - Bugs: bug #59597, bogus result returned by...

 
 

bug #59597: bogus result returned by mexCallMATLAB

Submitter:  Sébastien Villemot <svillemot>
Submitted:  Wed 02 Dec 2020 03:56:38 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * 6.1.0 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 11 Dec 2020 11:25:40 AM UTC, comment #26: 

OK, closing this bug report.

I opened the following topic about isargout vs. feval:

https://octave.discourse.group/t/isargout-vs-feval/522

John W. Eaton <jwe>
Group administrator
Fri 11 Dec 2020 11:02:53 AM UTC, comment #25: 

Thanks John, much appreciated!

I confirm that your patch fixes the problem (and also other problems that I had in similar contexts).

As far as I am concerned, this issue can now be closed.

Sébastien Villemot <svillemot>
Fri 11 Dec 2020 10:27:52 AM UTC, comment #24: 

I pushed the following more local change instead.

http://hg.savannah.gnu.org/hgweb/octave/rev/32fdcf67a683

I think that fixes the immediate problem but this bug report has made me aware of some fundamental trouble with the way isargout works.  I'll either open a separate bug report or discourse topic about that.

John W. Eaton <jwe>
Group administrator
Wed 09 Dec 2020 06:54:53 PM UTC, comment #23: 

The attached patch appears to avoid the problem for me but I'm not ready to push it yet because it affects the behavior of isargout for function calls made through feval and function handles.



(file #50430)

John W. Eaton <jwe>
Group administrator
Wed 09 Dec 2020 10:57:50 AM UTC, comment #22: 


commentaire #21 :

> In DynamicModelCaller using only dlopen/LoadLibrary to load a mex function may not work. Each mex function requires its own workspace/context to manage its memory. The function 'call_mex' in 'libinterp/corefcn/mex.cc' shows how the mex_context (memory manager) works. Instead of loading mex by dlopen calling the mex by its name and letting Octave to properly find and call the function I think is a better option. 'addpath' may be needed to help the interpreter to find the function.


When using dlopen(), we indeed load a MEX but we actually do not run the mexFunction(), we only run other subfunctions that do not give back control to Octave. So your point is moot.

Please remember that this MEX works fine with MATLAB (all versions from R2009b to R2020b), and has worked fine with all versions of Octave before 6.1.0. I consider it unlikely that the problem comes from a design issue in this MEX.

Sébastien Villemot <svillemot>
Wed 09 Dec 2020 10:28:05 AM UTC, comment #21: 

In DynamicModelCaller using only dlopen/LoadLibrary to load a mex function may not work. Each mex function requires its own workspace/context to manage its memory. The function 'call_mex' in 'libinterp/corefcn/mex.cc' shows how the mex_context (memory manager) works. Instead of loading mex by dlopen calling the mex by its name and letting Octave to properly find and call the function I think is a better option. 'addpath' may be needed to help the interpreter to find the function.

Anonymous
Wed 09 Dec 2020 07:28:41 AM UTC, comment #20: 


commentaire #19 :

> Taking a look into the perfect_foresight_problem.cc the DynamicModelMatlabCaller is evaluated that it indeed calls mexCallMATLABWithTrap in the OMP parallel region. Generally Octave interpreter and specially 'mexCallMATLAB' and its memory manager aren't thread safe and concurrent accesses possibly corrupt the mex memory manager.


No, this case won’t happen. mexCallMATLABWithTrap() is only called when use_dll=false, in which case OMP is disabled.

Sébastien Villemot <svillemot>
Wed 09 Dec 2020 06:47:48 AM UTC, comment #19: 

Taking a look into the perfect_foresight_problem.cc the DynamicModelMatlabCaller is evaluated that it indeed calls mexCallMATLABWithTrap in the OMP parallel region. Generally Octave interpreter and specially 'mexCallMATLAB' and its memory manager aren't thread safe and concurrent accesses possibly corrupt the mex memory manager.

Anonymous
Tue 08 Dec 2020 06:26:41 PM UTC, comment #18: 

Sébastien: Thanks.  I also had to install the Debian x13as package and the statistics package from Octave Forge.  Then with the statistics package loaded I'm able to duplicate the error.

Now that I can see the error I'll try to see if I can figure out what is causing it.

John W. Eaton <jwe>
Group administrator
Tue 08 Dec 2020 03:27:31 PM UTC, comment #17: 

It looks like a different issue so I don't want to waste your time but the one time  I observed MEX files behaving differently according to the order in which they were executed is described in bug #55363.

Guillaume <gyom>
Tue 08 Dec 2020 02:44:21 PM UTC, comment #16: 

commentaire #13 :

> So we can rule out whether this problem is due to the MEX interface or some other problem with the interpreter itself, can you replace the MEX function with a .oct file that does the same job?


The full source of the MEX is there:
https://git.dynare.org/Dynare/dynare/-/tree/master/mex/sources/perfect_foresight_problem
As you can see, rewriting it as an .oct file is not a straightforward task.

> Or, can you provide complete instructions for how to trigger the problem with Dynare, the way you originally discovered it?  If I could reproduce the problem, then maybe I could do the mex/oct file comparison?


Ok, let’s try that.

First you need to install the build dependencies of Dynare. There are many, and they are listed in this README.

Note that if you are on Debian or on a Debian-based system, all dependencies are packaged, and they are listed in the above README. An even faster way of installing dependencies is by running `apt build-dep dynare` (note that this will install Debian-specific stuff that you do not really need, and also the Debian package for octave).

Then you can do the following:

$ wget https://www.dynare.org/release/source/dynare-4.6.3.tar.xz
$ tar xf dynare-4.6.3.tar.xz
$ cd dynare-4.6.3
$ export OCTAVE_PATH=/path/to/your/local/Octave/6.1.0/copy
$ ./configure --disable-matlab OCTAVE=$OCTAVE_PATH/bin/octave MKOCTFILE=$OCTAVE_PATH/bin/mkoctfile
$ make -j$(nproc)
$ cd tests/ep
$ $OCTAVE_PATH/bin/octave
> addpath([ pwd '/../../matlab' ])
> dynare rbc


Under Octave 6.1.0, this should fail with:

error: perfect_foresight_problem: nzij_pred should be an int32 matrix with 2 columns


Under Octave 5.2.0, this should succeed.

Note that the m-file that is called via mexCallMATLAB() is actually generated at run-time. It will be located under `+rbc/dynamic_g1_nz.m`.

Sébastien Villemot <svillemot>
Mon 07 Dec 2020 06:34:09 PM UTC, comment #15: 

Rik: Yes, if you already have access to the interpreter object, then it is best to call feval directly through that instead of using the global octave::feval function.  The global function will just look up the interpreter object and then call the member function.  See the feval function definitions in oct-parse.yy.

I haven't tagged those as deprecated yet because they are widely used and I don't think we really want to eliminate them (yet).  But their use should be discouraged.  It might be good if we had a way to distinguish between "this function is deprecated and will be removed in two releases" and "using this function is discouraged but it will remain available".

John W. Eaton <jwe>
Group administrator
Mon 07 Dec 2020 06:11:13 PM UTC, comment #14: 

@jwe: Unfortunately my patch did not fix the bug, but do you understand whether it is okay to use octave::feval rather than interp.feval?  It seems like the patch might be a good thing to apply anyways.  For reference, here is the entire mexCallMATLAB function from mex.cc.


int
mexCallMATLAB (int nargout, mxArray *argout[], int nargin,
               mxArray *argin[], const char *fname)
{
  octave_value_list args;

  // FIXME: do we need unwind protect to clean up args?  Off hand, I
  // would say that this problem is endemic to Octave and we will
  // continue to have memory leaks after Ctrl-C until proper exception
  // handling is implemented.

  // FIXME: Proper exception handling has been implemented (Jan. 2016).
  //        Can this code be re-factored?
  args.resize (nargin);

  for (int i = 0; i < nargin; i++)
    args(i) = mxArray::as_octave_value (argin[i]);

  octave::interpreter& interp = octave::__get_interpreter__ ("mexCallMATLAB");

  bool execution_error = false;

  octave_value_list retval;

  try
    {
      retval = octave::feval (fname, args, nargout);
      // My proposed change below
      //retval = interp.feval (fname, args, nargout);
    }
  catch (const octave::execution_exception&)
    {
      if (mex_context->trap_feval_error)
        {
          // FIXME: is there a way to indicate what error occurred?
          // Should the error message be displayed here?  Do we need to
          // save the exception info for lasterror?

          interp.recover_from_exception ();

          execution_error = true;
        }
      else
        {
          args.resize (0);
          retval.resize (0);

          throw;
        }
    }

  int num_to_copy = retval.length ();

  if (nargout < retval.length ())
    num_to_copy = nargout;

  for (int i = 0; i < num_to_copy; i++)
    {
      // FIXME: it would be nice to avoid copying the value here,
      // but there is no way to steal memory from a matrix, never mind
      // that matrix memory is allocated by new[] and mxArray memory
      // is allocated by malloc().
      argout[i] = mex_context->make_value (retval(i));
    }

  while (num_to_copy < nargout)
    argout[num_to_copy++] = nullptr;

  return execution_error ? 1 : 0;
}



Rik <rik5>
Group administrator
Mon 07 Dec 2020 05:38:41 PM UTC, comment #13: 

Sébastien:

So we can rule out whether this problem is due to the MEX interface or some other problem with the interpreter itself, can you replace the MEX function with a .oct file that does the same job?

Or, can you provide complete instructions for how to trigger the problem with Dynare, the way you originally discovered it?  If I could reproduce the problem, then maybe I could do the mex/oct file comparison?

John W. Eaton <jwe>
Group administrator
Mon 07 Dec 2020 05:25:36 PM UTC, comment #12: 


commentaire #11 :

> I've attached a one-line patch which I think might have an effect on this bug.


Thanks. I’ve applied the patch on top of a pristine octave 6.1.0 source tree, recompiled, installed, rerun the test case (including recompiling the MEX).

Unfortunately the problem is still there.

Sébastien Villemot <svillemot>
Mon 07 Dec 2020 04:58:16 PM UTC, comment #11: 

I've attached a one-line patch which I think might have an effect on this bug.

Steps for testing

1) Get Octave source code for the stable branch (Either from tarball or hg clone)
2) Change directory to source dir and execute


patch -p1 < bug59597.diff


3) configure, make, and run 'make install' as you normally would with whatever options you traditionally use.  It is important that 'make install' be run because mex and mkoctfile don't work correctly if you just use run-octave on the newly created octave executable.

4) Compile the problem mex file.  In this case, it might be uninstalling the package and then re-installing the package to force the new code to take effect.

5) Finally, run the program.  I'm moderately hopeful, maybe 1/3rd chance, that this might fix things.

(file #50413)

Rik <rik5>
Group administrator
Mon 07 Dec 2020 01:42:26 PM UTC, comment #10: 


commentaire #8 :

> @Sebastien: Can you build Octave from source?  I noticed one odd thing in the mexCallMATLAB code that might be a problem.  I've tested the diff on my machine and it doesn't cause any problems, but it might resolve this issue.


Of course I would be happy to try a patch! (actually I am also the maintainer of the official Debian package for Octave, so I’ve built Octave multiple times in my life)

Sébastien Villemot <svillemot>
Mon 07 Dec 2020 01:41:30 PM UTC, comment #9: 


commentaire #7 :

> Question #1: Is there any variability about when the mex file fails?  For example, does it occasionally fail on the second invocation or occasionally on the fourth invocation or is it always exactly the third?


It consistently fails exactly on the third time whenever I run it manually. However I’ve not tried to execute it a large number of times to verify this more systematically.

> Question #2: Can you try putting a keyboard command at the bottom of the m-file.  Then call your code normally.  When the interpreter stops in the m-file, inspect the variables to see if there is any thing wrong with them at the interpreter level.


I did, and the variables look correct just before returning from the function in the m-file.

> Question #3: In the mex file, could you add this line to the start of the function?
>


>   mexSetTrapFlag (0);


>
> This should ensure that if something goes wrong with the call to feval() that an error is thrown.


I did, and it changes nothing.

Sébastien Villemot <svillemot>
Sat 05 Dec 2020 05:58:54 PM UTC, comment #8: 

@Sebastien: Can you build Octave from source?  I noticed one odd thing in the mexCallMATLAB code that might be a problem.  I've tested the diff on my machine and it doesn't cause any problems, but it might resolve this issue.

Rik <rik5>
Group administrator
Sat 05 Dec 2020 02:11:53 AM UTC, comment #7: 

Question #1: Is there any variability about when the mex file fails?  For example, does it occasionally fail on the second invocation or occasionally on the fourth invocation or is it always exactly the third?

Question #2: Can you try putting a keyboard command at the bottom of the m-file.  Then call your code normally.  When the interpreter stops in the m-file, inspect the variables to see if there is any thing wrong with them at the interpreter level.

Question #3: In the mex file, could you add this line to the start of the function?


  mexSetTrapFlag (0);


This should ensure that if something goes wrong with the call to feval() that an error is thrown.

Rik <rik5>
Group administrator
Fri 04 Dec 2020 05:45:39 PM UTC, comment #6: 


commentaire #5 :

> Can you add this code to the end of the mex C file?


I’ve done that and the problem remains the same.

Sébastien Villemot <svillemot>
Fri 04 Dec 2020 04:14:50 PM UTC, comment #5: 

Can you add this code to the end of the mex C file?


  int i;
  for (i = 0; i < 3; i++)
    mxDestroyArray (x[i]);


This frees the memory from the heap that was allocated internally by mexCallMATLAB.  I don't think it is necessary because it should be the case that when the mex file finishes execution Octave reclaims any memory associated with it.  But, if Octave is not doing that properly then it could cause issues.

Rik <rik5>
Group administrator
Fri 04 Dec 2020 08:59:58 AM UTC, comment #4: 


commentaire #3 :

> When you say that the code fails on the third attempt, does that mean you call mexCallMATLAB three times within a single mex file?  Or do you mean there is a mex file which calls mexCallMATLAB once, and you call the mex function three times from the Octave interpreter and it fails?


I mean the second option. The MEX is called several times, and it fails at the third call. Of course there are other computations done between calls.

> Does the m-file that you call from the mex file ever return less than three outputs under any circumstances?


No. I sent you the exact M-file that creates the problem.

Sébastien Villemot <svillemot>
Fri 04 Dec 2020 04:00:46 AM UTC, comment #3: 

When you say that the code fails on the third attempt, does that mean you call mexCallMATLAB three times within a single mex file?  Or do you mean there is a mex file which calls mexCallMATLAB once, and you call the mex function three times from the Octave interpreter and it fails?

Does the m-file that you call from the mex file ever return less than three outputs under any circumstances?


Rik <rik5>
Group administrator
Thu 03 Dec 2020 03:21:40 PM UTC, comment #2: 


commentaire #1 :

> mexCallMATLAB returns a status code.  Are you checking that code to see if an error happened during the function call?  Is that code correct in that when it is 0, the return values are okay and when it is non-zero, the return values are empty?


Yes, my code checks the return value of mexCallMATLAB. And when the problem manifests (return values empty), that return code is 0.

I attach the minimal test case that I had constructed. The M-file function is the same as in the context where the problem is encountered. The MEX file on the other hand is much simplified. As I told before, the problem does not manifest in this simplified test case, but that will give you a more precise idea of the problem.


(file #50388, file #50389)

Sébastien Villemot <svillemot>
Thu 03 Dec 2020 04:19:11 AM UTC, comment #1: 

mexCallMATLAB, if it encounters an error, should return to the Octave prompt.  Does your mex file do so or does it continue after the call to mexCallMATLAB but the output results are uninitialized?

Could you upload your minimal example, even if it doesn't fail, so we could get an idea of what you are trying to do?

mexCallMATLAB returns a status code.  Are you checking that code to see if an error happened during the function call?  Is that code correct in that when it is 0, the return values are okay and when it is non-zero, the return values are empty?


Rik <rik5>
Group administrator
Wed 02 Dec 2020 03:56:38 PM UTC, original submission:  

Hi,

I have spent several hours debugging a regression in Octave 6, without success so far.

The problem manifests with Dynare (see https://www.dynare.org), for which I’m upstream developer.

Essentially the scenario is the following one. We have a MEX file, which itself calls a given M-file function through mexCallMATLAB(). The first two runs of the MEX file go fine, but the third time, the return values of the function, as returned by mexCallMATLAB() are bogus: mxGetClassID() executed on them returns 0, same for mxGetM() and mxGetN().

I’ve used valgrind to see if I could spot any illegal memory access, but nothing of the like.

I’ve also tried to reduce the problem to a minimal test case, but without success. More precisely, I’ve created a MEX file that only calls the M-file, and then I executed that MEX file several times in a row, but this does not trigger the problem. This probably means that the problem comes from the interaction with other computations that are done by Dynare.

Note that the same code works fine under Octave 5.2.0 (and MATLAB), so this is clearly a regression.

I would be grateful if someone could give me advice on how to further debug this, because I’ve run out of ideas…

Sébastien Villemot <svillemot>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50430:  feval-argout-diff.txt added by jwe (2KiB - text/plain)
file #50413:  bug59597.diff added by rik5 (728B - text/x-patch)
file #50388:  bug59597.c added by svillemot (464B - text/x-csrc)
file #50389:  bug59597_fun.m added by svillemot (1KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by gyom (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by svillemot (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 group members can vote.

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-12-11 jwe StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2020-12-11 jwe StatusPatch Submitted Ready For Test
    2020-12-09 jwe Attached File- Added feval-argout-diff.txt, #50430
    2020-12-07 rik5 StatusNeed Info Patch Submitted
    2020-12-07 rik5 Attached File- Added bug59597.diff, #50413
    2020-12-03 svillemot Attached File- Added bug59597.c, #50388
        Attached File- Added bug59597_fun.m, #50389
    2020-12-03 rik5 StatusNone Need Info

    Back to the top

    Powered by Savane 3.13-3230.
    Corresponding source code