From dcaecff513dc48fe4d997da7d6c6a2d96701fb6e Mon Sep 17 00:00:00 2001 From: "Kartik K. Agaram" Date: Mon, 11 Sep 2017 06:48:32 -0700 Subject: 3993 Fully isolate routines from their arguments. I still need exceptions for containers that are *designed* to be shared between routines. The primary such case is channels; we need some way to share them between routines, and if we deep-copy them that defeats their entire purpose. A milder case is the use of fake file-systems in tests, though that's a hint that there'll be more of these as the OS gets more fleshed out. The pattern seems to be that we need to not deep-copy containers that contain lock fields, and so their operations internally do their own locking. We may have to stop hard-coding the list of exceptions and allow people to define new ones. Perhaps don't deep-copy any container with metadata of 'shared', and then ensure that get-location is only ever called on shared containers. This still isn't absolutely ironclad. People can now store something into a channel and then pass it into a routine to share arbitrary data. But perhaps the goal isn't to be ironclad, just to avoid easy mistakes. I'd still want an automated check for this, though. Some way to highlight it as an unsafe pattern. This completes step 1 in the plan of commit 3992 for making continuations safe. --- 071deep_copy.cc | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++------- 073scheduler.cc | 25 ++++------------ 2 files changed, 84 insertions(+), 30 deletions(-) diff --git a/071deep_copy.cc b/071deep_copy.cc index cc3a3424..d403200b 100644 --- a/071deep_copy.cc +++ b/071deep_copy.cc @@ -210,27 +210,33 @@ case DEEP_COPY: { } :(before "End Primitive Recipe Implementations") case DEEP_COPY: { - const reagent& input = current_instruction().ingredients.at(0); + products.push_back(deep_copy(current_instruction().ingredients.at(0))); + break; +} + +:(code) +vector deep_copy(const reagent& in) { // allocate a tiny bit of temporary space for deep_copy() trace(9991, "run") << "deep-copy: allocating space for temporary" << end(); reagent tmp("tmp:address:number"); tmp.set_value(allocate(1)); - products.push_back(deep_copy(input, tmp)); + map addresses_copied; + vector result = deep_copy(in, addresses_copied, tmp); // reclaim Mu memory allocated for tmp trace(9991, "run") << "deep-copy: reclaiming temporary" << end(); abandon(tmp.value, payload_type(tmp.type), payload_size(tmp)); // reclaim host memory allocated for tmp.type when tmp goes out of scope - break; -} - -:(code) -vector deep_copy(const reagent& in, const reagent& tmp) { - map addresses_copied; - return deep_copy(in, addresses_copied, tmp); + return result; } vector deep_copy(reagent/*copy*/ in, map& addresses_copied, const reagent& tmp) { canonize(in); + if (is_mu_address(in)) { + // hack: skip primitive containers that do their own locking; they're designed to be shared between routines + type_tree* payload = in.type->right; + if (payload->left->name == "channel" || payload->left->name == "resources") + return read_memory(in); + } vector result; if (is_mu_address(in)) result.push_back(deep_copy_address(in, addresses_copied, tmp)); @@ -280,6 +286,11 @@ void deep_copy(const reagent& canonized_in, map& addresses_copied, con for (map, set >::const_iterator p = metadata.address.begin(); p != metadata.address.end(); ++p) { if (!all_match(data, p->first)) continue; for (set::const_iterator info = p->second.begin(); info != p->second.end(); ++info) { + // hack: skip primitive containers that do their own locking; they're designed to be shared between routines + if (!info->payload_type->atom && info->payload_type->left->name == "channel") + continue; + if (info->payload_type->atom && info->payload_type->name == "resources") + continue; // construct a fake reagent that reads directly from the appropriate // field of the container reagent curr; @@ -301,8 +312,6 @@ int payload_address(reagent/*copy*/ x) { return x.value; } -//: moar tests, just because I can't believe it all works - :(scenario deep_copy_stress_test_1) container foo1 [ p:&:num @@ -388,3 +397,61 @@ def main [ +mem: storing 1 in location 2 # but it's a completely different (disjoint) cycle +mem: storing 0 in location 3 + +:(scenario deep_copy_skips_channel) +# ugly! dummy 'channel' type if we happen to be building without that layer +% if (!contains_key(Type_ordinal, "channel")) get_or_insert(Type, put(Type_ordinal, "channel", Next_type_ordinal++)).name = "channel"; +def main [ + local-scope + x:&:channel:num <- new {(channel num): type} + y:&:channel:num <- deep-copy x + 10:bool/raw <- equal x, y +] +# channels are never deep-copied ++mem: storing 1 in location 10 + +:(scenario deep_copy_skips_nested_channel) +# ugly! dummy 'channel' type if we happen to be building without that layer +% if (!contains_key(Type_ordinal, "channel")) get_or_insert(Type, put(Type_ordinal, "channel", Next_type_ordinal++)).name = "channel"; +container foo [ + c:&:channel:num +] +def main [ + local-scope + x:&:foo <- new foo:type + y:&:foo <- deep-copy x + xc:&:channel:num <- get *x, c:offset + yc:&:channel:num <- get *y, c:offset + 10:bool/raw <- equal xc, yc +] +# channels are never deep-copied ++mem: storing 1 in location 10 + +:(scenario deep_copy_skips_resources) +# ugly! dummy 'resources' type if we happen to be building without that layer +% if (!contains_key(Type_ordinal, "resources")) get_or_insert(Type, put(Type_ordinal, "resources", Next_type_ordinal++)).name = "resources"; +def main [ + local-scope + x:&:resources <- new resources:type + y:&:resources <- deep-copy x + 10:bool/raw <- equal x, y +] +# resources are never deep-copied ++mem: storing 1 in location 10 + +:(scenario deep_copy_skips_nested_resources) +# ugly! dummy 'resources' type if we happen to be building without that layer +% if (!contains_key(Type_ordinal, "resources")) get_or_insert(Type, put(Type_ordinal, "resources", Next_type_ordinal++)).name = "resources"; +container foo [ + c:&:resources +] +def main [ + local-scope + x:&:foo <- new foo:type + y:&:foo <- deep-copy x + xc:&:resources <- get *x, c:offset + yc:&:resources <- get *y, c:offset + 10:bool/raw <- equal xc, yc +] +# resources are never deep-copied ++mem: storing 1 in location 10 diff --git a/073scheduler.cc b/073scheduler.cc index 962ac915..fb1a3f51 100644 --- a/073scheduler.cc +++ b/073scheduler.cc @@ -179,9 +179,10 @@ case START_RUNNING: { new_routine->parent_index = Current_routine_index; // populate ingredients for (int i = /*skip callee*/1; i < SIZE(current_instruction().ingredients); ++i) { - new_routine->calls.front().ingredient_atoms.push_back(ingredients.at(i)); reagent/*copy*/ ingredient = current_instruction().ingredients.at(i); new_routine->calls.front().ingredients.push_back(ingredient); + vector new_ingredient_atoms = deep_copy(ingredient); + new_routine->calls.front().ingredient_atoms.push_back(new_ingredient_atoms); // End Populate start-running Ingredient } Routines.push_back(new_routine); @@ -252,7 +253,7 @@ def f2 n:&:num [ :(before "End is_indirect_call_with_ingredients Special-cases") if (r == START_RUNNING) return true; -//: more complex: refcounting management when starting up new routines +//: refcounting management when starting up new routines :(scenario start_running_immediately_updates_refcounts_of_ingredients) % Scheduling_interval = 1; @@ -275,24 +276,10 @@ def new-routine n:&:num [ load-ingredients 1:num/raw <- copy *n ] -# check that n wasn't reclaimed when create-new-routine returned +# check that n was successfully passed into new-routine before being reclaimed +mem: storing 34 in location 1 -//: to support the previous scenario we'll increment refcounts for all call -//: ingredients right at call time, and stop incrementing refcounts inside -//: next-ingredient -:(before "End Populate Call Ingredient") -increment_any_refcounts(ingredient, ingredients.at(i)); -:(before "End Populate start-running Ingredient") -increment_any_refcounts(ingredient, ingredients.at(i)); -:(after "should_update_refcounts() Special-cases When Writing Products Of Primitive Instructions") -if (inst.operation == NEXT_INGREDIENT || inst.operation == NEXT_INGREDIENT_WITHOUT_TYPECHECKING) { - if (space_index(inst.products.at(0)) > 0) return true; - if (has_property(inst.products.at(0), "raw")) return true; - return false; -} - -// ensure this works with indirect calls using 'call' as well +//: ensure this works with indirect calls using 'call' as well :(scenario start_running_immediately_updates_refcounts_of_ingredients_of_indirect_calls) % Scheduling_interval = 1; def main [ @@ -306,7 +293,7 @@ def f1 n:&:num [ local-scope load-ingredients ] -# check that n wasn't reclaimed when f1 returned +# check that n was successfully passed into new-routine before being reclaimed +mem: storing 34 in location 1 :(scenario next_ingredient_never_leaks_refcounts) -- cgit 1.4.1-2-gfad0