4089

Clean up how we reclaim local scopes.

It used to work like this (commit 3216):

  1. Update refcounts of products after every instruction, EXCEPT:

      a) when instruction is a non-primitive and the callee starts with
      'local-scope' (because it's already not decremented in 'return')

    OR:

      b) when instruction is primitive 'next-ingredient' or
      'next-ingredient-without-typechecking', and its result is saved to a
      variable in the default space (because it's already incremented at
      the time of the call)

  2. If a function starts with 'local-scope', force it to be reclaimed
  before each return. However, since locals may be returned, *very
  carefully* don't reclaim those. (See the logic in the old `escaping`
  and `should_update_refcount` functions.)

However, this approach had issues. We needed two separate commands for
'local-scope' (reclaim locals on exit) and 'new-default-space'
(programmer takes charge of reclaiming locals). The hard-coded
reclamation duplicated refcounting logic. In addition to adding
complexity, this implementation failed to work if a function overwrites
default-space after setting up a local-scope (the old default-space is
leaked). It also fails in the presence of continuations. Calling a
continuation more than once was guaranteed to corrupt memory (commit
3986).

After this commit, reclaiming local scopes now works like this:

  Update refcounts of products for every PRIMITIVE instruction.
  For non-primitive instructions, all the work happens in the `return`
  instruction:
    increment refcount of ingredients to `return`
      (unless -- one last bit of ugliness -- they aren't saved in the
      caller)
    decrement the refcount of the default-space
      use existing infrastructure for reclaiming as necessary
      if reclaiming default-space, first decrement refcount of each
      local
        again, use existing infrastructure for reclaiming as necessary

This commit (finally!) completes the bulk[1] of step 2 of the plan in
commit 3991. It was very hard until I gave up trying to tweak the
existing implementation and just test-drove layer 43 from scratch.

[1] There's still potential for memory corruption if we abuse
`default-space`. I should probably try to add warnings about that at
some point (todo in layer 45).

1 files changed, 3 insertions, 5 deletions

diff --git a/020run.cc b/020run.cc
index c4df88bd..d1bc77f4 100644
--- a/020run.cc
+++ b/020run.cc
@@ -95,15 +95,15 @@ void run_current_routine() {
     }
     //: used by a later layer
     if (write_products) {
-      Writing_products_of_instruction = true;
       if (SIZE(products) < SIZE(current_instruction().products)) {
         raise << SIZE(products) << " vs " << SIZE(current_instruction().products) << ": failed to write to all products in '" << to_original_string(current_instruction()) << "'\n" << end();
       }
       else {
-        for (int i = 0;  i < SIZE(current_instruction().products);  ++i)
+        for (int i = 0;  i < SIZE(current_instruction().products);  ++i) {
+          // Writing Instruction Product(i)
           write_memory(current_instruction().products.at(i), products.at(i));
+        }
       }
-      Writing_products_of_instruction = false;
     }
     // End Running One Instruction
     if (fall_through_to_next_instruction)
@@ -111,8 +111,6 @@ void run_current_routine() {
   }
   stop_running_current_routine:;
 }
-:(before "End Globals")
-bool Writing_products_of_instruction = false;
 
 :(code)
 //: hook replaced in a later layer