Add missing root for method instances used in opaque closure (#50359)

Usually the rooting path for method instances is something like this:

Module -> DataType -> TypeName -> MethodTable -> Method ->
MethodInstance

so these MethodInstances are effectively globally rooted. However, the
Method(instances) inside opaque closures do not have this rooting path
and can be deleted. Unfortunately, if we codegen'ed these opaque
closures, we do have a reference to the method instance in our global
debuginfo tables. This was causing crashes in cases where the oc was
gc'ed before a stack trace could be printed. Fix that by adding any
method instance that needs to be rooted to the global roots table.

Reported by @staticfloat

Author: Keno

src/clangsa/GCChecker.cpp

@@ -199,7 +199,7 @@ class GCChecker
   static bool isGCTracked(const Expr *E);
   bool isGloballyRootedType(QualType Type) const;
   static void dumpState(const ProgramStateRef &State);
-  static bool declHasAnnotation(const clang::Decl *D, const char *which);
+  static const AnnotateAttr *declHasAnnotation(const clang::Decl *D, const char *which);
   static bool isFDAnnotatedNotSafepoint(const clang::FunctionDecl *FD, const SourceManager &SM);
   static const SourceManager &getSM(CheckerContext &C) { return C.getSourceManager(); }
   bool isSafepoint(const CallEvent &Call, CheckerContext &C) const;
@@ -251,6 +251,18 @@ class GCChecker
     PDP VisitNode(const ExplodedNode *N, BugReporterContext &BRC, PathSensitiveBugReport &BR) override;
   };
 
+  class SafepointBugVisitor : public BugReporterVisitor {
+  public:
+    SafepointBugVisitor() {}
+
+    void Profile(llvm::FoldingSetNodeID &ID) const override {
+      static int X = 0;
+      ID.AddPointer(&X);
+    }
+
+    PDP VisitNode(const ExplodedNode *N, BugReporterContext &BRC, PathSensitiveBugReport &BR) override;
+  };
+
   class GCValueBugVisitor : public BugReporterVisitor {
   protected:
     SymbolRef Sym;
@@ -364,6 +376,33 @@ PDP GCChecker::GCBugVisitor::VisitNode(const ExplodedNode *N,
   return nullptr;
 }
 
+PDP GCChecker::SafepointBugVisitor::VisitNode(const ExplodedNode *N,
+                                       BugReporterContext &BRC, PathSensitiveBugReport &BR) {
+  const ExplodedNode *PrevN = N->getFirstPred();
+  unsigned NewSafepointDisabled = N->getState()->get<SafepointDisabledAt>();
+  unsigned OldSafepointDisabled = PrevN->getState()->get<SafepointDisabledAt>();
+  if (NewSafepointDisabled != OldSafepointDisabled) {
+    const Decl *D = &N->getCodeDecl();
+    const AnnotateAttr *Ann = declHasAnnotation(D, "julia_not_safepoint");
+    PathDiagnosticLocation Pos;
+    if (OldSafepointDisabled == (unsigned)-1) {
+      if (Ann) {
+        Pos = PathDiagnosticLocation{Ann->getLoc(), BRC.getSourceManager()};
+        return MakePDP(Pos, "Tracking JL_NOT_SAFEPOINT annotation here.");
+      } else {
+        PathDiagnosticLocation Pos = PathDiagnosticLocation::createDeclBegin(
+            N->getLocationContext(), BRC.getSourceManager());
+        return MakePDP(Pos, "Tracking JL_NOT_SAFEPOINT annotation here.");
+      }
+    } else if (NewSafepointDisabled == (unsigned)-1) {
+      PathDiagnosticLocation Pos = PathDiagnosticLocation::createDeclBegin(
+          N->getLocationContext(), BRC.getSourceManager());
+      return MakePDP(Pos, "Safepoints re-enabled here");
+    }
+  }
+  return nullptr;
+}
+
 PDP GCChecker::GCValueBugVisitor::ExplainNoPropagationFromExpr(
     const clang::Expr *FromWhere, const ExplodedNode *N,
     PathDiagnosticLocation Pos, BugReporterContext &BRC, PathSensitiveBugReport &BR) {
@@ -712,12 +751,12 @@ void GCChecker::checkEndFunction(const clang::ReturnStmt *RS,
   }
 }
 
-bool GCChecker::declHasAnnotation(const clang::Decl *D, const char *which) {
+const AnnotateAttr *GCChecker::declHasAnnotation(const clang::Decl *D, const char *which) {
   for (const auto *Ann : D->specific_attrs<AnnotateAttr>()) {
     if (Ann->getAnnotation() == which)
-      return true;
+      return Ann;
   }
-  return false;
+  return nullptr;
 }
 
 bool GCChecker::isFDAnnotatedNotSafepoint(const clang::FunctionDecl *FD, const SourceManager &SM) {
@@ -1302,6 +1341,7 @@ void GCChecker::checkPreCall(const CallEvent &Call, CheckerContext &C) const {
               Report->addNote(
                   "Tried to call method defined here",
                   PathDiagnosticLocation::create(FD, C.getSourceManager()));
+            Report->addVisitor(make_unique<SafepointBugVisitor>());
           },
           C, ("Calling potential safepoint as " +
               Call.getKindAsString() + " from function annotated JL_NOTSAFEPOINT").str());

src/debug-registry.h

@@ -137,7 +137,7 @@ class JITDebugInfoRegistry
     jl_method_instance_t *lookupLinfo(size_t pointer) JL_NOTSAFEPOINT;
     void registerJITObject(const llvm::object::ObjectFile &Object,
                         std::function<uint64_t(const llvm::StringRef &)> getLoadAddress,
-                        std::function<void*(void*)> lookupWriteAddress) JL_NOTSAFEPOINT;
+                        std::function<void*(void*)> lookupWriteAddress);
     objectmap_t& getObjectMap() JL_NOTSAFEPOINT;
     void add_image_info(image_info_t info) JL_NOTSAFEPOINT;
     bool get_image_info(uint64_t base, image_info_t *info) const JL_NOTSAFEPOINT;

src/debuginfo.cpp

@@ -372,9 +372,18 @@ void JITDebugInfoRegistry::registerJITObject(const object::ObjectFile &Object,
                 codeinst_in_flight.erase(codeinst_it);
             }
         }
