Merge remote-tracking branch 'origin/topic/vern/cpp-prep-factoring'

* origin/topic/vern/cpp-prep-factoring:
  test suite update due to factoring out coerce_to_record()
  removal of vestigial #include's (breaking an include loop in the process)
  better method name: HasCopySemantics()
  Use STL functionality to initialize coercion map
  comments for factored-out index slice functions
  support for subclassing ScriptFunc's, esp. for alternate lambda closures
  factor out record coercion; modernize management of coercion "map"
  lower-level method for adding fields to records
  factor out "print" statement's execution functionality
  functions for indexing slices and strings
  new function for getting the location, if any, associated with the current call

CHANGES

@@ -1,3 +1,26 @@
+4.1.0-dev.417 | 2021-03-25 11:37:55 -0700
+
+  * test suite update due to factoring out coerce_to_record() (Vern Paxson, Corelight)
+
+  * removal of vestigial #include's (breaking an include loop in the process) (Vern Paxson, Corelight)
+
+  * better method name: HasCopySemantics() (Vern Paxson, Corelight)
+
+  * Use STL functionality to initialize coercion map (Vern Paxson, Corelight)
+
+  * comments for factored-out index slice functions (Vern Paxson, Corelight)
+
+  * support for subclassing ScriptFunc's, esp. for alternate lambda closures (Vern Paxson, Corelight)
+
+  * factor out record coercion; modernize management of coercion "map" (Vern Paxson, Corelight)
+
+  * lower-level method for adding fields to records (Vern Paxson, Corelight)
+
+  * factor out "print" statement's execution functionality (Vern Paxson, Corelight)
+
+  * functions for indexing slices and strings (Vern Paxson, Corelight)
+
+  * new function for getting the location, if any, associated with the current call (Vern Paxson, Corelight)
 
 4.1.0-dev.404 | 2021-03-24 16:58:50 -0700
 

VERSION

@@ -1 +1 @@
-4.1.0-dev.404
+4.1.0-dev.417

doc

@@ -1 +1 @@
-Subproject commit afd337f4b00477401514ae3dc9c20476229318dc
+Subproject commit 412a46b035a646f890933de84ac2e7b71c6473a7

src/Expr.cc

@@ -2936,24 +2936,7 @@ ValPtr IndexExpr::Fold(Val* v1, Val* v2) const
 		if ( lv->Length() == 1 )
 			v = vect->ValAt(lv->Idx(0)->CoerceToUnsigned());
 		else
-			{
+			return index_slice(vect, lv);
-			size_t len = vect->Size();
-			auto result = make_intrusive<VectorVal>(vect->GetType<VectorType>());
-
-			bro_int_t first = get_slice_index(lv->Idx(0)->CoerceToInt(), len);
-			bro_int_t last = get_slice_index(lv->Idx(1)->CoerceToInt(), len);
-			bro_int_t sub_length = last - first;
-
-			if ( sub_length >= 0 )
-				{
-				result->Resize(sub_length);
-
-				for ( bro_int_t idx = first; idx < last; idx++ )
-					result->Assign(idx - first, vect->ValAt(idx));
-				}
-
-			return result;
-			}
 		}
 		break;
 
@@ -2962,9 +2945,22 @@ ValPtr IndexExpr::Fold(Val* v1, Val* v2) const
 		break;
 
 	case TYPE_STRING:
+		return index_string(v1->AsString(), v2->AsListVal());
+
+	default:
+		RuntimeError("type cannot be indexed");
+		break;
+	}
+
+	if ( v )
+		return v;
+
+	RuntimeError("no such index");
+	return nullptr;
+	}
+
+StringValPtr index_string(const String* s, const ListVal* lv)
 	{
-		const ListVal* lv = v2->AsListVal();
-		const String* s = v1->AsString();
 	int len = s->Len();
 	String* substring = nullptr;
 
@@ -2993,16 +2989,31 @@ ValPtr IndexExpr::Fold(Val* v1, Val* v2) const
 	return make_intrusive<StringVal>(substring ? substring : new String(""));
 	}
 
