diff --git a/CHANGES b/CHANGES index cb508d7b27..05be7f3b82 100644 --- a/CHANGES +++ b/CHANGES @@ -1,4 +1,26 @@ +3.3.0-dev.365 | 2020-10-06 12:19:49 -0700 + + * GH-425: Avoid temporary value while coercing records already of the right type. (Robin Sommer, Corelight) + + The combination of this commit with the previous one now lets the examples + in GH-425 all execute with the same performance. + + * GH-425: Optimize record constructor expression. (Robin Sommer, Corelight) + + We remove the inheritance from UnaryExpr because we know the type of the + operand precisely and can skip a temporary when evaluating the expression. + + * Unify type comparisions for records. (Robin Sommer, Corelight) + + For records, same_type(r1, r2) would not check if the fields' + attributes match as well. That seems like an oversight, and some + callers of same_type() did indeed add that check on their end. This + commit moves the check into same_type() itself. That generally doesn't + seem make any differences except for a couple of places validating + code, which we update a bit. That in turn leans to slightly different + (better?) error messages for a couple of test cases. + 3.3.0-dev.361 | 2020-10-06 10:13:37 -0700 * logging/ascii: Support leftover log rotation in non-supervisor setups (Arne Welzel, Corelight) diff --git a/VERSION b/VERSION index 9a01aa83e7..38b0881589 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -3.3.0-dev.361 +3.3.0-dev.365 diff --git a/src/Expr.cc b/src/Expr.cc index 2a08b630d2..ba8370065b 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2164,26 +2164,16 @@ bool AssignExpr::TypeCheck(const AttributesPtr& attrs) if ( attrs ) attr_copy = std::make_unique>(attrs->GetAttrs()); - bool empty_list_assignment = (op2->AsListExpr()->Exprs().empty()); - if ( op1->GetType()->IsSet() ) op2 = make_intrusive( - cast_intrusive(op2), std::move(attr_copy)); + cast_intrusive(op2), std::move(attr_copy), op1->GetType()); else op2 = make_intrusive( - cast_intrusive(op2), std::move(attr_copy)); + cast_intrusive(op2), std::move(attr_copy), op1->GetType()); - if ( ! empty_list_assignment && ! same_type(op1->GetType(), op2->GetType()) ) - { - if ( op1->GetType()->IsSet() ) - ExprError("set type mismatch in assignment"); - else - ExprError("table type mismatch in assignment"); - - return false; - } - - return true; + // The constructor expressions are performing the type + // checking and will set op2 to an error state on failure. + return ! op2->IsError(); } if ( bt1 == TYPE_VECTOR ) @@ -2207,21 +2197,7 @@ bool AssignExpr::TypeCheck(const AttributesPtr& attrs) op2->GetType()->Tag() == TYPE_RECORD ) { if ( same_type(op1->GetType(), op2->GetType()) ) - { - RecordType* rt1 = op1->GetType()->AsRecordType(); - RecordType* rt2 = op2->GetType()->AsRecordType(); - - // Make sure the attributes match as well. - for ( int i = 0; i < rt1->NumFields(); ++i ) - { - const TypeDecl* td1 = rt1->FieldDecl(i); - const TypeDecl* td2 = rt2->FieldDecl(i); - - if ( same_attrs(td1->attrs.get(), td2->attrs.get()) ) - // Everything matches. - return true; - } - } + return true; // Need to coerce. op2 = make_intrusive(std::move(op2), op1->GetType()); @@ -3077,7 +3053,7 @@ void HasFieldExpr::ExprDescribe(ODesc* d) const } RecordConstructorExpr::RecordConstructorExpr(ListExprPtr constructor_list) - : UnaryExpr(EXPR_RECORD_CONSTRUCTOR, std::move(constructor_list)) + : Expr(EXPR_RECORD_CONSTRUCTOR), op(std::move(constructor_list)) { if ( IsError() ) return; @@ -3130,22 +3106,30 @@ ValPtr RecordConstructorExpr::InitVal(const zeek::Type* t, ValPtr aggr) const return nullptr; } -ValPtr RecordConstructorExpr::Fold(Val* v) const +ValPtr RecordConstructorExpr::Eval(Frame* f) const { - ListVal* lv = v->AsListVal(); + if ( IsError() ) + return nullptr; + + const auto& exprs = op->Exprs(); auto rt = cast_intrusive(type); - if ( lv->Length() != rt->NumFields() ) + if ( exprs.length() != rt->NumFields() ) RuntimeErrorWithCallStack("inconsistency evaluating record constructor"); auto rv = make_intrusive(std::move(rt)); - for ( int i = 0; i < lv->Length(); ++i ) - rv->Assign(i, lv->Idx(i)); + for ( int i = 0; i < exprs.length(); ++i ) + rv->Assign(i, exprs[i]->Eval(f)); return rv; } +bool RecordConstructorExpr::IsPure() const + { + return op->IsPure(); + } + void RecordConstructorExpr::ExprDescribe(ODesc* d) const { d->Add("["); @@ -3153,6 +3137,18 @@ void RecordConstructorExpr::ExprDescribe(ODesc* d) const d->Add("]"); } +TraversalCode RecordConstructorExpr::Traverse(TraversalCallback* cb) const + { + TraversalCode tc = cb->PreExpr(this); + HANDLE_TC_EXPR_PRE(tc); + + tc = op->Traverse(cb); + HANDLE_TC_EXPR_PRE(tc); + + tc = cb->PostExpr(this); + HANDLE_TC_EXPR_POST(tc); + } + TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list, std::unique_ptr> arg_attrs, TypePtr arg_type) @@ -3195,17 +3191,18 @@ TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list, const auto& indices = type->AsTableType()->GetIndices()->GetTypes(); const ExprPList& cle = op->AsListExpr()->Exprs(); - // check and promote all index expressions in ctor list + // check and promote all assign expressions in ctor list for ( const auto& expr : cle ) { if ( expr->Tag() != EXPR_ASSIGN ) continue; - Expr* idx_expr = expr->AsAssignExpr()->Op1(); - - if ( idx_expr->Tag() != EXPR_LIST ) - continue; + auto idx_expr = expr->AsAssignExpr()->Op1(); + auto val_expr = expr->AsAssignExpr()->Op2(); + auto yield_type = GetType()->AsTableType()->Yield().get(); + // Promote LHS + assert(idx_expr->Tag() == EXPR_LIST); ExprPList& idx_exprs = idx_expr->AsListExpr()->Exprs(); if ( idx_exprs.length() != static_cast(indices.size()) ) @@ -3229,6 +3226,19 @@ TableConstructorExpr::TableConstructorExpr(ListExprPtr constructor_list, } ExprError("inconsistent types in table constructor"); + return; + } + + // Promote RHS + if ( auto promoted_val = check_and_promote_expr(val_expr, yield_type) ) + { + if ( promoted_val != val_expr ) + expr->AsAssignExpr()->SetOp2(promoted_val); + } + else + { + ExprError("inconsistent types in table constructor"); + return; } } } @@ -3328,8 +3338,8 @@ SetConstructorExpr::SetConstructorExpr(ListExprPtr constructor_list, Expr* ce = cle[i]; ListExpr* le = ce->AsListExpr(); - if ( ce->Tag() == EXPR_LIST && - check_and_promote_exprs(le, type->AsTableType()->GetIndices().get()) ) + assert(ce->Tag() == EXPR_LIST); + if ( check_and_promote_exprs(le, type->AsTableType()->GetIndices().get()) ) { if ( le != cle[i] ) cle.replace(i, le); @@ -3746,6 +3756,9 @@ ValPtr RecordCoerceExpr::InitVal(const zeek::Type* t, ValPtr aggr) const ValPtr RecordCoerceExpr::Fold(Val* v) const { + if ( same_type(GetType(), Op()->GetType()) ) + return IntrusivePtr{NewRef{}, v}; + auto val = make_intrusive(GetType()); RecordType* val_type = val->GetType()->AsRecordType(); @@ -4985,18 +4998,7 @@ ExprPtr check_and_promote_expr(Expr* const e, zeek::Type* t) RecordType* et_r = et->AsRecordType(); if ( same_type(t, et) ) - { - // Make sure the attributes match as well. - for ( int i = 0; i < t_r->NumFields(); ++i ) - { - const TypeDecl* td1 = t_r->FieldDecl(i); - const TypeDecl* td2 = et_r->FieldDecl(i); - - if ( same_attrs(td1->attrs.get(), td2->attrs.get()) ) - // Everything matches perfectly. - return {NewRef{}, e}; - } - } + return {NewRef{}, e}; if ( record_promotion_compatible(t_r, et_r) ) return make_intrusive( diff --git a/src/Expr.h b/src/Expr.h index 5bb2af6844..1c55055f82 100644 --- a/src/Expr.h +++ b/src/Expr.h @@ -504,6 +504,11 @@ public: ValPtr InitVal(const zeek::Type* t, ValPtr aggr) const override; bool IsPure() const override; + void SetOp2(ExprPtr e) + { + op2 = std::move(e); + } + protected: bool TypeCheck(const AttributesPtr& attrs = nullptr); bool TypeCheckArithmetics(TypeTag bt1, TypeTag bt2); @@ -625,16 +630,25 @@ protected: int field; }; -class RecordConstructorExpr final : public UnaryExpr { +class RecordConstructorExpr final : public Expr { public: explicit RecordConstructorExpr(ListExprPtr constructor_list); ~RecordConstructorExpr() override; + ListExpr* Op() const { return op.get(); } + + ValPtr Eval(Frame* f) const override; + + bool IsPure() const override; + + TraversalCode Traverse(TraversalCallback* cb) const override; + protected: ValPtr InitVal(const zeek::Type* t, ValPtr aggr) const override; - ValPtr Fold(Val* v) const override; void ExprDescribe(ODesc* d) const override; + + ListExprPtr op; }; class TableConstructorExpr final : public UnaryExpr { diff --git a/src/Type.cc b/src/Type.cc index 80dafc7396..35b3cfdc94 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -1690,6 +1690,9 @@ bool same_type(const Type& arg_t1, const Type& arg_t2, if ( (match_record_field_names && ! util::streq(td1->id, td2->id)) || ! same_type(td1->type, td2->type, is_init, match_record_field_names) ) return false; + + if ( ! same_attrs(td1->attrs.get(), td2->attrs.get()) ) + return false; } return true; diff --git a/testing/btest/Baseline/language.record-bad-ctor/out b/testing/btest/Baseline/language.record-bad-ctor/out index e6ff4a8fd5..950175f145 100644 --- a/testing/btest/Baseline/language.record-bad-ctor/out +++ b/testing/btest/Baseline/language.record-bad-ctor/out @@ -1,2 +1,2 @@ -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 6: no type given (asdfasdf) -expression error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 7: uninitialized list value ($ports=asdfasdf) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 6: no type given (asdfasdf) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.record-bad-ctor/record-bad-ctor.zeek, line 7: non-optional field "ports" missing in initialization ([ports=]) diff --git a/testing/btest/Baseline/language.table-list-assign-type-check/output b/testing/btest/Baseline/language.table-list-assign-type-check/output index 2062d3f4f3..d557c007b1 100644 --- a/testing/btest/Baseline/language.table-list-assign-type-check/output +++ b/testing/btest/Baseline/language.table-list-assign-type-check/output @@ -1,3 +1,7 @@ -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 15-20: table type mismatch in assignment (service_table_bad_yield = table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 23-28: table type mismatch in assignment (service_table_bad_index = table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) -error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: set type mismatch in assignment (test_set_bad = set(1, 2, 3)) +error in count and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 16: arithmetic mixed with non-arithmetic (count and Internal Web Server) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 15-20: inconsistent types in table constructor (table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) +error in count and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 24: arithmetic mixed with non-arithmetic (count and 80) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, lines 23-28: inconsistent types in table constructor (table(www, 80 = Internal Web Server, dns1, 53 = Internal DNS 1, dns2, 53 = Internal DNS 2, dhcp-for-wifi, 443 = DHCP Management interface for WiFi)) +error in string and /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: arithmetic mixed with non-arithmetic (string and 1) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31 and string: type mismatch (1 and string) +error in /Users/robin/bro/topic/testing/btest/.tmp/language.table-list-assign-type-check/table-list-assign-type-check.zeek, line 31: inconsistent type in set constructor (set(1, 2, 3))