bugGNU Octave - Bugs: bug #38236, invoking script in demo block to...

 
 

bug #38236: invoking script in demo block to define variables causes errors

Submitter:  Carlo de Falco <cdf>
Submitted:  Sat 02 Feb 2013 04:09:00 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Regression
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Fri 15 Sep 2017 09:00:31 PM UTC, comment #32: 

This seems to be well and truly fixed now.  I can run Markus' test script repeatedly and always get 1 as the output.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Thu 14 Sep 2017 04:24:31 AM UTC, comment #31: 

I pushed the following changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/21915520ac7b

I believe the random behavior was caused by the use of "mutable" data members in the symbol_table::symbol_reference class.  Once that was removed, I saw predictable behavior, but it was still not correct.

The symbol_reference class was introduced to handle symbol lookups in scripts when the evaluation scope changes.  Like functions, scripts may be parsed once and executed many times in different contexts.  But since they are supposed to be evaluated in the scope of the caller, a mechanism is needed to access the symbols from the scope outside of the script.  Originally, there was no scope created and stored for scripts.  Now I've added one, but it is not the scope that is used for evaluation.  It is only used to hold symbols for parsing.

To solve the evaluation problem, I made it possible for a symbol_record_rep object to point to a secondary symbol_record_rep that contains the actual data for the symbol.

Now when a script is evaluated, the symbols that are stored in the parse tree are bound to the evaluation scope just before the code for the script is evaluated.  Then these links are removed when the script exits.

The same approach is also used for nested functions now, though binding to the parent scope happens at parse time, not evaluation time, and the links are never removed since the parent scope of a nested function doesn't change.

Now to get a value from a symbol_record_rep requires checking to see if the "next rep" is valid and using it if it is.  This cost is small, and in my tests appears to be faster than the old symbol_reference class.

John W. Eaton <jwe>
Group administrator
Fri 01 Sep 2017 01:23:45 AM UTC, comment #30: 

Looking again, I see that even with octave-cli, it displays 1 on the first trip through the outer loop, then 100 for all the rest.  Then it displays 100 each time through on subsequent runs.

I get the same result with the GUI with the attached patch.  So clearly something is wrong with what we're asking the GUI to do inside the clear function.  It makes sense that this results in some kind of race condition given that it involves actions with octave_link.  I think we need to rethink the way this communication works (as Dan was urging me to do years ago).


