From f30e0166a71afcfa65ac87be960714e644c5be6a Mon Sep 17 00:00:00 2001 From: Arne Welzel Date: Sat, 27 Sep 2025 16:57:14 +0200 Subject: [PATCH] RecordVal: Use ZValSlot for fields --- src/Val.cc | 62 +++++++++++++++++++++++++++++++++++------------------- src/Val.h | 33 +++++++++++------------------ 2 files changed, 52 insertions(+), 43 deletions(-) diff --git a/src/Val.cc b/src/Val.cc index d07b8a3288..29f4c191d8 100644 --- a/src/Val.cc +++ b/src/Val.cc @@ -2924,7 +2924,7 @@ TableVal::TableRecordDependencies TableVal::parse_time_table_record_dependencies RecordVal::RecordTypeValMap RecordVal::parse_time_records; -RecordVal::RecordVal(RecordTypePtr t, bool init_fields) : Val(t), is_managed(t->ManagedFields()) { +RecordVal::RecordVal(RecordTypePtr t, bool init_fields) : Val(t) { rt = std::move(t); int n = rt->NumFields(); @@ -2935,6 +2935,10 @@ RecordVal::RecordVal(RecordTypePtr t, bool init_fields) : Val(t), is_managed(t-> if ( init_fields ) { record_val.resize(n); + // Properly initialize all slots. + for ( size_t i = 0; i < record_val.size(); i++ ) + record_val[i] = ZValSlot(rt->GetFieldType(i)); + for ( auto& e : rt->CreationInits() ) { try { record_val[e.first] = e.second->Generate(); @@ -2947,35 +2951,34 @@ RecordVal::RecordVal(RecordTypePtr t, bool init_fields) : Val(t), is_managed(t-> } else + // This needs to go through AppendField() which will do the right thing + // for the individual slots. record_val.reserve(n); } -RecordVal::RecordVal(RecordTypePtr t, std::vector> init_vals) - : Val(t), is_managed(t->ManagedFields()) { +RecordVal::RecordVal(RecordTypePtr t, std::vector> init_vals) : Val(t) { rt = std::move(t); - record_val = std::move(init_vals); -} -RecordVal::~RecordVal() { - notifier::detail::Modifiable::Unregister(); + // TODO: Change so that callers pass init_vals as ZValSlot instead? + record_val.reserve(rt->NumFields()); + size_t n = rt->NumFields(); - auto n = record_val.size(); + for ( size_t i = 0; i < n; i++ ) { + record_val.emplace_back(ZValSlot(rt->GetFieldType(i))); - for ( unsigned int i = 0; i < n; ++i ) { - auto f_i = record_val[i]; - if ( f_i && IsManaged(i) ) - ZVal::DeleteManagedType(*f_i); + if ( init_vals[i].has_value() ) + record_val[i] = init_vals[i].value(); } } +RecordVal::~RecordVal() { notifier::detail::Modifiable::Unregister(); } + ValPtr RecordVal::SizeVal() const { return val_mgr->Count(GetType()->AsRecordType()->NumFields()); } void RecordVal::Assign(int field, ValPtr new_val) { if ( new_val ) { - DeleteFieldIfManaged(field); - auto t = rt->GetFieldType(field); - record_val[field] = ZVal(new_val, t); + record_val[field] = ZValSlot(new_val, t); Modified(); } else @@ -2983,15 +2986,11 @@ void RecordVal::Assign(int field, ValPtr new_val) { } void RecordVal::Remove(int field) { - auto& f_i = record_val[field]; - if ( f_i ) { - if ( IsManaged(field) ) - ZVal::DeleteManagedType(*f_i); - - f_i = std::nullopt; + bool was_set = record_val[field].IsSet(); + record_val[field].Reset(); + if ( was_set ) Modified(); - } } ValPtr RecordVal::GetFieldOrDefault(int field) const { @@ -4227,3 +4226,22 @@ TEST_CASE("copy slot") { } TEST_SUITE_END(); + +TEST_SUITE_BEGIN("RecordVal"); + +TEST_CASE("assign string") { + auto t = zeek::id::find_type("PacketSource"); + auto v = zeek::make_intrusive(t); + + std::string path = "test-path"; + + v->Assign(1, path); + + auto sv = v->GetField(1); + CHECK(sv->RefCnt() == 2); // sv and v + + v.reset(); + CHECK(sv->RefCnt() == 1); // record was destroyed +} + +TEST_SUITE_END(); diff --git a/src/Val.h b/src/Val.h index 517ad51a3b..25cdaff0ca 100644 --- a/src/Val.h +++ b/src/Val.h @@ -1315,10 +1315,7 @@ public: void AssignInterval(int field, double new_val) { Assign(field, new_val); } void Assign(int field, StringVal* new_val) { - auto& fv = record_val[field]; - if ( fv ) - ZVal::DeleteManagedType(*fv); - fv = ZVal(new_val); + record_val[field] = ZVal(new_val); AddedField(field); } void Assign(int field, const char* new_val) { Assign(field, new StringVal(new_val)); } @@ -1383,7 +1380,7 @@ public: fv = fi->Generate(); } - return fv->ToVal(rt->GetFieldType(field)); + return fv.ToVal(rt->GetFieldType(field)); } /** @@ -1450,7 +1447,7 @@ public: // Returns true if the slot for the given field is initialized. // This helper can be used to guard GetFieldAs() accesses. - bool HasRawField(int field) const { return record_val[field].has_value(); } + bool HasRawField(int field) const { return record_val[field].IsSet(); } // The following return the given field converted to a particular // underlying value. We provide these to enable efficient @@ -1580,16 +1577,18 @@ protected: */ void AppendField(ValPtr v, const TypePtr& t) { if ( v ) - record_val.emplace_back(ZVal(v, t)); + record_val.emplace_back(ZValSlot(v, t)); else - record_val.emplace_back(std::nullopt); + record_val.emplace_back(ZValSlot(t)); } // For internal use by low-level ZAM instructions and event tracing. // Caller assumes responsibility for memory management. The first // version allows manipulation of whether the field is present at all. // The second version ensures that the optional value is present. - std::optional& RawOptField(int field) { + // + // TODO: Fix name! + ZValSlot& RawOptField(int field) { auto& f = record_val[field]; if ( ! f ) { const auto& fi = rt->DeferredInits()[field]; @@ -1600,10 +1599,13 @@ protected: return f; } + // TODO: Fix name! ZVal& RawField(int field) { auto& f = RawOptField(field); if ( ! f ) f = ZVal(); + + assert(f.IsSet()); return *f; } @@ -1617,14 +1619,6 @@ protected: static RecordTypeValMap parse_time_records; private: - void DeleteFieldIfManaged(unsigned int field) { - auto& f = record_val[field]; - if ( f && IsManaged(field) ) - ZVal::DeleteManagedType(*f); - } - - bool IsManaged(unsigned int offset) const { return is_managed[offset]; } - // Just for template inferencing. RecordVal* Get() { return this; } @@ -1636,10 +1630,7 @@ private: // Low-level values of each of the fields. // // Lazily modified during GetField(), so mutable. - mutable std::vector> record_val; - - // Whether a given field requires explicit memory management. - const std::vector& is_managed; + mutable std::vector record_val; }; class EnumVal final : public detail::IntValImplementation {