Fix three bugs with 'when' and 'return when' statements. Addresses #946

- 'when' statements were problematic when used in a function/event/hook
  that had local variables with an assigned function value.  This was
  because 'when' blocks operate on a clone of the frame and the cloning
  process serializes locals and the serialization of functions had an
  infinite cycle in it (ID -> BroFunc -> ID -> BroFunc ...).  The ID
  was only used for the function name and type information, so
  refactoring Func and subclasses to depend on those two things instead
  fixes the issue.

- 'return when' blocks, specifically, didn't work whenever execution
  of the containing function's body does another function call before
  reaching the 'return when' block, because of an assertion.  This was
  was due to logic in CallExpr::Eval always clearing the CallExpr
  associated with the Frame after doing the call, instead of restoring
  any previous CallExpr, which the code in Trigger::Eval expected to
  have available.

- An assert could be reached when the condition of a 'when' statement
  depended on checking the value of global state variables.  The assert
  in Trigger::QueueTrigger that checks that the Trigger isn't disabled
  would get hit because Trigger::Eval/Timeout disable themselves after
  running, but don't unregister themselves from the NotifierRegistry,
  which keeps calling QueueTrigger for every state access of the global.

src/Debug.cc

@@ -763,7 +763,7 @@ int dbg_handle_debug_input()
 	Frame* curr_frame = g_frame_stack.back();
 	const BroFunc* func = curr_frame->GetFunction();
 	if ( func )
-		current_module = func->GetID()->ModuleName();
+		current_module = extract_module_name(func->Name());
 	else
 		current_module = GLOBAL_MODULE_NAME;
 

src/Expr.cc