(file #41708)

John W. Eaton <jwe>
Group administrator
Thu 31 Aug 2017 11:39:19 PM UTC, comment #29: 

Weird, Markus' test script seems to always work for me if I use octave-cli and only occasionally displays "100" when I use the GUI, (or "octave --no-gui", which still uses Qt libraries).

I'll try some bisecting.

John W. Eaton <jwe>
Group administrator
Thu 31 Aug 2017 10:08:13 PM UTC, comment #28: 

Wonderful (sarcasm!).  So there is some weird race condition or worse.  Changing the "Operating System" => "Any".

Rik <rik5>
Group administrator
Thu 31 Aug 2017 09:54:33 PM UTC, comment #27: 

I just got a test failure here on Debian with make check.

Mike Miller <mtmiller>
Group Member
Sun 27 Aug 2017 12:51:30 PM UTC, comment #26: 

This seems to be due to some kind of race condition.
Executing this script in a recent tip (hg id 9c97909a6c94):

for i1 = 1:20;
  try
    for i2 = 1:100
      u_vr;
    endfor
  catch
  end_try_catch
  disp (i2)
endfor


I see the following on Windows:

>> test_clear_local
 39
 26
 20
 25
 14
 20
 9
 33
 2
 3
 3
 15
 3
 9
 22
 2
 3
 40
 1
 9


And the following on Linux:

>> test_clear_local
 6
 1
 2
 11
 6
 34
 26
 87
 11
 54
 1
 77
 4
 48


Although it always seems to fail in the test suite on Windows and I haven't seen it yet to fail "make check" on Linux.

Markus Mützel <mmuetzel>
Group administrator
Sun 27 Aug 2017 05:17:07 AM UTC, comment #25: 

Confirmed.  I tested in a Windows XP VM and the BIST test fails.  This is very weird because there is nothing that I see that is particularly tied to an operating system.

Rik <rik5>
Group administrator
Sat 26 Aug 2017 07:07:41 PM UTC, comment #24: 

This test still does not pass on Windows with hg id 9c736cc2ee94 and now appears as a regression (but I think it was never working).
Re-opening for Windows.

Markus Mützel <mmuetzel>
Group administrator
Thu 24 Aug 2017 07:31:15 PM UTC, comment #23: 

If you want to change the tests to be more specific that would be fine.

Rik <rik5>
Group administrator
Thu 24 Aug 2017 06:20:08 AM UTC, comment #22: 

I was wondering, maybe we should modify the test
to be more similar to the example posted with the
original submission rather than the abbreviated version
developed with comment #15 and comment #16 ?

Is what we really want to test the user visible
feature of being able to invoke scripts inside demo blocks?

I mean, could it be possible that the implementation
of "demo" changes in the feature introducing a regression
but without triggering the current bug which relies on the
way it is currently implemented?

Carlo de Falco <cdf>
Group Member
Wed 23 Aug 2017 11:34:41 PM UTC, comment #21: 

Okay, I marked the bug as fixed in the test suite (http://hg.savannah.gnu.org/hgweb/octave/rev/2ae8efbafb58) but left it in place.  If it is broken again the test suite will record it as a regression.

Rik <rik5>
Group administrator
Wed 23 Aug 2017 05:59:37 AM UTC, comment #20: 

Rik,

I tried also the example code from the original submission
and that now works correctly.

I think we can finally close this bug after more than 4 years and mark it as fixed, great!!!

This will probably also allow for a new release of the secs package after all this time ;)

What should be done with the test code?
I'd say leave it in place to avoid regressions, is this right?
Should the name of the test be left unchanged even if this ticket is closed?

Carlo de Falco <cdf>
Group Member
Tue 15 Aug 2017 06:42:33 AM UTC, comment #19: 

@carlo: I just ran the test bug-38236.tst and it passes with hg id 23915:adf580507691.  Can this bug be closed now?

Rik <rik5>
Group administrator
Fri 15 Jan 2016 11:30:04 PM UTC, comment #18: 

Since jwe said regressions are high priority bugs, I'm looking into this.

The problem seems to be that df_vr creates a vr symbol in the context of _demo_, but that symbol isn't cleared when _demo_ is.  Issuing "who" just after "v=7" in df_vr doesn't return anything the second time around.  The assignment is to the original symbol (it consistently has the same "this" address) but that symbol is not found in the context of the new _demo_.

The problem goes away if "clear -f df_vr" is issued when "clear _demo_" is issued.

Debugging this is made hard by my lack of understanding of how things are supposed to work.  I had assumed that variables are created on the fly during a function, but it seems that they are all created when the file is parsed and instantiated with <unknown value>.  I assume that this is to avoid a string-based look-up for every variable reference, right?  How is reuse of the script in different contexts supposed to work?

Each symbol seems to have its own value_stack indexed by context.  Is context numbered sequentially in the number of stack frames?  I assume that there isn't a value of each symbol for each stack frame, just the ones in which the variable occurs, right?  If so, are there "gaps" in the value_stack?

Thanks for your help!

Lachlan Andrew <lachlan>
Thu 21 Mar 2013 09:34:59 AM UTC, comment #17: 

After pulling and updating, the last effect described in the previous comment disappeared, it was probably due to bug #38549
which was fixed by this changeset:


changeset:   16337:6bfd8dbd7d3c
user:        John W. Eaton <jwe@octave.org>
date:        Tue Mar 19 18:03:23 2013 -0400
summary:     don't swallow statements that follow comment lines (bug #38549)

c.

Carlo de Falco <cdf>
Group Member
Thu 21 Mar 2013 07:27:44 AM UTC, comment #16: 

Rik,

The tests I had attached below (which are in the source tree in
test/bug-38236) are already implemented without calling demo.m

u_vr.m can be further simplified as follows:

+eval+
# u_vr.m

## define and exectute "__demo__" once
eval ("function _demo_ ();  df_vr;  v = vr * 2; endfunction");
_demo_;

## clear definition of "__demo__"
clear _demo_

## define and exectute "__demo__" once more
eval ("function _demo_ ();  df_vr;  v = vr * 2; endfunction");
_demo_;

-eval-


running some more experiments I noted even more oddities:

1) If the code avove is in a script file named u_vr, invoking
the script once gives an error, but all subsequent invocations
of u_vr do not err out:


