confluence fix for inlining

scripts/policy/protocols/ssl/ssl-log-ext.zeek

@@ -137,7 +137,7 @@ event ssl_extension_supported_versions(c: connection, is_client: bool, versions:
 		c$ssl$server_supported_version = versions[0];
 	}
 
-	event ssl_extension_psk_key_exchange_modes(c: connection, is_client: bool, modes: index_vec)
+event ssl_extension_psk_key_exchange_modes(c: connection, is_client: bool, modes: index_vec)
 	{
 	if ( ! c?$ssl || ! is_client )
 		return;

src/Stmt.h

@@ -122,6 +122,7 @@ public:
     StmtPtr DoReduce(Reducer* c) override;
 
     bool NoFlowAfter(bool ignore_break) const override;
+    bool CouldReturn(bool ignore_break) const override;
 
 protected:
     ValPtr DoExec(Frame* f, Val* v, StmtFlowType& flow) override;
@@ -182,6 +183,7 @@ public:
     StmtPtr DoReduce(Reducer* c) override;
 
     bool NoFlowAfter(bool ignore_break) const override;
+    bool CouldReturn(bool ignore_break) const override;
 
 protected:
     friend class ZAMCompiler;
@@ -301,6 +303,8 @@ public:
 
     // Note, no need for a NoFlowAfter method because the loop might
     // execute zero times, so it's always the default of "false".
+    // However, we do need to check for potential returns.
+    bool CouldReturn(bool ignore_break) const override;
 
 protected:
     ValPtr Exec(Frame* f, StmtFlowType& flow) override;
@@ -350,6 +354,8 @@ public:
 
     // Note, no need for a NoFlowAfter method because the loop might
     // execute zero times, so it's always the default of "false".
+    // However, we do need to check for potential returns.
+    bool CouldReturn(bool ignore_break) const override;
 
 protected:
     ValPtr DoExec(Frame* f, Val* v, StmtFlowType& flow) override;
@@ -395,6 +401,7 @@ public:
     StmtPtr Duplicate() override { return SetSucc(new BreakStmt()); }
 
     bool NoFlowAfter(bool ignore_break) const override { return ! ignore_break; }
+    bool CouldReturn(bool ignore_break) const override { return ! ignore_break; }
 
 protected:
 };
@@ -436,6 +443,7 @@ public:
     StmtPtr DoReduce(Reducer* c) override;
 
     bool NoFlowAfter(bool ignore_break) const override { return true; }
