bugGNU Octave - Bugs: bug #49536, many functions that expect a...

 
 

bug #49536: many functions that expect a "string" silently truncate char matrix arguments (octave_value::string_value)

Submitter:  Mike Miller <mtmiller>
Submitted:  Sat 05 Nov 2016 12:53:07 AM UTC
   
 
Category:  Libraries Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Other
Status:  Fixed Assigned to:  None
Originator Name:  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 Sep 2020 03:04:30 AM UTC, comment #11: 

I followed Mike's direction and made a changeset which throws a new warning ID, "Octave:charmat-truncated", when string_value() is called on a multi-row character matrix.  I also added a BIST test for it and documented the new ID in warning_ids.m.  See this cset: http://hg.savannah.gnu.org/hgweb/octave/rev/f9201e7cbe00.

Marking as fixed and closing report.

Rik <rik5>
Group administrator
Fri 20 Mar 2020 05:48:23 PM UTC, comment #10: 

seeing this bug report mentioned in bug #57867, is mike's suggestion still compatible and worth pushing?

Nicholas Jankowski <nrjank>
Group Member
Mon 28 Nov 2016 10:21:46 PM UTC, comment #9: 

I think the fix in comment #8 is worthwhile.  @Mike: Do you want to go ahead and push it?

Rik <rik5>
Group administrator
Wed 23 Nov 2016 08:32:21 PM UTC, comment #8: 

The warning comes from the following backtrace


#0  octave_char_matrix_str::string_value[abi:cxx11](bool) const (this=<optimized out>) at ../libinterp/octave-value/ov-str-mat.cc:238
#1  0x00007fe5b75c66bf in octave_char_matrix_str::short_disp (this=<optimized out>, os=...) at ../libinterp/octave-value/ov-str-mat.cc:274
#2  0x00007fe5b7b4f25b in octave_value::short_disp (this=0x7fe57cfed9b0, os=...) at ../libinterp/octave-value/ov.h:1228
#3  symbol_table::do_workspace_info[abi:cxx11]() const (this=0x7fe56019de80) at ../libinterp/corefcn/symtab.cc:1507
#4  0x00007fe5b7abfbd9 in symbol_table::workspace_info[abi:cxx11](int) (scope=1) at ../libinterp/corefcn/symtab.h:2095
#5  octave_link::set_workspace () at ../libinterp/corefcn/octave-link.cc:71


I am now using this change


diff --git a/libinterp/octave-value/ov-str-mat.cc b/libinterp/octave-value/ov-str-mat.cc
--- a/libinterp/octave-value/ov-str-mat.cc
+++ b/libinterp/octave-value/ov-str-mat.cc
@@ -234,6 +234,9 @@ octave_char_matrix_str::string_value (bo

   charMatrix chm (matrix);

+  if (chm.rows () > 1)
+    warning ("evaluating a character matrix as a string, only the first row is used");
+
   // FIXME: Is this correct?
   return chm.row_as_string (0);
 }
