Re: Crash when clearing "ans" variable in symbol table

[John W. Eaton]

On 14-Jan-2008, Michael Goffioul wrote:

| On 1/14/08, John W. Eaton <address@hidden> wrote:
| > Objects are pushed on the value_stack when functions are called
| > recursively.  My guess is that you've found some way to have a
| > recursive call that doesn't properly trigger the push, but still has a
| > pop of the stack.
| 
| Will you have a look into this? Or could you give me some hints
| where to look (I currently have no clue).
| [Note that this is not my code; I just found the problem while testing
| someone else's code; I have no more clues about the offending code]

Please try the following patch.

I think the problem is that "ans" is being added to the symbol table
in a recursive call, so the size of its value_stack is not equal to
the depth of the recursion.  The following patch addresses this by
removing symbols from the table when their value_stack becomes empty.

I think a simple test case is

  function foo (n)
    if (n > 0)
      n
      ans_exists = exist ("ans", "var")  # should be 0.
      foo (n-1);
      ans_exists = exist ("ans", "var")  # should be 0.
    else
      ans_exists = exist ("ans", "var")  # should be 0.
      eval ("13;");
      ans_exists = exist ("ans", "var")  # should be 1.
    endif
  endfunction

Try running this with something like

  foo (3)

With the patch it should only have ans in the deepest of the
recursive calls to foo.  Without it, Octave should crash.

Hey Sergei, here is a chance for you to help us out by creating some
tests.

Thanks,

jwe


2008-01-14  John W. Eaton  <address@hidden>

        * symtab.h (symbol_table::do_pop_context): Remove symbol_records
        which have no more context.
        (symbol_table::symbol_record::pop_context,
        (symbol_table::symbol_record::symbol_record_rep::pop_context):
        Return size of value_stack, or 1 if variable is persistent or global.


Index: src/symtab.h
===================================================================
RCS file: /cvs/octave/src/symtab.h,v
retrieving revision 1.94
diff -u -u -r1.94 symtab.h
--- src/symtab.h        4 Jan 2008 20:26:26 -0000       1.94
+++ src/symtab.h        14 Jan 2008 21:38:28 -0000
@@ -93,7 +93,11 @@
 
       void push_context (void) { value_stack.push (octave_value ()); }
 
-      void pop_context (void) { value_stack.pop (); }
+      size_t pop_context (void)
+      {
+       value_stack.pop ();
+       return value_stack.size ();
+      }
 
       void clear (void)
       {
@@ -254,10 +258,23 @@
        rep->push_context ();
     }
 
-    void pop_context (void)
+    // If pop_context returns 0, we are out of values and this element
+    // of the symbol table should be deleted.  This can happen for
+    // functions like
+    //
+    //   function foo (n)
+    //     if (n > 0)
+    //       foo (n-1);
+    //     else
+    //       eval ("x = 1");
+    //     endif
+    //   endfunction
+    //
+    // Here, X should only exist in the final stack frame.
+
+    size_t pop_context (void)
     {
-      if (! (is_persistent () || is_global ()))
-       rep->pop_context ();
+      return (is_persistent () || is_global ()) ? 1 : rep->pop_context ();
     }
 
     void clear (void) { rep->clear (); }
@@ -1618,8 +1635,13 @@
 
   void do_pop_context (void)
   {
-    for (table_iterator p = table.begin (); p != table.end (); p++)
-      p->second.pop_context ();
+    for (table_iterator p = table.begin (); p != table.end (); )
+      {
+       if (p->second.pop_context () == 0)
+         table.erase (p++);
+       else
+         p++;
+      }
   }
 
   void do_clear_variables (void)