From 72432921364f6dbb3783b3f0e893f36d2710fc81 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Wed, 29 May 2019 20:07:30 -0700 Subject: [PATCH 1/5] Move #define outside of max_type for clarity --- src/Type.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Type.cc b/src/Type.cc index f9cb915c71..3fda9aa7ce 100644 --- a/src/Type.cc +++ b/src/Type.cc @@ -2123,6 +2123,10 @@ int is_assignable(BroType* t) return 0; } +#define CHECK_TYPE(t) \ + if ( t1 == t || t2 == t ) \ + return t; + TypeTag max_type(TypeTag t1, TypeTag t2) { if ( t1 == TYPE_INTERVAL || t1 == TYPE_TIME ) @@ -2132,10 +2136,6 @@ TypeTag max_type(TypeTag t1, TypeTag t2) if ( BothArithmetic(t1, t2) ) { -#define CHECK_TYPE(t) \ - if ( t1 == t || t2 == t ) \ - return t; - CHECK_TYPE(TYPE_DOUBLE); CHECK_TYPE(TYPE_INT); CHECK_TYPE(TYPE_COUNT); From 8ca2cff13f3b170d2420fdd3f26ba868172931f0 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 May 2019 10:25:48 -0700 Subject: [PATCH 2/5] Add CLion directories to gitignore --- .gitignore | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.gitignore b/.gitignore index fa397f98d2..8dacf57dc7 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,7 @@ build tmp *.gcov + +# Configuration and build directories for CLion +.idea +cmake-build-debug \ No newline at end of file From 2d61ea5cd66c6c8f2e7e024a9eda27b6fb7d3090 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 30 May 2019 15:36:30 -0700 Subject: [PATCH 3/5] Allow passing a location to BroObj::Warning and BroObj::Error. This allows callers (such as check_and_promote) to pass an expression location to be logged if the location doesn't exist in the value being promoted. --- src/Obj.cc | 12 +++++++----- src/Obj.h | 6 +++--- src/Val.cc | 10 +++++----- src/Val.h | 2 +- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/Obj.cc b/src/Obj.cc index 9c3b50a950..fdf2b7ef99 100644 --- a/src/Obj.cc +++ b/src/Obj.cc @@ -100,21 +100,21 @@ BroObj::~BroObj() delete location; } -void BroObj::Warn(const char* msg, const BroObj* obj2, int pinpoint_only) const +void BroObj::Warn(const char* msg, const BroObj* obj2, int pinpoint_only, const Location* expr_location) const { ODesc d; - DoMsg(&d, msg, obj2, pinpoint_only); + DoMsg(&d, msg, obj2, pinpoint_only, expr_location); reporter->Warning("%s", d.Description()); reporter->PopLocation(); } -void BroObj::Error(const char* msg, const BroObj* obj2, int pinpoint_only) const +void BroObj::Error(const char* msg, const BroObj* obj2, int pinpoint_only, const Location* expr_location) const { if ( suppress_errors ) return; ODesc d; - DoMsg(&d, msg, obj2, pinpoint_only); + DoMsg(&d, msg, obj2, pinpoint_only, expr_location); reporter->Error("%s", d.Description()); reporter->PopLocation(); } @@ -200,7 +200,7 @@ void BroObj::UpdateLocationEndInfo(const Location& end) } void BroObj::DoMsg(ODesc* d, const char s1[], const BroObj* obj2, - int pinpoint_only) const + int pinpoint_only, const Location* expr_location) const { d->SetShort(); @@ -211,6 +211,8 @@ void BroObj::DoMsg(ODesc* d, const char s1[], const BroObj* obj2, if ( obj2 && obj2->GetLocationInfo() != &no_location && *obj2->GetLocationInfo() != *GetLocationInfo() ) loc2 = obj2->GetLocationInfo(); + else if ( expr_location ) + loc2 = expr_location; reporter->PushLocation(GetLocationInfo(), loc2); } diff --git a/src/Obj.h b/src/Obj.h index 047eec0856..21730ff367 100644 --- a/src/Obj.h +++ b/src/Obj.h @@ -118,9 +118,9 @@ public: // included in the message, though if pinpoint_only is non-zero, // then obj2 is only used to pinpoint the location. void Warn(const char* msg, const BroObj* obj2 = 0, - int pinpoint_only = 0) const; + int pinpoint_only = 0, const Location* expr_location = 0) const; void Error(const char* msg, const BroObj* obj2 = 0, - int pinpoint_only = 0) const; + int pinpoint_only = 0, const Location* expr_location = 0) const; // Report internal errors. void BadTag(const char* msg, const char* t1 = 0, @@ -178,7 +178,7 @@ private: friend class SuppressErrors; void DoMsg(ODesc* d, const char s1[], const BroObj* obj2 = 0, - int pinpoint_only = 0) const; + int pinpoint_only = 0, const Location* expr_location = 0) const; void PinPoint(ODesc* d, const BroObj* obj2 = 0, int pinpoint_only = 0) const; diff --git a/src/Val.cc b/src/Val.cc index 50c36d7239..d989bce461 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -3556,7 +3556,7 @@ bool OpaqueVal::DoUnserialize(UnserialInfo* info) return true; } -Val* check_and_promote(Val* v, const BroType* t, int is_init) +Val* check_and_promote(Val* v, const BroType* t, int is_init, const Location* expr_location) { if ( ! v ) return 0; @@ -3580,7 +3580,7 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) if ( same_type(t, vt, is_init) ) return v; - t->Error("type clash", v); + t->Error("type clash", v, 0, expr_location); Unref(v); return 0; } @@ -3589,9 +3589,9 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) (! IsArithmetic(v_tag) || t_tag != TYPE_TIME || ! v->IsZero()) ) { if ( t_tag == TYPE_LIST || v_tag == TYPE_LIST ) - t->Error("list mixed with scalar", v); + t->Error("list mixed with scalar", v, 0, expr_location); else - t->Error("arithmetic mixed with non-arithmetic", v); + t->Error("arithmetic mixed with non-arithmetic", v, 0, expr_location); Unref(v); return 0; } @@ -3604,7 +3604,7 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init) TypeTag mt = max_type(t_tag, v_tag); if ( mt != t_tag ) { - t->Error("over-promotion of arithmetic value", v); + t->Error("over-promotion of arithmetic value", v, 0, expr_location); Unref(v); return 0; } diff --git a/src/Val.h b/src/Val.h index c253c265db..c854d2b015 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1219,7 +1219,7 @@ protected: // Unref()'ing the original. If not a match, generates an error message // and returns nil, also Unref()'ing v. If is_init is true, then // the checking is done in the context of an initialization. -extern Val* check_and_promote(Val* v, const BroType* t, int is_init); +extern Val* check_and_promote(Val* v, const BroType* t, int is_init, const Location* expr_location = nullptr); // Given a pointer to where a Val's core (i.e., its BRO value) resides, // returns a corresponding newly-created or Ref()'d Val. ptr must already From 76fe643c87522552755effba6cab6d37793d94fd Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Wed, 29 May 2019 09:53:09 -0700 Subject: [PATCH 4/5] GH-159: Allow coercion of numeric values into other types --- src/Expr.cc | 36 +++++--- src/Val.cc | 47 +++++++++- src/Val.h | 2 + .../double_convert_failure1.out | 1 + .../double_convert_failure2.out | 1 + .../first_set.out | 16 ++++ .../btest/language/type-coerce-numerics.zeek | 85 +++++++++++++++++++ 7 files changed, 173 insertions(+), 15 deletions(-) create mode 100644 testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure1.out create mode 100644 testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure2.out create mode 100644 testing/btest/Baseline/language.type-coerce-numerics/first_set.out create mode 100644 testing/btest/language/type-coerce-numerics.zeek diff --git a/src/Expr.cc b/src/Expr.cc index 27d5de135b..f4a0ca31cd 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -4136,15 +4136,15 @@ RecordCoerceExpr::RecordCoerceExpr(Expr* op, RecordType* r) if ( ! same_type(sup_t_i, sub_t_i) ) { - if ( sup_t_i->Tag() != TYPE_RECORD || - sub_t_i->Tag() != TYPE_RECORD || - ! record_promotion_compatible(sup_t_i->AsRecordType(), - sub_t_i->AsRecordType()) ) + if ( ( ! BothArithmetic(sup_t_i->Tag(), sub_t_i->Tag()) || ( sub_t_i->Tag() == TYPE_DOUBLE && IsIntegral(sup_t_i->Tag()) ) ) && + ( sup_t_i->Tag() != TYPE_RECORD || + sub_t_i->Tag() != TYPE_RECORD || + ! record_promotion_compatible(sup_t_i->AsRecordType(), + sub_t_i->AsRecordType()) ) ) { - char buf[512]; - safe_snprintf(buf, sizeof(buf), + string error_msg = fmt( "type clash for field \"%s\"", sub_r->FieldName(i)); - Error(buf, sub_t_i); + Error(error_msg.c_str(), sub_t_i); SetError(); break; } @@ -4162,11 +4162,9 @@ RecordCoerceExpr::RecordCoerceExpr(Expr* op, RecordType* r) { if ( ! t_r->FieldDecl(i)->FindAttr(ATTR_OPTIONAL) ) { - char buf[512]; - safe_snprintf(buf, sizeof(buf), - "non-optional field \"%s\" missing", - t_r->FieldName(i)); - Error(buf); + string error_msg = fmt( + "non-optional field \"%s\" missing", t_r->FieldName(i)); + Error(error_msg.c_str()); SetError(); break; } @@ -4254,6 +4252,20 @@ Val* RecordCoerceExpr::Fold(Val* v) const rhs = new_val; } } + else if ( BothArithmetic(rhs_type->Tag(), field_type->Tag()) && + ! same_type(rhs_type, field_type) ) + { + if ( Val *new_val = check_and_promote(rhs, field_type, false, op->GetLocationInfo()) ) + { + // Don't call unref here on rhs because check_and_promote already called it. + rhs = new_val; + } + else + { + Unref(val); + RuntimeError("Failed type conversion"); + } + } val->Assign(i, rhs); } diff --git a/src/Val.cc b/src/Val.cc index d989bce461..3198457455 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -566,6 +566,35 @@ void Val::ValDescribeReST(ODesc* d) const } } + +bool Val::WouldOverflow(const BroType* from_type, const BroType* to_type, const Val* val) + { + if ( !to_type || !from_type ) + return true; + else if ( same_type(to_type, from_type) ) + return false; + + if ( to_type->InternalType() == TYPE_INTERNAL_DOUBLE ) + return false; + else if ( to_type->InternalType() == TYPE_INTERNAL_UNSIGNED ) + { + if ( from_type->InternalType() == TYPE_INTERNAL_DOUBLE ) + return (val->InternalDouble() < 0.0 || val->InternalDouble() > static_cast(UINT64_MAX)); + else if ( from_type->InternalType() == TYPE_INTERNAL_INT ) + return (val->InternalInt() < 0); + } + else if ( to_type->InternalType() == TYPE_INTERNAL_INT ) + { + if ( from_type->InternalType() == TYPE_INTERNAL_DOUBLE ) + return (val->InternalDouble() < static_cast(INT64_MIN) || + val->InternalDouble() > static_cast(INT64_MAX)); + else if ( from_type->InternalType() == TYPE_INTERNAL_UNSIGNED ) + return (val->InternalUnsigned() > INT64_MAX); + } + + return false; + } + MutableVal::~MutableVal() { for ( list::iterator i = aliases.begin(); i != aliases.end(); ++i ) @@ -3599,7 +3628,7 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init, const Location* ex if ( v_tag == t_tag ) return v; - if ( t_tag != TYPE_TIME ) + if ( t_tag != TYPE_TIME && ! BothArithmetic(t_tag, v_tag) ) { TypeTag mt = max_type(t_tag, v_tag); if ( mt != t_tag ) @@ -3621,7 +3650,13 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init, const Location* ex Val* promoted_v; switch ( it ) { case TYPE_INTERNAL_INT: - if ( t_tag == TYPE_INT ) + if ( ( vit == TYPE_INTERNAL_UNSIGNED || vit == TYPE_INTERNAL_DOUBLE ) && Val::WouldOverflow(vt, t, v) ) + { + t->Error("overflow promoting from unsigned/double to signed arithmetic value", v, 0, expr_location); + Unref(v); + return 0; + } + else if ( t_tag == TYPE_INT ) promoted_v = val_mgr->GetInt(v->CoerceToInt()); else if ( t_tag == TYPE_BOOL ) promoted_v = val_mgr->GetBool(v->CoerceToInt()); @@ -3635,7 +3670,13 @@ Val* check_and_promote(Val* v, const BroType* t, int is_init, const Location* ex break; case TYPE_INTERNAL_UNSIGNED: - if ( t_tag == TYPE_COUNT || t_tag == TYPE_COUNTER ) + if ( ( vit == TYPE_INTERNAL_DOUBLE || vit == TYPE_INTERNAL_INT) && Val::WouldOverflow(vt, t, v) ) + { + t->Error("overflow promoting from signed/double to unsigned arithmetic value", v, 0, expr_location); + Unref(v); + return 0; + } + else if ( t_tag == TYPE_COUNT || t_tag == TYPE_COUNTER ) promoted_v = val_mgr->GetCount(v->CoerceToUnsigned()); else // port { diff --git a/src/Val.h b/src/Val.h index c854d2b015..261c1edeb6 100644 --- a/src/Val.h +++ b/src/Val.h @@ -367,6 +367,8 @@ public: } #endif + static bool WouldOverflow(const BroType* from_type, const BroType* to_type, const Val* val); + protected: friend class EnumType; diff --git a/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure1.out b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure1.out new file mode 100644 index 0000000000..d139c43a0c --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure1.out @@ -0,0 +1 @@ +error in ./double_convert_failure1.zeek, line 7 and double: type clash for field "cc" ((coerce [$cc=5.0] to myrecord) and double) diff --git a/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure2.out b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure2.out new file mode 100644 index 0000000000..2cc83659de --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/double_convert_failure2.out @@ -0,0 +1 @@ +error in ./double_convert_failure2.zeek, line 7 and double: type clash for field "cc" ((coerce [$cc=-5.0] to myrecord) and double) diff --git a/testing/btest/Baseline/language.type-coerce-numerics/first_set.out b/testing/btest/Baseline/language.type-coerce-numerics/first_set.out new file mode 100644 index 0000000000..fa756b37d9 --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/first_set.out @@ -0,0 +1,16 @@ +error in count and ./first_set.zeek, line 46: overflow promoting from signed/double to unsigned arithmetic value (count and -5) +expression error in ./first_set.zeek, line 46: Failed type conversion ((coerce [$cc=-5] to record { ii:int; cc:count; dd:double; })) +error in int and ./first_set.zeek, line 53: overflow promoting from unsigned/double to signed arithmetic value (int and 9223372036854775808) +expression error in ./first_set.zeek, line 53: Failed type conversion ((coerce [$ii=9223372036854775808] to record { ii:int; cc:count; dd:double; })) +3 +int +4 +int +5 +int +6 +int +7.0 +double +-5.0 +double diff --git a/testing/btest/language/type-coerce-numerics.zeek b/testing/btest/language/type-coerce-numerics.zeek new file mode 100644 index 0000000000..c8b4a58f13 --- /dev/null +++ b/testing/btest/language/type-coerce-numerics.zeek @@ -0,0 +1,85 @@ +# @TEST-EXEC: zeek -b first_set.zeek >first_set.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff first_set.out +# @TEST-EXEC-FAIL: zeek -b double_convert_failure1.zeek >double_convert_failure1.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff double_convert_failure1.out +# @TEST-EXEC-FAIL: zeek -b double_convert_failure2.zeek >double_convert_failure2.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff double_convert_failure2.out + +@TEST-START-FILE first_set.zeek +type myrecord : record { + ii: int &optional; + cc: count &optional; + dd: double &optional; +}; + +# Allow coercion from count values to int +global globalint: myrecord &redef; +redef globalint = [$ii = 2]; + +# All of these cases should succeed +event zeek_init() + { + # Allow coercion from count values to int + local intconvert1 = myrecord($ii = 3); + print(intconvert1$ii); + print(type_name(intconvert1$ii)); + + local intconvert2: myrecord = record($ii = 4); + print(intconvert2$ii); + print(type_name(intconvert2$ii)); + + local intconvert3: myrecord = [$ii = 5]; + print(intconvert3$ii); + print(type_name(intconvert3$ii)); + + local intconvert4: myrecord; + intconvert4$ii = 6; + print(intconvert4$ii); + print(type_name(intconvert4$ii)); + + # Convert from count/integer values into doubles + local doubleconvert1 = myrecord($dd = 7); + print(doubleconvert1$dd); + print(type_name(doubleconvert1$dd)); + + local doubleconvert2 = myrecord($dd = -5); + print(doubleconvert2$dd); + print(type_name(doubleconvert2$dd)); + } + +# The following cases should throw errors. +event zeek_init() + { + # Throw an error for trying to coerce negative values to unsigned + local negative = myrecord($cc = -5); + } + +event zeek_init() + { + # This value is INT64_MAX+1, which overflows a signed integer and + # throws an error + local overflow = myrecord($ii = 9223372036854775808); + } +@TEST-END-FILE + +@TEST-START-FILE double_convert_failure1.zeek +type myrecord : record { + cc: count &optional; +}; + +event zeek_init() + { + local convert = myrecord($cc = 5.0); + } +@TEST-END-FILE + +@TEST-START-FILE double_convert_failure2.zeek +type myrecord : record { + cc: count &optional; +}; + +event zeek_init() + { + local convert = myrecord($cc = -5.0); + } +@TEST-END-FILE \ No newline at end of file From 394aec5a7227eabdbe35b2b741928fec80e12982 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Tue, 4 Jun 2019 14:59:17 -0700 Subject: [PATCH 5/5] GHI-155: set the type of a vector based on the variable's type, not the value's type --- src/Expr.cc | 2 +- .../language.type-coerce-numerics/vectors.out | 18 +++++++++ .../btest/language/type-coerce-numerics.zeek | 37 +++++++++++++++++++ 3 files changed, 56 insertions(+), 1 deletion(-) create mode 100644 testing/btest/Baseline/language.type-coerce-numerics/vectors.out diff --git a/src/Expr.cc b/src/Expr.cc index f4a0ca31cd..e4084ded7d 100644 --- a/src/Expr.cc +++ b/src/Expr.cc @@ -2591,7 +2591,7 @@ bool AssignExpr::TypeCheck(attr_list* attrs) if ( op2->Tag() == EXPR_LIST ) { - op2 = new VectorConstructorExpr(op2->AsListExpr()); + op2 = new VectorConstructorExpr(op2->AsListExpr(), op1->Type()); return true; } } diff --git a/testing/btest/Baseline/language.type-coerce-numerics/vectors.out b/testing/btest/Baseline/language.type-coerce-numerics/vectors.out new file mode 100644 index 0000000000..5baa5f67c7 --- /dev/null +++ b/testing/btest/Baseline/language.type-coerce-numerics/vectors.out @@ -0,0 +1,18 @@ +vector of count +vector of count +vector of count +[1, 2] +[3, 4] +[4, 6] +vector of int +vector of int +vector of int +[1, 2] +[3, 4] +[4, 6] +vector of double +vector of double +vector of double +[1.0, 2.0] +[3.0, 4.0] +[4.0, 6.0] diff --git a/testing/btest/language/type-coerce-numerics.zeek b/testing/btest/language/type-coerce-numerics.zeek index c8b4a58f13..36f74ce859 100644 --- a/testing/btest/language/type-coerce-numerics.zeek +++ b/testing/btest/language/type-coerce-numerics.zeek @@ -4,6 +4,8 @@ # @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff double_convert_failure1.out # @TEST-EXEC-FAIL: zeek -b double_convert_failure2.zeek >double_convert_failure2.out 2>&1 # @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff double_convert_failure2.out +# @TEST-EXEC: zeek -b vectors.zeek >vectors.out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff vectors.out @TEST-START-FILE first_set.zeek type myrecord : record { @@ -82,4 +84,39 @@ event zeek_init() { local convert = myrecord($cc = -5.0); } +@TEST-END-FILE + +@TEST-START-FILE vectors.zeek +event zeek_init() + { + local c1 : vector of count = { 1 , 2 }; + local c2 : vector of count = { 3 , 4 }; + local c3 = c1 + c2; + print type_name(c1); + print type_name(c2); + print type_name(c3); + print c1; + print c2; + print c3; + + local i1 : vector of int = { 1, 2 }; + local i2 : vector of int = { 3, 4 }; + local i3 = i1 + i2; + print type_name(i1); + print type_name(i2); + print type_name(i3); + print i1; + print i2; + print i3; + + local d1 : vector of double = { 1, 2 }; + local d2 : vector of double = { 3, 4 }; + local d3 = d1 + d2; + print type_name(d1); + print type_name(d2); + print type_name(d3); + print d1; + print d2; + print d3; + } @TEST-END-FILE \ No newline at end of file