@@ -268,7 +271,8 @@ octave_char_matrix_str::short_disp (std:
 {
   if (matrix.ndims () == 2 && numel () > 0)
     {
-      std::string tmp = string_value ();
+      charMatrix chm (matrix);
+      std::string tmp = chm.row_as_string (0);

       // FIXME: should this be configurable?
       size_t max_len = 100;


and can now work with the GUI and run the full test suite in the GUI with no warnings.

Needs some polish and a warning ID, but I think this is a good first start to draw a line.

Mike Miller <mtmiller>
Group Member
Wed 23 Nov 2016 08:12:12 PM UTC, comment #7: 

I made the following change in my local build


diff --git a/libinterp/octave-value/ov-str-mat.cc b/libinterp/octave-value/ov-str-mat.cc
--- a/libinterp/octave-value/ov-str-mat.cc
+++ b/libinterp/octave-value/ov-str-mat.cc
@@ -234,6 +234,9 @@ octave_char_matrix_str::string_value (bo

   charMatrix chm (matrix);

+  if (chm.rows () > 1)
+    warning ("evaluating a character matrix as a string, only the first row is used");
+
   // FIXME: Is this correct?
   return chm.row_as_string (0);
 }


The good news: I can run `make check RUN_OCTAVE_OPTIONS=-cli` and I get no new test failures or warnings printed on the console or in the log file.

The bad news: I can't run `make check` or run the test suite inside of the GUI. The test suite errors out completely when it gets to libinterp/corefcn/error.cc. The octave_qt_link appears to be doing something whenever a value is in the workspace that always evaluates a char matrix as a string. I haven't tracked that down yet. Even something as simple as `x = ["one"; "two"];` starts printing the warning message after every prompt until the value is cleared from the workspace.

Mike Miller <mtmiller>
Group Member
Tue 22 Nov 2016 02:55:34 PM UTC, comment #6: 

As a start, I agree with Colin because it at least doesn't silently discard data.  Longer term, I think we should identify those functions that are different from Matlab and consider individually whether they should be fixed for compatibility.

John W. Eaton <jwe>
Group administrator
Tue 22 Nov 2016 06:34:33 AM UTC, comment #5: 

I was surprised by this so I think its significant.

I'd vote for 2 (warning with current behaviour).  Seems like path of least resistance.

Colin Macdonald <cbm>
Mon 07 Nov 2016 08:02:28 PM UTC, comment #4: 

Thanks for the testing Ceral.

Philip ­- it could be argued that Octave behaves slightly worse here, because it is silently throwing away data. At least with the few examples I've shown here, Matlab either gives the entire array (yes, it is flattened in column major order, but it's complete), or throws an error saying that the argument must be a vector.

I would propose that Octave could do one of three things to make this better

1. Always return the entire array, squeezed into a vector following the normal column-major ordering.

2. Issue a warning while doing what it does today.

3. Throw an error when code tries to extract a string_value from an argument that has more than 1 non-singleton dimension.

Any of these changes can be made in just one place, in the octave_char_matrix_str::string_value method, and affect any function that tries to get a simple string from one of its arguments.

Mike Miller <mtmiller>
Group Member
Sat 05 Nov 2016 12:32:14 PM UTC, comment #3: 

Is this really a problem?
AFAICS char matrices aren't often used. cellstr arrays are much more practical.

Matlab has the same issue and even behaves slightly worse (which in a way is reassuring :-) )

Could transposing a char array before feeding it to string functions help?
For sprintf with-a-format-string, help and strrep it works to the extent that a 1xN vector is returned but in the right "order"; the other functions you mention are still oblivious.

Philip Nienhuis <philipnienhuis>
Group Member
Sat 05 Nov 2016 09:19:06 AM UTC, comment #2: 

Matlab:

% added spaces to make strings the same length

>> s = [' this '; ' is a '; ' char '; 'matrix']


s =

 this
 is a
 char
matrix

>> sprintf (s)


ans =

   mticahshti arsari   x
 

>> sprintf ('%s', s)


ans =

   mticahshti arsari   x

>> error (s)

   mticahshti arsari   x
 

>>  help (s)

Warning: The following error was caught while executing 'helpUtils.helpProcess' class destructor:
Input must be a row vector of characters.

> In help (line 45)
>> struct (s, 0)

Error using struct
Field name must be a string vector.
 

>> system (s);

Error using system
Argument must contain a string.
 

>> strrep (s, 'a', 'b')

Error using strrep
Input strings must have one row.

Ceral Paquet <octavebugs>
Sat 05 Nov 2016 09:13:33 AM UTC, comment #1: 

Matlab:


>> s = ["this"; "is a"; "char"; "matrix"]

 s = ["this"; "is a"; "char"; "matrix"]
      ↑
Error: The input character is not valid in MATLAB statements or expressions.
 

>> s = ['this'; 'is a'; 'char'; 'matrix']

Dimensions of matrices being concatenated are not consistent.
 

Ceral Paquet <octavebugs>
Sat 05 Nov 2016 12:53:07 AM UTC, original submission:  

Octave has many functions that operate on a char array with the concept of a "string", even though char arrays may be two- or higher-dimensional. Octave currently silently truncates char arrays when given to some functions that are expecting a string. Other functions flatten the char array into a vector (in column-major order). Some motivating examples:


>> s = ["this"; "is a"; "char"; "matrix"]
s =

this
is a
char
matrix

>> sprintf (s)
ans = this
>> sprintf ("%s", s)
ans = ticmhshai atsarr   i   x
>> error (s)
error: this
>> help (s)
error: help: 'ticmhshai atsarr   i   x' not found
>> struct (s, 0)
ans =

  scalar structure containing the fields:

    this   = 0

>> system (s);
sh: 1: this: not found
>> strrep (s, 'a', 'b')
ans = ticmhshbi btsbrr   i   x


The guilty function is octave_value::string_value. The code is labeled with a FIXME where it deliberately pulls out the first row of a 2-dimensional char array, so that seems like a prime candidate for discussion and resolution.

The list of functions affected is probably very long, but maybe we can gather together a short list and get a Matlab compatibility check as a starting point.

Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Carbon-Copy List
  • -email is unavailable- added by nrjank (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by cbm (Posted a comment)
  • -email is unavailable- added by octavebugs (Posted a comment)
  •  

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

    Date Changed by Updated Field Previous Value => Replaced by
    2020-09-14 rik5 StatusConfirmed Fixed
        Open/ClosedOpen Closed
    2020-06-19 rik5 Dependencies- bugs #58617 is dependent
    2019-02-26 mtmiller Carbon-CopyRemoved 80942 -
    2016-11-28 rik5 Dependencies- bugs #49696 is dependent
    2016-11-28 rik5 StatusPostponed Confirmed
    2016-11-05 mtmiller Summarymany functions that expect a &quot;string&quot; silently truncate char matrix arguments (octave_value::string_vector) many functions that expect a "string" silently truncate char matrix arguments (octave_value::string_value)

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code