Merge remote-tracking branch 'origin/topic/awelzel/2486-count-underflow'

* origin/topic/awelzel/2486-count-underflow:
  Expr: Warn on count underflow for c -= 1 and c = c - 1
  Reporter: Add ExprRuntimeWarning()

NEWS

@@ -146,6 +146,15 @@ Changed Functionality
   stopped. This fixes a few cases where we already had the logic to
   continue in place, but we still ended up considering them partial.
 
+- Count underflows via ``--c`` or subtract from statements (``c = c - 1``) are
+  now consistently warned about. Previously, underflows through ``--c`` were
+  treated as runtime errors, while "subtract from" underflows were silently
+  accepted. The following (surprising behavior) now causes a warning, too:
+
+	$ zeek -e 'print 1 - 2'
+	expression warning in <command line>, line 1: count underflow (1 - 2)
+	18446744073709551615
+
 Deprecated Functionality
 ------------------------
 

src/Expr.cc

@@ -274,14 +274,7 @@ void Expr::AssignToIndex(ValPtr v1, ValPtr v2, ValPtr v3) const
 	                                 iterators_invalidated);
 
 	if ( iterators_invalidated )
-		{
+		reporter->ExprRuntimeWarning(this, "possible loop/iterator invalidation");
-		ODesc d;
-		Describe(&d);
-		reporter->PushLocation(GetLocationInfo());
-		reporter->Warning("possible loop/iterator invalidation caused by expression: %s",
-		                  d.Description());
-		reporter->PopLocation();
-		}
 
 	if ( error_msg )
 		RuntimeErrorWithCallStack(error_msg);
@@ -895,6 +888,10 @@ ValPtr BinaryExpr::Fold(Val* v1, Val* v2) const
 		case EXPR_SUB:
 		case EXPR_REMOVE_FROM:
 			DO_FOLD(-);
+			// When subtracting and the result is larger than the left
+			// operand we mostly likely underflowed and log a warning.
+			if ( is_unsigned && u3 > u1 )
+				reporter->ExprRuntimeWarning(this, "count underflow");
 			break;
 		case EXPR_TIMES:
 			DO_FOLD(*);
@@ -1454,7 +1451,7 @@ ValPtr IncrExpr::DoSingleEval(Frame* f, Val* v) const
 		--k;
 
 		if ( k < 0 && v->GetType()->InternalType() == TYPE_INTERNAL_UNSIGNED )
-			RuntimeError("count underflow");
+			reporter->ExprRuntimeWarning(this, "count underflow");
 		}
 
 	const auto& ret_type = IsVector(GetType()->Tag()) ? GetType()->Yield() : GetType();
@@ -2974,14 +2971,7 @@ void IndexExpr::Add(Frame* f)
 	v1->AsTableVal()->Assign(std::move(v2), nullptr, true, &iterators_invalidated);
 
 	if ( iterators_invalidated )
-		{
+		reporter->ExprRuntimeWarning(this, "possible loop/iterator invalidation");
-		ODesc d;
-		Describe(&d);
-		reporter->PushLocation(GetLocationInfo());
-		reporter->Warning("possible loop/iterator invalidation caused by expression: %s",
-		                  d.Description());
-		reporter->PopLocation();
-		}
 	}
 
 void IndexExpr::Delete(Frame* f)
@@ -3003,14 +2993,7 @@ void IndexExpr::Delete(Frame* f)
 	v1->AsTableVal()->Remove(*v2, true, &iterators_invalidated);
 
 	if ( iterators_invalidated )
-		{
+		reporter->ExprRuntimeWarning(this, "possible loop/iterator invalidation");
-		ODesc d;
-		Describe(&d);
-		reporter->PushLocation(GetLocationInfo());
-		reporter->Warning("possible loop/iterator invalidation caused by expression: %s",
-		                  d.Description());
-		reporter->PopLocation();
-		}
 	}
 
 ExprPtr IndexExpr::MakeLvalue()

src/Reporter.cc

@@ -174,6 +174,21 @@ void Reporter::ExprRuntimeError(const detail::Expr* expr, const char* fmt, ...)
 	throw InterpreterException();
 	}
 
