diff --git a/src/Expr.cc b/src/Expr.cc index 02517b4286..8bc3b46e2b 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4339,11 +4339,7 @@ Val* LambdaExpr::Eval(Frame* f) const lamb->AddClosure(outer_ids, f); - ingredients->id->SetVal((new Val(lamb))->Ref()); - ingredients->id->SetConst(); - ingredients->id->ID_Val()->AsFunc()->SetScope(ingredients->scope); - - return ingredients->id->ID_Val(); + return (new Val(lamb)); } void LambdaExpr::ExprDescribe(ODesc* d) const diff --git a/src/Expr.h b/src/Expr.h index 27ed70ade6..8535a7a3d2 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -943,7 +943,7 @@ public: protected: friend class Expr; - CallExpr() { func = 0; args = 0; } + CallExpr() { func = 0; args = 0; } void ExprDescribe(ODesc* d) const override; @@ -951,16 +951,16 @@ protected: ListExpr* args; }; -/* - Class to handle the creation of anonymous functions with closures. - Facts: - - LambdaExpr creates a new BroFunc on every call to Eval. - - LambdaExpr must be given all the information to create a BroFunc on - construction except for the closure. - - The closure for created BroFuncs is the frame that the LambdaExpr is - evaluated in. -*/ +// Class to handle the creation of anonymous functions with closures. + +// Facts: +// - LambdaExpr creates a new BroFunc on every call to Eval. +// - LambdaExpr must be given all the information to create a BroFunc on +// construction except for the closure. +// - The closure for created BroFuncs is the frame that the LambdaExpr is +// evaluated in. + class LambdaExpr : public Expr { public: LambdaExpr(std::unique_ptr ingredients, diff --git a/src/Frame.cc b/src/Frame.cc index de110210dd..11cd188b42 100644 --- a/src/Frame.cc +++ b/src/Frame.cc @@ -8,6 +8,7 @@ #include "Trigger.h" #include +#include // std::any_of vector g_frame_stack; @@ -26,36 +27,44 @@ Frame::Frame(int arg_size, const BroFunc* func, const val_list* fn_args) call = 0; delayed = false; + is_view = false; + Clear(); } -Frame::Frame(const Frame* other) +Frame::Frame(const Frame* other, bool view) { - this->size = other->size; - this->frame = other->frame; - this->function = other->function; - this->func_args = other->func_args; + is_view = view; - this->next_stmt = 0; - this->break_before_next_stmt = false; - this->break_on_return = false; - this->delayed = false; + if ( is_view ) + { + size = other->size; + frame = other->frame; - if ( other->trigger ) - Ref(other->trigger); + function = other->function; + func_args = other->func_args; - for (int i = 0; i < size; i++) - if (frame[i]) - Ref(frame[i]); - - this->trigger = other->trigger; - this->call = other->call; + next_stmt = 0; + break_before_next_stmt = false; + + break_on_return = false; + delayed = false; + + trigger = other->trigger; + call = other->call; + } + else + reporter->InternalError("TODO: make Frame copy constructor."); } Frame::~Frame() { - Unref(trigger); - Release(); + // Deleting a Frame that is a view is a no-op. + if ( ! is_view ) + { + Unref(trigger); + Release(); + } } void Frame::SetElement(int n, Val* v) @@ -93,7 +102,7 @@ void Frame::Release() for ( int i = 0; i < size; ++i ) Unref(frame[i]); - delete [] frame; + delete [] frame; } void Frame::Describe(ODesc* d) const @@ -171,15 +180,13 @@ void Frame::ClearTrigger() } ClosureFrame::ClosureFrame(Frame* closure, Frame* not_closure, - std::shared_ptr outer_ids) : Frame(not_closure) + std::shared_ptr outer_ids) : Frame(not_closure, true) { assert(closure); this->closure = closure; - Ref(this->closure); - this->body = not_closure; - Ref(this->body); - + body = not_closure; + // To clone a ClosureFrame we null outer_ids and then copy // the set over directly, hence the check. if (outer_ids) @@ -190,20 +197,22 @@ ClosureFrame::ClosureFrame(Frame* closure, Frame* not_closure, { ID* id = (*tmp)[i]; if (id) - this->closure_elements.insert(id->Name()); + closure_elements.push_back(id->Name()); } } } ClosureFrame::~ClosureFrame() { - Unref(this->closure); - Unref(this->body); + // No need to Unref the closure. BroFunc handles this. + // Unref body though. When the ClosureFrame is done, so is + // the frame that is is wrapped around. + Unref(body); } Val* ClosureFrame::GetElement(ID* id) const { - if (this->closure_elements.find(id->Name()) != this->closure_elements.end()) + if ( ClosureContains(id) ) return ClosureFrame::GatherFromClosure(this, id); return this->NthElement(id->Offset()); @@ -211,7 +220,7 @@ Val* ClosureFrame::GetElement(ID* id) const void ClosureFrame::SetElement(const ID* id, Val* v) { - if (this->closure_elements.find(id->Name()) != this->closure_elements.end()) + if ( ClosureContains(id) ) ClosureFrame::SetInClosure(this, id, v); else this->Frame::SetElement(id->Offset(), v); @@ -219,11 +228,11 @@ void ClosureFrame::SetElement(const ID* id, Val* v) Frame* ClosureFrame::Clone() { - Frame* new_closure = this->closure->Clone(); - Frame* new_regular = this->body->Clone(); + Frame* new_closure = closure->Clone(); + Frame* new_regular = body->Clone(); ClosureFrame* cf = new ClosureFrame(new_closure, new_regular, nullptr); - cf->closure_elements = this->closure_elements; + cf->closure_elements = closure_elements; return cf; } @@ -236,7 +245,7 @@ Frame* ClosureFrame::SelectiveClone(id_list* choose) loop_over_list(*choose, i) { ID* we = (*choose)[i]; - if (closure_contains(we)) + if ( ClosureContains(we) ) us.append(we); else them.append(we); @@ -247,7 +256,7 @@ Frame* ClosureFrame::SelectiveClone(id_list* choose) Frame* you = this->body->SelectiveClone(&them); ClosureFrame* who = new ClosureFrame(me, you, nullptr); - who->closure_elements = this->closure_elements; + who->closure_elements = closure_elements; return who; } @@ -268,7 +277,7 @@ Val* ClosureFrame::GatherFromClosure(const Frame* start, const ID* id) if ( ! conductor ) return start->NthElement(id->Offset()); - if (conductor->closure_contains(id)) + if (conductor->ClosureContains(id)) return ClosureFrame::GatherFromClosure(conductor->closure, id); return conductor->NthElement(id->Offset()); @@ -281,9 +290,16 @@ void ClosureFrame::SetInClosure(Frame* start, const ID* id, Val* val) if ( ! conductor ) start->SetElement(id->Offset(), val); - else if (conductor->closure_contains(id)) + else if (conductor->ClosureContains(id)) ClosureFrame::SetInClosure(conductor->closure, id, val); else conductor->Frame::SetElement(id->Offset(), val); } + +bool ClosureFrame::ClosureContains(const ID* i) const + { + const char* target = i->Name(); + return std::any_of(closure_elements.begin(), closure_elements.end(), + [target](const char* in) { return strcmp(target, in) == 0; }); + } diff --git a/src/Frame.h b/src/Frame.h index 5767a1ebeb..37ed7437ba 100644 --- a/src/Frame.h +++ b/src/Frame.h @@ -4,8 +4,7 @@ #define frame_h #include -#include -#include +#include // std::shared_ptr #include "Val.h" @@ -20,7 +19,7 @@ class Frame : public BroObj { public: Frame(int size, const BroFunc* func, const val_list *fn_args); // The constructed frame becomes a view of the input frame. No copying is done. - Frame(const Frame* other); + Frame(const Frame* other, bool is_view = false); ~Frame() override; Val* NthElement(int n) const { return frame[n]; } @@ -88,22 +87,27 @@ protected: Trigger* trigger; const CallExpr* call; bool delayed; + + // For ClosureFrames, we don't want a Frame as much as we want a frame that + // is a view to another underlying one. Rather or not a Frame is a view + // impacts how the Frame handles deleting itself. + bool is_view; }; -/* -Class that allows for lookups in both a closure frame and a regular frame -according to a list of IDs passed into the constructor. -Facts: - - A ClosureFrame is created from two frames: a closure and a regular frame. - - ALL operations except GetElement operations operate on the regular frame. - - A ClosureFrame requires a list of outside ID's captured by the closure. - - Get operations on those IDs will be performed on the closure frame. +// Class that allows for lookups in both a closure frame and a regular frame +// according to a list of outer IDs passed into the constructor. + +// Facts: +// - A ClosureFrame is created from two frames: a closure and a regular frame. +// - ALL operations except Get/SetElement operations operate on the regular frame. +// - A ClosureFrame requires a list of outside ID's captured by the closure. +// - Get/Set operations on those IDs will be performed on the closure frame. + +// ClosureFrame allows functions that generate functions to be passed between +// different sized frames and still properly capture their closures. It also allows for +// cleaner handling of closures. -ClosureFrame allows functions that generate functions to be passed between -different sized frames and still properly capture their closures. It also allows for -cleaner handling of closures. -*/ class ClosureFrame : public Frame { public: ClosureFrame(Frame* closure, Frame* body, @@ -118,35 +122,16 @@ private: Frame* closure; Frame* body; - // Searches this frame and all sub-frame's closures for a value corresponding - // to the id. + // Searches the start frame and all sub-frame's closures for a value corresponding + // to the id. Returns it when its found. Will fail with little grace if the value + // does not actually exist in any of the sub-frames. static Val* GatherFromClosure(const Frame* start, const ID* id); // Moves through the closure frames and associates val with id. static void SetInClosure(Frame* start, const ID* id, Val* val); - bool closure_contains(const ID* i) const - { return this->closure_elements.find(i->Name()) != this->closure_elements.end(); } + bool ClosureContains(const ID* i) const; - // Hashes c style strings. The strings need to be null-terminated. - struct const_char_hasher { - size_t operator()(const char* in) const - { - // http://www.cse.yorku.ca/~oz/hash.html - size_t h = 5381; - int c; - - while ((c = *in++)) - h = ((h << 5) + h) + c; - - return h; - } - }; - - // NOTE: In a perfect world this would be best done with a trie or bloom - // filter. We only need to check if things are NOT in the closure. - // In reality though the size of a closure is small enough that operatons are - // fairly quick anyway. - std::unordered_set closure_elements; + std::vector closure_elements; }; extern vector g_frame_stack; diff --git a/src/Func.cc b/src/Func.cc index 57bc8a6b04..47bbd155b4 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -127,13 +127,11 @@ void Func::AddBody(Stmt* /* new_body */, id_list* /* new_inits */, } -Val* Func::DoClone() +Func* Func::DoClone() { // By default, ok just to return a reference. Func does not have any "state". // That is different across instances. - Val* v = new Val(this); - Ref(v); - return v; + return this; } void Func::DescribeDebug(ODesc* d, const val_list* args) const @@ -198,6 +196,21 @@ TraversalCode Func::Traverse(TraversalCallback* cb) const HANDLE_TC_STMT_POST(tc); } +void Func::CopyStateInto(Func* other) const + { + std::for_each(bodies.begin(), bodies.end(), [](const Body& b) { Ref(b.stmts); }); + other->bodies = bodies; + + other->scope = scope; + other->kind = kind; + + Ref(type); + other->type = type; + + other->name = name; + other->unique_id = unique_id; + } + std::pair Func::HandlePluginResult(std::pair plugin_result, val_list* args, function_flavor flavor) const { // Helper function factoring out this code from BroFunc:Call() for @@ -271,19 +284,15 @@ BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits, BroFunc::~BroFunc() { - for ( unsigned int i = 0; i < bodies.size(); ++i ) - Unref(bodies[i].stmts); + std::for_each(bodies.begin(), bodies.end(), [](Body& b) { Unref(b.stmts); }); - Unref(this->closure); + Unref(closure); } int BroFunc::IsPure() const { - for ( unsigned int i = 0; i < bodies.size(); ++i ) - if ( ! bodies[i].stmts->IsPure() ) - return 0; - - return 1; + return std::all_of(bodies.begin(), bodies.end(), + [](const Body& b) { return b.stmts->IsPure(); }); } Val* BroFunc::Call(val_list* args, Frame* parent) const @@ -318,7 +327,7 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const } Frame* f = new Frame(frame_size, this, args); - if (this->closure) + if ( closure ) { assert(outer_ids); f = new ClosureFrame(this->closure, f, this->outer_ids); @@ -483,16 +492,16 @@ void BroFunc::AddClosure(std::shared_ptr ids, Frame* f) void BroFunc::SetClosureFrame(Frame* f) { - if (this->closure) - reporter->InternalError - ("Tried to override closure for BroFunc %s.", this->Name()); + if ( closure ) + reporter->InternalError + ("Tried to override closure for BroFunc %s.", this->Name()); - this->closure = f ? f->SelectiveClone(this->outer_ids.get()) : nullptr; + closure = f ? f->SelectiveClone( outer_ids.get() ) : nullptr; } -Val* BroFunc::DoClone() +Func* BroFunc::DoClone() { - // A BroFunc could hold a closure. In this case a clone of it must copy this + // A BroFunc could hold a closure. In this case a clone of it must // store a copy of this closure. if ( ! this->closure ) { @@ -502,18 +511,17 @@ Val* BroFunc::DoClone() { BroFunc* other = new BroFunc(); - other->bodies = this->bodies; - other->scope = this->scope; - other->kind = this->kind; - other->type = this->type; - other->name = this->name; - other->unique_id = this->unique_id; - other->unique_ids = this->unique_ids; - other->frame_size = this->frame_size; - other->closure = this->closure->Clone(); - other->outer_ids = this->outer_ids; + CopyStateInto(other); - return new Val(other); + other->frame_size = frame_size; + + if (closure) + { + other->closure = closure->Clone(); + other->outer_ids = outer_ids; + } + + return other; } } @@ -536,8 +544,7 @@ Stmt* BroFunc::AddInits(Stmt* body, id_list* inits) return body; StmtList* stmt_series = new StmtList; - InitStmt* first = new InitStmt(inits); - stmt_series->Stmts().append(first); + stmt_series->Stmts().append(new InitStmt(inits)); stmt_series->Stmts().append(body); return stmt_series; diff --git a/src/Func.h b/src/Func.h index 97815d94dc..45fcd84279 100644 --- a/src/Func.h +++ b/src/Func.h @@ -20,7 +20,7 @@ class Frame; class ID; class CallExpr; -struct CloneState; +// struct CloneState; class Func : public BroObj { public: @@ -64,7 +64,7 @@ public: void Describe(ODesc* d) const override = 0; virtual void DescribeDebug(ODesc* d, const val_list* args) const; - virtual Val* DoClone(); + virtual Func* DoClone(); virtual TraversalCode Traverse(TraversalCallback* cb) const; @@ -74,6 +74,8 @@ public: protected: Func(); + // Copies this functions state into other. + void CopyStateInto(Func* other) const; // Helper function for handling result of plugin hook. std::pair HandlePluginResult(std::pair plugin_result, val_list* args, function_flavor flavor) const; @@ -100,9 +102,7 @@ public: void AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size, int priority) override; - void fsets(); - - Val* DoClone() override; + Func* DoClone() override; int FrameSize() const { return frame_size; } @@ -115,9 +115,6 @@ protected: int frame_size; private: - // Shifts the offsets of each id in "idl" by "shift". - static void ShiftOffsets(int shift, std::shared_ptr idl); - // Makes a deep copy of the input frame and captures it. void SetClosureFrame(Frame* f); @@ -129,6 +126,7 @@ private: std::shared_ptr outer_ids = nullptr; // The frame the Func was initialized in. This is not guaranteed to be // initialized and should be handled with care. + // A BroFunc Unrefs its closure on deletion. Frame* closure = nullptr; }; diff --git a/src/Val.cc b/src/Val.cc index 719b30bcf4..52beee8ed8 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -105,8 +105,7 @@ Val* Val::DoClone(CloneState* state) // Derived classes are responsible for this. Exception: // Functions and files. There aren't any derived classes. if ( type->Tag() == TYPE_FUNC ) - // Immutable. - return this->AsFunc()->DoClone(); + return new Val(this->AsFunc()->DoClone()); if ( type->Tag() == TYPE_FILE ) { diff --git a/src/Var.cc b/src/Var.cc index ab628d7936..c125b6a3e3 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -477,8 +477,8 @@ int get_func_priotity(attr_list* attrs) void end_func(Stmt* body) { - std::unique_ptr ingredients = - gather_function_ingredients(body); + std::unique_ptr ingredients = gather_function_ingredients(body); + pop_scope(); if ( streq(ingredients->id->Name(), "anonymous-function") ) @@ -505,6 +505,7 @@ void end_func(Stmt* body) ingredients->inits, ingredients->frame_size, ingredients->priority); + ingredients->id->SetVal(new Val(f)); ingredients->id->SetConst(); } @@ -516,7 +517,8 @@ void end_func(Stmt* body) // function and collects it into a function_ingredients struct. std::unique_ptr gather_function_ingredients(Stmt* body) { - std::unique_ptr ingredients (new function_ingredients); + std::unique_ptr ingredients = build_unique(); + ingredients->frame_size = current_scope()->Length(); ingredients->inits = current_scope()->GetInits(); diff --git a/src/parse.y b/src/parse.y index 0a2fe7fef6..5a867f1c6e 100644 --- a/src/parse.y +++ b/src/parse.y @@ -1216,12 +1216,8 @@ func_body: ; anonymous_function: - TOK_FUNCTION func_params - { - $$ = current_scope()->GenerateTemporary("lambda-function"); - begin_func($$, current_module.c_str(), FUNC_FLAVOR_FUNCTION, 0, $2); - } - + TOK_FUNCTION begin_func + '{' { saved_in_init.push_back(in_init); @@ -1229,6 +1225,7 @@ anonymous_function: } stmt_list + { in_init = saved_in_init.back(); saved_in_init.pop_back(); @@ -1239,9 +1236,9 @@ anonymous_function: // Every time a new LambdaExpr is evaluated it must return a new instance // of a BroFunc. Here, we collect the ingredients for a function and give // it to our LambdaExpr. - std::unique_ptr ingredients = - gather_function_ingredients($6); - std::shared_ptr outer_ids = gather_outer_ids(pop_scope(), $6); + std::unique_ptr ingredients = gather_function_ingredients($5); + + std::shared_ptr outer_ids = gather_outer_ids(pop_scope(), $5); $$ = new LambdaExpr(std::move(ingredients), std::move(outer_ids)); } diff --git a/testing/btest/language/function-closures.zeek b/testing/btest/language/function-closures.zeek index 71d2bdb04d..e1c9027bc5 100644 --- a/testing/btest/language/function-closures.zeek +++ b/testing/btest/language/function-closures.zeek @@ -34,7 +34,7 @@ event zeek_init() local cat_dog = 100; local add_n_and_m = function(n: count) : function(m : count) : function(o : count) : count { - cat_dog += 1; # segfault here. + cat_dog += 1; return function(m : count) : function(o : count) : count { return function(o : count) : count { return n + m + o + cat_dog; }; }; @@ -76,6 +76,7 @@ event zeek_init() print "expect: 225"; print twotwofive(15); + local hamster : count = 3; const modes: table[count] of string = { [1] = "symmetric active", [2] = "symmetric passive", @@ -84,9 +85,9 @@ event zeek_init() [5] = "broadcast server", [6] = "broadcast client", [7] = "reserved", - } &default=function(i: count):string { return fmt("unknown-%d", i); } &redef; - - + } &default=function(i: count):string { return fmt("unknown-%d. outside-%d", i, hamster); } &redef; + # TODO: update parsing - will break. + # print modes[11]; }