-	default:
+VectorValPtr index_slice(VectorVal* vect, const ListVal* lv)
-		RuntimeError("type cannot be indexed");
+	{
-		break;
+	auto first = lv->Idx(0)->CoerceToInt();
+	auto last = lv->Idx(1)->CoerceToInt();
+	return index_slice(vect, first, last);
 	}
 
-	if ( v )
+VectorValPtr index_slice(VectorVal* vect, int _first, int _last)
-		return v;
+	{
+	size_t len = vect->Size();
+	auto result = make_intrusive<VectorVal>(vect->GetType<VectorType>());
 
-	RuntimeError("no such index");
+	bro_int_t first = get_slice_index(_first, len);
-	return nullptr;
+	bro_int_t last = get_slice_index(_last, len);
+	bro_int_t sub_length = last - first;
+
+	if ( sub_length >= 0 )
+		{
+		result->Resize(sub_length);
+
+		for ( bro_int_t idx = first; idx < last; idx++ )
+			result->Assign(idx - first, vect->ValAt(idx));
+		}
+
+	return result;
 	}
 
 void IndexExpr::Assign(Frame* f, ValPtr v)
@@ -3748,8 +3759,7 @@ ValPtr ArithCoerceExpr::Fold(Val* v) const
 	}
 
 RecordCoerceExpr::RecordCoerceExpr(ExprPtr arg_op, RecordTypePtr r)
-	: UnaryExpr(EXPR_RECORD_COERCE, std::move(arg_op)),
+	: UnaryExpr(EXPR_RECORD_COERCE, std::move(arg_op))
-	  map(nullptr), map_size(0)
 	{
 	if ( IsError() )
 		return;
@@ -3767,13 +3777,10 @@ RecordCoerceExpr::RecordCoerceExpr(ExprPtr arg_op, RecordTypePtr r)
 		RecordType* t_r = type->AsRecordType();
 		RecordType* sub_r = op->GetType()->AsRecordType();
 
-		map_size = t_r->NumFields();
+		int map_size = t_r->NumFields();
-		map = new int[map_size];
+		map.resize(map_size, -1);	// -1 = field is not mapped
 
 		int i;
-		for ( i = 0; i < map_size; ++i )
-			map[i] = -1;	// -1 = field is not mapped
-
 		for ( i = 0; i < sub_r->NumFields(); ++i )
 			{
 			int t_i = t_r->FieldOffset(sub_r->FieldName(i));
@@ -3854,11 +3861,6 @@ RecordCoerceExpr::RecordCoerceExpr(ExprPtr arg_op, RecordTypePtr r)
 		}
 	}
 
