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

 
 

bug #51725: Adding mxGetProperty to Matlab API

Submitted by:  Piotr Held <jsoh425>
Submitted on:  Thu 10 Aug 2017 08:47:44 PM UTC  
 
Category: InterpreterSeverity: 3 - Normal
Priority: 5 - NormalItem Group: Feature Request
Status: FixedAssigned to: None
Originator Name: Piotr HeldOpen/Closed: Closed
Release: devOperating 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 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>
Project Administrator
Mon 14 Aug 2017 08:31:19 PM UTC, comment #20:

I pushed this change:

http://hg.savannah.gnu.org/hgweb/octave/rev/2e4033e1f28f

John W. Eaton <jwe>
Project 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>
Project 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>
Project 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>
Project Administrator
Mon 14 Aug 2017 03:50:50 PM UTC, comment #16:

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

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

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>
Project 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:

John W. Eaton <jwe>
Project 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:

And when executed

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>
Project 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

And after compiling and running tst_mexprop I get

With cset 23892:ba46a8015b26 just before I get

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>
Project Administrator
Sat 12 Aug 2017 12:34:04 AM UTC, comment #10:

Rik: This is what I see:

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>
Project 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>
Project 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

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>
Project 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>
Project 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>
Project 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>
Project 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:

tst_mexprop.m

(file #41487, file #41488)

Rik <rik5>
Project 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>
Project 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 File(s):
   
   
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
  • -unavailable- added by rik5 (Updated the item)
  • -unavailable- added by jwe (Posted a comment)
  • -unavailable- added by jsoh425 (Submitted the item)
  •  

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

    Only project members can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 8 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 12 Aug 2017 03:10:11 PM UTCrik5Attached File-=>Added nulltst.c, #41494
    Sat 12 Aug 2017 03:03:23 PM UTCrik5Attached File-=>Added mexprop.c, #41493
    Fri 11 Aug 2017 06:42:33 PM UTCrik5StatusReady For Test=>Fixed
      Open/ClosedOpen=>Closed
    Fri 11 Aug 2017 06:23:43 PM UTCrik5Attached File-=>Added mexprop.c, #41487
      Attached File-=>Added tst_mexprop.m, #41488
    Fri 11 Aug 2017 03:25:57 PM UTCjweStatusNone=>Ready For Test
    Thu 10 Aug 2017 08:47:44 PM UTCjsoh425Attached File-=>Added mxGetProperty.diff, #41476

    Back to the top


    Powered by Savane 3.1-cleanup1