bugGNU Octave - Bugs: bug #64783, bytecode interpreter: Default...

 
 

bug #64783: bytecode interpreter: Default number of output arguments not 1 for anonymous functions

Submitter:  Fernando <tutissanalio>
Submitted:  Tue 17 Oct 2023 11:20:18 AM UTC
   
 
Category:  Interpreter Severity:  2 - Minor
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  In Progress Assigned to:  None
Originator Name:  Open/Closed:  * Open
Release:  * dev Operating System:  * Any
Fixed Release:  9.1.0 Planned Release:  9.1.0
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 09 Feb 2024 11:50:57 AM UTC, comment #12: 

I realized we need to check if the cs-list is empty. Here is the fix again:


diff -r 3904d7116c8c libinterp/parse-tree/pt-bytecode-vm.cc
--- a/libinterp/parse-tree/pt-bytecode-vm.cc        Thu Feb 01 23:34:35 2024 -0500
+++ b/libinterp/parse-tree/pt-bytecode-vm.cc        Fri Feb 09 12:40:34 2024 +0100
@@ -6415,6 +6415,15 @@

 ret_anon:
   {
+    // Don't allow a comma-separated list to escape (see bug #64783).
+    octave_value &ret = TOP_OV ();
+    if (ret.is_cs_list ())
+      {
+        const octave_value_list lst = ret.list_value ();
+        if (!lst.empty ())
+          ret = lst(0);
+      }
+
     // We need to tell the bytecode frame we are unwinding so that it can save
     // variables on the VM stack if it is referenced from somewhere else.
     m_tw->get_current_stack_frame ()->vm_unwinds ();


Fernando <tutissanalio>
Fri 09 Feb 2024 08:39:28 AM UTC, comment #11: 

Here is a possible fix. make check is clean, at least for my configuration.


diff -r 3904d7116c8c libinterp/parse-tree/pt-bytecode-vm.cc
--- a/libinterp/parse-tree/pt-bytecode-vm.cc        Thu Feb 01 23:34:35 2024 -0500
+++ b/libinterp/parse-tree/pt-bytecode-vm.cc        Fri Feb 09 09:22:45 2024 +0100
@@ -6415,6 +6415,14 @@

 ret_anon:
   {
+    // Don't allow a comma-separated list to escape (see bug #64783).
+    octave_value &ret = TOP_OV ();
+    if (ret.is_cs_list ())
+      {
+        const octave_value_list lst = ret.list_value ();
+        ret = lst(0);
+      }
+
     // We need to tell the bytecode frame we are unwinding so that it can save
     // variables on the VM stack if it is referenced from somewhere else.
     m_tw->get_current_stack_frame ()->vm_unwinds ();


Fernando <tutissanalio>
Fri 02 Feb 2024 04:07:23 PM UTC, comment #10: 

There has been a merge to the bytecode branch since this was fixed on stable.  Re-titling big and re-opening.  Someone else who works with that part of the code will need to verify and/or fix this.

Rik <rik5>
Group administrator
Fri 02 Feb 2024 12:50:03 PM UTC, comment #9: 

Thanks for fixing this. I tried the bytecode-interpreter branch and the issue remains there, although I think the commits have been merged into that branch.

Fernando <tutissanalio>
Thu 01 Feb 2024 11:55:39 PM UTC, comment #8: 

I added a new file "anonymous-function.tst" in the test/ directory to hold BIST tests.  Then I added a test for this bug report.  See https://hg.savannah.gnu.org/hgweb/octave/rev/a575aa730d84.

Marking as Fixed and closing report.

Rik <rik5>
Group administrator
Thu 01 Feb 2024 05:43:56 AM UTC, comment #7: 

I pushed the following change.  We could still use a test.

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

John W. Eaton <jwe>
Group administrator
Tue 30 Jan 2024 08:46:25 PM UTC, comment #6: 

The change would probably be better here, where the special treatment is given to functions that have a body consisting of a single expression:


diff --git a/libinterp/parse-tree/pt-eval.cc b/libinterp/parse-tree/pt-eval.cc
--- a/libinterp/parse-tree/pt-eval.cc
+++ b/libinterp/parse-tree/pt-eval.cc
@@ -3678,6 +3678,11 @@ tree_evaluator::execute_user_function (o
               m_call_stack.set_location (stmt->line (), stmt->column ());

               retval = expr->evaluate_n (*this, nargout);
+
+              // Don't allow a comma-separated list to escape (see bug #64783).
+
+              if (nargout <= 1 && retval.length () == 1 && retval(0).is_cs_list ())
+                retval = retval(0).list_value ();
             }
         }
       else


Yeah, we could probably use a separate file (or files) for testing anonymous functions.

The trouble here is partly due the somewhat odd way we handle comma-separated lists.  Instead of those objects just being pushed onto an output stack where they would be consumed as needed, we store the whole list in a single special octave_value object.  I didn't see a better way to do it when adding the concept of comma-separated lists to the tree evaluator.

John W. Eaton <jwe>
Group administrator
Tue 30 Jan 2024 07:23:07 PM UTC, comment #5: 

Patch works fine, and 'make check' passes.  Probably should add a BIST test for this.  I looked in test/ directory and there is a file "func.tst" but it doesn't contain anything on anonymous functions.  There is a directory for private-functions/, local-functions/, and fcn-handles/ but none of these seem correct as a place to put tests either.  Maybe we should create a new file for anonymous function testing?

Rik <rik5>
Group administrator
Tue 30 Jan 2024 07:09:30 PM UTC, comment #4: 

Is there a different eval path for anonymous functions as opposed to regular functions which might easily explain this?  It does seem that the octave_value_list should be populated with all the values because the interpreter might not know if they all values will be passed on to something like a matrix constructor '[ ]'.  Maybe, as you say, we just should be printing the first output.

Rik <rik5>
Group administrator
Tue 30 Jan 2024 07:03:55 PM UTC, comment #3: 

Here is a possible fix.


diff --git a/libinterp/octave-value/ov-fcn-handle.cc b/libinterp/octave-value/ov-fcn-handle.cc
--- a/libinterp/octave-value/ov-fcn-handle.cc
+++ b/libinterp/octave-value/ov-fcn-handle.cc
@@ -2785,7 +2785,17 @@ anonymous_fcn_handle::call (int nargout,

   unwind_action act ([&tw] () { tw.pop_stack_frame (); });

-  return oct_usr_fcn->execute (tw, nargout, args);
+  octave_value_list retval = oct_usr_fcn->execute (tw, nargout, args);
+
+  // FIXME: Is this the correct (or best) place for the following check
+  // and transformation?
+
+  // Don't allow a comma-separated list to escape (see bug #64783).
+
+  if (nargout <= 1 && retval.length () == 1 && retval(0).is_cs_list ())
+    retval = retval(0).list_value ();
+
+  return retval;
 }

 octave_value


John W. Eaton <jwe>
Group administrator
Tue 30 Jan 2024 06:31:01 PM UTC, comment #2: 

It seems to only be a problem if nargout for the anonymous function call is 0:


octave:1> version
ans = 8.4.0
octave:2>  a=struct('re',{1,2},'im',{3,4});
octave:3> f=@(x) x.re
f =

@(x) x.re

octave:4> x = f(a)
x = 1
octave:5> [x,y] = deal (f(a))
x = 1
y = 2
octave:6> [x,y] = f(a)
x = 1
y = 2


So it looks like we are just incorrectly displaying (or capturing) all the values in the comma-separated list in this case instead of just the first.

John W. Eaton <jwe>
Group administrator
Tue 30 Jan 2024 06:18:24 PM UTC, comment #1: 

Confirmed.  I think the diagnosis is correct that Octave does not set the expected nargout to 1 for anonymous functions.  Here is a simple workaround


[y] = feval (f, a);


which uses the interpreter to enforce only a single output.

Rik <rik5>
Group administrator
Tue 17 Oct 2023 11:20:18 AM UTC, original submission:  

In octave (I tried with default, stable and ver 6.4.0), anonymous functions return multiple output arguments in contexts where normal m-functions return only one:

octave:1> a=struct('re',{1,2},'im',{3,4});
octave:2> a.re
ans = 1
ans = 2
octave:3> f=@(a)a.re;
octave:4> f(a)
ans = 1
ans = 2
octave:5> [f(a)]
ans =
   1   2


This is incompatible with Matlab. In Matlab:

>> a=struct('re',{1,2},'im',{3,4})
a =
  1×2 struct array with fields:
    re
    im
>> f=@(a)a.re
f =
  function_handle with value:
    @(a)a.re
>> f(a)
ans =
     1
>> [f(a)]
ans =
     1


Fernando <tutissanalio>

 

(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

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by jwe (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by tutissanalio (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2024-02-02 rik5 StatusFixed In Progress
        Open/ClosedClosed Open
        SummaryDefault number of output arguments not 1 for anonymous functions bytecode interpreter: Default number of output arguments not 1 for anonymous functions
    2024-02-01 rik5 StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.1.0
    2024-02-01 jwe Planned ReleaseNone 9.1.0
    2024-02-01 jwe StatusPatch Reviewed Ready For Test
    2024-01-30 rik5 StatusConfirmed Patch Reviewed
    2024-01-30 jwe Item GroupMatlab Compatibility Incorrect Result
    2024-01-30 rik5 Severity3 - Normal 2 - Minor
        StatusNone Confirmed

    Back to the top

    Powered by Savane 3.13-3736.
    Corresponding source code