2973 - reclaim refcounts for local scopes again

More thorough redo of commit 2767 (Mar 12), which was undone in commit
2810 (Mar 24). It's been a long slog. Next step: write a bunch of mu
code in the edit/ app in search of bugs.

5 files changed, 167 insertions, 46 deletions

diff --git a/020run.cc b/020run.cc
index f0e78b80..35eba974 100644
--- a/020run.cc
+++ b/020run.cc
@@ -90,9 +90,8 @@ void run_current_routine()
       raise << SIZE(products) << " vs " << SIZE(current_instruction().products) << ": failed to write to all products! " << to_original_string(current_instruction()) << '\n' << end();
     }
     else {
-      for (int i = 0; i < SIZE(current_instruction().products); ++i) {
-        write_memory(current_instruction().products.at(i), products.at(i));
-      }
+      for (int i = 0; i < SIZE(current_instruction().products); ++i)
+        write_memory(current_instruction().products.at(i), products.at(i), i);
     }
     // End of Instruction
     finish_instruction:;
@@ -267,7 +266,7 @@ vector<double> read_memory(reagent/*copy*/ x) {
   return result;
 }
 
-void write_memory(reagent/*copy*/ x, const vector<double>& data) {
+void write_memory(reagent/*copy*/ x, const vector<double>& data, const int /*only when called in the run loop above to save results; -1 otherwise*/ product_index) {
   // Begin Preprocess write_memory(x, data)
   if (!x.type) {
     raise << "can't write to " << to_string(x) << "; no type\n" << end();
diff --git a/036refcount.cc b/036refcount.cc
index ad87b089..53ccbd91 100644
--- a/036refcount.cc
+++ b/036refcount.cc
@@ -18,14 +18,22 @@ def main [
 +mem: decrementing refcount of 1000: 1 -> 0
 
 :(before "End write_memory(x) Special-cases")
-if (is_mu_address(x)) {
-  assert(scalar(data));
-  assert(x.value);
-  assert(!x.metadata.size);
-  update_refcounts(x, data.at(0));
+if (should_update_refcounts_in_write_memory(product_index)) {
+  if (is_mu_address(x)) {
+    assert(scalar(data));
+    assert(x.value);
+    assert(!x.metadata.size);
+    update_refcounts(x, data.at(0));
+  }
+  // End Update Refcounts in write_memory(x)
 }
 
 :(code)
+//: hook for a later layer
+bool should_update_refcounts_in_write_memory(int product_index) {
+  return true;
+}
+
 void update_refcounts(const reagent& old, int new_address) {
   assert(is_mu_address(old));
   update_refcounts(get_or_insert(Memory, old.value), new_address, old.type->right, payload_size(old));
@@ -323,7 +331,7 @@ int payload_size(const type_tree* type) {
 //: use metadata.address to update refcounts within containers, arrays and
 //: exclusive containers
 
-:(before "End write_memory(x) Special-cases")
+:(before "End Update Refcounts in write_memory(x)")
 if (is_mu_container(x) || is_mu_exclusive_container(x))
   update_container_refcounts(x, data);
 :(before "End Update Refcounts in PUT")
diff --git a/043space.cc b/043space.cc
index 094cb827..fe5b0a2f 100644
--- a/043space.cc
+++ b/043space.cc
@@ -192,8 +192,8 @@ if (x.name == "number-of-locals") {
   return;
 }
 
-//:: a little hook to automatically reclaim the default-space when returning
-//:: from a recipe
+//:: 'local-scope' is like 'new-default-space' except that we'll reclaim the
+//:: default-space when the routine exits
 
 :(scenario local_scope)
 def main [
@@ -209,13 +209,17 @@ def foo [
 # both calls to foo should have received the same default-space
 +mem: storing 1 in location 3
 
-:(code)  // pending test
-//? :(scenario local_scope_frees_up_allocations)
-//? def main [
-//?   local-scope
-//?   x:address:array:character <- new [abc]
-//? ]
-//? +mem: clearing x:address:array:character
+:(scenario local_scope_frees_up_addresses)
+def main [
+  local-scope
+  x:address:array:character <- new [abc]
+]
++mem: clearing x:address:array:character
+
+:(before "End Rewrite Instruction(curr, recipe result)")
+if (curr.name == "local-scope") {
+  rewrite_default_space_instruction(curr);
+}
 
 //: todo: do this in a transform, rather than magically in the reply instruction
 :(after "Falling Through End Of Recipe")
@@ -223,13 +227,6 @@ try_reclaim_locals();
 :(after "Starting Reply")
 try_reclaim_locals();
 
-//: now 'local-scope' is identical to 'new-default-space' except that we'll
-//: reclaim the default-space when the routine exits
-:(before "End Rewrite Instruction(curr, recipe result)")
-if (curr.name == "local-scope") {
-  rewrite_default_space_instruction(curr);
-}
-
 :(code)
 void try_reclaim_locals() {
   // only reclaim routines starting with 'local-scope'
@@ -239,40 +236,69 @@ void try_reclaim_locals() {
   const instruction& inst = exiting_recipe.steps.at(0);
   if (inst.old_name != "local-scope") return;
   // reclaim any local variables unless they're being returned
-  // TODO: this isn't working yet. Doesn't handle address stored in
-  // containers created by 'copy' or 'merge'. We'd end up deleting the address
-  // even if some container containing it was returned.
-  // This might doom our whole refcounting-based approach :/
-//?   vector<double> zero;
-//?   zero.push_back(0);
-//?   for (int i = /*leave default space for last*/1; i < SIZE(exiting_recipe.steps); ++i) {
-//?     const instruction& inst = exiting_recipe.steps.at(i);
-//?     for (int i = 0; i < SIZE(inst.products); ++i) {
-//?       if (!is_mu_address(inst.products.at(i))) continue;
-//?       // local variables only
-//?       if (has_property(inst.products.at(i), "space")) continue;
-//?       if (has_property(inst.products.at(i), "lookup")) continue;
-//?       if (escaping(inst.products.at(i))) continue;
-//?       trace(9999, "mem") << "clearing " << inst.products.at(i).original_string << end();
-//?       write_memory(inst.products.at(i), zero);
-//?     }
-//?   }
+  vector<double> zeros;
+  for (int i = /*leave default space for last*/1; i < SIZE(exiting_recipe.steps); ++i) {
+    const instruction& inst = exiting_recipe.steps.at(i);
+    for (int i = 0; i < SIZE(inst.products); ++i) {
+      // local variables only
+      if (has_property(inst.products.at(i), "space")) continue;
+      if (has_property(inst.products.at(i), "lookup")) continue;
+      if (has_property(inst.products.at(i), "raw")) continue;  // tests often want to check such locations after they run
+      if (escaping(inst.products.at(i))) continue;
+      trace(9999, "mem") << "clearing " << inst.products.at(i).original_string << end();
+      zeros.resize(size_of(inst.products.at(i)));
+      write_memory(inst.products.at(i), zeros, /*always update refcounts*/-1);
+    }
+  }
+  trace(9999, "mem") << "automatically abandoning " << current_call().default_space << end();
   abandon(current_call().default_space,
           inst.products.at(0).type->right,
           /*refcount*/1 + /*array length*/1 + /*number-of-locals*/Name[r][""]);
 }
 
+//: since we don't decrement refcounts for escaping values above, make sure we
+//: don't increment them when the caller saves them either
+
+:(replace{} "bool should_update_refcounts_in_write_memory(int product_index)")
+bool should_update_refcounts_in_write_memory(int product_index) {
+  assert(Current_routine);  // run-time only
+  if (product_index == -1) return true;
+  assert(product_index >= 0);
+  const instruction& inst = current_instruction();
+  if (inst.operation < MAX_PRIMITIVE_RECIPES) return true;
+  if (!contains_key(Recipe, inst.operation)) return true;
+  const recipe& caller = get(Recipe, inst.operation);
+  if (caller.steps.empty()) return true;
+  // if the recipe deosn't begin with 'local-scope', always update refcounts
+  return caller.steps.at(0).old_name != "local-scope";
+}
+
+:(code)
 // is this reagent one of the values returned by the current (reply) instruction?
+// is the corresponding ingredient saved in the caller?
 bool escaping(const reagent& r) {
+  assert(Current_routine);  // run-time only
   // nothing escapes when you fall through past end of recipe
   if (current_step_index() >= SIZE(Current_routine->steps())) return false;
   for (long long i = 0; i < SIZE(current_instruction().ingredients); ++i) {
-    if (r == current_instruction().ingredients.at(i))
+    if (r == current_instruction().ingredients.at(i)) {
+    if (caller_uses_product(i))
       return true;
+    }
   }
   return false;
 }
 
+bool caller_uses_product(int product_index) {
+  assert(Current_routine);  // run-time only
+  assert(!Current_routine->calls.empty());
+  if (Current_routine->calls.size() == 1) return false;
+  const call& caller = *++Current_routine->calls.begin();
+  const instruction& caller_inst = to_instruction(caller);
+  if (product_index >= SIZE(caller_inst.products)) return false;
+  return !is_dummy(caller_inst.products.at(product_index));
+}
+
 void rewrite_default_space_instruction(instruction& curr) {
   if (!curr.ingredients.empty())
     raise << to_original_string(curr) << " can't take any ingredients\n" << end();
@@ -284,6 +310,92 @@ void rewrite_default_space_instruction(instruction& curr) {
   curr.products.push_back(reagent("default-space:address:array:location"));
 }
 
+:(scenario local_scope_ignores_nonlocal_spaces)
+def new-scope [
+  new-default-space
+  x:address:number <- new number:type
+  *x <- copy 34
+  return default-space:address:array:location
+]
+def use-scope [
+  local-scope
+  outer:address:array:location <- next-ingredient
+  0:address:array:location/names:new-scope <- copy outer
+  return *x:address:number/space:1
+]
+def main [
+  1:address:array:location/raw <- new-scope
+  2:number/raw <- use-scope 1:address:array:location/raw
+]
++mem: storing 34 in location 2
+
+:(scenario local_scope_frees_up_addresses_inside_containers)
+container foo [
+  x:number
+  y:address:number
+]
+def main [
+  local-scope
+  x:address:number <- new number:type
+  y:foo <- merge 34, x
+  # x and y are both cleared when main returns
+]
++mem: clearing x:address:number
++mem: decrementing refcount of 1006: 2 -> 1
++mem: clearing y:foo
++mem: decrementing refcount of 1006: 1 -> 0
++mem: automatically abandoning 1006
+
+:(scenario local_scope_returns_addresses_inside_containers)
+container foo [
+  x:number
+  y:address:number
+]
+def f [
+  local-scope
+  x:address:number <- new number:type
+  *x <- copy 12
+  y:foo <- merge 34, x
+  # since y is 'escaping' f, it should not be cleared
+  return y
+]
+def main [
+  1:foo <- f
+  3:number <- get 1:foo, x:offset
+  4:address:number <- get 1:foo, y:offset
+  5:number <- copy *4:address:number
+  1:foo <- put 1:foo, y:offset, 0
+  4:address:number <- copy 0
+]
++mem: storing 34 in location 1
++mem: storing 1006 in location 2
++mem: storing 34 in location 3
+# refcount of 1:foo shouldn't include any stray ones from f
++run: {4: ("address" "number")} <- get {1: "foo"}, {y: "offset"}
++mem: incrementing refcount of 1006: 1 -> 2
+# 1:foo wasn't abandoned/cleared
++run: {5: "number"} <- copy {4: ("address" "number"), "lookup": ()}
++mem: storing 12 in location 5
++run: {1: "foo"} <- put {1: "foo"}, {y: "offset"}, {0: "literal"}
++mem: decrementing refcount of 1006: 2 -> 1
++run: {4: ("address" "number")} <- copy {0: "literal"}
++mem: decrementing refcount of 1006: 1 -> 0
++mem: automatically abandoning 1006
+
+:(scenario local_scope_claims_return_values_when_not_saved)
+def f [
+  local-scope
+  x:address:number <- new number:type
+  reply x:address:number
+]
+def main [
+  f  # doesn't save result
+]
+# x reclaimed
++mem: automatically abandoning 1004
+# f's local scope reclaimed
++mem: automatically abandoning 1000
+
 //:: all recipes must set default-space one way or another
 
 :(before "End Globals")
diff --git a/085scenario_console.cc b/085scenario_console.cc
index e97f67f1..a6b3dfa3 100644
--- a/085scenario_console.cc
+++ b/085scenario_console.cc
@@ -121,6 +121,8 @@ case ASSUME_CONSOLE: {
   put(Memory, console_address+/*skip refcount*/1+/*offset of 'data' in container 'events'*/1, event_data_address);
   // increment refcount for event data
   put(Memory, event_data_address, 1);
+  // increment refcount for console
+  put(Memory, console_address, 1);
   break;
 }
 
diff --git a/091run_interactive.cc b/091run_interactive.cc
index 7ebc2e16..a28dd7e0 100644
--- a/091run_interactive.cc
+++ b/091run_interactive.cc
@@ -85,7 +85,7 @@ bool run_interactive(int address) {
   Current_routine = NULL;
   // call run(string) but without the scheduling
   load(string("recipe! interactive [\n") +
-          "local-scope\n" +
+          "new-default-space\n" +  // disable automatic abandon so tests can see changes
           "screen:address:screen <- next-ingredient\n" +
           "$start-tracking-products\n" +
           command + "\n" +