bugGNU Octave - Bugs: bug #58686, Interpreter silently ignores inputs

 
 

bug #58686: Interpreter silently ignores inputs

Submitted by:  Kai Torben Ohlhus <siko1056>
Submitted on:  Wed 01 Jul 2020 06:09:05 AM UTC  
 
Category:  Interpreter Severity:  4 - Important
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  6.0.90 Operating 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)

Thu 09 Jul 2020 03:07:09 AM UTC, comment #15: 

jwe, again thanks for this great fix!  The message is perfect for me.  It contains a reason, and a position where to fix it!

>> __octave_config_info__.hg_id
ans = dc53b7b7af8b

>> test do_nothing
***** assert (do_nothing (3), rand (3))
!!!!! test failed
'retval' undefined near line 1, column 1

>> assert (do_nothing (3), rand (3))
error: 'retval' undefined near line 1, column 1
error: called from
    do_nothing at line 3 column 4

>> [2,do_nothing(),5,3]
error: 'retval' undefined near line 1, column 1
error: called from
    do_nothing at line 3 column 4

Consider this item as fixed, closing report.

Kai Torben Ohlhus <siko1056>
Project Member
Wed 08 Jul 2020 01:32:55 AM UTC, comment #14: 

I pushed the following changeset:

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

We could probably improve the error messages in the future, but at least now I think the behavior is correct.

John W. Eaton <jwe>
Project Administrator
Sat 04 Jul 2020 12:37:21 PM UTC, comment #13: 

