Make ClosureFrame safe & cleanup

TODO: make anonymous-funcs associated with tables capture closures,
implement copy constructor for Frame, & other cleanup.

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());
+	return (new Val(lamb));
-	ingredients->id->SetConst();
-	ingredients->id->ID_Val()->AsFunc()->SetScope(ingredients->scope);
-
-	return ingredients->id->ID_Val();
 	}
 
 void LambdaExpr::ExprDescribe(ODesc* d) const

src/Expr.h

@@ -951,16 +951,16 @@ protected:
 	ListExpr* args;
 };
 
-/*
-	Class to handle the creation of anonymous functions with closures.
 
-	Facts:
+// Class to handle the creation of anonymous functions with closures.
-		- LambdaExpr creates a new BroFunc on every call to Eval.
+
-		- LambdaExpr must be given all the information to create a BroFunc on
+// Facts:
-		  construction except for the closure.
+//     - LambdaExpr creates a new BroFunc on every call to Eval.
-		- The closure for created BroFuncs is the frame that the LambdaExpr is
+//     - LambdaExpr must be given all the information to create a BroFunc on
-		  evaluated in.
+// 	 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<function_ingredients> ingredients,

src/Frame.cc

@@ -8,6 +8,7 @@
 #include "Trigger.h"
 
 #include <string>
+#include <algorithm> // std::any_of
 
 vector<Frame*> g_frame_stack;
 
@@ -26,37 +27,45 @@ 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;
+	is_view = view;
-	this->frame = other->frame;
-	this->function = other->function;
-	this->func_args = other->func_args;
 
-	this->next_stmt = 0;
+	if ( is_view )
-	this->break_before_next_stmt = false;
+	  {
-	this->break_on_return = false;
+	  size = other->size;
-	this->delayed = false;
+	  frame = other->frame;
 
-	if ( other->trigger )
+	  function = other->function;
-		Ref(other->trigger);
+	  func_args = other->func_args;
 
-	for (int i = 0; i < size; i++)
+	  next_stmt = 0;
-	  if (frame[i])
+	  break_before_next_stmt = false;
-	    Ref(frame[i]);
 
-	this->trigger = other->trigger;
+	  break_on_return = false;
-	this->call = other->call;
+	  delayed = false;
+
+	  trigger = other->trigger;
+	  call = other->call;
+	  }
+	else
+	  reporter->InternalError("TODO: make Frame copy constructor.");
 	}
 
 Frame::~Frame()
+	{
+	// Deleting a Frame that is a view is a no-op.
+	if ( ! is_view )
 	  {
 	  Unref(trigger);
 	  Release();
 	  }
+	}
 
 void Frame::SetElement(int n, Val* v)
 	{
@@ -171,14 +180,12 @@ void Frame::ClearTrigger()
 	}
 
 ClosureFrame::ClosureFrame(Frame* closure, Frame* not_closure,
-	std::shared_ptr<id_list> outer_ids) : Frame(not_closure)
+			   std::shared_ptr<id_list> outer_ids) : Frame(not_closure, true)
 	{
 	assert(closure);
 
 	this->closure = closure;
-	Ref(this->closure);
+	body = not_closure;
-	this->body = not_closure;
-	Ref(this->body);
 	
 	// To clone a ClosureFrame we null outer_ids and then copy
 	// the set over directly, hence the check.
@@ -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);
+	// No need to Unref the closure. BroFunc handles this.
-	Unref(this->body);
+	// 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_closure = closure->Clone();
-	Frame* new_regular = this->body->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; });
+  }

src/Frame.h

@@ -4,8 +4,7 @@
 #define frame_h
 
 #include <vector>
-#include <unordered_set>
+#include <memory> // std::shared_ptr
-#include <memory>
 
 #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:
+// Class that allows for lookups in both a closure frame and a regular frame
-	- A ClosureFrame is created from two frames: a closure and a regular frame.
+// according to a list of outer IDs passed into the constructor.
-	- ALL operations except GetElement operations operate on the regular frame.
+
-	- A ClosureFrame requires a list of outside ID's captured by the closure.
+// Facts:
-	- Get operations on those IDs will be performed on the closure frame.
+// 	- 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
+	// Searches the start frame and all sub-frame's closures for a value corresponding
-	// to the id.
+	// 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
+        bool ClosureContains(const ID* i) const;
-            { return this->closure_elements.find(i->Name()) != this->closure_elements.end(); }
 
-        // Hashes c style strings. The strings need to be null-terminated.
+        std::vector<const char*> closure_elements;
-	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<const char*, const_char_hasher> closure_elements;
 };
 
 extern vector<Frame*> g_frame_stack;

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);
+	return this;
-	Ref(v);
-	return v;
 	}
 
 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<bool, Val*> Func::HandlePluginResult(std::pair<bool, Val*> 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 )
+	std::for_each(bodies.begin(), bodies.end(), [](Body& b) { Unref(b.stmts); });
-		Unref(bodies[i].stmts);
 
-	Unref(this->closure);
+	Unref(closure);
 	}
 
 int BroFunc::IsPure() const
 	{
-	for ( unsigned int i = 0; i < bodies.size(); ++i )
+	return std::all_of(bodies.begin(), bodies.end(),
-		if ( ! bodies[i].stmts->IsPure() )
+			   [](const Body& b) { return b.stmts->IsPure(); });
-			return 0;
-
-	return 1;
 	}
 
 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<id_list> ids, Frame* f)
 
 void BroFunc::SetClosureFrame(Frame* f)
 	{
-	if (this->closure)
+	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;
+		CopyStateInto(other);
-		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;
 
-		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(new InitStmt(inits));
-	stmt_series->Stmts().append(first);
 	stmt_series->Stmts().append(body);
 
 	return stmt_series;

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<bool, Val*> HandlePluginResult(std::pair<bool, Val*> 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();
+	Func* DoClone() override;
-
-	Val* 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<id_list> idl);
-
 	// Makes a deep copy of the input frame and captures it.
 	void SetClosureFrame(Frame* f);
 
@@ -129,6 +126,7 @@ private:
 	std::shared_ptr<id_list> 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;
 };
 

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 new Val(this->AsFunc()->DoClone());
-			return this->AsFunc()->DoClone();
 
 		if ( type->Tag() == TYPE_FILE )
 			{

src/Var.cc

@@ -477,8 +477,8 @@ int get_func_priotity(attr_list* attrs)
 
 void end_func(Stmt* body)
 	{
-	std::unique_ptr<function_ingredients> ingredients =
+	std::unique_ptr<function_ingredients> ingredients = gather_function_ingredients(body);
-		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<function_ingredients> gather_function_ingredients(Stmt* body)
 	{
-	std::unique_ptr<function_ingredients> ingredients (new function_ingredients);
+	std::unique_ptr<function_ingredients> ingredients = build_unique<function_ingredients>();
+
 	ingredients->frame_size = current_scope()->Length();
 	ingredients->inits = current_scope()->GetInits();
 

src/parse.y

@@ -1216,11 +1216,7 @@ func_body:
 	;
 
 anonymous_function:
-		TOK_FUNCTION func_params
+		TOK_FUNCTION begin_func
-			{
-			$<id>$ = current_scope()->GenerateTemporary("lambda-function");
-			begin_func($<id>$, current_module.c_str(), FUNC_FLAVOR_FUNCTION, 0, $2);
-			}
 		
 		'{'
 			{
@@ -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<function_ingredients> ingredients =
+			std::unique_ptr<function_ingredients> ingredients = gather_function_ingredients($5);
-				gather_function_ingredients($6);
+			
-			std::shared_ptr<id_list> outer_ids = gather_outer_ids(pop_scope(), $6);
+			std::shared_ptr<id_list> outer_ids = gather_outer_ids(pop_scope(), $5);
 
 			$$ = new LambdaExpr(std::move(ingredients), std::move(outer_ids));
 			}

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];
 
 	}