From 3925ff459203333ef2d80fe167272cf075f5557a Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Tue, 15 Aug 2023 16:07:49 -0700 Subject: [PATCH 1/7] addressed performance and correctness issues flagged by Coverity --- src/Expr.cc | 2 +- src/Func.cc | 6 +++--- src/Stmt.cc | 2 +- src/Stmt.h | 2 +- src/parse.y | 2 +- src/script_opt/Expr.cc | 17 +++++++++-------- src/script_opt/Stmt.cc | 2 +- src/script_opt/ZAM/Ops.in | 24 ++++++++++++------------ src/script_opt/ZAM/Vars.cc | 4 +++- 9 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index d001706196..af0cdb22f3 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4649,7 +4649,7 @@ LambdaExpr::LambdaExpr(FunctionIngredientsPtr arg_ing, IDPList arg_outer_ids, st auto ingr_t = ingredients->GetID()->GetType(); SetType(ingr_t); - if ( ! CheckCaptures(when_parent) ) + if ( ! CheckCaptures(std::move(when_parent)) ) { SetError(); return; diff --git a/src/Func.cc b/src/Func.cc index 5642b96775..885a4333cd 100644 --- a/src/Func.cc +++ b/src/Func.cc @@ -142,7 +142,7 @@ void Func::AddBody(detail::StmtPtr new_body, size_t new_frame_size) { std::vector no_inits; std::set no_groups; - AddBody(new_body, no_inits, new_frame_size, 0, no_groups); + AddBody(std::move(new_body), no_inits, new_frame_size, 0, no_groups); } void Func::AddBody(detail::StmtPtr /* new_body */, @@ -588,7 +588,7 @@ void ScriptFunc::CreateCaptures(std::unique_ptr> cvec) if ( c_i.IsManaged() ) ZVal::DeleteManagedType(cv_i); - cv_i = ZVal(new_cv_i, t); + cv_i = ZVal(std::move(new_cv_i), t); } } } @@ -725,7 +725,7 @@ FuncPtr ScriptFunc::DoClone() // Need to clone cv_i. auto& t_i = c.Id()->GetType(); auto cv_i_val = cv_i->ToVal(t_i)->Clone(); - other->captures_vec->push_back(ZVal(cv_i_val, t_i)); + other->captures_vec->push_back(ZVal(std::move(cv_i_val), t_i)); ++cv_i; } } diff --git a/src/Stmt.cc b/src/Stmt.cc index 5acacbc1f9..6d3abdc963 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -2079,7 +2079,7 @@ void WhenInfo::BuildProfile() if ( ! is_present ) { IDPtr wl_ptr = {NewRef{}, const_cast(wl)}; - cl->emplace_back(wl_ptr, false); + cl->emplace_back(std::move(wl_ptr), false); } // In addition, don't treat them as external locals that diff --git a/src/Stmt.h b/src/Stmt.h index 7829165f20..4e73da5728 100644 --- a/src/Stmt.h +++ b/src/Stmt.h @@ -647,7 +647,7 @@ private: StmtPtr s; StmtPtr timeout_s; ExprPtr timeout; - FuncType::CaptureList* cl; + FuncType::CaptureList* cl = nullptr; bool is_return = false; diff --git a/src/parse.y b/src/parse.y index 367c116831..94e6f524e3 100644 --- a/src/parse.y +++ b/src/parse.y @@ -1935,7 +1935,7 @@ stmt: | when_clause { std::shared_ptr wi($1); - $$ = new WhenStmt(wi); + $$ = new WhenStmt(std::move(wi)); script_coverage_mgr.AddStmt($$); } diff --git a/src/script_opt/Expr.cc b/src/script_opt/Expr.cc index 427f719b1b..3e8a71ce90 100644 --- a/src/script_opt/Expr.cc +++ b/src/script_opt/Expr.cc @@ -309,7 +309,7 @@ StmtPtr Expr::ReduceToSingletons(Reducer* c) if ( op3 && ! op3->IsSingleton(c) ) SetOp3(op3->ReduceToSingleton(c, red3_stmt)); - return MergeStmts(red1_stmt, red2_stmt, red3_stmt); + return MergeStmts(std::move(red1_stmt), std::move(red2_stmt), std::move(red3_stmt)); } ExprPtr Expr::ReduceToConditional(Reducer* c, StmtPtr& red_stmt) @@ -354,7 +354,7 @@ ExprPtr Expr::ReduceToConditional(Reducer* c, StmtPtr& red_stmt) { auto red2_stmt = ReduceToSingletons(c); auto res = ReduceToConditional(c, red_stmt); - red_stmt = MergeStmts(red2_stmt, red_stmt); + red_stmt = MergeStmts(std::move(red2_stmt), red_stmt); return res; } @@ -611,7 +611,7 @@ ExprPtr BinaryExpr::Reduce(Reducer* c, StmtPtr& red_stmt) if ( ! op2->IsSingleton(c) ) op2 = op2->ReduceToSingleton(c, red2_stmt); - red_stmt = MergeStmts(red_stmt, red2_stmt); + red_stmt = MergeStmts(red_stmt, std::move(red2_stmt)); auto op1_fold_val = op1->FoldVal(); auto op2_fold_val = op2->FoldVal(); @@ -1528,7 +1528,8 @@ StmtPtr CondExpr::ReduceToSingletons(Reducer* c) if ( ! red3_stmt ) red3_stmt = make_intrusive(); - if_else = make_intrusive(op1->Duplicate(), red2_stmt, red3_stmt); + if_else = make_intrusive(op1->Duplicate(), std::move(red2_stmt), + std::move(red3_stmt)); } return MergeStmts(red1_stmt, if_else); @@ -1897,7 +1898,7 @@ StmtPtr IndexExpr::ReduceToSingletons(Reducer* c) StmtPtr red2_stmt = op2->ReduceToSingletons(c); - return MergeStmts(red1_stmt, red2_stmt); + return MergeStmts(red1_stmt, std::move(red2_stmt)); } ExprPtr IndexExprWhen::Duplicate() @@ -1927,7 +1928,7 @@ ExprPtr HasFieldExpr::Reduce(Reducer* c, StmtPtr& red_stmt) if ( ! op->GetType()->FieldHasAttr(field, ATTR_OPTIONAL) ) { auto true_constant = make_intrusive(val_mgr->True()); - return TransformMe(true_constant, c, red_stmt); + return TransformMe(std::move(true_constant), c, red_stmt); } return UnaryExpr::Reduce(c, red_stmt); @@ -2334,7 +2335,7 @@ ExprPtr ScheduleExpr::Reduce(Reducer* c, StmtPtr& red_stmt) // We assume that EventExpr won't transform itself fundamentally. (void)event->Reduce(c, red2_stmt); - red_stmt = MergeStmts(red_stmt, red2_stmt); + red_stmt = MergeStmts(red_stmt, std::move(red2_stmt)); return ThisPtr(); } @@ -2419,7 +2420,7 @@ ExprPtr CallExpr::Reduce(Reducer* c, StmtPtr& red_stmt) // ### could check here for (1) pure function, and (2) all // arguments constants, and call it to fold right now. - red_stmt = MergeStmts(red_stmt, red2_stmt); + red_stmt = MergeStmts(red_stmt, std::move(red2_stmt)); if ( GetType()->Tag() == TYPE_VOID ) return ThisPtr(); diff --git a/src/script_opt/Stmt.cc b/src/script_opt/Stmt.cc index e6084138c0..fc042cdaf6 100644 --- a/src/script_opt/Stmt.cc +++ b/src/script_opt/Stmt.cc @@ -1012,7 +1012,7 @@ StmtPtr WhenStmt::DoReduce(Reducer* c) if ( red_e_stmt ) { auto s = make_intrusive(red_e_stmt, ThisPtr()); - return TransformMe(s, c); + return TransformMe(std::move(s), c); } } diff --git a/src/script_opt/ZAM/Ops.in b/src/script_opt/ZAM/Ops.in index 96e3e1c9d0..0747e8f00f 100644 --- a/src/script_opt/ZAM/Ops.in +++ b/src/script_opt/ZAM/Ops.in @@ -752,7 +752,7 @@ macro EvalVal2InTableCore(op1, op2) ListValPtr lvp = {NewRef{}, &lv}; macro EvalVal2InTableAssignCore(slot) - frame[z.v1].int_val = frame[z.slot].table_val->Find(lvp) != nullptr; + frame[z.v1].int_val = frame[z.slot].table_val->Find(std::move(lvp)) != nullptr; macro EvalVal2InTablePre(op1, op2, op3) auto& tt_ind = frame[z.op3].table_val->GetType()->AsTableType()->GetIndexTypes(); @@ -838,12 +838,12 @@ eval auto op1 = z.c.ToVal(z.t); internal-op List-Is-In-Table type VV eval auto op1 = z.aux->ToListVal(frame); - frame[z.v1].int_val = frame[z.v2].table_val->Find(op1) != nullptr; + frame[z.v1].int_val = frame[z.v2].table_val->Find(std::move(op1)) != nullptr; internal-op List-Is-In-Table type VC eval auto op1 = z.aux->ToListVal(frame); - frame[z.v1].int_val = z.c.table_val->Find(op1) != nullptr; + frame[z.v1].int_val = z.c.table_val->Find(std::move(op1)) != nullptr; internal-op Val-Is-In-Vector type VVV @@ -918,7 +918,7 @@ macro EvalIndexVecBoolSelect(op1, op2) auto vt = cast_intrusive(z.t); auto v2 = op1.vector_val; auto v3 = op2.vector_val; - auto v = vector_bool_select(vt, v2, v3); + auto v = vector_bool_select(std::move(vt), v2, v3); Unref(frame[z.v1].vector_val); frame[z.v1].vector_val = v.release(); @@ -936,7 +936,7 @@ macro EvalIndexVecIntSelect(op1, op2) auto vt = cast_intrusive(z.t); auto v2 = op1.vector_val; auto v3 = op2.vector_val; - auto v = vector_int_select(vt, v2, v3); + auto v = vector_int_select(std::move(vt), v2, v3); Unref(frame[z.v1].vector_val); frame[z.v1].vector_val = v.release(); @@ -1136,7 +1136,7 @@ type VV op1-read eval auto& tbl = frame[z.v1].table_val; auto lambda = frame[z.v2].ToVal(z.t); - tbl->InitDefaultVal(lambda); + tbl->InitDefaultVal(std::move(lambda)); direct-unary-op Set-Constructor ConstructSet @@ -1294,7 +1294,7 @@ internal-op Record-Coerce type VV eval auto rt = cast_intrusive(z.t); auto v = frame[z.v2].record_val; - auto to_r = coerce_to_record(rt, v, z.aux->map); + auto to_r = coerce_to_record(std::move(rt), v, z.aux->map); Unref(frame[z.v1].record_val); frame[z.v1].record_val = to_r.release(); @@ -1426,7 +1426,7 @@ eval ValPtr vec = {NewRef{}, frame[z.v1].vector_val}; auto slice = z.aux->ToListVal(frame); ValPtr vals = {NewRef{}, frame[z.v2].vector_val}; bool iterators_invalidated; - auto error = assign_to_index(vec, slice, vals, iterators_invalidated); + auto error = assign_to_index(std::move(vec), std::move(slice), std::move(vals), iterators_invalidated); if ( error ) ZAM_run_time_error(z.loc, error); if ( iterators_invalidated ) @@ -1442,7 +1442,7 @@ macro EvalTableElemAssign(value) auto indices = z.aux->ToListVal(frame); auto val = value.ToVal(z.t); bool iterators_invalidated = false; - frame[z.v1].table_val->Assign(indices, val, true, &iterators_invalidated); + frame[z.v1].table_val->Assign(std::move(indices), std::move(val), true, &iterators_invalidated); if ( iterators_invalidated ) ZAM_run_time_warning(z.loc, "possible loop/iterator invalidation"); @@ -1999,7 +1999,7 @@ method-post z.aux = v->aux; macro EvalAddStmt(ind) auto index = ind; bool iterators_invalidated = false; - frame[z.v1].table_val->Assign(index, nullptr, true, &iterators_invalidated); + frame[z.v1].table_val->Assign(std::move(index), nullptr, true, &iterators_invalidated); if ( iterators_invalidated ) ZAM_run_time_warning(z.loc, "possible loop/iterator invalidation"); @@ -2040,7 +2040,7 @@ eval auto r = new RecordVal(cast_intrusive(z.t)); internal-op Init-Vector type V eval auto vt = cast_intrusive(z.t); - auto vec = new VectorVal(vt); + auto vec = new VectorVal(std::move(vt)); Unref(frame[z.v1].vector_val); frame[z.v1].vector_val = vec; @@ -2071,7 +2071,7 @@ macro BuildWhen(timeout) auto wi = aux->wi; FuncPtr func{NewRef{}, frame[z.v1].func_val}; auto lambda = make_intrusive(func); - wi->Instantiate(lambda); + wi->Instantiate(std::move(lambda)); std::vector local_aggrs; for ( int i = 0; i < aux->n; ++i ) { diff --git a/src/script_opt/ZAM/Vars.cc b/src/script_opt/ZAM/Vars.cc index 61460d3cbd..1440320c94 100644 --- a/src/script_opt/ZAM/Vars.cc +++ b/src/script_opt/ZAM/Vars.cc @@ -32,7 +32,9 @@ bool ZAMCompiler::IsCapture(const ID* id) const int ZAMCompiler::CaptureOffset(const ID* id) const { - return pf->CapturesOffsets().find(id)->second; + auto id_offset = pf->CapturesOffsets().find(id); + ASSERT(id_offset != pf->CapturesOffsets().end()); + return id_offset->second; } void ZAMCompiler::LoadParam(const ID* id) From 3e0f8146354bbd89a975de1e5f430a3b9a9db413 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Wed, 16 Aug 2023 16:58:05 -0700 Subject: [PATCH 2/7] disambiguate lambdas by adding scoping and consideration of captures --- src/Expr.cc | 22 +++++++++++++++---- .../language.closure-binding-errors/.stderr | 8 +++---- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index af0cdb22f3..817dd70418 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4673,11 +4673,11 @@ LambdaExpr::LambdaExpr(FunctionIngredientsPtr arg_ing, IDPList arg_outer_ids, st else my_name = name; - // Install that in the global_scope - lambda_id = install_ID(my_name.c_str(), "", true, false); + // Install that in the current scope. + lambda_id = install_ID(my_name.c_str(), current_module.c_str(), true, false); // Update lamb's name - primary_func->SetName(my_name.c_str()); + primary_func->SetName(lambda_id->Name()); auto v = make_intrusive(primary_func); lambda_id->SetVal(std::move(v)); @@ -4800,6 +4800,17 @@ void LambdaExpr::BuildName() ODesc d; primary_func->Describe(&d); + if ( captures ) + for ( auto& c : *captures ) + { + if ( c.IsDeepCopy() ) + d.AddSP("copy"); + + if ( c.Id() ) + // c.Id() will be nil for some errors + c.Id()->Describe(&d); + } + for ( ;; ) { hash128_t h; @@ -4807,7 +4818,7 @@ void LambdaExpr::BuildName() my_name = "lambda_<" + std::to_string(h[0]) + ">"; auto fullname = make_full_var_name(current_module.data(), my_name.data()); - const auto& id = global_scope()->Find(fullname); + const auto& id = current_scope()->Find(fullname); if ( id ) // Just try again to make a unique lambda name. @@ -4873,6 +4884,9 @@ void LambdaExpr::ExprDescribe(ODesc* d) const if ( &c != &(*captures)[0] ) d->AddSP(", "); + if ( c.IsDeepCopy() ) + d->AddSP("copy"); + d->Add(c.Id()->Name()); } diff --git a/testing/btest/Baseline/language.closure-binding-errors/.stderr b/testing/btest/Baseline/language.closure-binding-errors/.stderr index 45bda39df5..91e54506a5 100644 --- a/testing/btest/Baseline/language.closure-binding-errors/.stderr +++ b/testing/btest/Baseline/language.closure-binding-errors/.stderr @@ -1,9 +1,9 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. -error in <...>/closure-binding-errors.zeek, line 12: a is captured but not used inside lambda (function(){ print no a!}) +error in <...>/closure-binding-errors.zeek, line 12: a is captured but not used inside lambda (function()[a]{ print no a!}) error in <...>/closure-binding-errors.zeek, line 13: no such local identifier: a2 -error in <...>/closure-binding-errors.zeek, line 14: b is used inside lambda but not captured (function(){ print b}) -error in <...>/closure-binding-errors.zeek, line 15: a is captured but not used inside lambda (function(){ print b}) -error in <...>/closure-binding-errors.zeek, line 16: b listed multiple times in capture (function(){ print b}) +error in <...>/closure-binding-errors.zeek, line 14: b is used inside lambda but not captured (function()[a]{ print b}) +error in <...>/closure-binding-errors.zeek, line 15: a is captured but not used inside lambda (function()[a, b]{ print b}) +error in <...>/closure-binding-errors.zeek, line 16: b listed multiple times in capture (function()[b, b]{ print b}) error in <...>/closure-binding-errors.zeek, line 18: cannot specify global in capture: c error in <...>/closure-binding-errors.zeek, line 19: cannot specify type in capture: t error in <...>/closure-binding-errors.zeek, line 9 and <...>/closure-binding-errors.zeek, line 20: already defined (a) From 4991693a9c89d7b57f6e7676a7d3ac105d0640f5 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Wed, 16 Aug 2023 17:00:57 -0700 Subject: [PATCH 3/7] fixes to avoid ambiguities in analyzing captures for script optimization --- src/Expr.cc | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/Expr.cc b/src/Expr.cc index 817dd70418..6513cacecb 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4648,6 +4648,7 @@ LambdaExpr::LambdaExpr(FunctionIngredientsPtr arg_ing, IDPList arg_outer_ids, st auto ingr_t = ingredients->GetID()->GetType(); SetType(ingr_t); + captures = ingr_t->GetCaptures(); if ( ! CheckCaptures(std::move(when_parent)) ) { @@ -4655,6 +4656,17 @@ LambdaExpr::LambdaExpr(FunctionIngredientsPtr arg_ing, IDPList arg_outer_ids, st return; } + // Now that we've validated that the captures match the outer_ids, + // we regenerate the latter to come in the same order as the captures. + // This avoids potentially subtle bugs when doing script optimization + // where one context uses the outer_ids and another uses the captures. + if ( captures ) + { + outer_ids.clear(); + for ( auto& c : *captures ) + outer_ids.append(c.Id().get()); + } + // Install a primary version of the function globally. This is used // by both broker (for transmitting closures) and script optimization // (replacing its AST body with a compiled one). @@ -4684,8 +4696,6 @@ LambdaExpr::LambdaExpr(FunctionIngredientsPtr arg_ing, IDPList arg_outer_ids, st lambda_id->SetType(ingr_t); lambda_id->SetConst(); - captures = ingr_t->GetCaptures(); - analyze_lambda(this); } @@ -4714,9 +4724,6 @@ LambdaExpr::LambdaExpr(LambdaExpr* orig) : Expr(EXPR_LAMBDA) bool LambdaExpr::CheckCaptures(StmtPtr when_parent) { - auto ft = type->AsFuncType(); - const auto& captures = ft->GetCaptures(); - auto desc = when_parent ? "\"when\" statement" : "lambda"; if ( ! captures ) From 6af0014a7b4aa4db4c085a73581a8d8e20ffc3f5 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Wed, 16 Aug 2023 17:03:37 -0700 Subject: [PATCH 4/7] fixes for compiling lambdas to C++ --- src/script_opt/CPP/Driver.cc | 3 ++- src/script_opt/ScriptOpt.cc | 5 +++++ src/script_opt/ScriptOpt.h | 3 ++- 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/script_opt/CPP/Driver.cc b/src/script_opt/CPP/Driver.cc index a93b0f603b..2cecb4923c 100644 --- a/src/script_opt/CPP/Driver.cc +++ b/src/script_opt/CPP/Driver.cc @@ -77,8 +77,9 @@ void CPPCompile::Compile(bool report_uncompilable) continue; } - if ( is_when_lambda(f) ) + if ( is_lambda(f) || is_when_lambda(f) ) { + // We deal with these separately. func.SetSkip(true); continue; } diff --git a/src/script_opt/ScriptOpt.cc b/src/script_opt/ScriptOpt.cc index f01ae2db1f..1b085a9dc2 100644 --- a/src/script_opt/ScriptOpt.cc +++ b/src/script_opt/ScriptOpt.cc @@ -58,6 +58,11 @@ void analyze_when_lambda(LambdaExpr* l) when_lambdas.insert(l->PrimaryFunc().get()); } +bool is_lambda(const ScriptFunc* f) + { + return lambdas.count(f) > 0; + } + bool is_when_lambda(const ScriptFunc* f) { return when_lambdas.count(f) > 0; diff --git a/src/script_opt/ScriptOpt.h b/src/script_opt/ScriptOpt.h index 0e8e0ed09f..1a6145f243 100644 --- a/src/script_opt/ScriptOpt.h +++ b/src/script_opt/ScriptOpt.h @@ -174,7 +174,8 @@ extern void analyze_lambda(LambdaExpr* f); // has already been called. extern void analyze_when_lambda(LambdaExpr* f); -// Whether a given script function is a "when" lambda. +// Whether a given script function is a lambda or (separately) a "when" lambda. +extern bool is_lambda(const ScriptFunc* f); extern bool is_when_lambda(const ScriptFunc* f); // Analyze the given top-level statement(s) for optimization. Returns From 4928e074d4a895d564fee8756c5342b6e23896ea Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Wed, 16 Aug 2023 17:04:39 -0700 Subject: [PATCH 5/7] addressed some nits re "-O gen-C++" script optimization --- src/script_opt/CPP/Compile.h | 4 ++-- src/script_opt/CPP/Driver.cc | 2 -- src/script_opt/CPP/Stmts.cc | 6 +++--- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/script_opt/CPP/Compile.h b/src/script_opt/CPP/Compile.h index d0809fbb6a..f8c5822270 100644 --- a/src/script_opt/CPP/Compile.h +++ b/src/script_opt/CPP/Compile.h @@ -830,7 +830,7 @@ private: // If "all_deep" is true, it means make all of the captures // deep copies, not just the ones that were explicitly marked - // as deep copies. That functionality is used to supporting + // as deep copies. That functionality is used to support // Clone() methods; it's not needed when creating a new lambda // instance. std::string GenLambdaClone(const LambdaExpr* l, bool all_deep); @@ -928,7 +928,7 @@ private: // an IntrusivePtr to such a type. const char* IntrusiveVal(const TypePtr& t); - // Maps types to indices in the global "types__CPP" array. + // Maps types to indices in the global "CPP__Type__" array. CPPTracker types = {"types", true}; // Used to prevent analysis of mutually-referring types from diff --git a/src/script_opt/CPP/Driver.cc b/src/script_opt/CPP/Driver.cc index 2cecb4923c..0b23b9bc91 100644 --- a/src/script_opt/CPP/Driver.cc +++ b/src/script_opt/CPP/Driver.cc @@ -124,8 +124,6 @@ void CPPCompile::Compile(bool report_uncompilable) types.AddKey(tp, pfs.HashType(t)); } - Emit("TypePtr types__CPP[%s];", Fmt(static_cast(types.DistinctKeys().size()))); - NL(); for ( auto& g : pfs.AllGlobals() ) diff --git a/src/script_opt/CPP/Stmts.cc b/src/script_opt/CPP/Stmts.cc index a28b4bae92..192fb4ed5c 100644 --- a/src/script_opt/CPP/Stmts.cc +++ b/src/script_opt/CPP/Stmts.cc @@ -89,6 +89,9 @@ void CPPCompile::GenStmt(const Stmt* s) Emit("return false;"); break; + case STMT_FALLTHROUGH: + break; + case STMT_PRINT: { auto el = static_cast(s)->ExprList(); @@ -96,9 +99,6 @@ void CPPCompile::GenStmt(const Stmt* s) } break; - case STMT_FALLTHROUGH: - break; - default: reporter->InternalError("bad statement type in CPPCompile::GenStmt"); } From 81a9745fb36fb6705873921423364bd2c438bfc4 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Wed, 16 Aug 2023 17:07:21 -0700 Subject: [PATCH 6/7] "-O gen-C++" support for "assert" statements --- src/Stmt.cc | 158 ++++++++++-------- src/Stmt.h | 12 ++ src/script_opt/CPP/Compile.h | 2 + src/script_opt/CPP/Stmts.cc | 33 ++++ .../Baseline.cpp/language.assert-misc/out | 7 + 5 files changed, 146 insertions(+), 66 deletions(-) create mode 100644 testing/btest/Baseline.cpp/language.assert-misc/out diff --git a/src/Stmt.cc b/src/Stmt.cc index 6d3abdc963..b7b5a4b667 100644 --- a/src/Stmt.cc +++ b/src/Stmt.cc @@ -164,6 +164,12 @@ const NullStmt* Stmt::AsNullStmt() const return (const NullStmt*)this; } +const AssertStmt* Stmt::AsAssertStmt() const + { + CHECK_TAG(tag, STMT_ASSERT, "Stmt::AsAssertStmt", stmt_name) + return (const AssertStmt*)this; + } + bool Stmt::SetLocationInfo(const Location* start, const Location* end) { if ( ! Obj::SetLocationInfo(start, end) ) @@ -1866,6 +1872,13 @@ AssertStmt::AssertStmt(ExprPtr arg_cond, ExprPtr arg_msg) if ( msg && ! IsString(msg->GetType()->Tag()) ) msg->Error("message must be string"); + + zeek::ODesc desc; + desc.SetShort(true); + desc.SetQuotes(true); + cond->Describe(&desc); + + cond_desc = desc.Description(); } ValPtr AssertStmt::Exec(Frame* f, StmtFlowType& flow) @@ -1873,81 +1886,41 @@ ValPtr AssertStmt::Exec(Frame* f, StmtFlowType& flow) RegisterAccess(); flow = FLOW_NEXT; - static auto assertion_failure_hook = id::find_func("assertion_failure"); static auto assertion_result_hook = id::find_func("assertion_result"); - bool run_result_hook = assertion_result_hook && assertion_result_hook->HasEnabledBodies(); - bool run_failure_hook = assertion_failure_hook && assertion_failure_hook->HasEnabledBodies(); - auto assert_result = cond->Eval(f)->AsBool(); - if ( assert_result && ! run_result_hook ) - return Val::nil; - - // Textual representation of cond from the AST. - static zeek::ODesc desc; - desc.Clear(); - desc.SetShort(true); - desc.SetQuotes(true); - cond->Describe(&desc); - auto cond_val = zeek::make_intrusive(desc.Len(), (const char*)desc.Bytes()); - - zeek::StringValPtr msg_val = zeek::val_mgr->EmptyString(); - if ( msg ) + if ( ! cond->Eval(f)->AsBool() || run_result_hook ) { - // Eval() may fail if expression assumes assert - // condition is F, but we still try to get it for - // the assertion_result hook. - try + zeek::StringValPtr msg_val = zeek::val_mgr->EmptyString(); + + if ( msg ) { - msg_val = cast_intrusive(msg->Eval(f)); - } - catch ( InterpreterException& e ) - { - desc.Clear(); - desc.Add("Describe(&desc); - desc.Add(">"); - msg_val = zeek::make_intrusive(desc.Len(), (const char*)desc.Bytes()); + // Eval() may fail if expression assumes assert + // condition is F, but we still try to get it for + // the assertion_result hook. + try + { + msg_val = cast_intrusive(msg->Eval(f)); + } + catch ( InterpreterException& e ) + { + static ODesc desc; + desc.Clear(); + desc.SetShort(true); + desc.SetQuotes(true); + desc.Add("Describe(&desc); + desc.Add(">"); + msg_val = zeek::make_intrusive(desc.Len(), + (const char*)desc.Bytes()); + } } + + report_assert(assert_result, cond_desc, msg_val, GetLocationInfo()); } - VectorValPtr bt = nullptr; - if ( run_result_hook || run_failure_hook ) - { - bt = get_current_script_backtrace(); - auto assert_elem = make_backtrace_element("assert", MakeEmptyCallArgumentVector(), - GetLocationInfo()); - bt->Insert(0, std::move(assert_elem)); - } - - // Breaking from either the assertion_failure() or assertion_result() - // hook can be used to suppress the default log message. - bool report_error = true; - - if ( run_result_hook ) - report_error &= assertion_result_hook - ->Invoke(zeek::val_mgr->Bool(assert_result), cond_val, msg_val, bt) - ->AsBool(); - - if ( assert_result ) - return Val::nil; - - if ( run_failure_hook ) - report_error &= assertion_failure_hook->Invoke(cond_val, msg_val, bt)->AsBool(); - - if ( report_error ) - { - std::string reporter_msg = util::fmt("assertion failure: %s", cond_val->CheckString()); - if ( msg_val->Len() > 0 ) - reporter_msg += util::fmt(" (%s)", msg_val->CheckString()); - - reporter->PushLocation(GetLocationInfo()); - reporter->Error("%s", reporter_msg.c_str()); - reporter->PopLocation(); - } - - throw InterpreterException(); + return Val::nil; } void AssertStmt::StmtDescribe(ODesc* d) const @@ -1992,6 +1965,59 @@ TraversalCode AssertStmt::Traverse(TraversalCallback* cb) const HANDLE_TC_STMT_POST(tc); } +class AssertException : public InterpreterException + { +public: + AssertException() { } + }; + +void report_assert(bool cond, std::string_view cond_desc, StringValPtr msg_val, const Location* loc) + { + static auto assertion_failure_hook = id::find_func("assertion_failure"); + static auto assertion_result_hook = id::find_func("assertion_result"); + + bool run_result_hook = assertion_result_hook && assertion_result_hook->HasEnabledBodies(); + bool run_failure_hook = assertion_failure_hook && assertion_failure_hook->HasEnabledBodies(); + + auto cond_val = zeek::make_intrusive(cond_desc); + + VectorValPtr bt = nullptr; + if ( run_result_hook || run_failure_hook ) + { + bt = get_current_script_backtrace(); + auto assert_elem = make_backtrace_element("assert", MakeEmptyCallArgumentVector(), loc); + bt->Insert(0, std::move(assert_elem)); + } + + // Breaking from either the assertion_failure() or assertion_result() + // hook can be used to suppress the default log message. + bool report_error = true; + + if ( run_result_hook ) + report_error &= assertion_result_hook + ->Invoke(zeek::val_mgr->Bool(cond), cond_val, msg_val, bt) + ->AsBool(); + + if ( cond ) + return; + + if ( run_failure_hook ) + report_error &= assertion_failure_hook->Invoke(cond_val, msg_val, bt)->AsBool(); + + if ( report_error ) + { + std::string reporter_msg = util::fmt("assertion failure: %s", cond_val->CheckString()); + if ( msg_val->Len() > 0 ) + reporter_msg += util::fmt(" (%s)", msg_val->CheckString()); + + reporter->PushLocation(loc); + reporter->Error("%s", reporter_msg.c_str()); + reporter->PopLocation(); + } + + throw AssertException(); + } + WhenInfo::WhenInfo(ExprPtr arg_cond, FuncType::CaptureList* arg_cl, bool arg_is_return) : cond(std::move(arg_cond)), cl(arg_cl), is_return(arg_is_return) { diff --git a/src/Stmt.h b/src/Stmt.h index 4e73da5728..3f2d956382 100644 --- a/src/Stmt.h +++ b/src/Stmt.h @@ -547,6 +547,10 @@ public: ValPtr Exec(Frame* f, StmtFlowType& flow) override; + const auto& Cond() const { return cond; } + const auto& CondDesc() const { return cond_desc; } + const auto& Msg() const { return msg; } + void StmtDescribe(ODesc* d) const override; TraversalCode Traverse(TraversalCallback* cb) const override; @@ -559,9 +563,17 @@ public: private: ExprPtr cond; + std::string cond_desc; ExprPtr msg; }; +// Helper function for reporting on asserts that either failed, or should +// be processed regardless due to the presence of a "assertion_result" hook. +// +// If "cond" is false, throws an InterpreterException after reporting. +extern void report_assert(bool cond, std::string_view cond_desc, StringValPtr msg_val, + const Location* loc); + // A helper class for tracking all of the information associated with // a "when" statement, and constructing the necessary components in support // of lambda-style captures. diff --git a/src/script_opt/CPP/Compile.h b/src/script_opt/CPP/Compile.h index f8c5822270..04a3591d6a 100644 --- a/src/script_opt/CPP/Compile.h +++ b/src/script_opt/CPP/Compile.h @@ -720,6 +720,8 @@ private: void GenForOverVector(const ExprPtr& tbl, const IDPtr& value_var, const IDPList* loop_vars); void GenForOverString(const ExprPtr& str, const IDPList* loop_vars); + void GenAssertStmt(const AssertStmt* a); + // Nested level of loops/switches for which "break"'s should be // C++ breaks rather than a "hook" break. int break_level = 0; diff --git a/src/script_opt/CPP/Stmts.cc b/src/script_opt/CPP/Stmts.cc index 192fb4ed5c..fd3790f725 100644 --- a/src/script_opt/CPP/Stmts.cc +++ b/src/script_opt/CPP/Stmts.cc @@ -78,6 +78,10 @@ void CPPCompile::GenStmt(const Stmt* s) GenForStmt(s->AsForStmt()); break; + case STMT_ASSERT: + GenAssertStmt(s->AsAssertStmt()); + break; + case STMT_NEXT: Emit("continue;"); break; @@ -567,4 +571,33 @@ void CPPCompile::GenForOverString(const ExprPtr& str, const IDPList* loop_vars) Emit("%s = std::move(sv__CPP);", IDName(lv0)); } +void CPPCompile::GenAssertStmt(const AssertStmt* a) + { + auto& cond = a->Cond(); + auto& msg = a->Msg(); + + Emit("{ // begin a new scope for internal \"assert\" variables"); + Emit("static auto assertion_result_hook = id::find_func(\"assertion_result\");"); + Emit("bool run_result_hook = assertion_result_hook && " + "assertion_result_hook->HasEnabledBodies();"); + Emit("auto assert_result = %s;", GenExpr(cond, GEN_NATIVE)); + Emit("if ( ! assert_result || run_result_hook )"); + + StartBlock(); + if ( msg ) + Emit("auto msg_val = %s;", GenExpr(msg, GEN_VAL_PTR)); + else + Emit("auto msg_val = zeek::val_mgr->EmptyString();"); + + auto loc = a->GetLocationInfo(); + Emit("static Location loc(\"%s\", %s, %s, %s, %s);", loc->filename, + std::to_string(loc->first_line), std::to_string(loc->last_line), + std::to_string(loc->first_column), std::to_string(loc->last_column)); + Emit("report_assert(assert_result, \"%s\", msg_val, &loc);", + CPPEscape(a->CondDesc().c_str()).c_str()); + EndBlock(); + + Emit("} // end of \"assert\" scope"); + } + } // zeek::detail diff --git a/testing/btest/Baseline.cpp/language.assert-misc/out b/testing/btest/Baseline.cpp/language.assert-misc/out new file mode 100644 index 0000000000..1eb5f0a922 --- /dev/null +++ b/testing/btest/Baseline.cpp/language.assert-misc/out @@ -0,0 +1,7 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +f, lambda_<505728364269398358> +compiled-C++ +g, lambda_<8496146571423528161> +compiled-C++ +test_function, test_function +compiled-C++ From 1473149579ad96868624b0d6e84f3e6903c630a0 Mon Sep 17 00:00:00 2001 From: Vern Paxson Date: Wed, 16 Aug 2023 17:08:04 -0700 Subject: [PATCH 7/7] updated notes regarding "-O gen-C++" maintenance --- src/script_opt/CPP/maint/README | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/script_opt/CPP/maint/README b/src/script_opt/CPP/maint/README index d48ba575b6..cb23e5632b 100644 --- a/src/script_opt/CPP/maint/README +++ b/src/script_opt/CPP/maint/README @@ -17,6 +17,15 @@ The maintenance workflow: ninja src/zeek -O use-C++ -r some.pcap + and that it can compile them standalone: + + rm CPP-gen.cc + ninja + echo | src/zeek -O gen-standalone-C++ + ninja + rm CPP-gen.cc + ninja + Do this first because if it can't, you'll be making changes to the compiler that you'll want to subsequent run against the test suite, per the following. @@ -62,11 +71,7 @@ The maintenance workflow: (which is Baseline.cpp). FYI: - There are two tests that in the past have demonstrated possible - variations due to presumed race conditions: + Tests that have demonstrated variations/failures due to presumed + race conditions: ../testing/btest/scripts/base/utils/active-http.test - ../testing/btest/supervisor/revive-leaf.zeek - - These haven't shown up in a while, as of May 2023, so we can remove - the above if they continue to not show up.