3212 - bugfix in refcount management

When updating refcounts for a typed segment of memory being copied over
with another, we were only ever using the new copy's data to determine
any tags for exclusive containers. Looks like the right way to do
refcounts is to increment and decrement separately.

Hopefully this is a complete fix for the intermittent but
non-deterministic errors we've been encountering while running the edit/
app.

5 files changed, 99 insertions, 68 deletions

diff --git a/003trace.cc b/003trace.cc
index 23fd8781..10bdca1c 100644
--- a/003trace.cc
+++ b/003trace.cc
@@ -276,6 +276,18 @@ int trace_count(string label, string line) {
   return result;
 }
 
+int trace_count_prefix(string label, string prefix) {
+  if (!Trace_stream) return 0;
+  long result = 0;
+  for (vector<trace_line>::iterator p = Trace_stream->past_lines.begin(); p != Trace_stream->past_lines.end(); ++p) {
+    if (label == p->label) {
+      if (starts_with(trim(p->contents), trim(prefix)))
+        ++result;
+    }
+  }
+  return result;
+}
+
 #define CHECK_TRACE_CONTAINS_ERROR()  CHECK(trace_count("error") > 0)
 #define CHECK_TRACE_DOESNT_CONTAIN_ERROR() \
   if (trace_count("error") > 0) { \
diff --git a/036refcount.cc b/036refcount.cc
index 85a86f21..dca3db90 100644
--- a/036refcount.cc
+++ b/036refcount.cc
@@ -27,30 +27,41 @@ if (Update_refcounts_in_write_memory)
 
 :(code)
 void update_any_refcounts(const reagent& canonized_x, const vector<double>& data) {
+  increment_any_refcounts(canonized_x, data);  // increment first so we don't reclaim on x <- copy x
+  decrement_any_refcounts(canonized_x);
+}
+
+void increment_any_refcounts(const reagent& canonized_x, const vector<double>& data) {
   if (is_mu_address(canonized_x)) {
     assert(scalar(data));
-    assert(canonized_x.value);
     assert(!canonized_x.metadata.size);
-    update_refcounts(canonized_x, data.at(0));
+    increment_refcount(data.at(0));
   }
-  // End Update Refcounts(canonized_x)
+  // End Increment Refcounts(canonized_x)
 }
 
-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));
+void increment_refcount(int new_address) {
+  assert(new_address >= 0);
+  if (new_address == 0) return;
+  int new_refcount = get_or_insert(Memory, new_address);
+  trace(9999, "mem") << "incrementing refcount of " << new_address << ": " << new_refcount << " -> " << new_refcount+1 << end();
+  put(Memory, new_address, new_refcount+1);
 }
 
