bugGNU Octave - Bugs: bug #64705, VM variable scope problems when...

 
 

bug #64705: VM variable scope problems when executing scripts

Submitter:  None
Submitted:  Thu 21 Sep 2023 01:18:24 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  None
Status:  Fixed Assigned to:  None
Originator Name:  Originator Email:  -email is unavailable-
Open/Closed:  * Closed 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 13 Oct 2023 07:26:32 PM UTC, comment #16: 

No problems from CI or users. Closing as fixed. If there are problems it can be reopened.

Arun Giridhar <arungiridhar>
Group Member
Sat 07 Oct 2023 04:42:42 PM UTC, comment #15: 

Thanks.

Pushed to https://hg.savannah.gnu.org/hgweb/octave/rev/67945d0339cf and https://hg.savannah.gnu.org/hgweb/octave/rev/262768aefb8b

Works for loading for me.

Marking as ready for test.

Arun Giridhar <arungiridhar>
Group Member
Sat 07 Oct 2023 04:11:48 PM UTC, comment #14: 

I made two patches that seem to solve the issues. It modifies the test case to enumerate every combination
of uncompiled and compiled script, function etc so hopefully everything is covered.


VM Handle mixing of non-bytecode and bytecode scripts (bug #64705)

Also add left-out 'ans' in variable pushing and popping in script frames.

* libinterp/corefcn/stack-frame.cc: Lookup unknown name instead of panicing in scope_flags.
                                    Pretend to be user script frame to make uncompiled scripts
                                    find the correct eval frame.
                                    Skip machine compiled frame with static_link instead of parent link.
* libinterp/octave-value/ov-usr-fcn.cc: Allow calling uncompiled scripts from compiled scripts.
* libinterp/parse-tree/pt-bytecode-walk.cc: Also invite 'ans' id to the script frame
* test/compile/: Tests updated



VM Fix loading and saving from bytecode script frame (bug #64705)

Makes load() and save() not panic in bytecode script frames.

* libinterp/corefcn/stack-frame.cc: Use varref() for scripts in insert_symbol
                                    that got proper support for scripts.
* test/compile/: Tests updated.


(file #55212, file #55213)

Petter <petter>
Sat 07 Oct 2023 12:45:55 AM UTC, comment #13: 

Thanks for the tests. I think I might have been able to fix the issue. I'll clean up the patch at post it here. There is a small issue left.

Essentially it is a mix of improper handling of calling uncompiled scripts from bytecode scripts exposed in this case by scripts in e.g. run('fooscript') or eval('fooscript') not getting compiled due to a "not really implemented yet check" in ov-usr-fcn.cc. The 'ans' problem is a trivial just-not-added-to-list thing. The load/save problem is due to insert_symbol() not being up to date to the changes supporting scripts.

Script and functions could be enabled separately somehow as a workaround, but hopefully scripts should work eventually. Getting the script frames to work properly seems to be quite complex, since they can change each other in so many ways compared to functions.

Petter <petter>
Fri 06 Oct 2023 04:33:06 PM UTC, comment #12: 

The source() information complicates it a bit. I had been thinking that since run.m calls evalin() and calling the script just by name goes to the interpreter directly (as in not through evalin()), the problem might be limited to calling evalin() from the VM. But source() is already in the parse-tree code, so the problem likely isn't with evalin() in the VM, but in scope contexts for variables.

IIUC when executing scripts, the VM creates a fictitious function that wraps the script in a new context / scope, then when execution is finished it copies those (local) variables back to the scope of the calling location (usually the top level scope, but not necessarily), and deletes the fictitious function that it created at the start. When executing scripts without the VM, that kind of two-level scope and copying isn't done, and all script-local variables are directly in the calling scope (almost always the top level scope), if I've understood the scope correctly. Evidently this difference is causing some scope-related errors like "non-existent variable foo".

@Petter: Do you think it'll help if the VM can be enabled for functions and scripts independently of each other?

Arun Giridhar <arungiridhar>
Group Member
Fri 06 Oct 2023 03:31:01 PM UTC, comment #11: 

@Arun-

thanks. Interesting and nice catch.

Originally I had used the "source" command where the error occurs.
 
Then I switched to the "run" command but no difference. Same error and I posted my comment.
 

But you are right: just invoking the script by using its name seems to work and does not cause the error to occur.

(These facts should likely help the developer to find bug).



Anonymous
Fri 06 Oct 2023 02:18:55 PM UTC, comment #10: 

@Anonymous in comment #6: Changing the "run" statement makes it work for me with and without the VM:

$ cat I_Use_Globals.m
function ret = I_Use_Globals()
  Snippet_Globalvars     ## <==== This line no longer uses "run".
  disp(x1);
  ret = x1
endfunction


$ cat Snippet_Globalvars.m
global x1;
global x2;
global x3;
global x4;


Not sure why calling run() exposes some problems.

Re the other problem I listed in comment #9, removing the line that causes the panic gives some more information:

octave:2> testme
vector::_M_range_check: __n (which is 29) >= this->size() (which is 29)
VM error 7483: Exception in testme escaped the VM
error: VM error 7484: Exception in testme escaped the VM

That is followed by a segfault on exit.

The test code change was this:

diff -r 2c0dc2ac3d3b libinterp/corefcn/stack-frame.cc
--- a/libinterp/corefcn/stack-frame.cc  Thu Oct 05 17:14:53 2023 -0400
+++ b/libinterp/corefcn/stack-frame.cc  Fri Oct 06 10:08:58 2023 -0400
@@ -356,9 +356,6 @@ public:

   void internal_resize (std::size_t arg)
   {
-    // Not ok to resize for scripts. Extra slots should be in the eval frame.
-    CHECK_PANIC (!m_fcn->is_user_script ());
-
     int diff = static_cast<int> (arg) - static_cast<int>(internal_size ());

     if (diff > 0)


From what I gather, there's a difference between "script scope" and "top level scope", and some of the variables are not making the transition properly when the VM is active.

Arun Giridhar <arungiridhar>
Group Member
Fri 06 Oct 2023 02:03:37 AM UTC, comment #9: 

Loading a file with the VM enabled now causes Octave to panic.

First create a data file. It doesn't make a difference whether VM is enabled for this step.

octave:1> varfoo = 123
varfoo = 123
octave:2> varbar = "blahblah"
varbar = blahblah
octave:3> save -binary data.mat
octave:4> exit


Create a script testme.m with this content. The aim is that it would load a data file and clear the variables it won't be using, as a prelude to more processing. In this case we will clear varbar:

clear
load data.mat
clear varbar
whos


Start Octave and run testme. With the VM enabled, this causes a crash just on loading the file. (My VM is enabled in my octaverc file so all my routine code tests the VM).

octave:1> testme
panic: VM internal error at stack-frame.cc:360, !m_fcn->is_user_script ()
fatal: caught signal Aborted -- stopping myself...
Aborted (core dumped)


With the VM disabled, it works properly:

octave:1> __vm_enable__ (0)
octave:2> testme
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
         varfoo      1x1                          8  double

Total is 1 element using 8 bytes


A quick bisection shows that e0a6dd4f6fef works properly, so the problem is with the following two patches (either this bug report or https://hg.savannah.gnu.org/hgweb/octave/rev/ff0859c6f361 for bug #64691)

o  changeset:   32371:ff0859c6f361
|  user:        Petter T. <petter.vilhelm@gmail.com>
|  date:        Wed Sep 20 22:00:45 2023 +0200
|  summary:     VM Freshen up auxilliary dev functions
|
o  changeset:   32370:9155a67023bc
|  user:        Petter T.
|  date:        Fri Sep 22 15:46:36 2023 +0200
|  summary:     VM Support globals in scripts properly (bug #64705)


Arun Giridhar <arungiridhar>
Group Member
Thu 05 Oct 2023 02:17:52 PM UTC, comment #8: 

On testing some more, the problem of comment #7 is restricted to ans in the top level scope not being cleared by scripts calling clear, and not other variables.

On the plus side, calling clear all inside scripts could give a varval error before this patch, but this patch fixes that bug.

Arun Giridhar <arungiridhar>
Group Member
Thu 05 Oct 2023 02:08:38 PM UTC, comment #7: 

I noticed another problem related to this patch. Calling clear or clear all from inside a script no longer clears variables present in the top level scope, including ans.

Marking as in progress.

Arun Giridhar <arungiridhar>
Group Member
Thu 05 Oct 2023 05:01:27 AM UTC, comment #6: 

Does not work fully and status should be reverted.

Not working use case:

The global variables are defined in a script. Actually the script is just a code snippet, which conveniently defines all global variables.
Via this code snippet the global variables can be accessed in all functions using these global variables.


Snippet_GlobalVars.m

global x1;
global x2;
global x3;
global x4;


Example function accessing these globals

function ret = I_Use_Globals()
  run ("Snippet_GlobalVars.m");
  disp(x1);
  ret = x1
endfunction


then testing

__vm_enable__(0);
run ("Snippet_GlobalVars.m");
x1 = 1;
I_Use_Globals();

works find and displays x1 = 1.


__vm_enable__(1);
run ("Snippet_GlobalVars.m");
x1 = 1;
I_Use_Globals();

results in

error: VM internal error: Trying to make unnamed symbol global
error: called from
    Snippet_GlobalVars at line 3 column 1
    run at line 93 column 5
    I_Use_Globals at line 2 column 3




Note: When instead of using the code snippet the global variables are declared directly, it works without error.

Anonymous
Wed 04 Oct 2023 09:23:44 PM UTC, comment #5: 
Arun Giridhar <arungiridhar>
Group Member
Sat 30 Sep 2023 12:35:39 AM UTC, comment #4: 

I've made a patch to address this issue, as well as some related. E.g. you couldn't define variables in eval:s in script called by script very well.
It required some rework of how bytecode script frames push and pop values from the "eval frame".


VM Support globals in scripts properly (bug #64705)

Fix globalization and deglobalization in scripts with the same top
scope.

* test/compile/bytecode.tst: Update tests
* test/compile/module.mk: Add new files
* test/compile/bytecode_script_topscope_setup.m: New
* test/compile/bytecode_script_topscope_assert.m: New
* test/compile/bytecode_script_topscope.m: New

* libinterp/corefcn/compile.cc: Support clearing scripts with __vm_compile__
* libinterp/corefcn/stack-frame.cc: Reorganize bytecode dynamic
                                    stackframe to support scripts.
* libinterp/octave-value/ov-ref.cc: New function to notify globalness change
* libinterp/octave-value/ov-ref.h:
* libinterp/octave-value/ov-usr-fcn.cc: Limit script vm execution to top or vm frames
* libinterp/octave-value/ov.h: base_value_stack_frame friend to access is_ref
* libinterp/parse-tree/pt-bytecode-vm.cc: Remove inaccurate comment
* libinterp/parse-tree/pt-bytecode-walk.cc: Add function name to scope in
                                            cleaner way. Set m_n_orig_scope_size.
* libinterp/parse-tree/pt-bytecode.h: Field m_n_orig_scope_size for original
                                      amount of symbols in scope


(file #55177)

Petter <petter>
Sun 24 Sep 2023 10:33:07 PM UTC, comment #3: 

This report should still be an issue after the other patches. I have been fiddling with a fix for this one, but it felt like a bandaid rather than a proper fix so it might need some more work.

Petter <petter>
Sun 24 Sep 2023 02:49:42 PM UTC, comment #2: 

@OP: There have been many recent patches for the VM. To ensure that your report is new and not already addressed, please mention the hg id of your Octave installation. This is also displayed with the command "ver" inside Octave.

Arun Giridhar <arungiridhar>
Group Member
Sun 24 Sep 2023 02:59:53 AM UTC, comment #1: 

Add on:

The VM throws an error when checking the status of a global variable in a script:

"error: VM internal error: Invalid call to get_scope_flag_internal"



file test4.m
------------------------
+/-

if (isglobal("x"))
  printf("x is global.\n");
else
  printf("x is NOT global.\n");
endif

+/-
----------------------------




+/-

_enable_vm_eval_ (0);
global x;

_enable_vm_eval_ (1);
source test4.m

+/-

--> "error: VM internal error: Invalid call to get_scope_flag_internal"

Anonymous
Thu 21 Sep 2023 01:18:24 PM UTC, original submission:  

An VM machine executed script clears a pre-declared global variable when being (re) declared as global within the executed script.
 

file test4.m
----------------
global x;
----------------


1) manually executed commands to declare x as global and set a value:

_enable_vm_eval_ (0);
global x;
x = 1;
display(x); whos

-->

x = 1
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
     g   x           1x1                          8  double


2) executing the script
source test4.m
disp(x); whos

-->
1
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
     g   x           1x1                          8  double

3) executing the script using the VM
_enable_vm_eval_ (1);
source test4.m
disp(x); whos

-->
[](0x0)
Variables visible from the current scope:

variables in scope: top scope

  Attr   Name        Size                     Bytes  Class
  ====   ====        ====                     =====  =====
     g   x           0x0                          0  double




Anonymous

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #55212:  octave_32487.patch added by petter (19KiB - text/plain)
file #55213:  octave_32488.patch added by petter (7KiB - text/plain)
file #55177:  octave_32477.patch added by petter (62KiB - 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 jwe (Updated the item)
  • -email is unavailable- added by petter (Posted a comment)
  • -email is unavailable- added by arungiridhar (Posted a comment)
  •  

    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 14 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2023-10-13 arungiridhar StatusReady For Test Fixed
        Open/ClosedOpen Closed
        Fixed ReleaseNone 9.1.0
    2023-10-09 jwe CategoryJIT Acceleration Interpreter
    2023-10-07 arungiridhar StatusIn Progress Ready For Test
    2023-10-07 petter Attached File- Added octave_32487.patch, #55212
        Attached File- Added octave_32488.patch, #55213
    2023-10-06 arungiridhar SummaryVM machine executed script clear the content of a global pre-declared global variable VM variable scope problems when executing scripts
    2023-10-05 arungiridhar StatusReady For Test In Progress
    2023-10-04 arungiridhar CategoryNone JIT Acceleration
        StatusNone Ready For Test
        Operating SystemSolaris/SunOS Any
        Planned ReleaseNone 9.1.0
    2023-09-30 petter Attached File- Added octave_32477.patch, #55177

    Back to the top

    Powered by Savane 3.13-0159.
    Corresponding source code