@@ -4639,12 +4639,13 @@ Val* CallExpr::Eval(Frame* f) const
 		{
 		const ::Func* func = func_val->AsFunc();
 		calling_expr = this;
+		const CallExpr* current_call = f ? f->GetCall() : 0;
 
 		if ( f )
 			f->SetCall(this);
 		ret = func->Call(v, f); // No try/catch here; we pass exceptions upstream.
 		if ( f )
-			f->ClearCall();
+			f->SetCall(current_call);
 		// Don't Unref() the arguments, as Func::Call already did that.
 		delete v;
 

src/Func.cc

@@ -54,13 +54,13 @@ bool did_builtin_init = false;
 
 vector<Func*> Func::unique_ids;
 
-Func::Func() : scope(0), id(0), return_value(0)
+Func::Func() : scope(0), type(0)
 	{
 	unique_id = unique_ids.size();
 	unique_ids.push_back(this);
 	}
 
-Func::Func(Kind arg_kind) : scope(0), kind(arg_kind), id(0), return_value(0)
+Func::Func(Kind arg_kind) : scope(0), kind(arg_kind), type(0)
 	{
 	unique_id = unique_ids.size();
 	unique_ids.push_back(this);
@@ -68,6 +68,7 @@ Func::Func(Kind arg_kind) : scope(0), kind(arg_kind), id(0), return_value(0)
 
 Func::~Func()
 	{
+	Unref(type);
 	}
 
 void Func::AddBody(Stmt* /* new_body */, id_list* /* new_inits */,
@@ -129,6 +130,12 @@ bool Func::DoSerialize(SerialInfo* info) const
 	if ( ! SERIALIZE(char(kind) ) )
 		return false;
 
+	if ( ! type->Serialize(info) )
+		return false;
+
+	if ( ! SERIALIZE(Name()) )
+		return false;
+
 	// We don't serialize scope as only global functions are considered here
 	// anyway.
 	return true;
@@ -160,12 +167,24 @@ bool Func::DoUnserialize(UnserialInfo* info)
 		return false;
 
 	kind = (Kind) c;
+
+	type = BroType::Unserialize(info);
+	if ( ! type )
+		return false;
+
+	const char* n;
+	if ( ! UNSERIALIZE_STR(&n, 0) )
+		return false;
+
+	name = n;
+	delete [] n;
+
 	return true;
 	}
 
 void Func::DescribeDebug(ODesc* d, const val_list* args) const
 	{
-	id->Describe(d);
+	d->Add(Name());
 	RecordType* func_args = FType()->Args();
 
 	if ( args )
@@ -196,21 +215,6 @@ void Func::DescribeDebug(ODesc* d, const val_list* args) const
 		}
 	}
 
-void Func::SetID(ID *arg_id)
-	{
-	id = arg_id;
-
-	return_value =
-		new ID(string(string(id->Name()) + "_returnvalue").c_str(),
-			SCOPE_FUNCTION, false);
-	return_value->SetType(FType()->YieldType()->Ref());
-	}
-
-ID* Func::GetReturnValueID() const
-	{
-	return return_value;
-	}
-
 TraversalCode Func::Traverse(TraversalCallback* cb) const
 	{
 	// FIXME: Make a fake scope for builtins?
@@ -226,12 +230,6 @@ TraversalCode Func::Traverse(TraversalCallback* cb) const
 		tc = scope->Traverse(cb);
 		HANDLE_TC_STMT_PRE(tc);
 
-		if ( GetReturnValueID() )
-			{
-			tc = GetReturnValueID()->Traverse(cb);
-			HANDLE_TC_STMT_PRE(tc);
-			}
-
 		for ( unsigned int i = 0; i < bodies.size(); ++i )
 			{
 			tc = bodies[i].stmts->Traverse(cb);
@@ -249,7 +247,8 @@ BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits,
 		int arg_frame_size, int priority)
 : Func(BRO_FUNC)
 	{
-	id = arg_id;
+	name = arg_id->Name();
+	type = arg_id->Type()->Ref();
 	frame_size = arg_frame_size;
 
 	if ( arg_body )
@@ -263,7 +262,6 @@ BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits,
 
 BroFunc::~BroFunc()
 	{
-	Unref(id);
 	for ( unsigned int i = 0; i < bodies.size(); ++i )
 		Unref(bodies[i].stmts);
 	}
@@ -378,7 +376,8 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const
 	     (flow != FLOW_RETURN /* we fell off the end */ ||
 	      ! result /* explicit return with no result */) &&
 	     ! f->HasDelayed() )
-		reporter->Warning("non-void function returns without a value: %s", id->Name());
+		reporter->Warning("non-void function returns without a value: %s",
+		                  Name());
 
 	if ( result && g_trace_state.DoTrace() )
 		{
@@ -421,8 +420,7 @@ void BroFunc::AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size,
 
 void BroFunc::Describe(ODesc* d) const
 	{
-	if ( id )
-		id->Describe(d);
+	d->Add(Name());
 
 	d->NL();
 	d->AddCount(frame_size);
@@ -450,14 +448,14 @@ IMPLEMENT_SERIAL(BroFunc, SER_BRO_FUNC);
 bool BroFunc::DoSerialize(SerialInfo* info) const
 	{
 	DO_SERIALIZE(SER_BRO_FUNC, Func);
-	return id->Serialize(info) && SERIALIZE(frame_size);
+	return SERIALIZE(frame_size);
 	}
 
 bool BroFunc::DoUnserialize(UnserialInfo* info)
 	{
 	DO_UNSERIALIZE(Func);
-	id = ID::Unserialize(info);
-	return id && UNSERIALIZE(&frame_size);
+
+	return UNSERIALIZE(&frame_size);
 	}
 
 BuiltinFunc::BuiltinFunc(built_in_func arg_func, const char* arg_name,
@@ -465,15 +463,16 @@ BuiltinFunc::BuiltinFunc(built_in_func arg_func, const char* arg_name,
 : Func(BUILTIN_FUNC)
 	{
 	func = arg_func;
-	name = copy_string(make_full_var_name(GLOBAL_MODULE_NAME, arg_name).c_str());
+	name = make_full_var_name(GLOBAL_MODULE_NAME, arg_name);
 	is_pure = arg_is_pure;
 
-	id = lookup_ID(name, GLOBAL_MODULE_NAME, false);
+	ID* id = lookup_ID(Name(), GLOBAL_MODULE_NAME, false);
 	if ( ! id )
-		reporter->InternalError("built-in function %s missing", name);
+		reporter->InternalError("built-in function %s missing", Name());
 	if ( id->HasVal() )
-		reporter->InternalError("built-in function %s multiply defined", name);
+		reporter->InternalError("built-in function %s multiply defined", Name());
 
+	type = id->Type()->Ref();
 	id->SetVal(new Val(this));
 	}
 
@@ -491,7 +490,7 @@ Val* BuiltinFunc::Call(val_list* args, Frame* parent) const
 #ifdef PROFILE_BRO_FUNCTIONS
 	DEBUG_MSG("Function: %s\n", Name());
 #endif
-	SegmentProfiler(segment_logger, name);
+	SegmentProfiler(segment_logger, Name());
 
 	if ( sample_logger )
 		sample_logger->FunctionSeen(this);
@@ -522,8 +521,7 @@ Val* BuiltinFunc::Call(val_list* args, Frame* parent) const
 
 void BuiltinFunc::Describe(ODesc* d) const
 	{
-	if ( id )
-	id->Describe(d);
+	d->Add(Name());
 	d->AddCount(is_pure);
 	}
 
@@ -532,16 +530,13 @@ IMPLEMENT_SERIAL(BuiltinFunc, SER_BUILTIN_FUNC);
 bool BuiltinFunc::DoSerialize(SerialInfo* info) const
 	{
 	DO_SERIALIZE(SER_BUILTIN_FUNC, Func);
-
-	// We ignore the ID. Func::Serialize() will rebind us anyway.
-	return SERIALIZE(name);
+	return true;
 	}
 
 bool BuiltinFunc::DoUnserialize(UnserialInfo* info)
 	{
 	DO_UNSERIALIZE(Func);
-	id = 0;
-	return UNSERIALIZE_STR(&name, 0);
+	return true;
 	}
 
 void builtin_error(const char* msg, BroObj* arg)

src/Func.h

@@ -47,15 +47,11 @@ public:
 	virtual void SetScope(Scope* newscope)	{ scope = newscope; }
 	virtual Scope* GetScope() const		{ return scope; }
 
-	virtual FuncType* FType() const
-		{
-		return (FuncType*) id->Type()->AsFuncType();
-		}
+	virtual FuncType* FType() const { return type->AsFuncType(); }
 
 	Kind GetKind() const	{ return kind; }
 
-	const ID* GetID() const { return id; }
-	void SetID(ID *arg_id);
+	const char* Name() const { return name.c_str(); }
 
 	virtual void Describe(ODesc* d) const = 0;
 	virtual void DescribeDebug(ODesc* d, const val_list* args) const;
@@ -64,7 +60,6 @@ public:
 	bool Serialize(SerialInfo* info) const;
 	static Func* Unserialize(UnserialInfo* info);
 
-	ID* GetReturnValueID() const;
 	virtual TraversalCode Traverse(TraversalCallback* cb) const;
 
 	uint32 GetUniqueFuncID() const { return unique_id; }
@@ -79,8 +74,8 @@ protected:
 	vector<Body> bodies;
 	Scope* scope;
 	Kind kind;
-	ID* id;
-	ID* return_value;
+	BroType* type;
+	string name;
 	uint32 unique_id;
 	static vector<Func*> unique_ids;
 };
@@ -119,18 +114,16 @@ public:
 
 	int IsPure() const;
 	Val* Call(val_list* args, Frame* parent) const;
-	const char* Name() const	{ return name; }
 	built_in_func TheFunc() const	{ return func; }
 
 	void Describe(ODesc* d) const;
 
 protected:
-	BuiltinFunc()	{ func = 0; name = 0; is_pure = 0; }
+	BuiltinFunc()	{ func = 0; is_pure = 0; }
 
 	DECLARE_SERIAL(BuiltinFunc);
 
 	built_in_func func;
-	const char* name;
 	int is_pure;
 };
 

src/Stats.cc

@@ -338,7 +338,7 @@ SampleLogger::~SampleLogger()
 
 void SampleLogger::FunctionSeen(const Func* func)
 	{
-	load_samples->Assign(new StringVal(func->GetID()->Name()), 0);
+	load_samples->Assign(new StringVal(func->Name()), 0);
 	}
 
 void SampleLogger::LocationSeen(const Location* loc)

src/Trigger.cc

@@ -251,6 +251,7 @@ bool Trigger::Eval()
 		timer_mgr->Cancel(timer);
 
 	Disable();
+	UnregisterAll();
 	Unref(this);
 
 	return true;
@@ -337,6 +338,7 @@ void Trigger::Timeout()
 		}
 
 	Disable();
+	UnregisterAll();
 	Unref(this);
 	}
 

src/Trigger.h

@@ -79,7 +79,6 @@ private:
 	friend class TriggerTimer;
 
 	void Init();
-	void DeleteTrigger();
 	void Register(ID* id);
 	void Register(Val* val);
 	void UnregisterAll();

src/input/Manager.cc

@@ -483,7 +483,7 @@ bool Manager::CreateEventStream(RecordVal* fval)
 	Unref(fields); // ref'd by lookupwithdefault
 	stream->num_fields = fieldsV.size();
 	stream->fields = fields->Ref()->AsRecordType();
-	stream->event = event_registry->Lookup(event->GetID()->Name());
+	stream->event = event_registry->Lookup(event->Name());
 	stream->want_record = ( want_record->InternalInt() == 1 );
 	Unref(want_record); // ref'd by lookupwithdefault
 
@@ -644,7 +644,7 @@ bool Manager::CreateTableStream(RecordVal* fval)
 	stream->tab = dst->AsTableVal();
 	stream->rtype = val ? val->AsRecordType() : 0;
 	stream->itype = idx->AsRecordType();
-	stream->event = event ? event_registry->Lookup(event->GetID()->Name()) : 0;
+	stream->event = event ? event_registry->Lookup(event->Name()) : 0;
 	stream->currDict = new PDict(InputHash);
 	stream->currDict->SetDeleteFunc(input_hash_delete_func);
 	stream->lastDict = new PDict(InputHash);

src/logging/Manager.cc

@@ -365,7 +365,7 @@ bool Manager::CreateStream(EnumVal* id, RecordVal* sval)
 	streams[idx]->id = id->Ref()->AsEnumVal();
 	streams[idx]->enabled = true;
 	streams[idx]->name = id->Type()->AsEnumType()->Lookup(idx);
-	streams[idx]->event = event ? event_registry->Lookup(event->GetID()->Name()) : 0;
+	streams[idx]->event = event ? event_registry->Lookup(event->Name()) : 0;
 	streams[idx]->columns = columns->Ref()->AsRecordType();
 
 	DBG_LOG(DBG_LOGGING, "Created new logging stream '%s', raising event %s",

testing/btest/Baseline/language.returnwhen/bro..stdout

@@ -0,0 +1,12 @@
+dummy from async_func() from bro_init()
+async_func() return result in bro_init(), flag in my_set
+dummy from bro_init() when block
+hi!
+dummy from async_func() from do_another()
+async_func() return result in do_another(), flag in my_set
+dummy from do_another() when block
+hi!
+dummy from async_func() from do_another()
+async_func() return result in do_another(), timeout
+dummy from do_another() when block
+hi!

testing/btest/language/returnwhen.bro

@@ -0,0 +1,79 @@
+# @TEST-EXEC: btest-bg-run bro bro -b %INPUT
+# @TEST-EXEC: btest-bg-wait 15
+# @TEST-EXEC: btest-diff bro/.stdout
+
+redef exit_only_after_terminate = T;
+
+global my_set: set[string] = set();
+global flag: string = "flag";
+global done: bool = F;
+
+function dummyfunc(s: string): string
+	{
+	return "dummy " + s;
+	}
+
+function async_func(s: string): string
+	{
+	print dummyfunc("from async_func() " + s);
+
+	return when ( flag in my_set )
+		{
+		return flag + " in my_set";
+		}
+	timeout 3sec
+		{
+		return "timeout";
+		}
+	}
+
+event set_flag()
+	{
+	add my_set[flag];
+	}
+
+event do_another()
+	{
+	delete my_set[flag];
+
+	local local_dummy = dummyfunc;
+
+	local anon = function(s: string): string { return s + "!"; };
+
+	if ( ! done )
+		schedule 1sec { set_flag() };
+
+	when ( local result = async_func("from do_another()") )
+		{
+		print "async_func() return result in do_another()", result;
+		print local_dummy("from do_another() when block");
+		print anon("hi");
+		if ( result == "timeout" )
+			terminate();
+		else
+			{
+			done = T;
+			schedule 10msec { do_another() };
+			}
+		}
+	}
+
+event bro_init()
+	{
+	local local_dummy = dummyfunc;
+
+	local anon = function(s: string): string { return s + "!"; };
+
+	schedule 1sec { set_flag() };
+
+	when ( local result = async_func("from bro_init()") )
+		{
+		print "async_func() return result in bro_init()", result;
+		print local_dummy("from bro_init() when block");
+		print anon("hi");
+		if ( result == "timeout" ) terminate();
+		schedule 10msec { do_another() };
+		}
+	}
+
+