+        jl_method_instance_t *mi = NULL;
+        if (codeinst) {
+            mi = codeinst->def;
+            // Non-opaque-closure MethodInstances are considered globally rooted
+            // through their methods, but for OC, we need to create a global root
+            // here.
+            if (jl_is_method(mi->def.value) && mi->def.method->is_for_opaque_closure)
+                mi = (jl_method_instance_t*)jl_as_global_root((jl_value_t*)mi);
+        }
         jl_profile_atomic([&]() JL_NOTSAFEPOINT {
-            if (codeinst)
-                linfomap[Addr] = std::make_pair(Size, codeinst->def);
+            if (mi)
+                linfomap[Addr] = std::make_pair(Size, mi);
             if (first) {
                 objectmap[SectionLoadAddr] = {&Object,
                     (size_t)SectionSize,
@@ -390,7 +399,7 @@ void JITDebugInfoRegistry::registerJITObject(const object::ObjectFile &Object,
 
 void jl_register_jit_object(const object::ObjectFile &Object,
                             std::function<uint64_t(const StringRef &)> getLoadAddress,
-                            std::function<void *(void *)> lookupWriteAddress) JL_NOTSAFEPOINT
+                            std::function<void *(void *)> lookupWriteAddress)
 {
     getJITDebugRegistry().registerJITObject(Object, getLoadAddress, lookupWriteAddress);
 }

src/jitlayers.cpp

@@ -690,7 +690,7 @@ void JuliaOJIT::OptSelLayerT::emit(std::unique_ptr<orc::MaterializationResponsib
 
 void jl_register_jit_object(const object::ObjectFile &debugObj,
                             std::function<uint64_t(const StringRef &)> getLoadAddress,
-                            std::function<void *(void *)> lookupWriteAddress) JL_NOTSAFEPOINT;
+                            std::function<void *(void *)> lookupWriteAddress);
 
 namespace {
 

test/opaque_closure.jl

@@ -297,3 +297,35 @@ for f in (const_int, const_int_barrier)
         @test_throws TypeError eval_oc_spec(oc_mismatch)
     end
 end
+
+
+# Attempting to construct an opaque closure backtrace after the oc is GC'ed
+f_oc_throws() = error("oops")
+@noinline function make_oc_and_collect_bt()
+    did_gc = Ref{Bool}(false)
+    bt = let ir = first(only(Base.code_ircode(f_oc_throws, ())))
+        sentinel = Ref{Any}(nothing)
+        oc = OpaqueClosure(ir, sentinel)
+        finalizer(sentinel) do x
+            did_gc[] = true
+        end
+        try
+            oc()
+            @test false
+        catch e
+            bt = catch_backtrace()
+            @test isa(e, ErrorException)
+            bt
+        end
+    end
+    return bt, did_gc
+end
+let (bt, did_gc) = make_oc_and_collect_bt()
+    GC.gc(true); GC.gc(true); GC.gc(true);
+    @test did_gc[]
+    @test any(stacktrace(bt)) do frame
+        isa(frame.linfo, Core.MethodInstance) || return false
+        isa(frame.linfo.def, Method) || return false
+        return frame.linfo.def.is_for_opaque_closure
+    end
+end