Mirror/zeek
1
0
Fork 0
mirror of https://github.com/zeek/zeek.git synced 2025-10-12 03:28:19 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

squashing buglets

Browse source
This commit is contained in:
Vern Paxson 2023-11-27 16:51:52 -08:00
parent ed7c087fdb
commit 1a97b0572a
7 changed files with 63 additions and 61 deletions
Download patch file Download diff file Expand all files Collapse all files

1
src/Expr.cc
View file

@ -431,7 +431,6 @@ ValPtr NameExpr::Eval(Frame* f) const {
if ( v ) if ( v )
return v; return v;
else { else {
if ( f )
RuntimeError("value used but not set"); RuntimeError("value used but not set");
return nullptr; return nullptr;
} }

6
src/script_opt/ProfileFunc.cc
View file

@ -1001,7 +1001,7 @@ void ProfileFuncs::ComputeSideEffects() {
std::unordered_set<const Type*> aggrs; std::unordered_set<const Type*> aggrs;
bool is_unknown = true; bool is_unknown = true;
for ( auto c : candidates ) { 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); SetSideEffects(c, non_local_ids, aggrs, is_unknown);
} }
break; break;
@ -1026,12 +1026,12 @@ void ProfileFuncs::SetSideEffects(const Attr* a, IDSet& non_local_ids, std::unor
at = SideEffectsOp::READ; at = SideEffectsOp::READ;
if ( non_local_ids.empty() && aggrs.empty() && ! is_unknown ) { 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. // Definitely no side effects.
seo_vec.push_back(std::make_shared<SideEffectsOp>()); seo_vec.push_back(std::make_shared<SideEffectsOp>());
} }
else { 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] ) { for ( auto ea_t : expr_attrs[a] ) {
auto seo = std::make_shared<SideEffectsOp>(at, ea_t); auto seo = std::make_shared<SideEffectsOp>(at, ea_t);

1
src/script_opt/ProfileFunc.h
View file

@ -378,7 +378,6 @@ protected:
bool AssessSideEffects(const ProfileFunc* e, IDSet& non_local_ids, std::unordered_set<const Type*>& types, bool AssessSideEffects(const ProfileFunc* e, IDSet& non_local_ids, std::unordered_set<const Type*>& types,
bool& is_unknown); bool& is_unknown);
// ### const? etc.
bool AssessAggrEffects(SideEffectsOp::AccessType access, const Type* t, IDSet& non_local_ids, bool AssessAggrEffects(SideEffectsOp::AccessType access, const Type* t, IDSet& non_local_ids,
std::unordered_set<const Type*>& aggrs, bool& is_unknown); std::unordered_set<const Type*>& aggrs, bool& is_unknown);

74
src/script_opt/Reduce.cc
View file

@ -16,7 +16,8 @@
namespace zeek::detail { namespace zeek::detail {
Reducer::Reducer(const ScriptFunc* func, std::shared_ptr<ProfileFunc> _pf) : pf(std::move(_pf)) { Reducer::Reducer(const ScriptFunc* func, std::shared_ptr<ProfileFunc> _pf, ProfileFuncs& _pfs)
: pf(std::move(_pf)), pfs(_pfs) {
auto& ft = func->GetType(); auto& ft = func->GetType();
// Track the parameters so we don't remap them. // 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" // * Same goes to modifications of aggregates via "add" or "delete"
// or "+=" append. // or "+=" append.
// //
// * No propagation of expressions that are based on aggregates // * Assessment of any record constructors or coercions, or
// across function calls. // table references or modifications, for possible invocation of
// associated handlers that have side effects.
// //
// * No propagation of expressions that are based on globals // * Assessment of function calls for potential side effects.
// across calls. //
// 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. // Tracks which ID's are germane for our analysis.
std::vector<const ID*> ids; std::vector<const ID*> ids;
@ -456,7 +460,7 @@ bool Reducer::ExprValid(const ID* id, const Expr* e1, const Expr* e2) const {
if ( e1->Tag() == EXPR_NAME ) if ( e1->Tag() == EXPR_NAME )
ids.push_back(e1->AsNameExpr()->Id()); ids.push_back(e1->AsNameExpr()->Id());
CSE_ValidityChecker vc(ids, e1, e2); CSE_ValidityChecker vc(pfs, ids, e1, e2);
reduction_root->Traverse(&vc); reduction_root->Traverse(&vc);
return vc.IsValid(); return vc.IsValid();
@ -785,14 +789,15 @@ std::shared_ptr<TempVar> Reducer::FindTemporary(const ID* id) const {
return tmp->second; return tmp->second;
} }
CSE_ValidityChecker::CSE_ValidityChecker(const std::vector<const ID*>& _ids, const Expr* _start_e, const Expr* _end_e) CSE_ValidityChecker::CSE_ValidityChecker(ProfileFuncs& _pfs, const std::vector<const ID*>& _ids, const Expr* _start_e,
: ids(_ids) { const Expr* _end_e)
: pfs(_pfs), ids(_ids) {
start_e = _start_e; start_e = _start_e;
end_e = _end_e; end_e = _end_e;
for ( auto i : ids ) for ( auto i : ids )
if ( i->IsGlobal() || IsAggr(i->GetType()) ) if ( i->IsGlobal() || IsAggr(i->GetType()) )
sensitive_to_calls = true; have_sensitive_IDs = true;
// Track whether this is a record assignment, in which case // Track whether this is a record assignment, in which case
// we're attuned to assignments to the same field for the // 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_ref = e->GetOp1()->AsRefExprPtr();
auto lhs = lhs_ref->GetOp1()->AsNameExpr(); auto lhs = lhs_ref->GetOp1()->AsNameExpr();
if ( CheckID(ids, lhs->Id(), false) ) { if ( CheckID(lhs->Id(), false) ) {
is_valid = false; is_valid = false;
return TC_ABORTALL; return TC_ABORTALL;
} }
// Note, we don't use CheckAggrMod() because this // Note, we don't use CheckAggrMod() because this is a plain
// is a plain assignment. It might be changing a variable's // assignment. It might be changing a variable's binding to
// binding to an aggregate, but it's not changing the // an aggregate, but it's not changing the aggregate itself.
// aggregate itself.
} break; } break;
case EXPR_INDEX_ASSIGN: { case EXPR_INDEX_ASSIGN: {
auto lhs_aggr = e->GetOp1(); auto lhs_aggr = e->GetOp1();
auto lhs_aggr_id = lhs_aggr->AsNameExpr()->Id(); 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; is_valid = false;
return TC_ABORTALL; return TC_ABORTALL;
} }
@ -894,7 +898,7 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) {
return TC_ABORTALL; return TC_ABORTALL;
} }
if ( CheckID(ids, lhs_aggr_id, true) || CheckAggrMod(ids, e) ) { if ( CheckID(lhs_aggr_id, true) || CheckAggrMod(e) ) {
is_valid = false; is_valid = false;
return TC_ABORTALL; return TC_ABORTALL;
} }
@ -903,14 +907,14 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) {
case EXPR_APPEND_TO: case EXPR_APPEND_TO:
// This doesn't directly change any identifiers, but does // This doesn't directly change any identifiers, but does
// alter an aggregate. // alter an aggregate.
if ( CheckAggrMod(ids, e) ) { if ( CheckAggrMod(e) ) {
is_valid = false; is_valid = false;
return TC_ABORTALL; return TC_ABORTALL;
} }
break; break;
case EXPR_CALL: case EXPR_CALL:
if ( sensitive_to_calls ) { if ( have_sensitive_IDs ) {
auto c = e->AsCallExpr(); auto c = e->AsCallExpr();
auto func = c->Func(); auto func = c->Func();
std::string desc; std::string desc;
@ -950,7 +954,7 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) {
case EXPR_RECORD_CONSTRUCTOR: case EXPR_RECORD_CONSTRUCTOR:
// If these have initializations done at construction // If these have initializations done at construction
// time, those can include function calls. // time, those can include function calls.
if ( sensitive_to_calls ) { if ( have_sensitive_IDs ) {
auto& et = e->GetType(); auto& et = e->GetType();
if ( et->Tag() == TYPE_RECORD && ! et->AsRecordType()->IdempotentCreation() ) { if ( et->Tag() == TYPE_RECORD && ! et->AsRecordType()->IdempotentCreation() ) {
is_valid = false; is_valid = false;
@ -961,30 +965,25 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) {
case EXPR_INDEX: case EXPR_INDEX:
case EXPR_FIELD: case EXPR_FIELD:
// We treat these together because they both have // We treat these together because they both have to be checked
// to be checked when inside an "add" or "delete" // when inside an "add" or "delete" statement.
// statement.
if ( in_aggr_mod_stmt ) { if ( in_aggr_mod_stmt ) {
auto aggr = e->GetOp1(); auto aggr = e->GetOp1();
auto aggr_id = aggr->AsNameExpr()->Id(); auto aggr_id = aggr->AsNameExpr()->Id();
if ( CheckID(ids, aggr_id, true) ) { if ( CheckID(aggr_id, true) ) {
is_valid = false; is_valid = false;
return TC_ABORTALL; return TC_ABORTALL;
} }
} }
if ( t == EXPR_INDEX && sensitive_to_calls ) { if ( t == EXPR_INDEX && have_sensitive_IDs ) {
// Unfortunately in isolation we can't // Unfortunately in isolation we can't statically determine
// statically determine whether this table // whether this table has a &default associated with it.
// has a &default associated with it. In // In principle we could track all instances of the table
// principle we could track all instances // type seen (across the entire set of scripts), and note
// of the table type seen (across the // whether any of those include an expression, but that's a
// entire set of scripts), and note whether // lot of work for what might be minimal gain.
// any of those include an expression, but
// that's a lot of work for what might be
// minimal gain.
is_valid = false; is_valid = false;
return TC_ABORTALL; return TC_ABORTALL;
} }
@ -997,7 +996,7 @@ TraversalCode CSE_ValidityChecker::PreExpr(const Expr* e) {
return TC_CONTINUE; return TC_CONTINUE;
} }
bool CSE_ValidityChecker::CheckID(const std::vector<const ID*>& 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. // Only check type info for aggregates.
auto id_t = IsAggr(id->GetType()) ? id->GetType() : nullptr; auto id_t = IsAggr(id->GetType()) ? id->GetType() : nullptr;
@ -1016,11 +1015,10 @@ bool CSE_ValidityChecker::CheckID(const std::vector<const ID*>& ids, const ID* i
return false; return false;
} }
bool CSE_ValidityChecker::CheckAggrMod(const std::vector<const ID*>& ids, const Expr* e) const { bool CSE_ValidityChecker::CheckAggrMod(const Expr* e) const {
const auto& e_i_t = e->GetType(); const auto& e_i_t = e->GetType();
if ( IsAggr(e_i_t) ) { if ( IsAggr(e_i_t) ) {
// This assignment sets an aggregate value. // This assignment sets an aggregate value. Look for type matches.
// Look for type matches.
for ( auto i : ids ) for ( auto i : ids )
if ( same_type(e_i_t, i->GetType()) ) if ( same_type(e_i_t, i->GetType()) )
return true; return true;

26
src/script_opt/Reduce.h
View file

@ -16,7 +16,7 @@ class TempVar;
class Reducer { class Reducer {
public: public:
Reducer(const ScriptFunc* func, std::shared_ptr<ProfileFunc> pf); Reducer(const ScriptFunc* func, std::shared_ptr<ProfileFunc> pf, ProfileFuncs& pfs);
StmtPtr Reduce(StmtPtr s); StmtPtr Reduce(StmtPtr s);
@ -237,6 +237,9 @@ protected:
// Profile associated with the function. // Profile associated with the function.
std::shared_ptr<ProfileFunc> pf; std::shared_ptr<ProfileFunc> pf;
// Profile across all script functions - used for optimization decisions.
ProfileFuncs& pfs;
// Tracks the temporary variables created during the reduction/ // Tracks the temporary variables created during the reduction/
// optimization process. // optimization process.
std::vector<std::shared_ptr<TempVar>> temps; std::vector<std::shared_ptr<TempVar>> temps;
@ -324,7 +327,7 @@ protected:
class CSE_ValidityChecker : public TraversalCallback { class CSE_ValidityChecker : public TraversalCallback {
public: public:
CSE_ValidityChecker(const std::vector<const ID*>& ids, const Expr* start_e, const Expr* end_e); CSE_ValidityChecker(ProfileFuncs& pfs, const std::vector<const ID*>& ids, const Expr* start_e, const Expr* end_e);
TraversalCode PreStmt(const Stmt*) override; TraversalCode PreStmt(const Stmt*) override;
TraversalCode PostStmt(const Stmt*) override; TraversalCode PostStmt(const Stmt*) override;
@ -342,20 +345,24 @@ public:
protected: protected:
// Returns true if an assignment involving the given identifier on // Returns true if an assignment involving the given identifier on
// the LHS is in conflict with the given list of identifiers. // the LHS is in conflict with the identifiers we're tracking.
bool CheckID(const std::vector<const ID*>& ids, const ID* id, bool ignore_orig) const; bool CheckID(const ID* id, bool ignore_orig) const;
// Returns true if the assignment given by 'e' modifies an aggregate // Returns true if the assignment given by 'e' modifies an aggregate
// with the same type as that of one of the identifiers. // with the same type as that of one of the identifiers we're tracking.
bool CheckAggrMod(const std::vector<const ID*>& ids, const Expr* e) const; 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 // The list of identifiers for which an assignment to one of them
// renders the CSE unsafe. // renders the CSE unsafe.
const std::vector<const ID*>& ids; const std::vector<const ID*>& ids;
// Whether the list of identifiers includes some that we should // Whether the list of identifiers includes some that we need to evaluate
// consider potentially altered by a function call. // for being affected by side effects from function calls, table
bool sensitive_to_calls = false; // accesses, etc.
bool have_sensitive_IDs = false;
// Where in the AST to start our analysis. This is the initial // Where in the AST to start our analysis. This is the initial
// assignment expression. // assignment expression.
@ -381,6 +388,7 @@ protected:
// Whether analyzed expressions occur in the context of // Whether analyzed expressions occur in the context of
// a statement that modifies an aggregate ("add" or "delete"). // a statement that modifies an aggregate ("add" or "delete").
// ###
bool in_aggr_mod_stmt = false; bool in_aggr_mod_stmt = false;
}; };

7
src/script_opt/ScriptOpt.cc
View file

@ -147,7 +147,8 @@ static bool optimize_AST(ScriptFunc* f, std::shared_ptr<ProfileFunc>& pf, std::s
return true; return true;
} }
static void optimize_func(ScriptFunc* f, std::shared_ptr<ProfileFunc> pf, ScopePtr scope, StmtPtr& body) { static void optimize_func(ScriptFunc* f, std::shared_ptr<ProfileFunc> pf, ProfileFuncs& pfs, ScopePtr scope,
StmtPtr& body) {
if ( reporter->Errors() > 0 ) if ( reporter->Errors() > 0 )
return; return;
@ -167,7 +168,7 @@ static void optimize_func(ScriptFunc* f, std::shared_ptr<ProfileFunc> pf, ScopeP
push_existing_scope(scope); push_existing_scope(scope);
auto rc = std::make_shared<Reducer>(f, pf); auto rc = std::make_shared<Reducer>(f, pf, pfs);
auto new_body = rc->Reduce(body); auto new_body = rc->Reduce(body);
if ( reporter->Errors() > 0 ) { if ( reporter->Errors() > 0 ) {
@ -486,7 +487,7 @@ static void analyze_scripts_for_ZAM() {
} }
auto new_body = f.Body(); 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); f.SetBody(new_body);
if ( is_lambda ) if ( is_lambda )

5
src/script_opt/ZAM/Expr.cc
View file

@ -965,10 +965,7 @@ const ZAMStmt ZAMCompiler::DoCall(const CallExpr* c, const NameExpr* n) {
} }
} }
else { else {
if ( indirect ) { if ( indirect && ! func_id->IsGlobal() ) {
if ( func_id->IsGlobal() )
z = ZInstI(op, -1);
else
z = ZInstI(op, FrameSlot(func)); z = ZInstI(op, FrameSlot(func));
z.op_type = OP_V; z.op_type = OP_V;
} }