bugGNU Octave - Bugs: bug #35772, Nested Function Support

 
 

bug #35772: Nested Function Support

Submitter:  Max Brister <fisheater>
Submitted:  Fri 09 Mar 2012 01:42:12 PM UTC
   
 
Category:  Interpreter Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Feature Request
Status:  Fixed Assigned to:  None
Originator Name:  Max Brister 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

Sat 31 Mar 2012 06:48:07 AM UTC, comment #14: 

jwe,

I have created a new version of the patch, nest3.patch, as requested. While creating the changelog I removed some pointless edits as well as fixing the expected error in a test case.

Thanks,
Max Brister

(file #25523)

Max Brister <fisheater>
Fri 30 Mar 2012 09:53:35 PM UTC, comment #13: 

Max,

Could you please take the new version of the patch that I've attached and add ChangeLog info to the commit message and resubmit it?

Thanks.

(file #25522)

John W. Eaton <jwe>
Group administrator
Sun 18 Mar 2012 07:23:02 AM UTC, comment #12: 

clear all is now bug 35881. segmentation fault wasn't reported - there seem to be quite a lot of them already, I wasn't sure without an extensive look if it won't be a duplicate.

Alon

Alon Levy <alon>
Sun 18 Mar 2012 07:11:45 AM UTC, comment #11: 

Jordi, Max,

 Thanks for pointing that out, I'll open separate bug reports.

Alon

Alon Levy <alon>
Sun 18 Mar 2012 01:00:16 AM UTC, comment #10: 

I'm just saying that UB is UB and Max's patch is simply bringing out the UB in another part of the code.

Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Sun 18 Mar 2012 12:53:20 AM UTC, comment #9: 

Jordi,

This also happens on octave 3.6.1, and 14479:05b59be209ed without nest2.patch. I do not think this issue has anything to do with the nest2.patch. valgrind also shows this happening in a different place as Alon's issue. Maybe this belongs in another bug report? It does look quite bad.

Alon,
It looks like you have two related issues.

1) The crash
I am able to reproduce your crash on revision 05b59be209ed and octave 3.6.1. As nest2.patch has not been applied to either of these, this seems to be some underlying interpreter issue. Interestingly, there is no crash (for me at least) if I place the code in a file before executing.

2) Incorrect behavior when clear all is called
When looking into the bug further it appears that this is an issue with subfunctions as well. For example,

function bad_clear
clear all;
f;
function f

produces

error: `f' undefined near line 3 column 1
error: called from:
error:   /home/max/projects/octave/bad_clear.m at line 3, column 1

when executed in octave 3.6.1 and revision 05b59be209ed. This leads me to believe that the bug is a related bug having to do with executing clear all when subfunctions exist. Fixing this problem for subfunctions should also fix the issue for nested functions.

I am willing to look into both of these issues, but I think they belong in a separate bug report.

Max

Max Brister <fisheater>
Sat 17 Mar 2012 11:24:30 PM UTC, comment #8: 

At revision 14479:05b59be209ed, when running


./run-octave -valgrind -q --eval 'sin(2)'


I get what looks like relevant output:


ans =  0.90930
==13302== Invalid read of size 4
==13302==    at 0x78FE360: fileno (fileno.c:37)
==13302==    by 0x5595E28: octave__init_buffer(yy_buffer_state*, _IO_FILE*) (lex.cc:3470)
==13302==    by 0x5595CF8: octave__create_buffer(_IO_FILE*, int) (lex.cc:3419)
==13302==    by 0x5595A6B: octave_restart(_IO_FILE*) (lex.cc:3344)
==13302==    by 0x55968FF: reset_parser() (lex.ll:1154)
==13302==    by 0x5596FB8: cleanup_parser() (lex.ll:1434)
==13302==    by 0x56E2E45: do_octave_atexit() (toplev.cc:1069)
==13302==    by 0x56E06BB: clean_up_and_exit(int) (toplev.cc:670)
==13302==    by 0x5691E0B: octave_main (octave.cc:892)
==13302==    by 0x400738: main (main.c:35)
==13302==  Address 0x10d14070 is 0 bytes inside a block of size 568 free'd
==13302==    at 0x4C248FE: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13302==    by 0x78FAA1C: fclose@@GLIBC_2.2.5 (iofclose.c:88)
==13302==    by 0x71DAD89: rpl_fclose (fclose.c:100)
==13302==    by 0x561C5EC: safe_fclose(_IO_FILE*) (oct-parse.yy:3253)
==13302==    by 0x562A18A: unwind_protect::fcn_arg_elem<_IO_FILE*>::run() (unwind-prot.h:87)
==13302==    by 0x5392756: unwind_protect::run_top() (unwind-prot.h:260)
==13302==    by 0x53927A7: unwind_protect::run() (unwind-prot.h:289)
==13302==    by 0x53927D3: unwind_protect::~unwind_protect() (unwind-prot.h:303)
==13302==    by 0x561D30B: parse_fcn_file(std::string const&, std::string const&, bool, bool, std::string const&) (oct-parse.yy:3600)
==13302==    by 0x561E3B0: load_fcn_from_file(std::string const&, std::string const&, std::string const&, std::string const&, bool) (oct-parse.yy:3772)
==13302==    by 0x56CE11E: symbol_table::fcn_info::fcn_info_rep::find_user_function() (symtab.cc:1009)
==13302==    by 0x56CD3E5: symbol_table::fcn_info::fcn_info_rep::xfind(octave_value_list const&, bool) (symtab.cc:782)
==13302==


Jordi GutiƩrrez Hermoso <jordigh>
Group Member
Sat 17 Mar 2012 10:21:42 PM UTC, comment #7: 

Hi Max,

 I've tested your patch, I found two issues so far:

1. clear all removes the nested functions. Example that fails with unresolved function sub:
function f();
# 1
function sub();
endfunction
# 2
clear all
sub() # fails, unresolved symbol
endfunction

f()
error: `sub' undefined near line 5 column 1
error: called from:
error:   f at line 5, column 1