>> clear all
>> u_vr
error: 'vr' undefined near line 1 column 36
error: called from:
error:   __demo__ at line 1, column 35
error:   /Users/carlo/Documents/_Programmi/Miei/C/octave/octave/test/bug-38236/u_vr.m at line 14, column 1
>> u_vr
>>


2) If I remove the "eval" command in u_vr and just redefine the function '__demo__' there is never any error:


# u_vr.m
1;

## define and exectute "__demo__" once
function __demo__ ();  df_vr;  v = vr * 2; endfunction
__demo__;

## clear definition of "__demo__"
clear __demo__

## define and exectute "__demo__" once more
function __demo__ ();  df_vr;  v = vr * 2; endfunction
__demo__;



>> clear all
>> u_vr
>> u_vr


3) Now this is the most amazing and unexpected effect I see.
If, rather than running the script u_vr I just copy the commands to the prompt, I get no error


>> eval ("function __demo__ ();  df_vr;  v = vr * 2; endfunction");
>> __demo__;
>> clear all
>> eval ("function __demo__ ();  df_vr;  v = vr * 2; endfunction");
>> __demo__;
>>


_BUT_, if I also copy the commented lines to the prompt, I get a DIFFERENT error:


>> ## define and exectute once
>> eval ("function __demo__ ();  df_vr;  v = vr * 2; endfunction");
>> __demo__;
error: '__demo__' undefined near line 1 column 1


I'll push the simplified version of u_vr.m to mercurial.
c.




Carlo de Falco <cdf>
Group Member
Wed 20 Mar 2013 08:25:21 PM UTC, comment #15: 

I've made an abbreviated set of files which demonstrates the problem without having to use demo.m.  I'm attaching the files, block1.m, block2.m, block3.m. to this report.

To see the problem, run block1 twice.

Sample code:


octave:1> block1
setting vr to 7
vr =  7
*** calling who to see variables present ***
Variables in the current scope:

vr  x

*** done calling who ***
a =  98
octave:2> block1
setting vr to 7
vr =  7
*** calling who to see variables present ***
*** done calling who ***
error: 'vr' undefined near line 3 column 13
error: called from:
error:   __tmp__ at line 3, column 8
error:   /home/rik/wip/Projects_Mine/octave-dbg/block1.m at line 8, column 3


What you can see from running is that the assignment statement is executed, vr = 7, and echoed to the screen.  But, the symbol table is not updated with actual values.  I had a keyboard() statement in block3.m and could see that nothing was defined during the second run of block1.  Manual assignment of variables at the debug prompt did work.  So the symbol table was not entirely messed up.

Another oddity is that if the lowest level function is removed before each run the code will always work.

Example


block1
clear -f block3
block1
clear -f block3
block1
... etc.




