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:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Release: 
Operating System:  * Any Fixed Release:  11.1.0 (current default)
Planned Release:  11.1.0 (current default)
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 11 Mar 2025 04:48:50 AM UTC, comment #19: 

I checked in a change to avoid the shadowed variable warning (https://hg.savannah.gnu.org/hgweb/octave/rev/623ceb1c7b22).

Rik <rik5>
Group administrator
Tue 11 Mar 2025 02:17:02 AM UTC, comment #18: 

At this point, I think we can safely eliminate the special case for GCC 4.

John W. Eaton <jwe>
Group administrator
Tue 11 Mar 2025 01:32:14 AM UTC, comment #17: 

@Arun: I think you found a bug in our build system.  I didn't get these shadow warnings and so I went back to check the logfile from my configure run.  The GXX_EXTRA_FLAGS does not contain '-Wshadow'.  So, I went back to configure.ac and I see


dnl Don't add -Wshadow for GCC 4.x.
case "$GCC_VERSION" in
  *4*) ;;
  *)   GCC_WSHADOW_OPTION=-Wshadow ;;
esac

GCC_EXTRA_FLAGS="-Wall -W $GCC_WSHADOW_OPTION -Wformat -Wpointer-arith -Wmissing-prototypes -Wstrict-prototypes -Wwrite-strings -Wcast-align -Wcast-qual"
GXX_EXTRA_FLAGS="-Wall -W $GCC_WSHADOW_OPTION -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual"



Turns out my GXX_VERSION is "11.4.0" and the switch statement is just picking up the '4' in the minor version.

Rik <rik5>
Group administrator
Mon 10 Mar 2025 11:37:08 PM UTC, comment #16: 

Your compiler is too new! I do see them now once I did:

$ touch ../libinterp/parse-tree/pt-bytecode-vm.cc
$ make V=1 CFLAGS="-Wshadow $CFLAGS" CXXFLAGS="-Wshadow $CXXFLAGS"

...

libtool: compile:  g++ -DHAVE_CONFIG_H -I. -I.. -DOCTINTERP_DLL -Iliboctave -I../liboctave -I../liboctave/array -Iliboctave/numeric -I../liboctave/numeric -Iliboctave/operators -I../liboctave/operators -I../liboctave/system -I../liboctave/util -I../libinterp/octave-value -Ilibinterp -I../libinterp -I../libinterp/operators -Ilibinterp/parse-tree -I../libinterp/parse-tree -Ilibinterp/corefcn -I../libinterp/corefcn -I../liboctave/wrappers -I/usr/include/GraphicsMagick -Dyysymbol_kind_t=octave_symbol_kind_t -fPIC -pthread -fopenmp -Wall -W -Woverloaded-virtual -Wold-style-cast -Wformat -Wpointer-arith -Wwrite-strings -Wcast-align -Wcast-qual -fvisibility=hidden -Wshadow -MT libinterp/parse-tree/libparse_tree_la-pt-bytecode-vm.lo -MD -MP -MF libinterp/parse-tree/.deps/libparse_tree_la-pt-bytecode-vm.Tpo -c ../libinterp/parse-tree/pt-bytecode-vm.cc  -fPIC -DPIC -o libinterp/parse-tree/.libs/libparse_tree_la-pt-bytecode-vm.o
../libinterp/parse-tree/pt-bytecode-vm.cc: In member function 'octave_value_list octave::vm::execute_code(const octave_value_list&, int)':
../libinterp/parse-tree/pt-bytecode-vm.cc:6484:27: warning: declaration of 'ret' shadows a previous local [-Wshadow]
 6484 |         octave_value_list ret;
      |                           ^~~
../libinterp/parse-tree/pt-bytecode-vm.cc:6417:19: note: shadowed declaration is here
 6417 |     octave_value &ret = TOP_OV ();
      |                   ^~~


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
Mon 10 Mar 2025 11:18:11 PM UTC, comment #15: 

I am getting a compilation warning after this patch:

../libinterp/parse-tree/pt-bytecode-vm.cc:6484:27: warning: declaration shadows a local variable [-Wshadow]
 6484 |         octave_value_list ret;
      |                           ^
../libinterp/parse-tree/pt-bytecode-vm.cc:6417:19: note: previous declaration is here
 6417 |     octave_value &ret = TOP_OV ();
      |                   ^


Is that shadowing intentional?

But it does build and pass "make check".

Arun Giridhar <arungiridhar>
Group Member
Mon 10 Mar 2025 09:21:59 PM UTC, comment #14: 

Patch from comment #12 was never applied.  I checked it in here under Fernando's name (https://hg.savannah.gnu.org/hgweb/octave/rev/299eb2896a17).

The anonymous-function.tst BIST now passes.  Marking bug as Fixed and closing report.


Rik <rik5>
Group administrator
Mon 10 Mar 2025 08:37:48 PM UTC, comment #13: 

Has anyone applied this patch? I am still getting this problem:

octave:3> test ../test/anonymous-function.tst
***** test <*64783>
 s = struct ('re',{pi,2}, 'im',{3,4});
 f = @(x) x.re ;
 f(s);
 assert (ans, pi);
!!!!! regression: https://octave.org/testfailure/?64783
ASSERT errors for:  assert (ans,pi)

  Location  |  Observed  |  Expected  |  Reason
     ()           2          3.1416      Abs err 1.1416 exceeds tol 0 by 1
PASSES 0 out of 1 test



octave:9> ver
----------------------------------------------------------------------
GNU Octave Version: 11.0.0 (hg id: 66126a37e68b)
GNU Octave License: GNU General Public License
Operating System: Linux 6.13.5-200.fc41.x86_64 #1 SMP PREEMPT_DYNAMIC Thu Feb 27 15:07:31 UTC 2025 x86_64


Dmitri.
--

Dmitri A. Sergatskov <dasergatskov>
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 arungiridhar (Posted a comment)
  • -email is unavailable- added by dasergatskov (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 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 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2025-03-10 rik5 StatusIn Progress Fixed
        Open/ClosedOpen Closed
        Fixed Release9.1.0 11.1.0 (current default)
        Planned Release9.1.0 11.1.0 (current default)
    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.14-962f.
    Corresponding source code