-void update_refcounts(int old_address, int new_address, const type_tree* payload_type, int /*just in case it's an array*/payload_size) {
-  if (old_address == new_address) {
-    trace(9999, "mem") << "copying address to itself; refcount unchanged" << end();
-    return;
+void decrement_any_refcounts(const reagent& canonized_x) {
+  if (is_mu_address(canonized_x)) {
+    assert(canonized_x.value);
+    assert(!canonized_x.metadata.size);
+    decrement_refcount(get_or_insert(Memory, canonized_x.value), canonized_x.type->right, payload_size(canonized_x));
   }
-  // decrement refcount of old address
+  // End Decrement Refcounts(canonized_x)
+}
+
+void decrement_refcount(int old_address, const type_tree* payload_type, int payload_size) {
   assert(old_address >= 0);
   if (old_address) {
     int old_refcount = get_or_insert(Memory, old_address);
-    trace(9999, "mem") << "decrementing refcount of " << old_address << ": " << old_refcount << " -> " << (old_refcount-1) << end();
+    trace(9999, "mem") << "decrementing refcount of " << old_address << ": " << old_refcount << " -> " << old_refcount-1 << end();
     --old_refcount;
     put(Memory, old_address, old_refcount);
     if (old_refcount < 0) {
@@ -66,13 +77,6 @@ void update_refcounts(int old_address, int new_address, const type_tree* payload
     }
     // End Decrement Refcount(old_address, payload_type, payload_size)
   }
-  // increment refcount of new address
-  if (new_address) {
-    int new_refcount = get_or_insert(Memory, new_address);
-    assert(new_refcount >= 0);  // == 0 only when new_address == old_address
-    trace(9999, "mem") << "incrementing refcount of " << new_address << ": " << new_refcount << " -> " << (new_refcount+1) << end();
-    put(Memory, new_address, new_refcount+1);
-  }
 }
 
 int payload_size(reagent/*copy*/ x) {
@@ -90,7 +94,8 @@ def main [
 +run: {1: ("address" "number")} <- new {number: "type"}
 +mem: incrementing refcount of 1000: 0 -> 1
 +run: {1: ("address" "number")} <- copy {1: ("address" "number")}
-+mem: copying address to itself; refcount unchanged
++mem: incrementing refcount of 1000: 1 -> 2
++mem: decrementing refcount of 1000: 2 -> 1
 
 :(scenario refcounts_call)
 def main [
@@ -175,6 +180,7 @@ def main [
 +mem: incrementing refcount of 1000: 2 -> 3
 
 :(after "Write Memory in Successful MAYBE_CONVERT")
+// TODO: double-check data here as well
 vector<double> data;
 for (int i = 0; i < size_of(product); ++i)
   data.push_back(get_or_insert(Memory, base_address+/*skip tag*/1+i));
@@ -374,21 +380,33 @@ int payload_size(const type_tree* type) {
 //: use metadata.address to update refcounts within containers, arrays and
 //: exclusive containers
 
-:(before "End Update Refcounts(canonized_x)")
-if (is_mu_container(canonized_x) || is_mu_exclusive_container(canonized_x))
-  update_container_refcounts(canonized_x, data);
+:(before "End Increment Refcounts(canonized_x)")
+if (is_mu_container(canonized_x) || is_mu_exclusive_container(canonized_x)) {
+  const container_metadata& metadata = get(Container_metadata, canonized_x.type);
+  for (map<set<tag_condition_info>, set<address_element_info> >::const_iterator p = metadata.address.begin(); p != metadata.address.end(); ++p) {
+    if (!all_match(data, p->first)) continue;
+    for (set<address_element_info>::const_iterator info = p->second.begin(); info != p->second.end(); ++info)
+      increment_refcount(data.at(info->offset));
+  }
+}
 
-:(code)
-void update_container_refcounts(const reagent& canonized_x, const vector<double>& data) {
-  assert(is_mu_container(canonized_x) || is_mu_exclusive_container(canonized_x));
+:(before "End Decrement Refcounts(canonized_x)")
+if (is_mu_container(canonized_x) || is_mu_exclusive_container(canonized_x)) {
+  trace(9999, "mem") << "need to read old value to figure out what refcounts to decrement" << end();
+  // read from canonized_x but without canonizing again
+  // todo: inline without running canonize all over again
+  reagent/*copy*/ tmp = canonized_x;
+  tmp.properties.push_back(pair<string, string_tree*>("raw", NULL));
+  vector<double> data = read_memory(tmp);
   const container_metadata& metadata = get(Container_metadata, canonized_x.type);
   for (map<set<tag_condition_info>, set<address_element_info> >::const_iterator p = metadata.address.begin(); p != metadata.address.end(); ++p) {
     if (!all_match(data, p->first)) continue;
     for (set<address_element_info>::const_iterator info = p->second.begin(); info != p->second.end(); ++info)
-      update_refcounts(get_or_insert(Memory, canonized_x.value + info->offset), data.at(info->offset), info->payload_type, size_of(info->payload_type)+/*refcount*/1);
+      decrement_refcount(get_or_insert(Memory, canonized_x.value + info->offset), info->payload_type, size_of(info->payload_type)+/*refcount*/1);
   }
 }
 
+:(code)
 bool all_match(const vector<double>& data, const set<tag_condition_info>& conditions) {
   for (set<tag_condition_info>::const_iterator p = conditions.begin(); p != conditions.end(); ++p) {
     if (data.at(p->offset) != p->tag)
@@ -592,6 +610,36 @@ def main [
 +run: {2: "foo"} <- merge {3: ("address" "array" "number")}
 +mem: decrementing refcount of 1000: 2 -> 1
 
+:(scenario refcounts_handle_exclusive_containers_with_different_tags)
+container foo1 [
+  x:address:number
+  y:number
+]
+container foo2 [
+  x:number
+  y:address:number
+]
+exclusive-container bar [
+  a:foo1
+  b:foo2
+]
+def main [
+  1:address:number <- copy 12000/unsafe  # pretend allocation
+  *1:address:number <- copy 34
+  2:bar <- merge 0/foo1, 1:address:number, 97
+  5:address:number <- copy 13000/unsafe  # pretend allocation
+  *5:address:number <- copy 35
+  6:bar <- merge 1/foo2, 98, 5:address:number
+  2:bar <- copy 6:bar
+]
++run: {2: "bar"} <- merge {0: "literal", "foo1": ()}, {1: ("address" "number")}, {97: "literal"}
++mem: incrementing refcount of 12000: 1 -> 2
++run: {6: "bar"} <- merge {1: "literal", "foo2": ()}, {98: "literal"}, {5: ("address" "number")}
++mem: incrementing refcount of 13000: 1 -> 2
++run: {2: "bar"} <- copy {6: "bar"}
++mem: incrementing refcount of 13000: 2 -> 3
++mem: decrementing refcount of 12000: 2 -> 1
+
 :(code)
 bool is_mu_container(const reagent& r) {
   return is_mu_container(r.type);
diff --git a/037abandon.cc b/037abandon.cc
index 2c681f7f..014747d7 100644
--- a/037abandon.cc
+++ b/037abandon.cc
@@ -36,27 +36,17 @@ void abandon(int address, const type_tree* payload_type, int payload_size) {
     element.type = copy_array_element(payload_type);
     int array_length = get_or_insert(Memory, address+/*skip refcount*/1);
     assert(element.type->name != "array");
-    if (is_mu_address(element)) {
-      for (element.set_value(address+/*skip refcount*/1+/*skip length*/1); element.value < address+/*skip refcount*/1+/*skip length*/1+array_length; ++element.value)
-        update_refcounts(element, 0);
-    }
-    else if (is_mu_container(element) || is_mu_exclusive_container(element)) {
-      int element_size = size_of(element);
-      vector<double> zeros;
-      zeros.resize(element_size);
-      for (int i = 0; i < array_length; ++i) {
-        element.set_value(address + /*skip refcount*/1 + /*skip array length*/1 + i*element_size);
-        update_container_refcounts(element, zeros);
-      }
+    int element_size = size_of(element);
+    for (int i = 0; i < array_length; ++i) {
+      element.set_value(address + /*skip refcount*/1 + /*skip array length*/1 + i*element_size);
+      decrement_any_refcounts(element);
     }
   }
   else if (is_mu_container(payload_type) || is_mu_exclusive_container(payload_type)) {
     reagent tmp;
-    tmp.set_value(address + /*skip refcount*/1);
     tmp.type = new type_tree(*payload_type);
-    vector<double> zeros;
-    zeros.resize(size_of(payload_type));
-    update_container_refcounts(tmp, zeros);
+    tmp.set_value(address + /*skip refcount*/1);
+    decrement_any_refcounts(tmp);
   }
   // clear memory
   for (int curr = address; curr < address+payload_size; ++curr)
diff --git a/055shape_shifting_container.cc b/055shape_shifting_container.cc
index ce595730..88e9fb97 100644
--- a/055shape_shifting_container.cc
+++ b/055shape_shifting_container.cc
@@ -189,14 +189,19 @@ def main [
   3:foo:point <- merge 0/x, 15, 16
   6:foo:point <- merge 1/y, 23
 ]
++run: {1: ("foo" "number")} <- merge {0: "literal", "x": ()}, {34: "literal"}
 +mem: storing 0 in location 1
 +mem: storing 34 in location 2
++run: {3: ("foo" "point")} <- merge {0: "literal", "x": ()}, {15: "literal"}, {16: "literal"}
 +mem: storing 0 in location 3
 +mem: storing 15 in location 4
 +mem: storing 16 in location 5
++run: {6: ("foo" "point")} <- merge {1: "literal", "y": ()}, {23: "literal"}
 +mem: storing 1 in location 6
 +mem: storing 23 in location 7
-$mem: 7
++run: reply
+# no other stores
+% CHECK(trace_count_prefix("mem", "storing") == 7);
 
 :(scenario get_on_shape_shifting_container)
 container foo:_t [
diff --git a/072scheduler.cc b/072scheduler.cc
index de5f5ca5..0b0dd54f 100644
--- a/072scheduler.cc
+++ b/072scheduler.cc
@@ -262,30 +262,6 @@ increment_any_refcounts(ingredient, ingredients.at(i));
 :(before "End should_update_refcounts_in_write_memory Special-cases For Primitives")
 if (inst.operation == NEXT_INGREDIENT || inst.operation == NEXT_INGREDIENT_WITHOUT_TYPECHECKING)
   return false;
-:(code)
-void increment_any_refcounts(const reagent& x, const vector<double>& data) {
-  if (is_mu_address(x)) {
-    assert(scalar(data));
-    assert(x.value);
-    assert(!x.metadata.size);
-    increment_refcount(data.at(0));
-  }
-  if (is_mu_container(x) || is_mu_exclusive_container(x)) {
-    const container_metadata& metadata = get(Container_metadata, x.type);
-    for (map<set<tag_condition_info>, set<address_element_info> >::const_iterator p = metadata.address.begin(); p != metadata.address.end(); ++p) {
-      if (!all_match(data, p->first)) continue;
-      for (set<address_element_info>::const_iterator info = p->second.begin(); info != p->second.end(); ++info)
-        increment_refcount(data.at(info->offset));
-    }
-  }
-}
-
-void increment_refcount(int address) {
-  if (address == 0) return;
-  int refcount = get_or_insert(Memory, address);
-  trace(9999, "mem") << "incrementing refcount of " << address << ": " << refcount << " -> " << refcount+1 << end();
-  put(Memory, address, refcount+1);
-}
 
 :(scenario start_running_returns_routine_id)
 def f1 [