Fix GUFA on calls to function refs in open world (#7135)

kripken · web-flow · commit 47f9a78e5d42 · 2024-12-04T12:47:15.000-08:00
In open world we must assume that a funcref that escapes to the outside
might be called.
diff --git a/src/ir/possible-contents.cpp b/src/ir/possible-contents.cpp
@@ -501,6 +501,12 @@ struct CollectedFuncInfo {
   // when we update the child we can find the parent and handle any special
   // behavior we need there.
   std::unordered_map<Expression*, Expression*> childParents;
+
+  // All functions that might be called from the outside. Any RefFunc suggests
+  // that, in open world. (We could be more precise and use our flow analysis to
+  // see which, in fact, flow outside, but it is unclear how useful that would
+  // be. Anyhow, closed-world is more important to optimize, and avoids this.)
+  std::unordered_set<Name> calledFromOutside;
 };
 
 // Does a walk while maintaining a map of names of branch targets to those
@@ -528,8 +534,10 @@ struct BreakTargetWalker : public PostWalker<SubType, VisitorType> {
 struct InfoCollector
   : public BreakTargetWalker<InfoCollector, OverriddenVisitor<InfoCollector>> {
   CollectedFuncInfo& info;
+  const PassOptions& options;
 
-  InfoCollector(CollectedFuncInfo& info) : info(info) {}
+  InfoCollector(CollectedFuncInfo& info, const PassOptions& options)
+    : info(info), options(options) {}
 
   // Check if a type is relevant for us. If not, we can ignore it entirely.
   bool isRelevant(Type type) {
@@ -665,6 +673,10 @@ struct InfoCollector
       info.links.push_back(
         {ResultLocation{func, i}, SignatureResultLocation{func->type, i}});
     }
+
+    if (!options.closedWorld) {
+      info.calledFromOutside.insert(curr->func);
+    }
   }
   void visitRefEq(RefEq* curr) {
     addRoot(curr);
@@ -2092,7 +2104,7 @@ Flower::Flower(Module& wasm, const PassOptions& options)
   // First, collect information from each function.
   ModuleUtils::ParallelFunctionAnalysis<CollectedFuncInfo> analysis(
     wasm, [&](Function* func, CollectedFuncInfo& info) {
-      InfoCollector finder(info);
+      InfoCollector finder(info, options);
 
       if (func->imported()) {
         // Imports return unknown values.
@@ -2114,7 +2126,7 @@ Flower::Flower(Module& wasm, const PassOptions& options)
   // Also walk the global module code (for simplicity, also add it to the
   // function map, using a "function" key of nullptr).
   auto& globalInfo = analysis.map[nullptr];
-  InfoCollector finder(globalInfo);
+  InfoCollector finder(globalInfo, options);
   finder.walkModuleCode(&wasm);
 
 #ifdef POSSIBLE_CONTENTS_DEBUG
@@ -2153,6 +2165,16 @@ Flower::Flower(Module& wasm, const PassOptions& options)
   // above.
   InsertOrderedMap<Location, PossibleContents> roots;
 
+  // Any function that may be called from the outside, like an export, is a
+  // root, since they can be called with unknown parameters.
+  auto calledFromOutside = [&](Name funcName) {
+    auto* func = wasm.getFunction(funcName);
+    auto params = func->getParams();
+    for (Index i = 0; i < func->getParams().size(); i++) {
+      roots[ParamLocation{func, i}] = PossibleContents::fromType(params[i]);
+    }
+  };
+
   for (auto& [func, info] : analysis.map) {
     for (auto& link : info.links) {
       links.insert(getIndexes(link));
@@ -2171,6 +2193,10 @@ Flower::Flower(Module& wasm, const PassOptions& options)
       childParents[getIndex(ExpressionLocation{child, 0})] =
         getIndex(ExpressionLocation{parent, 0});
     }
+
+    for (auto func : info.calledFromOutside) {
+      calledFromOutside(func);
+    }
   }
 
   // We no longer need the function-level info.
@@ -2180,16 +2206,7 @@ Flower::Flower(Module& wasm, const PassOptions& options)
   std::cout << "external phase\n";
 #endif
 
-  // Parameters of exported functions are roots, since exports can have callers
-  // that we can't see, so anything might arrive there.
-  auto calledFromOutside = [&](Name funcName) {
-    auto* func = wasm.getFunction(funcName);
-    auto params = func->getParams();
-    for (Index i = 0; i < func->getParams().size(); i++) {
-      roots[ParamLocation{func, i}] = PossibleContents::fromType(params[i]);
-    }
-  };
-
+  // Exports can be modified from the outside.
   for (auto& ex : wasm.exports) {
     if (ex->kind == ExternalKind::Function) {
       calledFromOutside(ex->value);
diff --git a/test/lit/passes/gufa-closed-open.wast b/test/lit/passes/gufa-closed-open.wast
@@ -0,0 +1,68 @@
+;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
+;; RUN: foreach %s %t wasm-opt -all --gufa                -S -o - | filecheck %s --check-prefix OPEND
+;; RUN: foreach %s %t wasm-opt -all --gufa --closed-world -S -o - | filecheck %s --check-prefix CLOSE
+
+;; Compare behavior on closed and open world. In open world we must assume that
+;; funcrefs, for example, can be called from outside.
+
+(module
+ ;; OPEND:      (type $0 (func (param funcref)))
+
+ ;; OPEND:      (type $1 (func))
+
+ ;; OPEND:      (type $2 (func (param i32)))
+
+ ;; OPEND:      (import "fuzzing-support" "call-ref-catch" (func $external-caller (type $0) (param funcref)))
+ ;; CLOSE:      (type $0 (func (param funcref)))
+
+ ;; CLOSE:      (type $1 (func))
+
+ ;; CLOSE:      (type $2 (func (param i32)))
+
+ ;; CLOSE:      (import "fuzzing-support" "call-ref-catch" (func $external-caller (type $0) (param funcref)))
+ (import "fuzzing-support" "call-ref-catch" (func $external-caller (param funcref)))
+
+ ;; OPEND:      (elem declare func $func)
+
+ ;; OPEND:      (export "call-import" (func $call-import))
+
+ ;; OPEND:      (func $call-import (type $1)
+ ;; OPEND-NEXT:  (call $external-caller
+ ;; OPEND-NEXT:   (ref.func $func)
+ ;; OPEND-NEXT:  )
+ ;; OPEND-NEXT: )
+ ;; CLOSE:      (elem declare func $func)
+
+ ;; CLOSE:      (export "call-import" (func $call-import))
+
+ ;; CLOSE:      (func $call-import (type $1)
+ ;; CLOSE-NEXT:  (call $external-caller
+ ;; CLOSE-NEXT:   (ref.func $func)
+ ;; CLOSE-NEXT:  )
+ ;; CLOSE-NEXT: )
+ (func $call-import (export "call-import")
+  ;; Send a reference to $func to the outside, which may call it.
+  (call $external-caller
+   (ref.func $func)
+  )
+ )
+
+ ;; OPEND:      (func $func (type $2) (param $0 i32)
+ ;; OPEND-NEXT:  (drop
+ ;; OPEND-NEXT:   (local.get $0)
+ ;; OPEND-NEXT:  )
+ ;; OPEND-NEXT: )
+ ;; CLOSE:      (func $func (type $2) (param $0 i32)
+ ;; CLOSE-NEXT:  (drop
+ ;; CLOSE-NEXT:   (unreachable)
+ ;; CLOSE-NEXT:  )
+ ;; CLOSE-NEXT: )
+ (func $func (param $0 i32)
+  ;; This is called from the outside, so this is not dead code, and nothing
+  ;; should change here in open world. In closed world, this can become an
+  ;; unreachable, since nothing can call it.
+  (drop
+   (local.get $0)
+  )
+ )
+)
diff --git a/test/lit/passes/gufa-refs.wast b/test/lit/passes/gufa-refs.wast
@@ -1,5 +1,8 @@
 ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
-;; RUN: foreach %s %t wasm-opt -all --gufa -S -o - | filecheck %s
+;; RUN: foreach %s %t wasm-opt -all --gufa --closed-world -S -o - | filecheck %s
+
+;; Closed-world is applied here to avoid treating all ref.funcs as callable
+;; from outside (and this is the more important mode to test on).
 
 (module
   ;; CHECK:      (type $struct (struct))
diff --git a/test/lit/passes/gufa.wast b/test/lit/passes/gufa.wast
@@ -1,5 +1,8 @@
 ;; NOTE: Assertions have been generated by update_lit_checks.py --all-items and should not be edited.
-;; RUN: foreach %s %t wasm-opt -all --gufa -S -o - | filecheck %s
+;; RUN: foreach %s %t wasm-opt -all --gufa --closed-world -S -o - | filecheck %s
+
+;; Closed-world is applied here to avoid treating all ref.funcs as callable
+;; from outside (and this is the more important mode to test on).
 
 (module
   ;; CHECK:      (type $0 (func (result i32)))
