From 1bf0cd29fddfc364ca397e8cb896de2e1489a3a7 Mon Sep 17 00:00:00 2001 From: Robin Sommer Date: Thu, 18 Jul 2019 23:07:34 +0000 Subject: [PATCH] Edit pass over changes before merge. --- src/Expr.cc | 31 ++++----- src/Expr.h | 16 +++-- src/Frame.cc | 152 ++++++++++++++++++++++-------------------- src/Frame.h | 78 ++++++++++++---------- src/Func.cc | 39 +++++------ src/Func.h | 57 ++++++++-------- src/Scope.cc | 4 +- src/Val.cc | 19 +++--- src/Val.h | 4 +- src/Var.cc | 77 ++++++++++----------- src/Var.h | 8 +++ src/broker/Data.cc | 103 ++++++++++++---------------- src/broker/Manager.cc | 14 ++-- src/parse.y | 8 ++- 14 files changed, 312 insertions(+), 298 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 22c1ca4dcf..50a1bf3cdb 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2639,7 +2639,7 @@ Val* IndexExpr::Eval(Frame* f) const } } else - result = Fold(v1, v2); + result = Fold(v1, v2); Unref(v1); Unref(v2); @@ -2694,7 +2694,7 @@ Val* IndexExpr::Fold(Val* v1, Val* v2) const break; case TYPE_TABLE: - v = v1->AsTableVal()->Lookup(v2); // Then, we jump into the TableVal here. + v = v1->AsTableVal()->Lookup(v2); // Then, we jump into the TableVal here. break; case TYPE_STRING: @@ -3865,7 +3865,6 @@ FlattenExpr::FlattenExpr(Expr* arg_op) SetType(tl); } - Val* FlattenExpr::Fold(Val* v) const { RecordVal* rv = v->AsRecordVal(); @@ -4317,11 +4316,11 @@ void CallExpr::ExprDescribe(ODesc* d) const args->Describe(d); } -LambdaExpr::LambdaExpr(std::unique_ptr ing, - std::shared_ptr outer_ids) : Expr(EXPR_LAMBDA) +LambdaExpr::LambdaExpr(std::unique_ptr arg_ing, + std::shared_ptr arg_outer_ids) : Expr(EXPR_LAMBDA) { - ingredients = std::move(ing); - this->outer_ids = std::move(outer_ids); + ingredients = std::move(arg_ing); + outer_ids = std::move(arg_outer_ids); SetType(ingredients->id->Type()->Ref()); @@ -4334,7 +4333,7 @@ LambdaExpr::LambdaExpr(std::unique_ptr ing, ingredients->frame_size, ingredients->priority); - dummy_func->SetOuterIDs(this->outer_ids); + dummy_func->SetOuterIDs(outer_ids); // Get the body's "string" representation. ODesc d; @@ -4342,23 +4341,23 @@ LambdaExpr::LambdaExpr(std::unique_ptr ing, const char* desc = d.Description(); // Install that in the global_scope + // + // TODO(robin): Let's canonify these names a bit, otherwise somebody + // iterating through the namespace might be up surprised ... How + // about calling them "lambda_"? ID* id = install_ID(desc, current_module.c_str(), true, false); // Update lamb's name dummy_func->SetName(desc); - // When the id goes away it will unref v. Val* v = new Val(dummy_func); - - // id will unref v when its done. - id->SetVal(v); + id->SetVal(v); // id will unref v when its done. id->SetType(ingredients->id->Type()->Ref()); id->SetConst(); } Val* LambdaExpr::Eval(Frame* f) const { - BroFunc* lamb = new BroFunc( ingredients->id, ingredients->body, @@ -4368,6 +4367,8 @@ Val* LambdaExpr::Eval(Frame* f) const lamb->AddClosure(outer_ids, f); + // TODO(robin): Similar to above, plus: Could we cache this name? + // Isn't the same on every on Eval()? ODesc d; lamb->Describe(&d); const char* desc = d.Description(); @@ -4375,7 +4376,7 @@ Val* LambdaExpr::Eval(Frame* f) const // Set name to corresponding dummy func. // Allows for lookups by the receiver. lamb->SetName(desc); - + return new Val(lamb); } @@ -4390,7 +4391,7 @@ TraversalCode LambdaExpr::Traverse(TraversalCallback* cb) const TraversalCode tc = cb->PreExpr(this); HANDLE_TC_EXPR_PRE(tc); - tc = ingredients->body->Traverse(cb); + tc = ingredients->body->Traverse(cb); HANDLE_TC_EXPR_POST(tc); tc = cb->PostExpr(this); diff --git a/src/Expr.h b/src/Expr.h index 53ffbd2e1c..d6166805c5 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -12,10 +12,9 @@ #include "Debug.h" #include "EventHandler.h" #include "TraverseTypes.h" -#include "Func.h" // function_ingredients -#include // std::shared_ptr -#include // std::move +#include +#include typedef enum { EXPR_ANY = -1, @@ -661,7 +660,7 @@ protected: friend class Expr; IndexExpr() { } - Val* Fold(Val* v1, Val* v2) const override; + Val* Fold(Val* v1, Val* v2) const override; void ExprDescribe(ODesc* d) const override; @@ -944,7 +943,7 @@ public: protected: friend class Expr; - CallExpr() { func = 0; args = 0; } + CallExpr() { func = 0; args = 0; } void ExprDescribe(ODesc* d) const override; @@ -971,6 +970,13 @@ protected: private: std::unique_ptr ingredients; + + // TODO(robin): I'm wondering if we should just generally pass all + // the new "id_list" instances by value (or const ref where it + // works). It's going to be very small and easy to copy; and a + // shared_ptr with dynamic memory comes with overhead, too. (I'm + // actually tempted to suggest the same for "ingredients" though + // that's less clear.) std::shared_ptr outer_ids; }; diff --git a/src/Frame.cc b/src/Frame.cc index 9f39278f97..b7ec854623 100644 --- a/src/Frame.cc +++ b/src/Frame.cc @@ -3,7 +3,8 @@ #include "zeek-config.h" #include -#include // std::any_of +#include +#include #include "Frame.h" #include "Stmt.h" @@ -11,7 +12,6 @@ #include "Trigger.h" #include "broker/Data.h" -#include vector g_frame_stack; @@ -50,28 +50,27 @@ Frame::Frame(const Frame* other, bool view) break_before_next_stmt = false; break_on_return = false; delayed = false; - + if ( is_view ) - { - frame = other->frame; - } + frame = other->frame; else - { - if (trigger) - Ref(trigger); - for ( int i = 0; i < size; ++i ) - frame[i] = other->frame[i] ? other->frame[i]->Clone() : 0; - } + { + if ( trigger ) + Ref(trigger); + + for ( int i = 0; i < size; ++i ) + frame[i] = other->frame[i] ? other->frame[i]->Clone() : 0; + } } Frame::~Frame() { // Deleting a Frame that is a view is a no-op. if ( ! is_view ) - { - Unref(trigger); - Release(); - } + { + Unref(trigger); + Release(); + } } void Frame::SetElement(int n, Val* v) @@ -85,19 +84,19 @@ void Frame::SetElement(const ID* id, Val* v) SetElement(id->Offset(), v); } - Val* Frame::GetElement(const ID* id) const { - if (HasOuterIDs()) + if ( HasOuterIDs() ) { auto where = offset_map.find(std::string(id->Name())); - if (where != offset_map.end()) - return frame[ where->second ]; + if ( where != offset_map.end() ) + return frame[where->second]; } + return frame[id->Offset()]; } -void Frame::AddElement(ID* id, Val* v) +void Frame::AddElement(const ID* id, Val* v) { this->SetElement(id, v); } @@ -152,7 +151,7 @@ Frame* Frame::Clone() { Frame* f = new Frame(size, function, func_args); f->Clear(); - + for ( int i = 0; i < size; ++i ) f->frame[i] = frame[i] ? frame[i]->Clone() : 0; @@ -341,15 +340,14 @@ bool Frame::CaptureContains(const ID* i) const return where != offset_map.end(); } -ClosureFrame::ClosureFrame(Frame* closure, Frame* not_closure, +ClosureFrame::ClosureFrame(Frame* arg_closure, Frame* not_closure, std::shared_ptr outer_ids) : Frame(not_closure, true) { - assert(closure); - assert(outer_ids); + assert(arg_closure); - this->closure = closure; + closure = arg_closure; body = not_closure; - + SetOuterIDs(outer_ids); } @@ -358,6 +356,9 @@ ClosureFrame::~ClosureFrame() // 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. + + // TODO(robin): It would be good handle body & closure the same in + // terms needing to ref/unref; it's easy to get confused otherwise. Unref(body); } @@ -368,18 +369,20 @@ Val* ClosureFrame::GetElement(const ID* id) const int my_offset = offset_map.at(std::string(id->Name())); return ClosureFrame::GatherFromClosure(this, id, my_offset); } - return this->NthElement(id->Offset()); + + return NthElement(id->Offset()); } void ClosureFrame::SetElement(const ID* id, Val* v) { if ( CaptureContains(id) ) - { - int my_offset = offset_map.at(std::string(id->Name())); - ClosureFrame::SetInClosure(this, id, v, my_offset); - } - else - this->Frame::SetElement(id->Offset(), v); + { + int my_offset = offset_map.at(std::string(id->Name())); + ClosureFrame::SetInClosure(this, id, v, my_offset); + return; + } + + Frame::SetElement(id->Offset(), v); } Frame* ClosureFrame::Clone() @@ -397,18 +400,18 @@ Frame* ClosureFrame::SelectiveClone(id_list* choose) id_list us; // and id_list them; - - for (const auto& we : *choose) - { - if ( CaptureContains(we) ) - us.append(we); - else - them.append(we); - } - Frame* me = this->closure->SelectiveClone(&us); + for (const auto& we : *choose) + { + if ( CaptureContains(we) ) + us.append(we); + else + them.append(we); + } + + Frame* me = closure->SelectiveClone(&us); // and - Frame* you = this->body->SelectiveClone(&them); + Frame* you = body->SelectiveClone(&them); ClosureFrame* who = new ClosureFrame(me, you, nullptr); who->offset_map = offset_map; @@ -422,17 +425,22 @@ broker::expected ClosureFrame::Serialize() const rval.emplace_back(std::string("ClosureFrame")); auto om = SerializeOffsetMap(); - if ( ! om ) return broker::ec::invalid_data; + if ( ! om ) + return broker::ec::invalid_data; + rval.emplace_back( *om ); auto cl = closure->Serialize(); - if ( ! cl ) broker::ec::invalid_data; + if ( ! cl ) + return broker::ec::invalid_data; + rval.emplace_back( *cl ); auto bo = body->Serialize(); - if ( ! bo ) broker::ec::invalid_data; - rval.emplace_back( *bo ); + if ( ! bo ) + return broker::ec::invalid_data; + rval.emplace_back(*bo); return {std::move(rval)}; } @@ -440,8 +448,8 @@ broker::expected Frame::SerializeOffsetMap() const { broker::vector rval; - std::for_each(offset_map.begin(), offset_map.end(), - [&rval] (const std::pair& e) + std::for_each(offset_map.begin(), offset_map.end(), + [&rval] (const std::pair& e) { rval.emplace_back(e.first); rval.emplace_back(e.second);}); return {std::move(rval)}; @@ -449,34 +457,32 @@ broker::expected Frame::SerializeOffsetMap() const bool ClosureFrame::UnserializeIntoOffsetMap(const broker::vector& data, std::unordered_map& target) { - #define GET_OR_RETURN(type, name, index) \ - if (auto __##name##__ = broker::get_if(data[index])) \ - name = *__##name##__; \ - else \ - return false; \ - assert(target.size() == 0); std::unordered_map rval; for (broker::vector::size_type i = 0; i < data.size(); i += 2) { - std::string key; - int offset; - GET_OR_RETURN(std::string, key, i) - GET_OR_RETURN(broker::integer, offset, i+1) - target.insert( {std::move(key), std::move(offset)} ); + auto key = broker::get_if(data[i]); + if ( ! key ) + return false; + + auto offset = broker::get_if(data[i+1]); + if ( ! offset ) + return false; + + target.insert( {std::move(*key), std::move(*offset)} ); } return true; - #undef GET_OR_RETURN } -// Each ClosureFrame knows all of the outer IDs that are used inside of it. This is known at -// parse time. These leverage that. If frame_1 encloses frame_2 then the location of a lookup -// for an outer id in frame_2 can be determined by checking if that id is also an outer id in -// frame_2. If it is not, then frame_2 owns the id and the lookup is done there, otherwise, -// go deeper. +// Each ClosureFrame knows all of the outer IDs that are used inside of it. +// This is known at parse time. These leverage that. If frame_1 encloses +// frame_2 then the location of a lookup for an outer id in frame_2 can be +// determined by checking if that id is also an outer id in frame_2. If it is +// not, then frame_2 owns the id and the lookup is done there, otherwise, go +// deeper. Val* ClosureFrame::GatherFromClosure(const Frame* start, const ID* id, const int offset) { const ClosureFrame* conductor = dynamic_cast(start); @@ -485,13 +491,13 @@ Val* ClosureFrame::GatherFromClosure(const Frame* start, const ID* id, const int // was born. We differ to its maping as it is older and wiser. Otherwise, we use our own. if ( ! conductor ) { - if (start->HasOuterIDs()) + if ( start->HasOuterIDs() ) return start->GetElement(id); return start->NthElement(offset); } - if (conductor->CaptureContains(id)) + if ( conductor->CaptureContains(id) ) return ClosureFrame::GatherFromClosure(conductor->closure, id, offset); return conductor->NthElement(offset); @@ -502,11 +508,11 @@ void ClosureFrame::SetInClosure(Frame* start, const ID* id, Val* val, const int ClosureFrame* conductor = dynamic_cast(start); if ( ! conductor ) - start->SetElement(offset, val); + start->SetElement(offset, val); - else if (conductor->CaptureContains(id)) - ClosureFrame::SetInClosure(conductor->closure, id, val, offset); + else if ( conductor->CaptureContains(id) ) + ClosureFrame::SetInClosure(conductor->closure, id, val, offset); else - conductor->Frame::SetElement(offset, val); + conductor->Frame::SetElement(offset, val); } diff --git a/src/Frame.h b/src/Frame.h index baeed905bb..df30e05816 100644 --- a/src/Frame.h +++ b/src/Frame.h @@ -3,11 +3,11 @@ #ifndef frame_h #define frame_h -#include -#include +#include #include -#include // std::shared_ptr -#include // std::pair +#include +#include +#include #include #include @@ -19,21 +19,26 @@ class Trigger; class CallExpr; class Val; +// TODO(robin): As discussed, I think this would benefit from merging the two closes. +// I don't think having that one derived class buys us much in the end in terms +// of performance, and it makes the code quite a bit more complex. + class Frame : public BroObj { -friend class BroFunc; public: Frame(int size, const BroFunc* func, const val_list *fn_args); - // Constructs a copy or view of other. If a view is constructed the - // destructor will not change other's state on deletion. + + // Constructs a copy, or view, of another frame. If a view is + // constructed the destructor will not change other's state on + // deletion. Frame(const Frame* other, bool is_view = false); + ~Frame() override; - Val* NthElement(int n) const { return frame[n]; } + Val* NthElement(int n) const { return frame[n]; } void SetElement(int n, Val* v); virtual void SetElement(const ID* id, Val* v); - virtual Val* GetElement(const ID* id) const; - void AddElement(ID* id, Val* v); + void AddElement(const ID* id, Val* v); void Reset(int startIdx); void Release(); @@ -44,6 +49,9 @@ public: const BroFunc* GetFunction() const { return function; } const val_list* GetFuncArgs() const { return func_args; } + // Changes the function associated with the frame. + void SetFunction(BroFunc* func) { function = func; } + // Next statement to be executed in the context of this frame. void SetNextStmt(Stmt* stmt) { next_stmt = stmt; } Stmt* GetNextStmt() const { return next_stmt; } @@ -83,23 +91,25 @@ public: /** * Instantiates a Frame from a serialized one. * - * @return a pair. the first item is the status of the serialization, - * the second is the Unserialized frame with reference count +1 + * @return a pair in which the first item is the status of the serialization; + * and the second is the unserialized frame with reference count +1, or + * null if the serialization wasn't succesful. */ static std::pair Unserialize(const broker::vector& data); /** - * Installs *outer_ids* in this Frame's offset_map. - * - * Note: This needs to be done before serializing a Frame to guarantee that + * Installs *outer_ids* into the set of IDs the frame knows offsets for. + * + * Note: This needs to be done before serializing a Frame to guarantee that * the unserialized frame will perform lookups properly. - * + * * @param outer_ids the ids that this frame holds */ void SetOuterIDs(std::shared_ptr outer_ids); /** - * @return does this frame have an initialized offset_map? + * @return does this frame have any IDs installed to know their + * offsets? */ bool HasOuterIDs() const { return offset_map.size(); } @@ -129,7 +139,7 @@ protected: /** * Serializes this Frame's offset map. - * + * * @return a serialized version of the offset map. */ broker::expected SerializeOffsetMap() const; @@ -148,11 +158,11 @@ protected: const CallExpr* call; bool delayed; - /** + /** * Maps ID names to the offsets they had when passed into the frame. - * + * * A frame that has been serialized maintains its own map between IDs and - * their offsets. This is because a serialized frame is not guaranteed to + * their offsets. This is because a serialized frame is not guaranteed to * be unserialized somewhere where the offsets for the IDs that it contains * are the same. */ @@ -160,8 +170,8 @@ protected: private: - /** - * Rather or not this frame is a view of another one. Frames that + /** + * Wether or not this frame is a view of another one. Frames that * are views do not delete their underlying frame on deletion. */ bool is_view; @@ -169,16 +179,17 @@ private: /** - * Class that allows for actions in both a regular frame and a closure frame - * according to a list of outer IDs captured in the closure passed into the - * constructor. + * Frame variant that allows for performing operation on both a regular frame + * and an additional closure frame according to a list of outer IDs captured + * in the closure passed into the constructor. */ class ClosureFrame : public Frame { public: /** - * Constructs a closure Frame from a closure and body frame, and a list of ids - * that this frame should refer to its closure to for values. For non closure - * related operations the ClosureFrame is just a view of the body frame. + * Constructs a closure frame from a closure and body frame, and a + * list of ids for which this frame should refer to its closure for + * values. For other, non-closure ID the ClosureFrame is just a view + * of the body frame. * * @param closure the frame that holds IDs in *outer_ids*. * @param body the frame to refer to for all non-closure actions. @@ -199,15 +210,14 @@ public: (const broker::vector& data, std::unordered_map& target); private: - /** - * Finds the Value corresponding to *id* in the closure of *start*. - * + * Finds the value corresponding to *id* in the closure of *start*. + * * @param start the frame to begin the search from * @param id the ID whose corresponding value is to be collected. * @param offset the offset at which to look for id's value when its * frame has been found. - * @return the Value corresponding to *id*. + * @return the value corresponding to *id*. */ static Val* GatherFromClosure(const Frame* start, const ID* id, const int offset); @@ -215,7 +225,7 @@ private: * Sets the Value corresponding to *id* in the closure of *start* to *val* * * @param start the frame to begin the search from - * @param val the Value to associate with *id* in the closure. + * @param val the value to associate with *id* in the closure. * @param id the ID whose corresponding value is to be updated. * @param offset the offset at which to look for id's value when its * frame has been found. diff --git a/src/Func.cc b/src/Func.cc index 0abd7dac3a..5bc1da7209 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -28,6 +28,7 @@ #include #include + #include #include "Base64.h" @@ -127,7 +128,6 @@ void Func::AddBody(Stmt* /* new_body */, id_list* /* new_inits */, Internal("Func::AddBody called"); } - Func* Func::DoClone() { // By default, ok just to return a reference. Func does not have any state @@ -267,7 +267,7 @@ std::pair Func::HandlePluginResult(std::pair plugin_resu } BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits, - int arg_frame_size, int priority) : Func(BRO_FUNC) + int arg_frame_size, int priority) : Func(BRO_FUNC) { name = arg_id->Name(); type = arg_id->Type()->Ref(); @@ -284,7 +284,7 @@ BroFunc::BroFunc(ID* arg_id, Stmt* arg_body, id_list* aggr_inits, BroFunc::~BroFunc() { - std::for_each(bodies.begin(), bodies.end(), + std::for_each(bodies.begin(), bodies.end(), [](Body& b) { Unref(b.stmts); }); Unref(closure); } @@ -327,10 +327,9 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const } Frame* f = new Frame(frame_size, this, args); + if ( closure ) - { - f = new ClosureFrame(this->closure, f, this->outer_ids); - } + f = new ClosureFrame(closure, f, outer_ids); // Hand down any trigger. if ( parent ) @@ -389,6 +388,7 @@ Val* BroFunc::Call(val_list* args, Frame* parent) const if ( Flavor() == FUNC_FLAVOR_FUNCTION ) { Unref(f); + // TODO(robin): Shouldn't "result" have been left unset in this case? Unref(result); throw; } @@ -485,15 +485,15 @@ void BroFunc::AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size, void BroFunc::AddClosure(std::shared_ptr ids, Frame* f) { - this->SetOuterIDs(ids); - this->SetClosureFrame(f); + SetOuterIDs(ids); + SetClosureFrame(f); } void BroFunc::SetClosureFrame(Frame* f) { if ( closure ) - reporter->InternalError - ("Tried to override closure for BroFunc %s.", this->Name()); + reporter->InternalError("Tried to override closure for BroFunc %s.", + Name()); closure = f ? f->SelectiveClone( outer_ids.get() ) : nullptr; } @@ -501,19 +501,22 @@ void BroFunc::SetClosureFrame(Frame* f) bool BroFunc::UpdateClosure(const broker::vector& data) { auto result = Frame::Unserialize(data); - if ( ! result.first ) return false; + if ( ! result.first ) + return false; Frame* new_closure = result.second; + if ( new_closure ) + new_closure->SetFunction(this); - if (new_closure) - new_closure->function = this; + if ( closure ) + Unref(closure); - if (closure) Unref(closure); closure = new_closure; return true; } + Func* BroFunc::DoClone() { // A BroFunc could hold a closure. In this case a clone of it must @@ -523,7 +526,6 @@ Func* BroFunc::DoClone() CopyStateInto(other); other->frame_size = frame_size; - other->closure = closure ? closure->Clone() : nullptr; other->outer_ids = outer_ids; @@ -532,16 +534,15 @@ Func* BroFunc::DoClone() broker::expected BroFunc::SerializeClosure() const { - if (! closure) - { + if ( ! closure ) return broker::vector({"Frame"}); - } closure->SetOuterIDs(outer_ids); auto e = closure->Serialize(); + if ( ! e ) + return broker::ec::invalid_data; - if ( !e ) return broker::ec::invalid_data; return *e; } diff --git a/src/Func.h b/src/Func.h index b448e59f1f..7e8bf1ab17 100644 --- a/src/Func.h +++ b/src/Func.h @@ -4,7 +4,7 @@ #define func_h #include -#include // std::shared_ptr, std::unique_ptr +#include #include #include @@ -74,7 +74,8 @@ public: protected: Func(); - // Copies this functions state into other. + + // Copies this function's state into other. void CopyStateInto(Func* other) const; // Helper function for handling result of plugin hook. @@ -96,13 +97,12 @@ public: ~BroFunc() override; int IsPure() const override; - Val* Call(val_list* args, Frame* parent) const override; /** - * Adds adds a closure to the function. Closures are cloned and - * future calls to BroFunc will not modify *f* - * + * Adds adds a closure to the function. Closures are cloned and + * future calls to BroFunc methods will not modify *f*. + * * @param ids IDs that are captured by the closure. * @param f the closure to be captured. */ @@ -110,26 +110,21 @@ public: /** * Replaces the current closure with one built from *data* - * + * * @param data a serialized closure */ bool UpdateClosure(const broker::vector& data); - void AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size, - int priority) override; - - /** - * Clones this function along with its closures. - */ - Func* DoClone() override; - /** * Serializes this function's closure. - * + * * @return a serialized version of the function's closure. */ broker::expected SerializeClosure() const; + void AddBody(Stmt* new_body, id_list* new_inits, int new_frame_size, + int priority) override; + /** Sets this function's outer_id list. */ void SetOuterIDs(std::shared_ptr ids) { outer_ids = std::move(ids); } @@ -140,17 +135,22 @@ protected: BroFunc() : Func(BRO_FUNC) {} Stmt* AddInits(Stmt* body, id_list* inits); - int frame_size; + /** + * Clones this function along with its closures. + */ + Func* DoClone() override; -private: /** * Performs a selective clone of *f* using the IDs that were * captured in the function's closure. - * + * * @param f the frame to be cloned. */ void SetClosureFrame(Frame* f); +private: + int frame_size; + // List of the outer IDs used in the function. std::shared_ptr outer_ids = nullptr; // The frame the BroFunc was initialized in. @@ -190,16 +190,15 @@ struct CallInfo { const val_list* args; }; -// Struct that collects the arguments for a Func. -// Used for BroFuncs with closures. -struct function_ingredients - { - ID* id; - Stmt* body; - id_list* inits; - int frame_size; - int priority; - Scope* scope; +// Struct that collects all the specifics defining a Func. Used for BroFuncs +// with closures. +struct function_ingredients { + ID* id; + Stmt* body; + id_list* inits; + int frame_size; + int priority; + Scope* scope; }; extern vector call_stack; diff --git a/src/Scope.cc b/src/Scope.cc index 53f62fbe6a..56a9968417 100644 --- a/src/Scope.cc +++ b/src/Scope.cc @@ -171,7 +171,7 @@ ID* install_ID(const char* name, const char* module_name, if ( is_export || ! module_name || (is_global && normalized_module_name(module_name) == GLOBAL_MODULE_NAME) ) - scope = SCOPE_GLOBAL; + scope = SCOPE_GLOBAL; else if ( is_global ) scope = SCOPE_MODULE; else @@ -188,7 +188,7 @@ ID* install_ID(const char* name, const char* module_name, id->SetOffset(top_scope->Length()); top_scope->Insert(full_name, id); } - + return id; } diff --git a/src/Val.cc b/src/Val.cc index dcdc50c206..26630150c8 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -115,7 +115,8 @@ 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 ) - return new Val(this->AsFunc()->DoClone()); + return new Val(AsFunc()->DoClone()); + if ( type->Tag() == TYPE_FILE ) { // I think we can just ref the file here - it is unclear what else @@ -1734,7 +1735,7 @@ Val* TableVal::Default(Val* index) } else - def_val = def_attr->AttrExpr()->Eval(0); + def_val = def_attr->AttrExpr()->Eval(0); } if ( ! def_val ) @@ -2144,23 +2145,23 @@ int TableVal::CheckAndAssign(Val* index, Val* new_val) } void TableVal::InitDefaultFunc(Frame* f) - { + { // Value aready initialized. if ( def_val ) - return; + return; Attr* def_attr = FindAttr(ATTR_DEFAULT); if ( ! def_attr ) - return; + return; BroType* ytype = Type()->YieldType(); BroType* dtype = def_attr->AttrExpr()->Type(); if ( dtype->Tag() == TYPE_RECORD && ytype->Tag() == TYPE_RECORD && - ! same_type(dtype, ytype) && - record_promotion_compatible(dtype->AsRecordType(), - ytype->AsRecordType()) ) - return; // TableVal::Default will handle this. + ! same_type(dtype, ytype) && + record_promotion_compatible(dtype->AsRecordType(), + ytype->AsRecordType()) ) + return; // TableVal::Default will handle this. def_val = def_attr->AttrExpr()->Eval(f); } diff --git a/src/Val.h b/src/Val.h index 59d9de5dc5..fba5ad0c6b 100644 --- a/src/Val.h +++ b/src/Val.h @@ -880,8 +880,8 @@ public: void InitTimer(double delay); void DoExpire(double t); - // If default attribute is not a function, or the functon has - // already been initialized this does nothing. Otherwise, evaluates + // If the &default attribute is not a function, or the functon has + // already been initialized, this does nothing. Otherwise, evaluates // the function in the frame allowing it to capture its closure. void InitDefaultFunc(Frame* f); diff --git a/src/Var.cc b/src/Var.cc index d9e4deefd3..5b3be2f728 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -438,46 +438,43 @@ TraversalCode OuterIDBindingFinder::PreExpr(const Expr* expr) // Gets a function's priority from its Scope's attributes. Errors if it sees any // problems. -int get_func_priotity(attr_list* attrs) +static int get_func_priotity(const attr_list& attrs) { int priority = 0; - if ( attrs ) + + for ( const auto& a : attrs ) { - for ( const auto& a : *attrs ) + if ( a->Tag() == ATTR_DEPRECATED ) + continue; + + if ( a->Tag() != ATTR_PRIORITY ) { - if ( a->Tag() == ATTR_DEPRECATED ) - continue; - - if ( a->Tag() != ATTR_PRIORITY ) - { - a->Error("illegal attribute for function body"); - continue; - } - - Val* v = a->AttrExpr()->Eval(0); - if ( ! v ) - { - a->Error("cannot evaluate attribute expression"); - continue; - } - - if ( ! IsIntegral(v->Type()->Tag()) ) - { - a->Error("expression is not of integral type"); - continue; - } - - priority = v->InternalInt(); + a->Error("illegal attribute for function body"); + continue; } + + Val* v = a->AttrExpr()->Eval(0); + if ( ! v ) + { + a->Error("cannot evaluate attribute expression"); + continue; + } + + if ( ! IsIntegral(v->Type()->Tag()) ) + { + a->Error("expression is not of integral type"); + continue; + } + + priority = v->InternalInt(); } - return priority; + + return priority; } void end_func(Stmt* body) { - std::unique_ptr ingredients = gather_function_ingredients(body); - - pop_scope(); + std::unique_ptr ingredients = gather_function_ingredients(pop_scope(), body); if ( streq(ingredients->id->Name(), "anonymous-function") ) { @@ -503,7 +500,7 @@ void end_func(Stmt* body) ingredients->inits, ingredients->frame_size, ingredients->priority); - + ingredients->id->SetVal(new Val(f)); ingredients->id->SetConst(); } @@ -511,21 +508,19 @@ void end_func(Stmt* body) ingredients->id->ID_Val()->AsFunc()->SetScope(ingredients->scope); } -// Gathers all of the information from the current scope needed to build a -// function and collects it into a function_ingredients struct. -std::unique_ptr gather_function_ingredients(Stmt* body) +std::unique_ptr gather_function_ingredients(Scope* scope, Stmt* body) { - std::unique_ptr ingredients = build_unique(); + auto ingredients = build_unique(); - ingredients->frame_size = current_scope()->Length(); - ingredients->inits = current_scope()->GetInits(); + ingredients->frame_size = scope->Length(); + ingredients->inits = scope->GetInits(); - ingredients->scope = current_scope(); - ingredients->id = ingredients->scope->ScopeID(); + ingredients->scope = scope; + ingredients->id = scope->ScopeID(); - auto attrs = ingredients->scope->Attrs(); + auto attrs = scope->Attrs(); - ingredients->priority = get_func_priotity(attrs); + ingredients->priority = (attrs ? get_func_priotity(*attrs) : 0); ingredients->body = body; return ingredients; diff --git a/src/Var.h b/src/Var.h index 397a8a486e..4189b0e947 100644 --- a/src/Var.h +++ b/src/Var.h @@ -29,6 +29,14 @@ extern void end_func(Stmt* body); extern std::unique_ptr gather_function_ingredients(Stmt* body); extern std::shared_ptr gather_outer_ids(Scope* scope, Stmt* body); +// Gathers all of the information from a scope and a function body needed to +// build a function and collects it into a function_ingredients struct. +// Gathered elements are not refeed. +extern std::unique_ptr gather_function_ingredients(Scope* scope, Stmt* body); + +// Gather all IDs referenced inside a body that aren't part of a given scope. +extern std::shared_ptr gather_outer_ids(Scope* scope, Stmt* body); + extern Val* internal_val(const char* name); extern Val* internal_const_val(const char* name); // internal error if not const extern Val* opt_internal_val(const char* name); // returns nil if not defined diff --git a/src/broker/Data.cc b/src/broker/Data.cc index d4303dc28f..a70b39f9b8 100644 --- a/src/broker/Data.cc +++ b/src/broker/Data.cc @@ -342,54 +342,45 @@ struct val_converter { return rval; } - else if (type->Tag() == TYPE_FUNC) + else if ( type->Tag() == TYPE_FUNC ) { - #define GET_OR_RETURN(type, name, index) \ - if (auto __##name##__ = broker::get_if(a[index])) \ - name = *__##name##__; \ - else \ - return nullptr; \ - - if (a.size() > 2) + if ( a.size() < 1 || a.size() > 2 ) return nullptr; - - std::string name; - GET_OR_RETURN(std::string, name, 0); - auto id = global_scope()->Lookup(name.c_str()); + auto name = broker::get_if(a[0]); + if ( ! name ) + return nullptr; + auto id = global_scope()->Lookup(name->c_str()); if ( ! id ) return nullptr; auto rval = id->ID_Val(); - if ( ! rval ) return nullptr; auto t = rval->Type(); - if ( ! t ) return nullptr; - if ( t->Tag() != TYPE_FUNC ) return nullptr; - if (a.size() == 2) // We have a closure. + if ( a.size() == 2 ) // We have a closure. { - broker::vector frame; - GET_OR_RETURN(broker::vector, frame, 1); + auto frame = broker::get_if(a[1]); + if ( ! frame ) + return nullptr; - bool status = false; - if ( BroFunc* b = dynamic_cast(rval->AsFunc())) - { - status = b->UpdateClosure(frame); - } - if ( ! status ) return nullptr; + auto b = dynamic_cast(rval->AsFunc()); + if ( ! b ) + return nullptr; + + if ( ! b->UpdateClosure(*frame) ) + return nullptr; } return rval->Ref(); - #undef GET_OR_RETURN } else if ( type->Tag() == TYPE_RECORD ) { @@ -705,30 +696,22 @@ struct type_checker { } else if ( type->Tag() == TYPE_FUNC ) { - #define GET_OR_RETURN(type, name, index) \ - if (auto __##name##__ = broker::get_if(a[index])) \ - name = *__##name##__; \ - else \ - return false; \ - - if (a.size() > 2) + if ( a.size() < 1 || a.size() > 2 ) return false; - - std::string name; - GET_OR_RETURN(std::string, name, 0); - auto id = global_scope()->Lookup(name.c_str()); + auto name = broker::get_if(a[0]); + if ( ! name ) + return false; + auto id = global_scope()->Lookup(name->c_str()); if ( ! id ) return false; auto rval = id->ID_Val(); - if ( ! rval ) return false; auto t = rval->Type(); - if ( ! t ) return false; @@ -881,30 +864,32 @@ broker::expected bro_broker::val_to_data(Val* v) return {string(v->AsFile()->Name())}; case TYPE_FUNC: { - Func* f = v->AsFunc(); - std::string name(f->Name()); - broker::vector rval; - rval.push_back(name); + Func* f = v->AsFunc(); + std::string name(f->Name()); - auto starts_with = [](const std::string& s ,const std::string& in) -> bool - { return (0 == s.find(in)); }; + broker::vector rval; + rval.push_back(name); - if ( starts_with(name, "anonymous-function") ) + if ( name.find("anonymous-function") == 0 ) + { + // Only BroFuncs have closures. + if ( auto b = dynamic_cast(f) ) { - // Only BroFuncs have closures. - if (BroFunc* b = dynamic_cast(f)) - { - auto bc = dynamic_cast(f)->SerializeClosure(); - if ( ! bc ) - return broker::ec::invalid_data; - rval.emplace_back(std::move(*bc)); - } - else + auto bc = dynamic_cast(f)->SerializeClosure(); + if ( ! bc ) return broker::ec::invalid_data; - } - return {std::move(rval)}; + rval.emplace_back(std::move(*bc)); + } + else + { + reporter->InternalWarning("closure with non-BroFunc"); + return broker::ec::invalid_data; + } } + + return {std::move(rval)}; + } case TYPE_TABLE: { auto is_set = v->Type()->IsSet(); @@ -1058,10 +1043,10 @@ RecordVal* bro_broker::make_data_val(Val* v) auto rval = new RecordVal(BifType::Record::Broker::Data); auto data = val_to_data(v); - if ( ! data ) reporter->Warning("Didn't get a value from val_to_data"); - - if ( data ) + if ( data ) rval->Assign(0, new DataVal(move(*data))); + else + reporter->Warning("did not get a value from val_to_data"); return rval; } diff --git a/src/broker/Manager.cc b/src/broker/Manager.cc index c7b3f97a65..126e8d81b7 100644 --- a/src/broker/Manager.cc +++ b/src/broker/Manager.cc @@ -1042,20 +1042,20 @@ void Manager::ProcessEvent(const broker::topic& topic, broker::zeek::Event ev) vl.append(val); else { - const char* expected_name = type_name(expected_type->Tag()); + auto expected_name = type_name(expected_type->Tag()); reporter->Warning("failed to convert remote event '%s' arg #%d," " got %s, expected %s", name.data(), i, got_type, expected_name); - // If we got a vector and expected a function this is possibly because of a mismatch - // between anonymous-function bodies. - if ( (strcmp( expected_name, "func") == 0) && (strcmp("vector", got_type) == 0) ) - { + // If we got a vector and expected a function this is + // possibly because of a mismatch between + // anonymous-function bodies. + if ( strcmp(expected_name, "func") == 0 && strcmp("vector", got_type) == 0 ) reporter->Warning("when sending functions the receiver must have access to a" - " version of that function.\nFor anonymous functions, that function must have the same body."); - } + " version of that function.\nFor anonymous functions, that function must have the same body."); + break; } } diff --git a/src/parse.y b/src/parse.y index 18ba453698..42af95c8b9 100644 --- a/src/parse.y +++ b/src/parse.y @@ -1214,7 +1214,7 @@ func_body: anonymous_function: TOK_FUNCTION begin_func - + '{' { saved_in_init.push_back(in_init); @@ -1222,7 +1222,6 @@ anonymous_function: } stmt_list - { in_init = saved_in_init.back(); saved_in_init.pop_back(); @@ -1230,8 +1229,11 @@ anonymous_function: '}' { + // TODO(robin): Can move the following into end_func()? + // If so, we could reuse the "func_body" rule here and avoid the code duplication. + // Gather the ingredients for a BroFunc from the current scope - std::unique_ptr ingredients = gather_function_ingredients($5); + std::unique_ptr ingredients = gather_function_ingredients(current_scope(), $5); std::shared_ptr outer_ids = gather_outer_ids(pop_scope(), $5); $$ = new LambdaExpr(std::move(ingredients), std::move(outer_ids));