bugGNU Octave - Bugs: bug #51725, Adding mxGetProperty to Matlab API

 
 

bug #51725: Adding mxGetProperty to Matlab API

Submitter:  Piotr Held <jsoh425>
Submitted:  Thu 10 Aug 2017 08:47:44 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Piotr Held Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 14 Aug 2017 11:21:36 PM UTC, comment #21: 

Alas, it does mess existing code up.  I'll file a new report.

Rik <rik5>
Group administrator
Mon 14 Aug 2017 08:31:19 PM UTC, comment #20: 
John W. Eaton <jwe>
Group administrator
Mon 14 Aug 2017 07:59:59 PM UTC, comment #19: 

I'll look at making the change.  Then I'd say open a new report about testing the MEX interface?  Low priority for me, but I guess it would be good to have tests if someone wants to write them.

John W. Eaton <jwe>
Group administrator
Mon 14 Aug 2017 07:56:54 PM UTC, comment #18: 

Yes.  NULL values should be converted to undefined, rather than to empty matrix.

After that change it might be necessary to check whether some of the MEX routines have been relying on that behavior to actually return an empty matrix.

Should this sequence of fixes continue on this bug report, or should I file a new one?

Rik <rik5>
Group administrator
Mon 14 Aug 2017 05:30:28 PM UTC, comment #17: 

So, we are actually not compatible here.  Instead, we should be converting NULL mxArray pointers to be undefined octave_values on return from MEX functions.  Correct?

John W. Eaton <jwe>
Group administrator
Mon 14 Aug 2017 03:50:50 PM UTC, comment #16: 

So I compiled nulltst.c And when I ran I got:


>> a = nulltst
plhs[0] initialized to 0
One or more output arguments not assigned during call to "nullstr".


And in John's case (you can't define a function inline like that :)), but a similar function defined in a file gave:


>> [a,b] = foo()
Error using foo
Too many output arguments


Piotr Held <jsoh425>
Sat 12 Aug 2017 05:19:41 PM UTC, comment #15: 

Sure I can do it on Monday and tell you what happens then.

Piotr Held <jsoh425>
Sat 12 Aug 2017 05:16:49 PM UTC, comment #14: 

@Piotr: Could you compile nulltst.c with mex in Matlab and see how it behaves?  If you are busy I can also ask on the Octave-Maintainers list for some help with this.

Rik <rik5>
Group administrator
Sat 12 Aug 2017 04:47:33 PM UTC, comment #13: 

I checked in the following changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/a490e729f2f2

Now the tests pass for me.  Sorry for the confusion.

As for the nulltst function, it seems weird to me that we should get empty matrices for values that are not even assigned to be anything.  Is that really how Matlab works?  That's not what happens for a function like this, for example:


octave:1> function r = foo () r = 1; end
octave:2> [a,b] = foo ()
a =  1
error: element number 2 undefined in return list


John W. Eaton <jwe>
Group administrator
Sat 12 Aug 2017 03:09:41 PM UTC, comment #12: 

I also did some tests with NULL pointers to mxArray to see how they should behave.  It would be very good if someone could test in Matlab to see if Octave is compatible.  The summary is that a NULL pointer seems to yield an empty 0x0 matrix.

nulltst.c:


#include "mex.h"

void mexFunction (int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[])
{
  if (nlhs < 1)
    mexErrMsgTxt ("Must have at least one output");

  mexPrintf ("plhs[0] initialized to %ld\n", plhs[0]);

  plhs[0] = NULL;
}


And when executed


octave:1> mex nulltst.c
octave:2> x = nulltst
plhs[0] initialized to 0
x = [](0x0)
octave:3> [x,y,z] = nulltst
plhs[0] initialized to 0
x = [](0x0)
y = [](0x0)
z = [](0x0)


So it seems that the plhs array of pointers is always initialized to 0 (NULL).  And that the interpretation of a NULL pointer to an mxArray is always an empty double matrix in Octave. 


Rik <rik5>
Group administrator
Sat 12 Aug 2017 03:03:23 PM UTC, comment #11: 

The problem seems to be cset 23893:75eff5b667b3 (new mxSetProperty function for MEX API).  The function mxGetProperty no longer returns NULL for a non-matching property name.

I modified mexprop.c to


#include "mex.h"

/*
Octave syntax: val = mexprop (Map_obj, "property_name")
Fetches property "property_name" from containers.Map object.
*/

