bugGNU Octave - Bugs: bug #51708, multiple --eval options results in...

 
 

bug #51708: multiple --eval options results in corrupted parsing of command line

Submitter:  Mike Miller <mtmiller>
Submitted:  Wed 09 Aug 2017 07:41:25 PM UTC
   
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * GNU/Linux
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Thu 10 Aug 2017 01:07:03 AM UTC, comment #10: 

I tediously reviewed the cset and I believe that the issue identified here of mixing char with char * occurred only once.  I fixed it here http://hg.savannah.gnu.org/hgweb/octave/rev/4d4ba038d103.

For the testing, the advantage of staying within Octave is that the tests will be counted in the summary tallies at the end of fntests.log.  A disadvantage may be trying to determine whether run-octave or regular octave should be used in the system command.

Rik <rik5>
Group administrator
Wed 09 Aug 2017 11:28:16 PM UTC, comment #9: 

Here's an example of a simple test that just runs "run-octave --version" and expects a success exit status.

If any test script fails, make check fails. The only weird bit is that it runs concurrently with the existing fntests.m test suite, but I'm sure we can work out a way to serialize them.

(file #41467)

Mike Miller <mtmiller>
Group Member
Wed 09 Aug 2017 11:16:58 PM UTC, comment #8: 

Automake includes basic support for running series of test scripts or programs and collecting the exit statuses, that's the test framework I've used mostly with autotools in the past.

Autoconf includes Autotest, but that's just another m4-based macro language like Autoconf for building portable test scripts, seems like overkill for what we need.

Dejagnu seems to still be around, I've never used it.

I think most of the more fully-featured testing frameworks are going to be huge libraries built on perl, python, ruby, node, etc, and we probably don't want to add that bloat just to run some command-line permutations of the octave executable. I would lean towards just using Automake's built-in mechanism for running scripts.

Mike Miller <mtmiller>
Group Member
Wed 09 Aug 2017 10:59:46 PM UTC, comment #7: 

Kind of clunky, but if we are only doing a few tests then we could use system() in an m-file.  I just tried it and works.


octave:1> [a,b] = system ('./run-octave -f --eval "pi"')
a = 0
b = ans =  3.1416

octave:2> [a,b] = system ('./run-octave -f --eval "pi" --eval "+ 1"')
error: 'pi1' undefined near line 1 column 1
a =  1
b =



Rik <rik5>
Group administrator
Wed 09 Aug 2017 10:54:20 PM UTC, comment #6: 

Tests for invoking Octave would be useful, but that will require a completely different testing mechanism.  What modern testing framework could we use?  Octave originally used dejagnu.  I'd rather not go back to that if we can avoid it.

John W. Eaton <jwe>
Group administrator
Wed 09 Aug 2017 09:48:43 PM UTC, comment #5: 

I bisected independently and came to 336f89b6208b as the culprit as well.

Mike Miller <mtmiller>
Group Member
Wed 09 Aug 2017 09:26:41 PM UTC, comment #4: 

I checked 23829 completely and it is fine (Eliminate unnecessary std::string ("...") constructor calls when "..." suffices.).  The problem cset is very large and will take some time.

Rik <rik5>
Group administrator
Wed 09 Aug 2017 09:19:16 PM UTC, comment #3: 

Yeah I was thinking we could add some facility to the test directory to run octave in various ways to test command line options, etc.

Mike Miller <mtmiller>
Group Member
Wed 09 Aug 2017 09:07:26 PM UTC, comment #2: 

Grrr.  I made the change from " " to ' ' to be more specific and because there were certain cases, such as appending with +=, where it was faster.  And then I made a second set of changes for readability where I removed the std::string () qualifying the bare string literal because it was unnecessary.

Again, this points out that we don't have a full set of regression tests because I ran 'make check' and it passed.

Can we add some invocation tests to the tests/ directory or is it really only running m-files and .tst files?  For this case,


run-octave --eval "1" --eval "+ 1"


should return 2.

Rik <rik5>
Group administrator
Wed 09 Aug 2017 08:43:27 PM UTC, comment #1: 

It was this changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/336f89b6208b


diff --git a/libinterp/octave.cc b/libinterp/octave.cc
--- a/libinterp/octave.cc
+++ b/libinterp/octave.cc
@@ -144,7 +144,7 @@ namespace octave
                 if (m_code_to_eval.empty ())
                   m_code_to_eval = octave_optarg_wrapper ();
                 else
-                  m_code_to_eval += std::string (" ") + octave_optarg_wrapper ();
+                  m_code_to_eval += ' ' + octave_optarg_wrapper ();
               }
             break;


octave_optarg_wrapper returns char*, not std::string, so adding that to ' ' is trouble.

I haven't looked to see whether there are other places where similar problems may have been introduced.

Adding Rik to the notification list.

John W. Eaton <jwe>
Group administrator
Wed 09 Aug 2017 07:41:25 PM UTC, original submission:  

Multiple --eval options should be combined into a single command string to be evaluated in sequence. This worked in all versions up to the current stable branch, but looks like it's regressed on the default branch.

Stable:


$ ./run-octave -cli --eval="1+" --eval=1
ans =  2


Default:


$ ./run-octave -cli --eval="1+" --eval=1
parse error:

  invalid left hand side of assignment

>>> 1+E=1
       ^


Depending on the contents of the --eval options, a different string is passed to the parser. It looks like it's coming from the environment. I think the environ array normally follows the argv array in memory.

Mike Miller <mtmiller>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #41467:  basic-test.diff added by mtmiller (1KiB - 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 (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by jwe
  • -email is unavailable- added by mtmiller (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 6 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-08-10 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
    2017-08-10 rik5 StatusConfirmed Ready For Test
    2017-08-09 mtmiller Attached File- Added basic-test.diff, #41467
    2017-08-09 rik5 StatusNone Confirmed
    2017-08-09 jwe Carbon-Copy- Added rik

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code