(file #27640, file #27641, file #27642)

Rik <rik5>
Group administrator
Wed 20 Mar 2013 10:04:48 AM UTC, comment #14: 

I finished bisecting and arrived at the same conclusion as Rik concerning the first bad revision.

Julien Bect <jbect>
Wed 20 Mar 2013 08:43:13 AM UTC, comment #13: 

Before the patch for bug 34497, clear _debug_ did not actually clear the _debug_ function (as it was defined on the command line).

So, it looks like there is some problem caused by actually clearing _debug_.

Max Brister <fisheater>
Wed 20 Mar 2013 01:33:48 AM UTC, comment #12: 

The changeset that causes the problem for me, from hg bisect, is


The first bad revision is:
changeset:   15003:1f5dbfc23fc2
user:        Max Brister <max@2bass.com>
date:        Mon Jul 23 06:53:09 2012 -0500
summary:     'clear -f' does not work for command line functions (bug #34497)


As I thought, it is something with clearing functions.  I'll e-mail Max and see if he recalls anything about this changeset.

Rik <rik5>
Group administrator
Tue 19 Mar 2013 05:47:47 PM UTC, comment #11: 

In the meantime I keep on bisecting ;-)

bad : 3d8ace26c5b4 (Thu, 26 Jul 2012 08:13:22)
http://hg.savannah.gnu.org/hgweb/octave/rev/3d8ace26c5b4

good : 0ef151f9fdc9 (Fri, 20 Jul 2012 16:13:35)
http://hg.savannah.gnu.org/hgweb/octave/rev/0ef151f9fdc9

The regression happened last July.

Julien Bect <jbect>
Tue 19 Mar 2013 05:37:37 PM UTC, comment #10: 

I think this problem is related to problems clearing functions.

Inside test.m we use eval to create a function that has the %!test code inside it.  We then execute that function and clear it when we are done.  I made a small cset where I made the name of the function unique by just adding an incrementing counter to the function name and this bug disappeared.  I'll see if I can find the cset and attach it.  It really wasn't meant as a fix though, just a way to diagnose the problem.

Rik <rik5>
Group administrator
Tue 19 Mar 2013 03:25:48 PM UTC, comment #9: 

Just to keep you informed: I'm still bisecting, at the moment I've narrowed it down to 80 changesets ;-)

Julien Bect <jbect>
Mon 18 Mar 2013 05:51:04 PM UTC, comment #8: 

Julien,
On my system I have installed the development version
and version 3.6.2 and I get:


>> version
ans = 3.6.2
>> test bug-38236.tst
PASSES 1 out of 1 tests


and



>> version
ans = 3.7.2+
>> test bug-38236.tst
  ***** test
 u_vr
!!!!! test failed
'vr' undefined near line 1 column 37
>>



I don't have 3.6.4 installed.
So maybe we can conclude the offending
changeset is between the 3.6.2 release tag
and changeset d048ef58bb72?


Jordi,
As I said in a post on the list I am unable
to do an "hg bisect" as many versions fail to build
on my system so I cannot run the test at all intermediate
stages.





Carlo de Falco <cdf>
Group Member
Mon 18 Mar 2013 03:22:56 PM UTC, comment #7: 

If you're digging blindly, you might like Mercurial's bisect feature:

http://mercurial.selenic.com/wiki/BisectExtension

Jordi Gutiérrez Hermoso <jordigh>
Group Member
Mon 18 Mar 2013 03:13:56 PM UTC, comment #6: 

The problem was already there on default cs d048ef58bb72

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

I will dig further in the past.

Julien Bect <jbect>
Tue 05 Feb 2013 02:45:14 PM UTC, comment #5: 

you're right sorry, I just pushed a new changeset.

http://hg.savannah.gnu.org/hgweb/octave/rev/9e710110401c

c.

Carlo de Falco <cdf>
Group Member
Tue 05 Feb 2013 01:08:02 PM UTC, comment #4: 

