bugGNU Octave - Bugs: bug #59704, function call on classdef...

 
 

bug #59704: function call on classdef subobject fails with "numel: invalid return value"

Submitted by:  Sébastien Villemot <svillemot>
Submitted on:  Wed 16 Dec 2020 04:13:21 PM UTC  
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  Closed
Release:  6.1.0 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)

Tue 02 Feb 2021 07:57:58 AM UTC, comment #18: 

Seems to be working now.

Closing as fixed.

Markus Mützel <mmuetzel>
Project Member
Wed 27 Jan 2021 12:36:23 PM UTC, comment #17: 

Thanks.
Should be fixed here:
https://hg.savannah.gnu.org/hgweb/octave/rev/34088c4828fe

I accidentally made the change on the default branch. But it's now also grafted to stable:
https://hg.savannah.gnu.org/hgweb/octave/rev/1ceeab926dc6

Markus Mützel <mmuetzel>
Project Member
Wed 27 Jan 2021 09:32:38 AM UTC, comment #16: 

Great. Just a small correction: the name of the function bug59704_test should match its filename.

Fernando <tutissanalio>
Sat 23 Jan 2021 02:40:14 PM UTC, comment #15: 

I added your test case to your second patch and also pushed it to stable here:
https://hg.savannah.gnu.org/hgweb/octave/rev/09f99fe5190c

Marking as ready for test.

Markus Mützel <mmuetzel>
Project Member
Sat 23 Jan 2021 01:32:39 PM UTC, comment #14: 

I added the test case from comment #2 to your first patch and pushed it here:
https://hg.savannah.gnu.org/hgweb/octave/rev/67857e94604a

Markus Mützel <mmuetzel>
Project Member
Wed 20 Jan 2021 09:37:13 AM UTC, comment #13: 

@Fernando: I'll test that when I'll come around to it.

In the meantime, I can confirm that the error described in comment #6 is also a regression from Octave 5.2.0 (where this still worked without error).

Markus Mützel <mmuetzel>
Project Member
Tue 19 Jan 2021 07:18:21 PM UTC, comment #12: 

I think a good solution is to apply both patches proposed:

- Patch from comment #4 (file #50583). In octave_classdef::xnumel(), temporarily set to null the left-value list of the current statement before calling a user-defined numel method. This patch alone is enough to fix the test case from comment #1. It does not affect sentences where numel is called explicitly, such as

n=o.numel();

because in this case the call to numel is not done through octave_classdef::xnumel.

- Patch from comment #9 (file #50634). Narrow the scope in which the lvalue list of the sentence is considered by the tree_evaluator. This fixes the problem with overloaded subsasgn mentioned in comment #6.

Fernando <tutissanalio>
Sun 17 Jan 2021 08:54:56 PM UTC, comment #11: 

Oops! Sorry. It was supposed to solve test case from comment #1 but I did not check it properly. I tried it with:

octave:1> o=bug59704();
octave:2> [~,o.p]=size(rand(2,5));

and it worked. However, as you say, it does not work with

octave:2> o.test();

or with

[~,o.p]=bug59704_test(o.p)

After some debugging, I saw that it happens because bug59704_test is a user function. In that case, the function numel of the lvalue o.p is called also when evaluating the right hand side of the asignment, which is something I had not anticipated.

Not sure how to best fix this.

Fernando <tutissanalio>
Sun 17 Jan 2021 06:06:08 PM UTC, comment #10: 

I applied the patch from comment #9 locally and re-built.
After that, the test case from comment #1 still fails with the same error.
Is the patch supposed to fix that case? Or should it go on top of another one?

Markus Mützel <mmuetzel>
Project Member
Sun 03 Jan 2021 08:15:28 PM UTC, comment #9: 

I think you're right and there is a better place to fix this bug. I think what needs to be done is to narrow the scope in which the lvalue list of the sentence is considered by the tree_evaluator. This scope could be just the processing of the right side of the assignment.

