Fix leak in closures with circular references

include/Ark/VM/ExecutionContext.hpp

@@ -38,7 +38,8 @@ namespace Ark::internal
 
         std::optional<ClosureScope> saved_scope {};  ///< Scope created by CAPTURE <x> instructions, used by the MAKE_CLOSURE instruction
         std::optional<uint16_t> capture_rename_id {};
-        std::vector<std::shared_ptr<ClosureScope>> stacked_closure_scopes {};  ///< Stack the closure scopes to keep the closure alive as long as we are calling them
+        // todo: handle deletion of scopes
+        std::vector<std::shared_ptr<ClosureScope>> closure_scopes {};  ///< Closure scopes to keep the closures alive as long as the VM is running
 
         std::vector<ScopeView> locals {};
         std::array<ScopeView::pair_t, ScopeStackSize> scopes_storage {};  ///< All the ScopeView use this array to store id->value

include/Ark/VM/VM.inl

@@ -238,7 +238,6 @@ inline ARK_ALWAYS_INLINE Value* VM::popAndResolveAsPtr(internal::ExecutionContex
 inline ARK_ALWAYS_INLINE void VM::returnFromFuncCall(internal::ExecutionContext& context)
 {
     --context.fc;
-    context.stacked_closure_scopes.pop_back();
     context.locals.pop_back();
 }
 
@@ -274,8 +273,6 @@ inline void VM::call(internal::ExecutionContext& context, const uint16_t argc, V
             page_addr = maybe_value_ptr->pageAddr();
     }
 
-    context.stacked_closure_scopes.emplace_back(nullptr);
-
     switch (call_type)
     {
         // is it a builtin function name?
@@ -309,7 +306,6 @@ inline void VM::call(internal::ExecutionContext& context, const uint16_t argc, V
             context.locals.emplace_back(context.scopes_storage.data(), context.locals.back().storageEnd());
             // load saved scope
             c.refScope().mergeRefInto(context.locals.back());
-            context.stacked_closure_scopes.back() = c.scopePtr();
 
             context.fc++;
             context.pp = new_page_pointer;

include/Ark/VM/Value/Closure.hpp

@@ -36,22 +36,21 @@ namespace Ark::internal
     public:
         /**
          * @brief Construct a new Closure object
-         *
-         * @param scope the scope of the function turned into a closure
+         * @param scope_ptr a shared pointer to the scope of the function turned into a closure
          * @param pa the current page address of the function turned into a closure
          */
-        Closure(const ClosureScope& scope, PageAddr_t pa) noexcept;
+        Closure(const std::shared_ptr<ClosureScope>& scope_ptr, PageAddr_t pa) noexcept;
 
         /**
          * @brief Construct a new Closure object
-         * @param scope_ptr a shared pointer to the scope of the function turned into a closure
+         * @param scope_ptr a weak pointer to the scope of the function turned into a closure
          * @param pa the current page address of the function turned into a closure
          */
-        Closure(const std::shared_ptr<ClosureScope>& scope_ptr, PageAddr_t pa) noexcept;
+        Closure(const std::weak_ptr<ClosureScope>& scope_ptr, PageAddr_t pa) noexcept;
 
-        [[nodiscard]] const ClosureScope& scope() const noexcept { return *m_scope; }
-        [[nodiscard]] ClosureScope& refScope() const noexcept { return *m_scope; }
-        [[nodiscard]] const std::shared_ptr<ClosureScope>& scopePtr() const { return m_scope; }
+        [[nodiscard]] const ClosureScope& scope() const noexcept { return *m_scope.lock(); }
+        [[nodiscard]] ClosureScope& refScope() const noexcept { return *m_scope.lock(); }
+        [[nodiscard]] const std::weak_ptr<ClosureScope>& scopePtr() const { return m_scope; }
 
         /**
          *
@@ -80,7 +79,7 @@ namespace Ark::internal
         friend struct std::hash<Ark::internal::Closure>;
 
     private:
-        std::shared_ptr<ClosureScope> m_scope;
+        std::weak_ptr<ClosureScope> m_scope;
         // keep track of the code page number, in case we need it later
         PageAddr_t m_page_addr;
     };
@@ -96,7 +95,7 @@ struct std::hash<Ark::internal::Closure>
 {
     [[nodiscard]] std::size_t operator()(const Ark::internal::Closure& s) const noexcept
     {
-        return std::hash<Ark::internal::ClosureScope*> {}(s.m_scope.get());
+        return std::hash<Ark::internal::ClosureScope*> {}(s.m_scope.lock().get());
     }
 };
 

src/arkreactor/VM/Debugger.cpp

@@ -47,14 +47,14 @@ namespace Ark::internal
                 context.sp,
                 context.fc,
                 context.locals,
-                context.stacked_closure_scopes));
+                context.closure_scopes));
     }
 
     void Debugger::resetContextToSavedState(ExecutionContext& context)
     {
         const auto& [ip, pp, sp, fc, locals, closure_scopes] = *m_states.back();
         context.locals = locals;
-        context.stacked_closure_scopes = closure_scopes;
+        context.closure_scopes = closure_scopes;
         context.ip = ip;
         context.pp = pp;
         context.sp = sp;

src/arkreactor/VM/VM.cpp

@@ -39,8 +39,7 @@ namespace Ark
         context.fc = 1;
 
         m_shared_lib_objects.clear();
-        context.stacked_closure_scopes.clear();
-        context.stacked_closure_scopes.emplace_back(nullptr);
+        context.closure_scopes.clear();
 
         context.saved_scope.reset();
         m_exit_code = 0;
@@ -272,7 +271,7 @@ namespace Ark
                 // reset the context before using it
                 ctx->sp = 0;
                 ctx->saved_scope.reset();
-                ctx->stacked_closure_scopes.clear();
+                ctx->closure_scopes.clear();
                 ctx->locals.clear();
             }
         }
@@ -286,7 +285,6 @@ namespace Ark
         const ExecutionContext& primary_ctx = *m_execution_contexts.front();
         ctx->locals.reserve(primary_ctx.locals.size());
         ctx->scopes_storage = primary_ctx.scopes_storage;
-        ctx->stacked_closure_scopes.emplace_back(nullptr);
         ctx->fc = 1;
 
         for (const auto& scope_view : primary_ctx.locals)
@@ -830,7 +828,8 @@ namespace Ark
 
                 TARGET(MAKE_CLOSURE)
                 {
-                    push(Value(Closure(context.saved_scope.value(), m_state.m_constants[arg].pageAddr())), context);
+                    std::shared_ptr<ClosureScope>& scope = context.closure_scopes.emplace_back(std::make_shared<ClosureScope>(context.saved_scope.value()));
+                    push(Value(Closure(scope, m_state.m_constants[arg].pageAddr())), context);
                     context.saved_scope.reset();
                     DISPATCH();
                 }

src/arkreactor/VM/Value/Closure.cpp

@@ -7,34 +7,36 @@
 
 namespace Ark::internal
 {
-    Closure::Closure(const ClosureScope& scope, const PageAddr_t pa) noexcept :
-        m_scope(std::make_shared<ClosureScope>(scope)),
+    Closure::Closure(const std::shared_ptr<ClosureScope>& scope_ptr, const PageAddr_t pa) noexcept :
+        m_scope(scope_ptr),
         m_page_addr(pa)
     {}
 
-    Closure::Closure(const std::shared_ptr<ClosureScope>& scope_ptr, const PageAddr_t pa) noexcept :
+    Closure::Closure(const std::weak_ptr<ClosureScope>& scope_ptr, const PageAddr_t pa) noexcept :
         m_scope(scope_ptr),
         m_page_addr(pa)
     {}
 
     bool Closure::hasFieldEndingWith(const std::string& end, const VM& vm) const
     {
-        return std::ranges::any_of(std::ranges::views::keys(m_scope->m_data), [&vm, &end](const auto& id) {
+        const std::shared_ptr<ClosureScope> scope_ptr = m_scope.lock();
+        return std::ranges::any_of(std::ranges::views::keys(scope_ptr->m_data), [&vm, &end](const auto& id) {
             return end.ends_with(":" + vm.m_state.m_symbols[id]);
         });
     }
 
     std::string Closure::toString(VM& vm) const noexcept
     {
+        const std::shared_ptr<ClosureScope> scope_ptr = m_scope.lock();
         std::string out = "(";
-        for (std::size_t i = 0, end = m_scope->m_data.size(); i < end; ++i)
+        for (std::size_t i = 0, end = scope_ptr->m_data.size(); i < end; ++i)
         {
-            const auto& [id, value] = m_scope->m_data[i];
+            const auto& [id, value] = scope_ptr->m_data[i];
             if (i != 0)
                 out += ' ';
 
             out += '.' + vm.m_state.m_symbols[id] + '=';
-            if (value.valueType() == ValueType::Closure && value.closure().scopePtr() == scopePtr())
+            if (value.valueType() == ValueType::Closure && value.closure().scopePtr().lock().get() == scope_ptr.get())
                 out += "Ref(self)";
             else
                 out += value.toString(vm);
@@ -47,10 +49,14 @@ namespace Ark::internal
         // they do not come from the same closure builder
         if (A.m_page_addr != B.m_page_addr)
             return false;
+
+        const std::shared_ptr<ClosureScope> scope_a = A.m_scope.lock();
+        const std::shared_ptr<ClosureScope> scope_b = B.m_scope.lock();
+
         // pointers are identical, we are dealing with the same object
-        if (A.m_scope.get() == B.m_scope.get())
+        if (scope_a.get() == scope_b.get())
             return true;
 
-        return *A.m_scope == *B.m_scope;
+        return *scope_a == *scope_b;
     }
 }