Merge remote-tracking branch 'origin/topic/vern/zam-subnet-fix'

* origin/topic/vern/zam-subnet-fix:
  Fix for ZAM inlining of nested function calls with the same parameter names
  Fixed ZAM logic error in canonicalizing specialized min/max instructions
  Fixed order-of-evaluation bug in ZAM Subnet-To-Addr instruction
  "-a zam" BTest baseline update reflecting recent Spicy baseline change

CHANGES

@@ -1,3 +1,13 @@
+7.0.0-dev.265 | 2024-05-16 11:09:23 -0700
+
+  * Fix for ZAM inlining of nested function calls with the same parameter names (Vern Paxson, Corelight)
+
+  * Fixed ZAM logic error in canonicalizing specialized min/max instructions (Vern Paxson, Corelight)
+
+  * Fixed order-of-evaluation bug in ZAM Subnet-To-Addr instruction (Vern Paxson, Corelight)
+
+  * "-a zam" BTest baseline update reflecting recent Spicy baseline change (Vern Paxson, Corelight)
+
 7.0.0-dev.260 | 2024-05-16 09:34:55 -0700
 
   * CI: Disable spicy_head task for release branches (Tim Wojtulewicz, Corelight)

VERSION

@@ -1 +1 @@
-7.0.0-dev.260
+7.0.0-dev.265

src/script_opt/Expr.cc