+    bool CouldReturn(bool ignore_break) const override { return true; }
 };
 
 class StmtList : public Stmt {
@@ -460,6 +468,7 @@ public:
     StmtPtr DoReduce(Reducer* c) override;
 
     bool NoFlowAfter(bool ignore_break) const override;
+    bool CouldReturn(bool ignore_break) const override;
 
     // Idioms commonly used in reduction.
     StmtList(StmtPtr s1, StmtPtr s2);
@@ -725,7 +734,7 @@ public:
 
     // Note, no need for a NoFlowAfter() method because anything that
     // has "NoFlowAfter" inside the body still gets caught and we
-    // continue afterwards.
+    // continue afterwards.  Same goes for CouldReturn().
 
     StmtPtr Duplicate() override;
 

src/StmtBase.h

@@ -138,6 +138,12 @@ public:
     // in that case, they do lead to flow reaching the end.
     virtual bool NoFlowAfter(bool ignore_break) const { return false; }
 
+    // True if the statement could potentially execute a return. This is
+    // used by script optimization to track confluence inside catch-return
+    // blocks. Here, ignore_break is false if we're analyzing a coalesced
+    // hook body.
+    virtual bool CouldReturn(bool ignore_break) const { return false; }
+
     // Access to the original statement from which this one is derived,
     // or this one if we don't have an original.  Returns a bare pointer
     // rather than a StmtPtr to emphasize that the access is read-only.

src/script_opt/GenIDDefs.cc

@@ -62,17 +62,31 @@ TraversalCode GenIDDefs::PreStmt(const Stmt* s) {
             auto block = cr->Block();
 
             cr_active.push_back(confluence_blocks.size());
-            cr_return_seen.push_back(false);
+
+	    bool did_confluence = false;
+
+	    if ( block->Tag() == STMT_LIST )
+		{
+		auto prev_stmt = s;
+		auto& stmts = block->AsStmtList()->Stmts();
+		for ( auto& st : stmts )
+			{
+			if ( ! did_confluence && st->CouldReturn(false) )
+				{
+				StartConfluenceBlock(prev_stmt);
+				did_confluence = true;
+				}
+
+			st->Traverse(this);
+			}
+		}
+	    else
 		    block->Traverse(this);
 
-            if ( cr_return_seen.back() )
+            if ( did_confluence )
-                // We encountered a return along the way.  curr_stmt is
+                EndConfluenceBlock();
-                // now set to the last statement in the block, so mark
-                // it with a return.
-                ReturnAt(curr_stmt);
 
             cr_active.pop_back();
-            cr_return_seen.pop_back();
 
             auto retvar = cr->RetVar();
             if ( retvar )
@@ -445,11 +459,9 @@ void GenIDDefs::ReturnAt(const Stmt* s) {
     // If we're right at a catch-return then we don't want to make the
     // identifier as encountering a scope-ending "return" here.  By avoiding
     // that, we're able to do optimization across catch-return blocks.
-    if ( cr_active.empty() || cr_active.back() != confluence_blocks.size() || cr_return_seen.back() )
+    if ( cr_active.empty() || cr_active.back() != confluence_blocks.size() )
         for ( auto id : modified_IDs.back() )
             id->GetOptInfo()->ReturnAt(s);
-    if ( ! cr_active.empty() )
-        cr_return_seen.back() = true;
 }
 
 void GenIDDefs::TrackID(const ID* id, const ExprPtr& e) {

src/script_opt/GenIDDefs.h

@@ -99,9 +99,6 @@ private:
     // (new) confluence blocks.
     std::vector<zeek_uint_t> cr_active;
 
-    // Parallel array that tracks whether a return has occurred.
-    std::vector<zeek_uint_t> cr_return_seen;
-
     // The following is parallel to confluence_blocks except
     // the front entry tracks identifiers at the outermost
     // (non-confluence) scope.  Thus, to index it for a given

src/script_opt/Stmt.cc

@@ -273,6 +273,10 @@ bool IfStmt::NoFlowAfter(bool ignore_break) const {
     return false;
 }
 
+bool IfStmt::CouldReturn(bool ignore_break) const {
+    return (s1 && s1->CouldReturn(ignore_break)) || (s2 && s2->CouldReturn(ignore_break));
+}
+
 IntrusivePtr<Case> Case::Duplicate() {
     if ( expr_cases ) {
         auto new_exprs = expr_cases->Duplicate()->AsListExprPtr();
@@ -404,6 +408,14 @@ bool SwitchStmt::NoFlowAfter(bool ignore_break) const {
     return default_seen_with_no_flow_after;
 }
 
+bool SwitchStmt::CouldReturn(bool ignore_break) const {
+    for ( const auto& c : *Cases() )
+        if ( c->Body()->CouldReturn(true) )
+            return true;
+
+    return false;
+}
+
 bool AddDelStmt::IsReduced(Reducer* c) const { return e->HasReducedOps(c); }
 
 StmtPtr AddDelStmt::DoReduce(Reducer* c) {
@@ -499,6 +511,10 @@ StmtPtr WhileStmt::DoReduce(Reducer* c) {
     return ThisPtr();
 }
 
+bool WhileStmt::CouldReturn(bool ignore_break) const {
+	return body->CouldReturn(false);
+}
+
 StmtPtr ForStmt::Duplicate() {
     auto expr_copy = e->Duplicate();
 
@@ -568,6 +584,10 @@ StmtPtr ForStmt::DoReduce(Reducer* c) {
     return ThisPtr();
 }
 
+bool ForStmt::CouldReturn(bool ignore_break) const {
+	return body->CouldReturn(false);
+}
+
 StmtPtr ReturnStmt::Duplicate() { return SetSucc(new ReturnStmt(e ? e->Duplicate() : nullptr, true)); }
 
 ReturnStmt::ReturnStmt(ExprPtr arg_e, bool ignored) : ExprStmt(STMT_RETURN, std::move(arg_e)) {}
@@ -784,6 +804,14 @@ bool StmtList::NoFlowAfter(bool ignore_break) const {
     return false;
 }
 
+bool StmtList::CouldReturn(bool ignore_break) const {
+    for ( auto& s : stmts )
+        if ( s->CouldReturn(ignore_break) )
+            return true;
+
+    return false;
+}
+
 StmtPtr InitStmt::Duplicate() {
     // Need to duplicate the initializer list since later reductions
     // can modify it in place.