Here is a simple patch which does it. It fixes the problem in the original post and also the problem in comment #6. I'm not completely sure if the idea is correct or not, so let's wait for jwe's view.

(file #50634)

Fernando <tutissanalio>
Sat 02 Jan 2021 03:54:09 PM UTC, comment #8: 

Thanks for the explanations and the additional example.
That makes me suspect that it might be possible to fix the issue more generally (i.e. have the correct value for the lvalue list when the functions are called) instead of in each affected function.
I don't know where that place would be though. Maybe in the tree-evaluator or some place else?
I also don't know if we'd have enough information at that point already.
Let's wait for jwe's opinion on this.

Markus Mützel <mmuetzel>
Project Member
Sat 02 Jan 2021 03:26:50 PM UTC, comment #7: 

I forgot the attachment.

(file #50613)

Fernando <tutissanalio>
Sat 02 Jan 2021 03:24:53 PM UTC, comment #6: 

This bug is related to bug #58727. It's again the problem of ignoring a function return value because "~" appears in the lvalue list of the current instruction. In that case, it happened while evaluating an argument list, and in this case, it happens when evaluating numel for an lvalue.

The solution proposed (comment #4) is basically the same used for that bug: temporarily set the lvalue list to null. However, in this case, this can be done in different moments of the evaluation:
- as soon as octave_lvalue::numel is called (although it might not be necessary).
- as late as possible, and only when a user-defined numel function is being called.

I chose the second option, just to avoid unnecessary overhead.

I have also detected the same problem in another situation: evaluating overloaded subsasgn. I don't know if this should be a separate bug. For example (using the file class_subsasgn attached):

>> obj=class_subsasgn();
>> obj(:)
ans =
   1001
   1002
   1003
   1004
   1005

>> [x,obj(1)]=size(rand(2,5)); % This works without problems
>> obj(:)
ans =
      5
   1002
   1003
   1004
   1005

>> [~,obj(1)]=size(rand(2,5)); % But this does not work
error: cannot convert 'scalar' into 'object'

In this case, the output of overloaded subsasgn is being ignored.

This can also be solved in the same way. It could also be done early in the evaluation process, or later, and only if user-defined subsasgn is being called (in octave_classdef::subsasgn).

Fernando <tutissanalio>
Thu 31 Dec 2020 01:19:36 PM UTC, comment #5: 

I'm not able to review because I don't understand possible side effects of that change.
I wonder if this is the correct place to fix this.
Should the lvalue list be scoped differently in this case?

CC'ing jwe because I hope he'll better understand that part of the code.

Markus Mützel <mmuetzel>
Project Member
Mon 28 Dec 2020 05:29:54 PM UTC, comment #4: 

Here is the patch for the solution pointed out in comment #2.

(file #50583)

Fernando <tutissanalio>
Fri 25 Dec 2020 08:11:21 PM UTC, comment #3: 

I also wonder if it is correct to call the overloaded numel method here to get the number of outputs of the sentence.

If we modify the bug59704/numel method to return the value 2, we would have:

>> o=bug59704();
>> o.p
ans = [](0x0)
>> [o.p]=size(rand(2,5))
o =
  bug59704 object with properties:
      p: [1x2 cs-list]
>> o.p
ans = 2
ans = 5

Initially, o.p is an empty array. However, when used as an lvalue, it acts as  a cs-list of two elements (because the numel method returned 2). As a result, o.p ends up storing a cs-list. Is that behavior correct? I thought cs-lists could not be stored in a variable.

Fernando <tutissanalio>
Thu 24 Dec 2020 05:25:21 PM UTC, comment #2: 

I did some debugging. It seems that, when calling "numel" in:

https://hg.savannah.gnu.org/hgweb/octave/file/45abff4199d8/libinterp/octave-value/ov-classdef.cc#l254

octave incorrectly uses the left-value list [~, this.p] of the current statement:
+verbose+
      [~, this.p] = bug59704_test(this.p);
-verbose-
to bind the result of the execution of numel. Because of the ~, the output of numel is ignored, and that causes the error.

A possible fix would be to temporarily set to null the left-value list of the current statement before calling numel. That could be done by inserting the following lines

          octave::interpreter& interp = octave::__get_interpreter__ ("octave_classdef::xnumel");
          octave::tree_evaluator& tw = interp.get_evaluator();

          octave::unwind_action act ([&tw] (const std::list<octave::octave_lvalue> *lvl)
                            {
                              tw.set_lvalue_list (lvl);
                            }, tw.lvalue_list ());
          tw.set_lvalue_list (nullptr);

just before

https://hg.savannah.gnu.org/hgweb/octave/file/45abff4199d8/libinterp/octave-value/ov-classdef.cc#l254

You also need to include "oct-lvalue.h"

I tried it and it works, but maybe there is a better solution. I am not too familiarized with the code.

Fernando <tutissanalio>
Tue 22 Dec 2020 04:56:39 PM UTC, comment #1: 

I also don't understand the reason for this error message.

The attached much simpler classdef class exhibits the same error.

To reproduce, place both files into the same directory and run:

o = bug59704();
o.test()

The error is issued here:
https://hg.savannah.gnu.org/hgweb/octave/file/45abff4199d8/libinterp/octave-value/ov-classdef.cc#l256

(file #50544, file #50545)

Markus Mützel <mmuetzel>
Project Member
Wed 16 Dec 2020 04:13:21 PM UTC, original submission:  

Hi,

I found another classdef-related regression that impacts Dynare.

It can be reproduced with the following steps (sorry for not providing a minimal testcase, but I really don’t understand what’s going on):

$ git clone https://git.dynare.org/Dynare/dseries.git
$ cd dseries/
$ git checkout af4d2cc361a10f1b98c65bf05c0a71234566f9dc
$ octave
> addpath([ pwd '/src'])
> initialize_dseries_class
> d = randn(100,1);
> o = dseries(d);
> p = o.onesidedhpcycle();
error: @dseries/numel: invalid return value
error: called from
    one_sided_hp_filter
    onesidedhpcycle_ at line 79 column 17
    onesidedhpcycle at line 44 column 5
    subsref at line 135 column 19

Note that this example succeeds with Octave 5.2.0 (as well as with MATLAB).

Sébastien Villemot <svillemot>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #50634:  bug59704-2.patch added by tutissanalio (2KiB - text/x-patch)
file #50613:  class_subsasgn.m added by tutissanalio (619B - text/x-objcsrc)
file #50583:  bug59704.patch added by tutissanalio (2KiB - text/x-patch)
file #50544:  bug59704.m added by mmuetzel (254B - text/plain)
file #50545:  bug59704_test.m added by mmuetzel (73B - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by mmuetzel
  • -email is unavailable- added by tutissanalio (Posted a comment)
  • -email is unavailable- added by mmuetzel (Updated the item)
  • -email is unavailable- added by svillemot (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 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2021-02-02 mmuetzel StatusReady For Test => Fixed
        Open/ClosedOpen => Closed
    2021-01-23 mmuetzel StatusIn Progress => Ready For Test
    2021-01-23 mmuetzel StatusConfirmed => In Progress
    2021-01-03 tutissanalio Attached File- => Added bug59704-2.patch, #50634
    2021-01-02 tutissanalio Attached File- => Added class_subsasgn.m, #50613
    2020-12-31 mmuetzel Carbon-Copy- => Added jwe
    2020-12-28 tutissanalio Attached File- => Added bug59704.patch, #50583
    2020-12-22 mmuetzel Attached File- => Added bug59704.m, #50544
        Attached File- => Added bug59704_test.m, #50545
        StatusNone => Confirmed
        Operating SystemGNU/Linux => Any

    Back to the top


    Powered by Savane 3.5