@@ -2529,22 +2529,41 @@ ExprPtr InlineExpr::Duplicate() {
 bool InlineExpr::IsReduced(Reducer* c) const { return NonReduced(this); }
 
 ExprPtr InlineExpr::Reduce(Reducer* c, StmtPtr& red_stmt) {
-    // First, reduce each argument and assign it to a parameter.
-    // We do this one at a time because that will often allow the
-    // optimizer to collapse the final assignment.
+    // We have to be careful regarding the order in which we evaluate
+    // the various elements that go into inlining the call. First, the
+    // arguments need to be reduced in the current scope, not the block
+    // we'll create for the inlined code. Second, we need to generate the
+    // identifiers for the formal parameters *after* creating that inner
+    // block scope, so the variables are distinct to that context. Finally,
+    // when done we need to create the return variable within that scope
+    // (so it's unique to the inlined instance) even though we'll use it -
+    // and possibly other locals from the inlining, via optimization - in
+    // the outer scope.
+
+    auto args_list = args->Exprs();
+    std::vector<ExprPtr> red_args; // holds the arguments as singletons
 
     red_stmt = nullptr;
 
-    auto args_list = args->Exprs();
-
+    // Gather up the reduced arguments.
     loop_over_list(args_list, i) {
         StmtPtr arg_red_stmt;
-        auto red_i = args_list[i]->Reduce(c, arg_red_stmt);
-        auto assign_stmt = with_location_of(c->GenParam(params[i], red_i, param_is_modified[i]), this);
-        red_stmt = MergeStmts(red_stmt, arg_red_stmt, assign_stmt);
+        red_args.emplace_back(args_list[i]->Reduce(c, arg_red_stmt));
+        red_stmt = MergeStmts(red_stmt, arg_red_stmt);
     }
 
-    auto ret_val = c->PushInlineBlock(type);
+    // Start the inline block, so the parameters we generate pick up
+    // its naming scope.
+    c->PushInlineBlock();
+
+    // Generate the parameters and assign them to the reduced arguments.
+    loop_over_list(args_list, j) {
+        auto assign_stmt = with_location_of(c->GenParam(params[j], red_args[j], param_is_modified[j]), this);
+        red_stmt = MergeStmts(red_stmt, assign_stmt);
+    }
+
+    // Generate the return variable distinct to the inner block.
+    auto ret_val = c->GetRetVar(type);
     if ( ret_val )
         ret_val->SetLocationInfo(GetLocationInfo());
 

src/script_opt/Reduce.cc

@@ -359,11 +359,27 @@ NameExprPtr Reducer::GenInlineBlockName(const IDPtr& id) {
     return make_intrusive<NameExpr>(GenLocal(id));
 }
 
-NameExprPtr Reducer::PushInlineBlock(TypePtr type) {
+void Reducer::PushInlineBlock() {
     ++inline_block_level;
-
     block_locals.emplace_back(std::unordered_map<const ID*, IDPtr>());
+}
 
+void Reducer::PopInlineBlock() {
+    --inline_block_level;
+
+    for ( auto& l : block_locals.back() ) {
+        auto key = l.first;
+        auto prev = l.second;
+        if ( prev )
+            orig_to_new_locals[key] = prev;
+        else
+            orig_to_new_locals.erase(key);
+    }
+
+    block_locals.pop_back();
+}
+
+NameExprPtr Reducer::GetRetVar(TypePtr type) {
     if ( ! type || type->Tag() == TYPE_VOID )
         return nullptr;
 
@@ -382,21 +398,6 @@ NameExprPtr Reducer::PushInlineBlock(TypePtr type) {
     return GenInlineBlockName(ret_id);
 }
 
-void Reducer::PopInlineBlock() {
-    --inline_block_level;
-
-    for ( auto& l : block_locals.back() ) {
-        auto key = l.first;
-        auto prev = l.second;
-        if ( prev )
-            orig_to_new_locals[key] = prev;
-        else
-            orig_to_new_locals.erase(key);
-    }
-
-    block_locals.pop_back();
-}
-
 ExprPtr Reducer::NewVarUsage(IDPtr var, const Expr* orig) {
     auto var_usage = make_intrusive<NameExpr>(var);
     BindExprToCurrStmt(var_usage);

src/script_opt/Reduce.h

@@ -57,11 +57,15 @@ public:
 
     int NumNewLocals() const { return new_locals.size(); }
 
+    // These should be used as a balanced pair to start and end a
+    // block being inlined.
+    void PushInlineBlock();
+    void PopInlineBlock();
+
     // Returns the name of a temporary for holding the return
     // value of the block, or nil if the type indicates there's
-    // o return value.
-    NameExprPtr PushInlineBlock(TypePtr type);
-    void PopInlineBlock();
+    // no return value. Call before popping the block.
+    NameExprPtr GetRetVar(TypePtr type);
 
     // Whether it's okay to split a statement into two copies for if-else
     // expansion.  We only allow this to a particular depth because

src/script_opt/ZAM/Expr.cc

@@ -242,7 +242,6 @@ const ZAMStmt ZAMCompiler::CompileZAMBuiltin(const NameExpr* lhs, const ScriptOp
             if ( op1->Tag() == EXPR_CONST ) {
                 ASSERT(op2->Tag() != EXPR_CONST);
                 std::swap(op1, op2);
-                is_min = ! is_min;
             }
 
             ZOp op;

src/script_opt/ZAM/OPs/ZAM.op

@@ -2412,8 +2412,9 @@ eval	auto handle = frame[z.v1].string_val;
 
 internal-op Subnet-To-Addr
 type VV
-eval	Unref(frame[z.v1].addr_val);
-	frame[z.v1] = ZVal(make_intrusive<AddrVal>(frame[z.v2].subnet_val->Prefix()));
+eval	auto addr_v = make_intrusive<AddrVal>(frame[z.v2].subnet_val->Prefix());
+	Unref(frame[z.v1].addr_val);
+	frame[z.v1] = ZVal(addr_v);
 
 internal-op Sub-Bytes
 type VVVV

testing/btest/Baseline.zam/spicy.replaces/conn.log

@@ -0,0 +1,11 @@
+### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63.
+#separator \x09
+#set_separator	,
+#empty_field	(empty)
+#unset_field	-
+#path	conn
+#open XXXX-XX-XX-XX-XX-XX
+#fields	ts	uid	id.orig_h	id.orig_p	id.resp_h	id.resp_p	proto	service	duration	orig_bytes	resp_bytes	conn_state	local_orig	local_resp	missed_bytes	history	orig_pkts	orig_ip_bytes	resp_pkts	resp_ip_bytes	tunnel_parents
+#types	time	string	addr	port	addr	port	enum	string	interval	count	count	string	bool	bool	count	string	count	count	count	count	set[string]
+XXXXXXXXXX.XXXXXX	CHhAvVGS1DHFjwGM9	172.16.238.1	49656	172.16.238.131	80	tcp	spicy_ssh	9.953807	2405	2887	SF	T	T	0	ShAdDaFf	40	4497	30	4455	-
+#close XXXX-XX-XX-XX-XX-XX

testing/btest/Baseline.zam/spicy.replaces/output

@@ -2,3 +2,6 @@
 AllAnalyzers::ANALYZER_ANALYZER_SSH, 3
 SSH banner, [orig_h=192.150.186.169, orig_p=49244/tcp, resp_h=131.159.14.23, resp_p=22/tcp], F, 1.99, OpenSSH_3.9p1
 SSH banner, [orig_h=192.150.186.169, orig_p=49244/tcp, resp_h=131.159.14.23, resp_p=22/tcp], T, 2.0, OpenSSH_3.8.1p1
+AllAnalyzers::ANALYZER_ANALYZER_SSH, 6
+SSH banner, [orig_h=172.16.238.1, orig_p=49656/tcp, resp_h=172.16.238.131, resp_p=80/tcp], F, 2.0, OpenSSH_5.8p1 Debian-1ubuntu3
+SSH banner, [orig_h=172.16.238.1, orig_p=49656/tcp, resp_h=172.16.238.131, resp_p=80/tcp], T, 2.0, OpenSSH_5.2