2. segmentation fault with an almost identical code to the above (just moving clear all from #2 to #1):

function f();
# 1
clear all
function sub();
endfunction
# 2
sub() # fails, unresolved symbol
endfunction

f()


#0  0x0000000000000041 in ?? ()
#1  0x00007fd910b0c1a1 in tree_evaluator::visit_function_def (this=<optimized out>, cmd=...) at ../../src/pt-eval.cc:515
#2  0x00007fd910b0b525 in tree_evaluator::visit_statement (this=0x7fd911224bf8, stmt=...) at ../../src/pt-eval.cc:706
#3  0x00007fd910b0ad0e in visit_statement_list (lst=..., this=0x7fd911224bf8) at ../../src/pt-eval.cc:772
#4  tree_evaluator::visit_statement_list (this=0x7fd911224bf8, lst=...) at ../../src/pt-eval.cc:753
#5  0x00007fd910a92013 in octave_user_function::do_multi_index_op (this=0x1104e00, nargout=0, args=..., lvalue_list=<optimized out>) at ../../src/ov-usr-fcn.cc:479
#6  0x00007fd910a8f1ff in octave_user_function::subsref (this=0x1104e00, type="(", idx=Python Exception <type 'exceptions.IndexError'> list index out of range:
std::list, nargout=0, lvalue_list=lvalue_list@entry=0x0) at ../../src/ov-usr-fcn.cc:329
#7  0x00007fd910a8f3cc in octave_user_function::subsref (this=<optimized out>, type=..., idx=..., nargout=<optimized out>) at ../../src/ov-usr-fcn.cc:312
#8  0x00007fd910a9c638 in octave_value::subsref (this=<optimized out>, type=..., idx=..., nargout=<optimized out>) at ../../src/ov.cc:1203
#9  0x00007fd910a9c6a5 in octave_value::subsref (this=<optimized out>, type=..., idx=..., nargout=<optimized out>, lvalue_list=<optimized out>) at ../../src/ov.cc:1214
#10 0x00007fd910b132eb in tree_index_expression::rvalue (this=<optimized out>, nargout=0, lvalue_list=lvalue_list@entry=0x0) at ../../src/pt-idx.cc:414
#11 0x00007fd910b13a3b in tree_index_expression::rvalue (this=<optimized out>, nargout=<optimized out>) at ../../src/pt-idx.cc:284
#12 0x00007fd910b114b8 in tree_index_expression::rvalue1 (this=0x11026a0, nargout=0) at ../../src/pt-idx.cc:425
#13 0x00007fd910b0b5f4 in tree_evaluator::visit_statement (this=0x7fd911224bf8, stmt=...) at ../../src/pt-eval.cc:736
#14 0x00007fd910b0ad0e in visit_statement_list (lst=..., this=0x7fd911224bf8) at ../../src/pt-eval.cc:772
#15 tree_evaluator::visit_statement_list (this=0x7fd911224bf8, lst=...) at ../../src/pt-eval.cc:753
#16 0x00007fd9109aed34 in main_loop () at ../../src/toplev.cc:595
#17 0x00007fd91095689f in octave_main (argc=1, argv=<optimized out>, embedded=0) at ../../src/octave.cc:943
#18 0x00000039c2421735 in __libc_start_main (main=0x400750 <main>, argc=1, ubp_av=0x7fff71c7b338, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_e
nd=0x7fff71c7b328) at libc-start.c:226
#19 0x0000000000400781 in _start ()

I think this backtrace is almost useless but I need to rebuild with -O0 to have any parameters. It's just showing a fault during evaluation of the user function.

