bugGNU Octave - Bugs: bug #29475, new incompatibility of MEX...

 
 

bug #29475: new incompatibility of MEX function in Octave 3.2.x

Submitter:  Xiangmin Jiao <xjiao>
Submitted:  Thu 08 Apr 2010 03:56:01 PM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  jwe
Originator Name:  Xiangmin Jiao Open/Closed:  * Closed
Release:  * 3.2.3 Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 09 Apr 2010 07:03:23 PM UTC, comment #9: 

It should be enough (at least currently I think it's true) to do something like
if (! issparse (x))
  x = full (x); # ensure there's no packed representation
endif
if (! isempty (x))
  x(1) = x(1); # force unsharing
endif

Jaroslav Hajek <highegg>
Fri 09 Apr 2010 06:23:31 PM UTC, comment #8: 

I see. Thanks for the warning. I will keep that in mind and make sure it does not happen in my library.

Xiangmin Jiao <xjiao>
Fri 09 Apr 2010 06:09:41 PM UTC, comment #7: 

OK, I checked in my change.

"Yes, I modify mxArray only for full matrices and arrays of types char, integer, and real numbers."

The point Jaroslav and I are trying to make is that you can still expect to have trouble with your assumptions if someone passes a range or a diagonal matrix or a permutation matrix instead of an actual full matrix object because ranges and diagonal matrices and permutation matrices are special data types in Octave that do not have the same representation as a full matrix, so it is not possible to directly access the data to them in the same way as in Matlab.  So Octave creates a full matrix from these special types first, then gives you a pointer to that data.  So in those cases, there is no way for you do to what you are trying to do (modify in place the argument to a MEX function).

John W. Eaton <jwe>
Group administrator
Fri 09 Apr 2010 05:44:08 PM UTC, comment #6: 

John's proposed change fixed it, and mexCGNS works again for Octave with this change. Thank you very much for your help and fixing it so quickly!

Yes, I modify mxArray only for full matrices and arrays of types char, integer, and real numbers. I do expect there would be some differences for any sparse matrix and for complex numbers.

Xiangmin Jiao <xjiao>
Fri 09 Apr 2010 12:41:45 PM UTC, comment #5: 

Yes, ranges and lazy indexes are other instances.
They may have the full() conversion cached, so you may happen to modify just the cache but not the original value, letting the djinn out of the bottle.
Diagonal matrices are particularly nasty because here even the copy-on-write trick may not save you:

a = eye(10); #1
a(1,1) = a(1,1); #2
my_cheating_mex_fcn (a);

the problem is that after statement #2, a is still diagonal (try it). With other "packed" internal types, an assignment is enough to unpack, but in this case you need to assign to an off-diagonal element to force unpacking.
So if you cheat and modify the input argument in your mex function, the change may or may not be visible outside (depending on what you do next).

You were warned, so now you can happily proceed :)

Jaroslav Hajek <highegg>
Fri 09 Apr 2010 12:23:27 PM UTC, comment #4: 

I couldn't remember why I made this change, but I'm fairly certain it was in response to some problem and not some arbitrary change.  So I started looking for some bug report or email exchange about it and found the following:

Subject: mxGetPr and diag or perm arrays
From: "John W. Eaton" <jwe@octave.org>
To: Jaroslav Hajek <highegg@gmail.com>
Cc: "John W. Eaton" <jwe@octave.org>
Date: Wed, 17 Jun 2009 21:14:38 -0400
Message-ID: <19001.38142.28366.476134@segfault.lan>

The following patch should also be applied to the 3.2.x branch.  It
fixes a problem with mxGetPr not being able to extract a pointer to
the data (in a Matlab compatible way) for diagonal or permutation
(and possibly other) matrices.

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

Thanks,

jwe


So I think the reason for the change was that the previous check

-    if (is_char ()
-       || (is_numeric () && is_real_type () && ! is_range ()))

no longer worked when diagonal and permutation matrices were introduced because they are numeric and real, but will not give the expected data result for mxGetPr, so that must be generated rather than simply returned as a pointer to internal Octave data.

You are right that before this change, we clearly allowed a pointer to internal data to be returned for character data, but I did not add a method in the character array class.  I've attached a proposed patch to "fix" the problem.  If you verify that it works for you, I will commit it.

However, I would like to echo Jaroslav's comments.  Things that work in Matlab are not guaranteed to work in Octave, and can likely fail in mysterious ways.  For example, it seems that
you would be able to use mxGetPr and modify the data of a matrix in Matlab and that would apparently work in Octave for full matrices, but fail for diagonal or permutation matrices.  So your program would appear to work correctly in Octave, but then fail if you pass eye(2) instead of [1,0;0;1].