I reverted my change (comment #11) and I am back at the start looking for a smarter change regarding the struct issue (comment #8, comment #9) which raised bug #58698 (pkg).

https://hg.savannah.gnu.org/hgweb/octave/rev/171a2857d6d1

Kai Torben Ohlhus <siko1056>
Project Member
Sat 04 Jul 2020 06:16:34 AM UTC, comment #12: 

@siko1056, I checked that bug #58695 has not been caused by fixing this one: I updated my hg working copy to revision 28524:455fe4a6f22c, which does not contain the fix, then executed make and ran Octave. I got

octave:3> test do_nothing
PASSES 1 out of 1 test
octave:4> [2,do_nothing(),5,3]
ans = [](0x0)

So, we have both this bug and bug #58695.

Fernando <tutissanalio>
Fri 03 Jul 2020 05:12:20 PM UTC, comment #11: 

Agree, I am also not happy about bug #58695 raised by fixing this one.  If nobody is faster, I'll undo the changes only for

libinterp/parse-tree/pt-eval.cc

https://hg.savannah.gnu.org/hgweb/octave/rev/286fe9352cd6

One fixed problem is not worth two new ones 😞

Kai Torben Ohlhus <siko1056>
Project Member
Fri 03 Jul 2020 05:08:01 PM UTC, comment #10: 

It took a while for me to see comment #4. Sorry for that.

So, it looks like something was wrong in Octave after all. But the fix should probably be different.

Markus Mützel <mmuetzel>
Project Member
Fri 03 Jul 2020 05:01:56 PM UTC, comment #9: 

I'm not sure if anything needed to be fixed in the parser here.
In Matlab R2020a, I see:

>> a = struct('b', {});
>> isempty(a.b,[])
ans =
  logical
   1
>> isempty(a.b,1)
ans =
  logical
   0
>> isempty(a.b)
Error using isempty
Not enough input arguments.

This is what Octave did before the change.

The unexpected behavior in the test suite should probably be fixed there instead.

Markus Mützel <mmuetzel>
Project Member
Fri 03 Jul 2020 04:35:57 PM UTC, comment #8: 

This change probably broke some tests on the no-extras buildbots.
The first one is:

>>>>> processing /scratch/buildbot/workers/jwe-debian-x86_64-2/stable-no-extras-debian/src/test/publish/publish.tst
***** test
visibility = get (0, "defaultfigurevisible");
toolkit = graphics_toolkit ();
unwind_protect
   if (! __have_feature__ ("QT_OFFSCREEN")
       || ! strcmp (graphics_toolkit (), "qt"))
     try
       graphics_toolkit ("gnuplot");
     catch
       ## The system doesn't support gnuplot for drawing hidden
       ## figures.  Just return and have test marked as passing.
       return;
     end_try_catch
   endif
   set (0, "defaultfigurevisible", "off");
   scripts = dir ("test_script*.m");
   tmpDir = tempname ();
   mkdir (tmpDir);
   opts.outputDir = tmpDir;
   for fname = {scripts.name}
     publish (fname{1}, opts);
   endfor
   confirm_recursive_rmdir (false, "local");
   rmdir (tmpDir, "s");
unwind_protect_cleanup
   set (0, "defaultfigurevisible", visibility);
   graphics_toolkit (toolkit);
end_unwind_protect
!!!!! test failed
Element 1 in argument list is not defined

Easier steps to reproduce the same error are:

a = struct ("b", {});
{a.b}

The same works in Matlab R2020a without error:

>> a = struct('b', {})
a =
  0×0 empty struct array with fields:
    b
>> {a.b}
ans =
  0×0 empty cell array

Markus Mützel <mmuetzel>
Project Member
Fri 03 Jul 2020 08:01:38 AM UTC, comment #7: 

@jwe comment #6, I did not find a report for this problem.  Thus I opened bug #58691 and marked the xtest in cset https://hg.savannah.gnu.org/hgweb/octave/rev/7d601759c23d

Kai Torben Ohlhus <siko1056>
Project Member
Fri 03 Jul 2020 02:35:21 AM UTC, comment #6: 

Thanks for fixing this problem.

Is there a bug report for the test that was changed to xtest?

John W. Eaton <jwe>
Project Administrator
Thu 02 Jul 2020 06:17:12 AM UTC, comment #5: 

I dug a little deeper in Octave's parser and found a very suitable place to fix this issue.

Fixed on stable https://hg.savannah.gnu.org/hgweb/octave/rev/286fe9352cd6

The message for comment #0 is

>> test do_nothing
***** assert (do_nothing (3), rand (3))
!!!!! test failed
Element 1 in argument list is not defined

and the fix also revealed another overseen bug which I marked as xtest.

Also the assert comparison of structures had to be refactored, as the recursion with unassigned values was somehow spooky and now errored reliably.

Marking as fixed, closing report.

Kai Torben Ohlhus <siko1056>
Project Member
Thu 02 Jul 2020 02:17:28 AM UTC, comment #4: 

Thank you for looking into this Rik 🙂

Matlab R2020b gives an error for calling the function in comment #0.

>> assert (do_nothing (4), rand (3))
Output argument "retval" (and maybe others) not assigned
during call to "do_nothing".

The proposed suggestion of comment #0 can only detect that some octave_value_list (ovl) element was not properly assigned, because we are already inside the called function.

The desired solution is, that during the creation of that ovl an error with nice message is thrown.

Workarounds with empty elements, I suspect to be the root of new inconsistencies.

Kai Torben Ohlhus <siko1056>
Project Member
Wed 01 Jul 2020 03:21:35 PM UTC, comment #3: 

Is there a simple way to test what Matlab does here?  It may be that their default value is an empty matrix ([]) of class double.  Or it may be that they error out because a value was expected and none was provided.

Rik <rik5>
Project Administrator
Wed 01 Jul 2020 03:19:56 PM UTC, comment #2: 

Just for reference, Octave does detect the situation if there is a return value.  For example, with Octave-3.2.4

octave-3.2.4:2> x = do_nothing(3)
warning: do_nothing: some elements in list of return values are undefined
x = [](0x0)

And with the development version of Octave,

octave:2> x = do_nothing (3)
error: value on right hand side of assignment is undefined

But you are right that octave_value_lists are being shortened, and this is both unexpected and probably wrong.

I used the debugger to set a breakpoint in assert.m to get an idea about what the interpreter is seeing.

The code is

function assert (cond, varargin)

When I run 'test do_nothing', I find that nargin = 1 and cond = rand (3).  The result of do_nothing(3) has been entirely removed.

Rik <rik5>
Project Administrator
Wed 01 Jul 2020 06:13:15 AM UTC, comment #1: 

Just for the files, I added the detection in

libinterp/parse-tree/pt-eval.cc (tree_evaluator::execute_user_function)

https://hg.savannah.gnu.org/hgweb/octave/file/49f4d7814760/libinterp/parse-tree/pt-eval.cc#l2725

Kai Torben Ohlhus <siko1056>
Project Member
Wed 01 Jul 2020 06:09:05 AM UTC, original submission:  

Today I discovered Octave is ignoring an input and does not error or notify me about it.  This applies for Octave 5.2.0, 6, and 7

I created a function and wrote an assert test for checking if it is working.

Unfortunately, I made a mistake and did not assign the result of my heavy computation to "retval" properly.

function retval = do_nothing (n)

endfunction

%!assert (do_nothing (3), rand (3))

The worst is:  The test passed!?

>> test do_nothing
PASSES 1 out of 1 test

I analysed the problem and found, that the "octave_value_list" given to "assert" has only one element.  This might be related when concatenating "octave_values", and one is empty.

The following patch adds an ad-hoc detection:

diff -r 49f4d7814760 libinterp/parse-tree/pt-eval.cc
--- a/libinterp/parse-tree/pt-eval.cc        Mon Jun 29 15:01:27 2020 -0400
+++ b/libinterp/parse-tree/pt-eval.cc        Wed Jul 01 15:05:37 2020 +0900
@@ -2739,6 +2739,13 @@ namespace octave
     octave_value_list args (xargs);
     octave_value_list ret_args;

+    if (args.length () != xargs.name_tags ().numel ())
+      error ("DAMN!!!! Undefined elements!!"
+        "\n\targs.length () %d -  %d"
+        "\n\txargs.name_tags ().numel () %d -  %d",
+        args.length (), xargs.length (),
+        args.name_tags ().numel (), xargs.name_tags ().numel ());
+
     if (user_function.is_classdef_constructor ())
       {
         if (args.length () > 0)

Then

>> test do_nothing
***** assert (do_nothing (3), rand (3))
!!!!! test failed
DAMN!!!! Undefined elements!!
        args.length () 1 -  1
        xargs.name_tags ().numel () 2 -  2

Here it becomes obvious, that at the construction time of the argument "octave_value_list" detected two "name_tags", but only the defined element is passed ==> the "octave_value_list" is inconsistent.

I do not think, that my patch with detection is a good way to go.  Maybe someone with more insight into the interpreter can make a check at the construction time of the argument "octave_value_list" for non defined elements and throw a proper error, like for example Matlab R2020b does.

Kai Torben Ohlhus <siko1056>
Project Member

 

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

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Digest:
   bug dependencies.

 

Carbon-Copy List
  • -email is unavailable- added by tutissanalio (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by siko1056 (Submitted the item)
  •  

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

    Only project members can vote.

     

     

     

    Follow 9 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-07-09 siko1056 StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2020-07-08 jwe StatusIn Progress => Ready For Test
    2020-07-04 mmuetzel Dependencies- => bugs #58698 is dependent
    2020-07-03 mmuetzel StatusFixed => In Progress
        Open/ClosedClosed => Open
    2020-07-02 siko1056 StatusConfirmed => Fixed
        Open/ClosedOpen => Closed
    2020-07-01 rik5 StatusNeed Info => Confirmed

    Back to the top


    Powered by Savane 3.5