Other then that I do need nested function support :(

Thanks for all the great work,
Alon

Alon Levy <alon>
Sat 17 Mar 2012 09:15:03 PM UTC, comment #6: 

The revised nested function patch has been completed. It should be much more stable, however it has the following issues:
- handles to nested functions are not supported.
- errors are not properly issued if a nested child declares a nonlocal variable as global or persistent. (should not cause a crash though, and existing matlab code should not be affected by this)

I will post on the mailing list explaining the reasons behind the approach I used.

Alon,
The bug you found is probably due to the nested function patch. It is amazing how many interesting errors can be caused by changing the core interpreter functionality.

Max

(file #25387)

Max Brister <fisheater>
Sat 17 Mar 2012 01:46:28 AM UTC, comment #5: 

Great.

I just found another bug but not really related, although it could be at the same code area, just thought you might be interested but really I'm writing it so as to not forget :)

If you run the following line multiple times:
 - with mercurial head plus your old patch it fails after about 5 times with memory corruption (glibc double linked list corrupted, stack trace not interesting so I won't post, also this is the wrong bug to post it on..)
 - with 3.6.1 (fedora latest on fedora 17) it doesn't fail immediately even after 40 invocations, but fails when I quit the interpreter, also with double linked list corruption
 - on both occasions the failure is inside get_cwd, which is why I keep repeating it probably has nothing to do with nested functions.

The magic line to repeat to get a crash immediately or on quiting the interpreter:

function f(); function t(x); x=5; disp(x); endfunction; x=7; t(7); endfunction; f()

(no EOLs in there, just a single line)

Happy Hacking,
Alon

Alon Levy <alon>
Sat 17 Mar 2012 01:29:25 AM UTC, comment #4: 

Yes, I forgot to take into account formal parameters correctly in my initial patch. I now have test cases for this (as well as several other things I forgot to take into account previously). I should be done with the revised patch either tonight or early tomorrow morning.

Max

Max Brister <fisheater>
Sat 17 Mar 2012 01:24:51 AM UTC, comment #3: 

OK, great to know. So you might want to use the following test, your first patch fails it: (fails the first, second is same just using a rename to pinpoint the error - it seems the nested f redefined x, and in your do_update_nested I think you unmark x as nonlocal, but forget to mark it back as normal (or maybe the unmark is not being called), thereby causing it to be unresolved in the parent function (just a theory, didn't check yet - and if your revised patch would be available in a day or two I will patiently wait :)

octave:1> function test(x); function f(x); endfunction; f(x); disp(x); endfunction
octave:2> test(11)
error: `x' undefined near line 1 column 58
error: evaluating argument list element number 1
error: called from:
error:   test at line 1, column 53
octave:2> function test(x); function f(y); endfunction; f(x); disp(x); endfunction
octave:3> test(11)
 11

Alon Levy <alon>
Sat 17 Mar 2012 01:19:21 AM UTC, comment #2: 

Alon,

I am currently working on a revised patch. The revised patch takes a slightly different implementation approach, and should be done soon. The revised patch should also fix several formatting/stylistic issues.

Max

Max Brister <fisheater>
Sat 17 Mar 2012 12:55:54 AM UTC, comment #1: 

Hi Max,

 Thanks very much for writing this, I'm testing it right now. Some whitespace got into your patch in finish_function, maybe you'd like to know:

+         else
+           {         <<<<<< white space
+             symbol_table::install_subfunction (nm, octave_value (fcn),
+                                                primary_fcn_scope);

Alon

Alon Levy <alon>
Fri 09 Mar 2012 01:42:12 PM UTC, original submission:  

Currently octave coerces nested functions into subfunctions. I have developed a patch which should fix this.

Max Brister <fisheater>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #25523:  nest3.patch added by fisheater (43KiB - text/x-patch)
file #25522:  nested-functions-patch.txt added by jwe (38KiB - text/plain)
file #25387:  nest2.patch added by fisheater (40KiB - text/x-patch)
file #25307:  nest.patch added by fisheater (22KiB - text/x-patch)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Updated the item)
  • -email is unavailable- added by jwe (Updated the item)
  • -email is unavailable- added by jordigh (Posted a comment)
  • -email is unavailable- added by alon (Posted a comment)
  • -email is unavailable- added by fisheater (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 8 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2012-04-11 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2012-03-31 fisheater Attached File- Added nest3.patch, #25523
    2012-03-30 jwe Attached File- Added nested-functions-patch.txt, #25522
    2012-03-18 rik5 Carbon-CopyRemoved 72865 -
    2012-03-17 fisheater Attached File- Added nest2.patch, #25387
    2012-03-09 rik5 StatusNone Patch Submitted
    2012-03-09 fisheater Attached File- Added nest.patch, #25307

    Back to the top

    Powered by Savane 3.13-02a9.
    Corresponding source code