void mexFunction (int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[])
{
  if (nrhs != 2)
    mexErrMsgTxt ("mexprop: syntax is mexprop (Map_obj, property_name)");

  /* Check for Map object. */
  if (! mxIsClass (prhs[0], "containers.Map"))
    mexErrMsgTxt ("mexprop: first argument must be a containers.map object");

  if (! mxIsChar (prhs[1]))
    mexErrMsgTxt ("mexprop: second argument must be a property name");

  mxArray *retval = mxGetProperty (prhs[0], 0, mxArrayToString (prhs[1]));
  mexPrintf ("retval = %ld\n", retval);

  plhs[0] = retval;
}


And after compiling and running tst_mexprop I get


octave:1> mex mexprop.c
octave:2> tst_mexprop
retval = 26828816
retval = 27562656
retval = 27574032
error: assert (mexprop (x, "FooBar"),[]) failed
error: called from
    assert at line 92 column 11
    tst_mexprop at line 6 column 1


With cset 23892:ba46a8015b26 just before I get


octave:4> tst_mexprop
retval = 20596784
retval = 21330992
retval = 0


I'm attaching the new mexprop.c to the bug report.  I also made some tests of null pointers for the MEX API that I will post.


(file #41493)

Rik <rik5>
Group administrator
Sat 12 Aug 2017 12:34:04 AM UTC, comment #10: 

Rik: This is what I see:


octave:1> x = containers.Map;
octave:2> mexprop (x, "FooBar")
octave:3> assert (mexprop (x, "FooBar"), [])
error: assert (mexprop (x, "FooBar"),[]) failed
error: called from
    assert at line 92 column 11
octave:3> x = mexprop (x, "FooBar")
error: value on right hand side of assignment is undefined


so it looks to me like converting a NULL mxArray pointer to an octave_value object creates an undefined octave_value, not an empty Matrix.

What does Matlab do if plhs[0] is NULL?

John W. Eaton <jwe>
Group administrator
Fri 11 Aug 2017 11:14:26 PM UTC, comment #9: 

@Piotr: Even though Matlab happily lets the code segfault, we could decide to be a bit better than that and implement some checks.  However, given that time is limited I would rather spend time on Octave than the MEX API which isn't used too frequently.  For that reason, rather than correctness, I think we can let this be.

@jwe: I suppose you could introduce an intermediate variable and then check for NULL and issue an error.  For my purposes, NOT_FOUND = NULL = [] so I just checked for [].

I don't absolutely know that it is safe, but I believe the translation back from *mxArray to octave_value involves calling the octave_value constructor.  If the input is NULL this is translated to an empty matrix of double type.

Rik <rik5>
Group administrator
Fri 11 Aug 2017 10:54:47 PM UTC, comment #8: 

Somehow I thought it did check if the pointers made sense, but I just verified on R2017a that it doesn't. It just crashes :). So I totally agree that no checks are necessary.

Piotr Held <jsoh425>
Fri 11 Aug 2017 10:07:32 PM UTC, comment #7: 

Rik: I'm surprised that your test


assert (mexprop (x, "FooBar"), [])


worked since mxGetProperty returns NULL for unknown property names.  Shouldn't your mexproto function check for that and explicitly assign an empty matrix or throw an error in that case?

John W. Eaton <jwe>
Group administrator
Fri 11 Aug 2017 09:39:01 PM UTC, comment #6: 

AFAIK, Matlab just crashes if you send an invalid mxArray pointer to MEX functions, so we are just being compatible.  ;-)  If that's changed, then sure, then I suppose we could add checks too.

We don't really have any systematic tests for the MEX API yet.  Before we start adding random tests for MEX functions, I think we should decide how these tests should work more generally since they will require some functions to be compiled separately from Octave.  It's easy to test Octave's internal functions using Octave language code because all the Octave functions are present in Octave.  But the MEX interface is not used anywhere in Octave itself, so that means writing some compiled code to do the testing.  It's a bit more work than adding new %!test blocks to mex.cc.

John W. Eaton <jwe>
Group administrator
Fri 11 Aug 2017 09:31:22 PM UTC, comment #5: 

Question 1 :

Yes, it would probably be a good idea to check that we are not de-referencing a nullptr.  On the other hand, I don't think Matlab's interface has been very good about input validation.  The whole idea behind leaving the the Matlab language for C or Fortran was too have maximum performance, and if you crashed the program it was your own fault.

