From 1a97b0572a146dad4b53e2583044ec394b558632 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Mon, 27 Nov 2023 16:51:52 -0800 Subject: [PATCH] squashing buglets --- src/Expr.cc | 3 +- src/script_opt/ProfileFunc.cc | 6 +-- src/script_opt/ProfileFunc.h | 1 - src/script_opt/Reduce.cc | 74 +++++++++++++++++------------------ src/script_opt/Reduce.h | 26 +++++++----- src/script_opt/ScriptOpt.cc | 7 ++-- src/script_opt/ZAM/Expr.cc | 7 +--- 7 files changed, 63 insertions(+), 61 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index fc2ffb7792..fcc84ced05 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -431,8 +431,7 @@ ValPtr NameExpr::Eval(Frame* f) const { if ( v ) return v; else { - if ( f ) - RuntimeError("value used but not set"); + RuntimeError("value used but not set"); return nullptr; } } diff --git a/src/script_opt/ProfileFunc.cc b/src/script_opt/ProfileFunc.cc index d68cf47f1f..6a7255ea56 100644 --- a/src/script_opt/ProfileFunc.cc +++ b/src/script_opt/ProfileFunc.cc @@ -1001,7 +1001,7 @@ void ProfileFuncs::ComputeSideEffects() { std::unordered_set aggrs; bool is_unknown = true; for ( auto c : candidates ) { - printf("jackpot for %s\n", obj_desc(c).c_str()); + // printf("jackpot for %s\n", obj_desc(c).c_str()); SetSideEffects(c, non_local_ids, aggrs, is_unknown); } break; @@ -1026,12 +1026,12 @@ void ProfileFuncs::SetSideEffects(const Attr* a, IDSet& non_local_ids, std::unor at = SideEffectsOp::READ; if ( non_local_ids.empty() && aggrs.empty() && ! is_unknown ) { - printf("%s has no side effects\n", obj_desc(a).c_str()); + // printf("%s has no side effects\n", obj_desc(a).c_str()); // Definitely no side effects. seo_vec.push_back(std::make_shared()); } else { - printf("%s has side effects\n", obj_desc(a).c_str()); + // printf("%s has side effects\n", obj_desc(a).c_str()); for ( auto ea_t : expr_attrs[a] ) { auto seo = std::make_shared(at, ea_t); diff --git a/src/script_opt/ProfileFunc.h b/src/script_opt/ProfileFunc.h index c42c8e99f8..87ff7dc31c 100644 --- a/src/script_opt/ProfileFunc.h +++ b/src/script_opt/ProfileFunc.h @@ -378,7 +378,6 @@ protected: bool AssessSideEffects(const ProfileFunc* e, IDSet& non_local_ids, std::unordered_set& types, bool& is_unknown); - // ### const? etc. bool AssessAggrEffects(SideEffectsOp::AccessType access, const Type* t, IDSet& non_local_ids, std::unordered_set& aggrs, bool& is_unknown); diff --git a/src/script_opt/Reduce.cc b/src/script_opt/Reduce.cc index de1b144d12..fb8d3de78a 100644 --- a/src/script_opt/Reduce.cc +++ b/src/script_opt/Reduce.cc @@ -16,7 +16,8 @@ namespace zeek::detail { -Reducer::Reducer(const ScriptFunc* func, std::shared_ptr _pf) : pf(std::move(_pf)) { +Reducer::Reducer(const ScriptFunc* func, std::shared_ptr _pf, ProfileFuncs& _pfs) + : pf(std::move(_pf)), pfs(_pfs) { auto& ft = func->GetType(); // Track the parameters so we don't remap them. @@ -437,11 +438,14 @@ bool Reducer::ExprValid(const ID* id, const Expr* e1, const Expr* e2) const { // * Same goes to modifications of aggregates via "add" or "delete" // or "+=" append. // - // * No propagation of expressions that are based on aggregates - // across function calls. + // * Assessment of any record constructors or coercions, or + // table references or modifications, for possible invocation of + // associated handlers that have side effects. // - // * No propagation of expressions that are based on globals - // across calls. + // * Assessment of function calls for potential side effects. + // + // These latter two are guided by the global profile of the full set + // of script functions. // Tracks which ID's are germane for our analysis. std::vector ids; @@ -456,7 +460,7 @@ bool Reducer::ExprValid(const ID* id, const Expr* e1, const Expr* e2) const { if ( e1->Tag() == EXPR_NAME ) ids.push_back(e1->AsNameExpr()->Id()); - CSE_ValidityChecker vc(ids, e1, e2); + CSE_ValidityChecker vc(pfs, ids, e1, e2); reduction_root->Traverse(&vc); return vc.IsValid(); @@ -785,14 +789,15 @@ std::shared_ptr Reducer::FindTemporary(const ID* id) const { return tmp->second; } -CSE_ValidityChecker::CSE_ValidityChecker(const std::vector& _ids, const Expr* _start_e, const Expr* _end_e) - : ids(_ids) { +CSE_ValidityChecker::CSE_ValidityChecker(ProfileFuncs& _pfs, const std::vector& _ids, const Expr* _start_e, + const Expr* _end_e) + : pfs(_pfs), ids(_ids) { start_e = _start_e; end_e = _end_e; for ( auto i : ids ) if ( i->IsGlobal() || IsAggr(i->GetType()) ) - sensitive_to_calls = true; + have_sensitive_IDs = true; // Track whether this is a record assignment, in which case // we're attuned to assignments to the same field for the @@ -858,22 +863,21 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) { auto lhs_ref = e->GetOp1()->AsRefExprPtr(); auto lhs = lhs_ref->GetOp1()->AsNameExpr(); - if ( CheckID(ids, lhs->Id(), false) ) { + if ( CheckID(lhs->Id(), false) ) { is_valid = false; return TC_ABORTALL; } - // Note, we don't use CheckAggrMod() because this - // is a plain assignment. It might be changing a variable's - // binding to an aggregate, but it's not changing the - // aggregate itself. + // Note, we don't use CheckAggrMod() because this is a plain + // assignment. It might be changing a variable's binding to + // an aggregate, but it's not changing the aggregate itself. } break; case EXPR_INDEX_ASSIGN: { auto lhs_aggr = e->GetOp1(); auto lhs_aggr_id = lhs_aggr->AsNameExpr()->Id(); - if ( CheckID(ids, lhs_aggr_id, true) || CheckAggrMod(ids, e) ) { + if ( CheckID(lhs_aggr_id, true) || CheckAggrMod(e) ) { is_valid = false; return TC_ABORTALL; } @@ -894,7 +898,7 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) { return TC_ABORTALL; } - if ( CheckID(ids, lhs_aggr_id, true) || CheckAggrMod(ids, e) ) { + if ( CheckID(lhs_aggr_id, true) || CheckAggrMod(e) ) { is_valid = false; return TC_ABORTALL; } @@ -903,14 +907,14 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) { case EXPR_APPEND_TO: // This doesn't directly change any identifiers, but does // alter an aggregate. - if ( CheckAggrMod(ids, e) ) { + if ( CheckAggrMod(e) ) { is_valid = false; return TC_ABORTALL; } break; case EXPR_CALL: - if ( sensitive_to_calls ) { + if ( have_sensitive_IDs ) { auto c = e->AsCallExpr(); auto func = c->Func(); std::string desc; @@ -950,7 +954,7 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) { case EXPR_RECORD_CONSTRUCTOR: // If these have initializations done at construction // time, those can include function calls. - if ( sensitive_to_calls ) { + if ( have_sensitive_IDs ) { auto& et = e->GetType(); if ( et->Tag() == TYPE_RECORD && ! et->AsRecordType()->IdempotentCreation() ) { is_valid = false; @@ -961,30 +965,25 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) { case EXPR_INDEX: case EXPR_FIELD: - // We treat these together because they both have - // to be checked when inside an "add" or "delete" - // statement. + // We treat these together because they both have to be checked + // when inside an "add" or "delete" statement. if ( in_aggr_mod_stmt ) { auto aggr = e->GetOp1(); auto aggr_id = aggr->AsNameExpr()->Id(); - if ( CheckID(ids, aggr_id, true) ) { + if ( CheckID(aggr_id, true) ) { is_valid = false; return TC_ABORTALL; } } - if ( t == EXPR_INDEX && sensitive_to_calls ) { - // Unfortunately in isolation we can't - // statically determine whether this table - // has a &default associated with it. In - // principle we could track all instances - // of the table type seen (across the - // entire set of scripts), and note whether - // any of those include an expression, but - // that's a lot of work for what might be - // minimal gain. - + if ( t == EXPR_INDEX && have_sensitive_IDs ) { + // Unfortunately in isolation we can't statically determine + // whether this table has a &default associated with it. + // In principle we could track all instances of the table + // type seen (across the entire set of scripts), and note + // whether any of those include an expression, but that's a + // lot of work for what might be minimal gain. is_valid = false; return TC_ABORTALL; } @@ -997,7 +996,7 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) { return TC_CONTINUE; } -bool CSE_ValidityChecker::CheckID(const std::vector& ids, const ID* id, bool ignore_orig) const { +bool CSE_ValidityChecker::CheckID(const ID* id, bool ignore_orig) const { // Only check type info for aggregates. auto id_t = IsAggr(id->GetType()) ? id->GetType() : nullptr; @@ -1016,11 +1015,10 @@ bool CSE_ValidityChecker::CheckID(const std::vector& ids, const ID* i return false; } -bool CSE_ValidityChecker::CheckAggrMod(const std::vector& ids, const Expr* e) const { +bool CSE_ValidityChecker::CheckAggrMod(const Expr* e) const { const auto& e_i_t = e->GetType(); if ( IsAggr(e_i_t) ) { - // This assignment sets an aggregate value. - // Look for type matches. + // This assignment sets an aggregate value. Look for type matches. for ( auto i : ids ) if ( same_type(e_i_t, i->GetType()) ) return true; diff --git a/src/script_opt/Reduce.h b/src/script_opt/Reduce.h index 68532130e5..7553803802 100644 --- a/src/script_opt/Reduce.h +++ b/src/script_opt/Reduce.h @@ -16,7 +16,7 @@ class TempVar; class Reducer { public: - Reducer(const ScriptFunc* func, std::shared_ptr pf); + Reducer(const ScriptFunc* func, std::shared_ptr pf, ProfileFuncs& pfs); StmtPtr Reduce(StmtPtr s); @@ -237,6 +237,9 @@ protected: // Profile associated with the function. std::shared_ptr pf; + // Profile across all script functions - used for optimization decisions. + ProfileFuncs& pfs; + // Tracks the temporary variables created during the reduction/ // optimization process. std::vector> temps; @@ -324,7 +327,7 @@ protected: class CSE_ValidityChecker : public TraversalCallback { public: - CSE_ValidityChecker(const std::vector& ids, const Expr* start_e, const Expr* end_e); + CSE_ValidityChecker(ProfileFuncs& pfs, const std::vector& ids, const Expr* start_e, const Expr* end_e); TraversalCode PreStmt(const Stmt*) override; TraversalCode PostStmt(const Stmt*) override; @@ -342,20 +345,24 @@ public: protected: // Returns true if an assignment involving the given identifier on - // the LHS is in conflict with the given list of identifiers. - bool CheckID(const std::vector& ids, const ID* id, bool ignore_orig) const; + // the LHS is in conflict with the identifiers we're tracking. + bool CheckID(const ID* id, bool ignore_orig) const; // Returns true if the assignment given by 'e' modifies an aggregate - // with the same type as that of one of the identifiers. - bool CheckAggrMod(const std::vector& ids, const Expr* e) const; + // with the same type as that of one of the identifiers we're tracking. + bool CheckAggrMod(const Expr* e) const; + + // Profile across all script functions. + ProfileFuncs& pfs; // The list of identifiers for which an assignment to one of them // renders the CSE unsafe. const std::vector& ids; - // Whether the list of identifiers includes some that we should - // consider potentially altered by a function call. - bool sensitive_to_calls = false; + // Whether the list of identifiers includes some that we need to evaluate + // for being affected by side effects from function calls, table + // accesses, etc. + bool have_sensitive_IDs = false; // Where in the AST to start our analysis. This is the initial // assignment expression. @@ -381,6 +388,7 @@ protected: // Whether analyzed expressions occur in the context of // a statement that modifies an aggregate ("add" or "delete"). + // ### bool in_aggr_mod_stmt = false; }; diff --git a/src/script_opt/ScriptOpt.cc b/src/script_opt/ScriptOpt.cc index 5e7527fc4b..849c6cd480 100644 --- a/src/script_opt/ScriptOpt.cc +++ b/src/script_opt/ScriptOpt.cc @@ -147,7 +147,8 @@ static bool optimize_AST(ScriptFunc* f, std::shared_ptr& pf, std::s return true; } -static void optimize_func(ScriptFunc* f, std::shared_ptr pf, ScopePtr scope, StmtPtr& body) { +static void optimize_func(ScriptFunc* f, std::shared_ptr pf, ProfileFuncs& pfs, ScopePtr scope, + StmtPtr& body) { if ( reporter->Errors() > 0 ) return; @@ -167,7 +168,7 @@ static void optimize_func(ScriptFunc* f, std::shared_ptr pf, ScopeP push_existing_scope(scope); - auto rc = std::make_shared(f, pf); + auto rc = std::make_shared(f, pf, pfs); auto new_body = rc->Reduce(body); if ( reporter->Errors() > 0 ) { @@ -486,7 +487,7 @@ static void analyze_scripts_for_ZAM() { } auto new_body = f.Body(); - optimize_func(func, f.ProfilePtr(), f.Scope(), new_body); + optimize_func(func, f.ProfilePtr(), *pfs, f.Scope(), new_body); f.SetBody(new_body); if ( is_lambda ) diff --git a/src/script_opt/ZAM/Expr.cc b/src/script_opt/ZAM/Expr.cc index 693173bc36..47dc9de05a 100644 --- a/src/script_opt/ZAM/Expr.cc +++ b/src/script_opt/ZAM/Expr.cc @@ -965,11 +965,8 @@ const ZAMStmt ZAMCompiler::DoCall(const CallExpr* c, const NameExpr* n) { } } else { - if ( indirect ) { - if ( func_id->IsGlobal() ) - z = ZInstI(op, -1); - else - z = ZInstI(op, FrameSlot(func)); + if ( indirect && ! func_id->IsGlobal() ) { + z = ZInstI(op, FrameSlot(func)); z.op_type = OP_V; } else {