+void Reporter::ExprRuntimeWarning(const detail::Expr* expr, const char* fmt, ...)
+	{
+	ODesc d;
+	expr->Describe(&d);
+
+	PushLocation(expr->GetLocationInfo());
+	va_list ap;
+	va_start(ap, fmt);
+	FILE* out = EmitToStderr(warnings_to_stderr) ? stderr : nullptr;
+	DoLog("expression warning", reporter_warning, out, nullptr, nullptr, true, true,
+	      d.Description(), fmt, ap);
+	va_end(ap);
+	PopLocation();
+	}
+
 void Reporter::RuntimeError(const detail::Location* location, const char* fmt, ...)
 	{
 	++errors;

src/Reporter.h

@@ -112,6 +112,10 @@ public:
 	[[noreturn]] void RuntimeError(const detail::Location* location, const char* fmt, ...)
 		__attribute__((format(printf, 3, 4)));
 
+	// Report a rutnime warning in evaluating a Zeek script expression.
+	void ExprRuntimeWarning(const detail::Expr* expr, const char* fmt, ...)
+		__attribute__((format(printf, 3, 4)));
+
 	// Report a runtime error in executing a compiled script. This
 	// function will not return but raise an InterpreterException.
 	[[noreturn]] void CPPRuntimeError(const char* fmt, ...) __attribute__((format(printf, 2, 3)));

testing/btest/Baseline/language.count-underflow/.stderr

@@ -0,0 +1,5 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+expression warning in <...>/count-underflow.zeek, line 10: count underflow (--c1)
+expression warning in <...>/count-underflow.zeek, line 14: count underflow (c2 - 1)
+expression warning in <...>/count-underflow.zeek, line 18: count underflow (c3 - 1)
+expression warning in <...>/count-underflow.zeek, line 22: count underflow (1 - 2)

testing/btest/Baseline/language.count-underflow/out

@@ -0,0 +1,8 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+local c1 = 0; --c1; c1 == 18446744073709551615; T
+local c2 = 0; c2 -= 1; c2 == 18446744073709551615; T
+local c3 = 0; c3 = c3 - 1; c3 == 18446744073709551615; T
+1 - 2, 18446744073709551615
+local c4 = count_max; ++c4; c4 == 0; T
+local c5 = count_max; c5 += 1; c5 == 0; T
+local c6 = count_max; c6 = c6 + 1; c6 == 0; T

testing/btest/Baseline/language.sizeof/.stderr

@@ -0,0 +1,2 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+expression warning in <...>/sizeof.zeek, line 73: count underflow (5 - 9)

testing/btest/Baseline/language.table-set-iterator-invalidation/err

@@ -1,5 +1,5 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-warning in <...>/table-set-iterator-invalidation.zeek, line 22: possible loop/iterator invalidation caused by expression: t[4]
+expression warning in <...>/table-set-iterator-invalidation.zeek, line 22: possible loop/iterator invalidation (t[4])
-warning in <...>/table-set-iterator-invalidation.zeek, line 31: possible loop/iterator invalidation caused by expression: t[4]
+expression warning in <...>/table-set-iterator-invalidation.zeek, line 31: possible loop/iterator invalidation (t[4])
-warning in <...>/table-set-iterator-invalidation.zeek, line 54: possible loop/iterator invalidation caused by expression: s[4]
+expression warning in <...>/table-set-iterator-invalidation.zeek, line 54: possible loop/iterator invalidation (s[4])
-warning in <...>/table-set-iterator-invalidation.zeek, line 63: possible loop/iterator invalidation caused by expression: s[4]
+expression warning in <...>/table-set-iterator-invalidation.zeek, line 63: possible loop/iterator invalidation (s[4])

testing/btest/language/count-underflow.zeek

@@ -0,0 +1,36 @@
+# @TEST-EXEC: zeek -b %INPUT >out
+# @TEST-EXEC: btest-diff out
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr
+
+global count_max: count = 0xffffffffffffffff;
+
+event zeek_init()
+{
+	local c1 = 0;
+	--c1;
+	print fmt("local c1 = 0; --c1; c1 == %s; %s", c1, c1 == count_max);
+
+	local c2 = 0;
+	c2 -= 1;
+	print fmt("local c2 = 0; c2 -= 1; c2 == %s; %s", c2, c2 == count_max);
+
+	local c3 = 0;
+	c3 = c3 - 1;
+	print fmt("local c3 = 0; c3 = c3 - 1; c3 == %s; %s", c3, c3 == count_max);
+
+	# This also triggers a warning now;
+	print "1 - 2", 1 - 2;
+
+	# The following ones all overflow back to 0, but do not log a warning.
+	local c4 = count_max;
+	++c4;
+	print fmt("local c4 = count_max; ++c4; c4 == %s; %s", c4, c4 == 0);
+
+	local c5 = count_max;
+	c5 += 1;
+	print fmt("local c5 = count_max; c5 += 1; c5 == %s; %s", c5, c5 == 0);
+
+	local c6 = count_max;
+	c6 = c6 + 1;
+	print fmt("local c6 = count_max; c6 = c6 + 1; c6 == %s; %s", c6, c6 == 0);
+}

testing/btest/language/sizeof.zeek

@@ -1,5 +1,6 @@
-# @TEST-EXEC: zeek -b %INPUT  >output 2>&1
+# @TEST-EXEC: zeek -b %INPUT  >output
 # @TEST-EXEC: btest-diff output
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff .stderr
 
 # Demo policy for the sizeof operator "|x|".
 # ------------------------------------------