Given that the usage pattern is
1) Translate Matlab data to C data
2) Perform complex algorithm
3) Translate C data back to Matlab data

it seems that the mxDoSomething functions are only called twice per usage and so adding a test for nullptr wouldn't slow things down much.

If you have access to Matlab it would be interesting to test and see if they happily let you segfault.

Question 2:

True QA testing would test both the basic function, and then corner cases and illegal input, etc.  I just wanted to verify that I could actually fetch a property value as the function is supposed to.

Rik <rik5>
Group administrator
Fri 11 Aug 2017 08:54:44 PM UTC, comment #4: 

I have 2 more questions to this:

1. Shouldn't all the mxDoSomething (mxArray *ptr) check if the ptr is not nullptr if we are calling a mxArray method?

2. Shouldn't you try to throw garbage at mxGetProperty and write some tests like that as well?
(eg. give it a nullptr, or give it a struct as input)

Piotr Held <jsoh425>
Fri 11 Aug 2017 06:42:33 PM UTC, comment #3: 

I changed some names to avoid CamelCase (http://hg.savannah.gnu.org/hgweb/octave/rev/142a9c7e403a).  Marking as fixed and closing report.

For completeness, I will open a bug report for mexSetProperty.  It seems like that shouldn't be too hard to implement now.

Rik <rik5>
Group administrator
Fri 11 Aug 2017 06:23:43 PM UTC, comment #2: 

I tested the new API function with the attached mexprop.c and tst_mexprop.m using a container.Map object.  It seems to work nicely.

mexprop.c:


#include "mex.h"

/*
Octave syntax: val = mexprop (Map_obj, "property_name")
Fetches property "property_name" from containers.Map object.
*/

void mexFunction (int nlhs, mxArray *plhs[], int nrhs, const mxArray *prhs[])
{
  if (nrhs != 2)
    mexErrMsgTxt ("mexprop: syntax is mexprop (Map_obj, property_name)");

  /* Check for Map object. */
  if (! mxIsClass (prhs[0], "containers.Map"))
    mexErrMsgTxt ("mexprop: first argument must be a containers.map object");

  if (! mxIsChar (prhs[1]))
    mexErrMsgTxt ("mexprop: second argument must be a property name");

  plhs[0] = mxGetProperty (prhs[0], 0, mxArrayToString (prhs[1]));

}


tst_mexprop.m


# Needed once, to compile file
# mex mexprop.c
x = containers.Map;
assert (mexprop (x, "KeyType"), "char")
assert (mexprop (x, "ValueType"), "any")
assert (mexprop (x, "FooBar"), [])




(file #41487, file #41488)

Rik <rik5>
Group administrator
Fri 11 Aug 2017 03:25:57 PM UTC, comment #1: 

Thanks for the new function.  I checked it in here with some small edits.

Instead of dynamic cast, I added a new function to the octave_value class to access classdef object values.

I stripped some trailing whitespace from the sources and made some other minor style fixes.

http://hg.savannah.gnu.org/hgweb/octave/rev/bd9e719f04cc

John W. Eaton <jwe>
Group administrator
Thu 10 Aug 2017 08:47:44 PM UTC, original submission:  

Hi,

I wrote an implementation of mxGetProperty and am attaching it to this post.

This implementation does not throw errors when calling mxGetProperty on something other than a class. It only returns a nullptr in that case. This behavior is compatible with the Matlab API documentation.

Piotr Held <jsoh425>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #41494:  nulltst.c added by rik5 (243B - text/x-csrc)
file #41493:  mexprop.c added by rik5 (708B - text/x-csrc)
file #41487:  mexprop.c added by rik5 (642B - text/x-csrc)
file #41488:  tst_mexprop.m added by rik5 (183B - text/x-matlab)
file #41476:  mxGetProperty.diff added by jsoh425 (4KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by jsoh425 (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-12 rik5 Attached File- Added nulltst.c, #41494
    2017-08-12 rik5 Attached File- Added mexprop.c, #41493
    2017-08-11 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2017-08-11 rik5 Attached File- Added mexprop.c, #41487
        Attached File- Added tst_mexprop.m, #41488
    2017-08-11 jwe StatusNone Ready For Test
    2017-08-10 jsoh425 Attached File- Added mxGetProperty.diff, #41476

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code