-RecordCoerceExpr::~RecordCoerceExpr()
-	{
-	delete [] map;
-	}
-
 ValPtr RecordCoerceExpr::InitVal(const zeek::Type* t, ValPtr aggr) const
 	{
 	if ( auto v = Eval(nullptr) )
@@ -3881,7 +3883,15 @@ ValPtr RecordCoerceExpr::Fold(Val* v) const
 	if ( same_type(GetType(), Op()->GetType()) )
 		return IntrusivePtr{NewRef{}, v};
 
-	auto val = make_intrusive<RecordVal>(GetType<RecordType>());
+	auto rt = cast_intrusive<RecordType>(GetType());
+	return coerce_to_record(rt, v, map);
+	}
+
+RecordValPtr coerce_to_record(RecordTypePtr rt, Val* v,
+				const std::vector<int>& map)
+	{
+	auto map_size = map.size();
+	auto val = make_intrusive<RecordVal>(rt);
 	RecordType* val_type = val->GetType()->AsRecordType();
 
 	RecordVal* rv = v->AsRecordVal();
@@ -3894,14 +3904,15 @@ ValPtr RecordCoerceExpr::Fold(Val* v) const
 
 			if ( ! rhs )
 				{
-				const auto& def = rv->GetType()->AsRecordType()->FieldDecl(
+				auto rv_rt = rv->GetType()->AsRecordType();
-					map[i])->GetAttr(ATTR_DEFAULT);
+				const auto& def = rv_rt->FieldDecl(map[i])->
+							GetAttr(ATTR_DEFAULT);
 
 				if ( def )
 					rhs = def->GetExpr()->Eval(nullptr);
 				}
 
-			assert(rhs || GetType()->AsRecordType()->FieldDecl(i)->GetAttr(ATTR_OPTIONAL));
+			assert(rhs || rt->FieldDecl(i)->GetAttr(ATTR_OPTIONAL));
 
 			if ( ! rhs )
 				{
@@ -3923,21 +3934,19 @@ ValPtr RecordCoerceExpr::Fold(Val* v) const
 			else if ( BothArithmetic(rhs_type->Tag(), field_type->Tag()) &&
 			          ! same_type(rhs_type, field_type) )
 				{
-				if ( auto new_val = check_and_promote(rhs, field_type.get(), false, op->GetLocationInfo()) )
+				auto new_val = check_and_promote(rhs, field_type.get(), false);
 				rhs = std::move(new_val);
-				else
-					RuntimeError("Failed type conversion");
 				}
 
 			val->Assign(i, std::move(rhs));
 			}
 		else
 			{
-			if ( const auto& def = GetType()->AsRecordType()->FieldDecl(i)->GetAttr(ATTR_DEFAULT) )
+			if ( const auto& def = rt->FieldDecl(i)->GetAttr(ATTR_DEFAULT) )
 				{
 				auto def_val = def->GetExpr()->Eval(nullptr);
 				const auto& def_type = def_val->GetType();
-				const auto& field_type = GetType()->AsRecordType()->GetFieldType(i);
+				const auto& field_type = rt->GetFieldType(i);
 
 				if ( def_type->Tag() == TYPE_RECORD &&
 				     field_type->Tag() == TYPE_RECORD &&

src/Expr.h

@@ -941,6 +941,22 @@ protected:
 	bool is_slice;
 };
 
+// The following execute the heart of IndexExpr functionality for
+// vector slices and strings.
+
+// Extracts a slice of a vector, where the span of the slice is specified
+// by a list of (exactly) two values.  This is how the interpreter develops
+// the components of a slice.
+extern VectorValPtr index_slice(VectorVal* vect, const ListVal* lv);
+
+// Lower-level access to the slice, where its span is expressed
+// directly as integers.
+extern VectorValPtr index_slice(VectorVal* vect, int first, int last);
+
+// Returns a subset of a string, with the span specified by a list of
+// (exactly) two values.
+extern StringValPtr index_string(const String* s, const ListVal* lv);
+
 class IndexExprWhen final : public IndexExpr {
 public:
 	static inline std::vector<ValPtr> results = {};
@@ -1166,21 +1182,24 @@ protected:
 class RecordCoerceExpr final : public UnaryExpr {
 public:
 	RecordCoerceExpr(ExprPtr op, RecordTypePtr r);
-	~RecordCoerceExpr() override;
 
 	// Optimization-related:
 	ExprPtr Duplicate() override;
 
+	const std::vector<int>& Map() const	{ return map; }
+
 protected:
 	ValPtr InitVal(const zeek::Type* t, ValPtr aggr) const override;
 	ValPtr Fold(Val* v) const override;
 
 	// For each super-record slot, gives subrecord slot with which to
 	// fill it.
-	int* map;
+	std::vector<int> map;
-	int map_size;	// equivalent to Type()->AsRecordType()->NumFields()
 };
 
+extern RecordValPtr coerce_to_record(RecordTypePtr rt, Val* v,
+					const std::vector<int>& map);
+
 class TableCoerceExpr final : public UnaryExpr {
 public:
 	TableCoerceExpr(ExprPtr op, TableTypePtr r);

src/File.h

@@ -12,8 +12,7 @@
 #include <krb5.h>
 #endif // NEED_KRB5_H
 
-#include "zeek/Obj.h"
+#include "zeek/Val.h"
-#include "zeek/IntrusivePtr.h"
 #include "zeek/util.h"
 
 namespace zeek {
@@ -23,6 +22,8 @@ namespace detail {
 class PrintStmt;
 class Attributes;
 
+extern void do_print_stmt(const std::vector<ValPtr>& vals);
+
 } // namespace detail;
 
 class RecordVal;
@@ -84,7 +85,7 @@ public:
 
 protected:
 
-	friend class detail::PrintStmt;
+	friend void detail::do_print_stmt(const std::vector<ValPtr>& vals);
 
 	File()	{ Init(); }
 	void Init();

src/Frame.cc

@@ -616,6 +616,11 @@ void Frame::CaptureClosure(Frame* c, IDPList arg_outer_ids)
 	// if (c) closure = c->SelectiveClone(outer_ids);
 	}
 
+const detail::Location* Frame::GetCallLocation() const
+	{
+	return call ? call->GetLocationInfo() : nullptr;
+	}
+
 void Frame::SetTrigger(trigger::TriggerPtr arg_trigger)
 	{
 	trigger = std::move(arg_trigger);

src/Frame.h

@@ -262,6 +262,7 @@ public:
 	void SetCall(const CallExpr* arg_call)	{ call = arg_call; }
 	void ClearCall()			{ call = nullptr; }
 	const CallExpr* GetCall() const		{ return call; }
+	const detail::Location* GetCallLocation() const;
 
 	void SetDelayed()	{ delayed = true; }
 	bool HasDelayed() const	{ return delayed; }

src/Func.cc

@@ -587,6 +587,11 @@ bool ScriptFunc::StrengthenClosureReference(Frame* f)
 	return true;
 	}
 
+bool ScriptFunc::HasCopySemantics() const
+	{
+	return type->GetCaptures().has_value();
+	}
+
 void ScriptFunc::SetClosureFrame(Frame* f)
 	{
 	if ( closure )

src/Func.h

@@ -145,7 +145,7 @@ protected:
 
 namespace detail {
 
-class ScriptFunc final : public Func {
+class ScriptFunc : public Func {
 public:
 	ScriptFunc(const IDPtr& id, StmtPtr body,
 	        const std::vector<IDPtr>& inits,
@@ -210,12 +210,17 @@ public:
 	 */
 	bool StrengthenClosureReference(Frame* f);
 
+	/**
+	 * Whether the function's closure uses copy semantics.
+	 */
+	virtual bool HasCopySemantics() const;
+
 	/**
 	 * Serializes this function's closure or capture frame.
 	 *
 	 * @return a serialized version of the function's closure/capture frame.
 	 */
-	broker::expected<broker::data> SerializeClosure() const;
+	virtual broker::expected<broker::data> SerializeClosure() const;
 
 	/**
 	 * Sets the captures frame to one built from *data*.
@@ -257,6 +262,7 @@ public:
 
 protected:
 	ScriptFunc() : Func(SCRIPT_FUNC)	{}
+
 	StmtPtr AddInits(
 		StmtPtr body,
 		const std::vector<IDPtr>& inits);
@@ -280,7 +286,7 @@ protected:
 	 *
 	 * @param f  the frame holding the values of capture variables
 	 */
-	void SetCaptures(Frame* f);
+	virtual void SetCaptures(Frame* f);
 
 private:
 	size_t frame_size;

src/Stmt.cc

@@ -306,18 +306,23 @@ ValPtr PrintStmt::DoExec(std::vector<ValPtr> vals,
                          StmtFlowType& /* flow */)
 	{
 	RegisterAccess();
+	do_print_stmt(vals);
+	return nullptr;
+	}
 
+void do_print_stmt(const std::vector<ValPtr>& vals)
+	{
 	if ( ! print_stdout )
 		print_stdout = new File(stdout);
 
 	File* f = print_stdout;
 	int offset = 0;
 
-	if ( vals.size() > 0 && (vals)[0]->GetType()->Tag() == TYPE_FILE )
+	if ( vals.size() > 0 && vals[0] && vals[0]->GetType()->Tag() == TYPE_FILE )
 		{
 		f = (vals)[0]->AsFile();
 		if ( ! f->IsOpen() )
-			return nullptr;
+			return;
 
 		++offset;
 		}
@@ -331,7 +336,7 @@ ValPtr PrintStmt::DoExec(std::vector<ValPtr> vals,
 	case BifEnum::Log::REDIRECT_ALL:
 		{
 		print_log(vals);
-		return nullptr;
+		return;
 		}
 	case BifEnum::Log::REDIRECT_STDOUT:
 		if ( f->FileHandle() == stdout )
@@ -339,7 +344,7 @@ ValPtr PrintStmt::DoExec(std::vector<ValPtr> vals,
 			// Should catch even printing to a "manually opened" stdout file,
 			// like "/dev/stdout" or "-".
 			print_log(vals);
-			return nullptr;
+			return;
 			}
 		break;
 	default:
@@ -368,8 +373,6 @@ ValPtr PrintStmt::DoExec(std::vector<ValPtr> vals,
 		describe_vals(vals, &d, offset);
 		f->Write("\n", 1);
 		}
-
-	return nullptr;
 	}
 
 ExprStmt::ExprStmt(ExprPtr arg_e) : Stmt(STMT_EXPR), e(std::move(arg_e))

src/Stmt.h

@@ -62,6 +62,8 @@ protected:
 	StmtPtr DoSubclassReduce(ListExprPtr singletons, Reducer* c) override;
 };
 
+extern void do_print_stmt(const std::vector<ValPtr>& vals);
+
 class ExprStmt : public Stmt {
 public:
 	explicit ExprStmt(ExprPtr e);

src/Type.cc

@@ -1043,6 +1043,17 @@ const char* RecordType::AddFields(const type_decl_list& others,
 
 	TableVal::SaveParseTimeTableState(this);
 
+	AddFieldsDirectly(others, add_log_attr);
+
+	RecordVal::ResizeParseTimeRecords(this);
+	TableVal::RebuildParseTimeTables();
+
+	return nullptr;
+	}
+
+void RecordType::AddFieldsDirectly(const type_decl_list& others,
+					bool add_log_attr)
+	{
 	for ( const auto& td : others )
 		{
 		if ( add_log_attr )
@@ -1059,9 +1070,6 @@ const char* RecordType::AddFields(const type_decl_list& others,
 		}
 
 	num_fields = types->length();
-	RecordVal::ResizeParseTimeRecords(this);
-	TableVal::RebuildParseTimeTables();
-	return nullptr;
 	}
 
 void RecordType::DescribeFields(ODesc* d) const

src/Type.h

@@ -623,6 +623,9 @@ public:
 	const char* AddFields(const type_decl_list& types,
 	                      bool add_log_attr = false);
 
+	void AddFieldsDirectly(const type_decl_list& types,
+				bool add_log_attr = false);
+
 	void Describe(ODesc* d) const override;
 	void DescribeReST(ODesc* d, bool roles_only = false) const override;
 	void DescribeFields(ODesc* d) const;

src/ZVal.cc

@@ -5,7 +5,6 @@
 #include "zeek/Func.h"
 #include "zeek/OpaqueVal.h"
 #include "zeek/Reporter.h"
-#include "zeek/Desc.h"
 
 using namespace zeek;
 

src/ZVal.h

@@ -6,11 +6,6 @@
 
 #include "zeek/zeek-config.h"
 
-#include <unordered_set>
-
-#include "zeek/Dict.h"
-#include "zeek/Expr.h"
-
 namespace zeek {
 
 class StringVal;

src/broker/Data.cc

@@ -392,6 +392,17 @@ struct val_converter {
 
 			if ( a.size() == 2 ) // we have a closure/capture frame
 				{
+				// Note, seems if we already have a separate
+				// instance of the same lambda, then unless
+				// we use a cloned value, we'll step on that
+				// one's captures, too.  This is because
+				// the capture mapping lives with the Func
+				// object rather than the FuncVal.  However,
+				// we can't readily Clone() here because
+				// rval is const (and, grrr, Clone() is not).
+				// -VP
+				// rval = rval->Clone();
+
 				auto frame = broker::get_if<broker::vector>(a[1]);
 				if ( ! frame )
 					return nullptr;
@@ -400,9 +411,7 @@ struct val_converter {
 				if ( ! b )
 					return nullptr;
 
-				auto copy_semantics = b->GetType()->GetCaptures().has_value();
+				if ( b->HasCopySemantics() )
-
-				if ( copy_semantics )
 					{
 					if ( ! b->DeserializeCaptures(*frame) )
 						return nullptr;
@@ -1177,7 +1186,7 @@ broker::data& opaque_field_to_data(RecordVal* v, zeek::detail::Frame* f)
 	const auto& d = v->GetField(0);
 
 	if ( ! d )
-		reporter->RuntimeError(f->GetCall()->GetLocationInfo(),
+		reporter->RuntimeError(f->GetCallLocation(),
 		                             "Broker::Data's opaque field is not set");
 
 	// RuntimeError throws an exception which causes this line to never exceute.

src/broker/Data.h

@@ -194,7 +194,7 @@ T& require_data_type(broker::data& d, zeek::TypeTag tag, zeek::detail::Frame* f)
 	{
 	auto ptr = caf::get_if<T>(&d);
 	if ( ! ptr )
-		zeek::reporter->RuntimeError(f->GetCall()->GetLocationInfo(),
+		zeek::reporter->RuntimeError(f->GetCallLocation(),
 		                             "data is of type '%s' not of type '%s'",
 		                             caf::visit(type_name_getter{tag}, d),
 		                             zeek::type_name(tag));

src/broker/Manager.cc

@@ -74,7 +74,7 @@ int Manager::script_scope = 0;
 struct scoped_reporter_location {
 	scoped_reporter_location(zeek::detail::Frame* frame)
 		{
-		reporter->PushLocation(frame->GetCall()->GetLocationInfo());
+		reporter->PushLocation(frame->GetCallLocation());
 		}
 
 	~scoped_reporter_location()

src/reporter.bif

@@ -22,7 +22,7 @@ module Reporter;
 ## .. zeek:see:: reporter_info
 function Reporter::info%(msg: string%): bool
 	%{
-	reporter->PushLocation(frame->GetCall()->GetLocationInfo());
+	reporter->PushLocation(frame->GetCallLocation());
 	reporter->Info("%s", msg->CheckString());
 	reporter->PopLocation();
 	return zeek::val_mgr->True();
@@ -37,7 +37,7 @@ function Reporter::info%(msg: string%): bool
 ## .. zeek:see:: reporter_warning
 function Reporter::warning%(msg: string%): bool
 	%{
-	reporter->PushLocation(frame->GetCall()->GetLocationInfo());
+	reporter->PushLocation(frame->GetCallLocation());
 	reporter->Warning("%s", msg->CheckString());
 	reporter->PopLocation();
 	return zeek::val_mgr->True();
@@ -53,7 +53,7 @@ function Reporter::warning%(msg: string%): bool
 ## .. zeek:see:: reporter_error
 function Reporter::error%(msg: string%): bool
 	%{
-	reporter->PushLocation(frame->GetCall()->GetLocationInfo());
+	reporter->PushLocation(frame->GetCallLocation());
 	reporter->Error("%s", msg->CheckString());
 	reporter->PopLocation();
 	return zeek::val_mgr->True();
@@ -66,7 +66,7 @@ function Reporter::error%(msg: string%): bool
 ## Returns: Always true.
 function Reporter::fatal%(msg: string%): bool
 	%{
-	reporter->PushLocation(frame->GetCall()->GetLocationInfo());
+	reporter->PushLocation(frame->GetCallLocation());
 	reporter->FatalError("%s", msg->CheckString());
 	reporter->PopLocation();
 	return zeek::val_mgr->True();
@@ -80,7 +80,7 @@ function Reporter::fatal%(msg: string%): bool
 ## Returns: Always true.
 function Reporter::fatal_error_with_core%(msg: string%): bool
 	%{
-	reporter->PushLocation(frame->GetCall()->GetLocationInfo());
+	reporter->PushLocation(frame->GetCallLocation());
 	reporter->FatalErrorWithCore("%s", msg->CheckString());
 	reporter->PopLocation();
 	return zeek::val_mgr->True();

testing/btest/Baseline/language.type-coerce-numerics/first_set.out

@@ -1,6 +1,5 @@
 ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
-error in int and ./first_set.zeek, line 46: overflow promoting from unsigned/double to signed arithmetic value (int and 9223372036854775808)
+error in int: overflow promoting from unsigned/double to signed arithmetic value (int and 9223372036854775808)
-expression error in ./first_set.zeek, line 46: Failed type conversion ((coerce [$ii=9223372036854775808] to record { ii:int &optional; cc:count &optional; dd:double &optional; }))
 3
 int
 4