Apparently you forgot to include the new files (test/bug-38236/*) in your changeset.

Julien Bect <jbect>
Tue 05 Feb 2013 09:38:27 AM UTC, comment #3: 

looking at how actually simple the function "demo ()" is,
I think the bug is probably rather in eval or clear.
While I try to look more deeply into this issue, I pushed a changeset:

http://hg.savannah.gnu.org/hgweb/octave/rev/59715612ea72

with a test for this bug that does not use demo.  
Probably I should change the title for this bug report but
I can not come up with a better one at the moment.
c.

Carlo de Falco <cdf>
Group Member
Tue 05 Feb 2013 06:49:34 AM UTC, comment #2: 

forget my previous post, it was just my mistake, even with JIT disabled the error is still there, sorry for the noise.
c.

Carlo de Falco <cdf>
Group Member
Tue 05 Feb 2013 05:44:56 AM UTC, comment #1: 

with JIT disabled at configure time the problem seems
to disappear.

c.

Carlo de Falco <cdf>
Group Member
Sat 02 Feb 2013 04:09:00 PM UTC, original submission:  

let the file 'df_vr.m' contain the following


 vr = 7;


and the file 'u_vr' be as follows



function ret = u_vr (vr)
  ret = 7 * vr;
endfunction

%!demo
%! df_vr
%! v = 2 * vr;
%! a = u_vr (v)


running 'demo u_vr' once gives a correct result:


>> demo u_vr
u_vr example 1:
 df_vr
 v = 2 * vr;
 a = u_vr (v)

a =  98


but running the same command a second time causes an error


u_vr example 1:
 df_vr
 v = 2 * vr;
 a = u_vr (v)

u_vr example 1: failed
'vr' undefined near line 4 column 10


I marked this as a regression because it seems to work correctly
in the stable release (at least on 3.6.3, I do not have a build of 3.6.4 to try on)

I attach copies of the two simple scripts above for testing convenience.

c.

Carlo de Falco <cdf>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #41708:  diffs.txt added by jwe (480B - text/plain)
file #27640:  block1.m added by rik5 (217B - text/x-octave)
file #27641:  block3.m added by rik5 (148B - text/x-octave)
file #27642:  block2.m added by rik5 (62B - text/x-octave)
file #27371:  u_vr.m added by cdf (102B - application/octet-stream)
file #27370:  df_vr.m added by cdf (8B - application/octet-stream)

 

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 mtmiller (Posted a comment)
  • -email is unavailable- added by mmuetzel (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by cbm
  • -email is unavailable- added by rik5
  • -email is unavailable- added by fisheater (Posted a comment)
  • -email is unavailable- added by rik5
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by jbect (Posted a comment)
  • -email is unavailable- added by cdf (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 23 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2017-12-21 jwe Open/ClosedOpen Closed
    2017-09-15 rik5 StatusReady For Test Fixed
    2017-09-14 jwe StatusConfirmed Ready For Test
    2017-09-01 jwe Attached File- Added diffs.txt, #41708
    2017-08-31 rik5 Summaryinvoking script in demo block to define variables causes errors (Win32) invoking script in demo block to define variables causes errors
    2017-08-31 rik5 Operating SystemMicrosoft Windows Any
    2017-08-27 rik5 StatusNone Confirmed
        Summaryinvoking script in demo block to define variables causes errors invoking script in demo block to define variables causes errors (Win32)
    2017-08-26 mmuetzel StatusFixed None
        Open/ClosedClosed Open
        Operating SystemAny Microsoft Windows
    2017-08-23 rik5 Open/ClosedOpen Closed
    2017-08-23 cdf StatusReady For Test Fixed
    2017-08-15 rik5 StatusNone Ready For Test
    2014-07-07 cbm Carbon-Copy- Added -email is unavailable-
    2013-03-20 rik5 Attached File- Added block1.m, #27640
        Attached File- Added block3.m, #27641
        Attached File- Added block2.m, #27642
        Carbon-Copy- Added jwe
    2013-03-20 rik5 Summaryinvoking script in demo block to define varibles causes errors invoking script in demo block to define variables causes errors
        Carbon-Copy- Added -email is unavailable-
    2013-02-02 cdf Attached File- Added df_vr.m, #27370
        Attached File- Added u_vr.m, #27371

    Back to the top

    Powered by Savane 3.13-cf05.
    Corresponding source code