John W. Eaton <jwe>
Group administrator
Fri 09 Apr 2010 08:09:51 AM UTC, comment #3: 

The change has been caused by John's changeset 9358:d4b1314a7c31,
that improved the decision procedure whether to make a copy.
John, this works for double and integer matrices; we should either make it work for chars as well, or remove it from everywhere.

Note that it is still a dangerous trick that should be reserved for internal use. The data sharing mechanisms in Octave are probably quite different from those of Matlab. In Matlab, apparently mxArray is a central structure; in Octave, it's just a wrapper, so implementing mxUnshare would be quite difficult because the wrapper would need to be provided with enough data to reach the original octave_value.
Octave also often makes shared copies of values for its internal purposes, like caching some conversions, Matlab probably doesn't.
Octave shares data in some situations where Matlab doesn't; for instance, contiguous subarrays or conversions between cells and cs-lists. You should bear these differences in mind.

In version 3.6 (or whatever major will be after 3.4), I intend to work on some optimizations that will allow oct-files exploit calls like

X = myfun (X, y)

and, if applicable, operate on X in-place.

In any case, it will probably always hold that using the native C++ interface is a better option if you want to do optimization tricks.


Jaroslav Hajek <highegg>
Fri 09 Apr 2010 05:47:31 AM UTC, comment #2: 

Thanks for your quick reply. This feature is not documented for end-users, but it is an undocumented feature that Mathworks and some MEX-function developers use. Here are a couple of pieces of evidence for it:

MATLAB has two undocumented (but widely used) functions mxIsSharedArray and mxUnshareArray for MEX functions. Because MATLAB is "copy on write", there functions are used to unshare an input mxArray argument before a C-MEX function modifies it. For example, MPITB uses it (for performance reasons). If you search the MEX binaries that come with MATLAB, some Mathworks' own functions also use them. There are also discussions about these on MATLAB Central. For example, the thread
http://www.mathworks.com/matlabcentral/newsreader/view_thread/163743

Note that I am not requesting adding the above two functions in Octave, because a MEX developer can ensure the array is unshared in the caller by using the property of "copy on write", and thankfully it works for both MATLAB and Octave. (I use it in mexCGNS for MATLAB and Octave http://code.google.com/p/mexcgns/).

However, there are a few compelling reasons to change the behavior of passing character string back to how it was in 3.0.x:

1. If Octave sticks to "copy on write" as in MATLAB, then there is no reason to make a copy of the character string in mxArray before passing to mexFunction (unless Octave has to make a copy for some reason).

2. It would make the code more efficient both in terms of memory and speed, and both within Octave and in user' mex function.

3. It delivers better compatibility between Octave and MATLAB, and would not break some mex functions that used to work with Octave 3.0.x (like mexCGNS).

Xiangmin Jiao <xjiao>
Thu 08 Apr 2010 04:50:18 PM UTC, comment #1: 

I don't think that's a feature I would like to copy.

Functions in Octave (and Matlab) are not supposed to be able to alter the values of their arguments in the calling scope.  I think that property should hold for all functions, regardless of how they are defined.

Where is it documented that it is OK for a MEX file to modify its arguments?

John W. Eaton <jwe>
Group administrator
Thu 08 Apr 2010 03:56:01 PM UTC, original submission:  

In Octave 3.2.x, if the MEX function changes an input mxArray containing a character string, the change will not be propagated back to the caller. Some MEX functions often use the trick of changing the input mxArray to pass an in-out variable to achieve efficiency. This kind of MEX functions behaved correctly in Octave 3.0.x (giving the same results as in MATLAB), but they behave incorrectly in 3.2.x (giving different results from MATLAB).

Since the main point of MEX functions if for comparability with MATLAB, a different behavior from MATLAB should be considered as a bug.

I suspect that Octave 3.2.x seems to have changed the behavior when passing a character string to MEX functions. It seems to make a local copy of a character string in an input mxArray before passing it to the MEX function. In Octave 3.0.x, an input character string behaved the same as other data types, i.e., it did not make a local copy.

Xiangmin Jiao <xjiao>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #20172:  diffs added by jwe (1KiB - 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 highegg (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by xjiao (Submitted the item)
  • -email is unavailable- added by xjiao
  •  

    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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2010-04-09 jwe StatusNeed Info Fixed
        Open/ClosedOpen Closed
    2010-04-09 jwe Attached File- Added diffs, #20172
    2010-04-09 jwe StatusNone Need Info
        Assigned toNone jwe
    2010-04-08 xjiao Carbon-Copy- Added -email is unavailable-

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code