Merge pull request #661 from scratchcpp/fix_threads_stopping_from_procedures

Fix #660: Fix threads not stopping when stopped from procedures

Author: adazem009

include/scratchcpp/compiler.h

@@ -149,8 +149,11 @@ class LIBSCRATCHCPP_EXPORT Compiler
         void warp();
 
         void createYield();
+
         void createStop();
-        void createStopWithoutSync();
+        void createThreadStop();
+
+        void invalidateTarget();
 
         void createProcedureCall(BlockPrototype *prototype, const Compiler::Args &args);
 

src/blocks/controlblocks.cpp

@@ -90,7 +90,8 @@ CompilerValue *ControlBlocks::compileStop(Compiler *compiler)
 
         if (str == "all") {
             compiler->addFunctionCallWithCtx("control_stop_all", Compiler::StaticType::Void);
-            compiler->createStop();
+            compiler->invalidateTarget(); // if this is a clone, it doesn't exist anymore
+            compiler->createThreadStop();
         } else if (str == "this script")
             compiler->createStop();
         else if (str == "other scripts in sprite" || str == "other scripts in stage")
@@ -181,7 +182,8 @@ CompilerValue *ControlBlocks::compileDeleteThisClone(Compiler *compiler)
 {
     CompilerValue *deleted = compiler->addTargetFunctionCall("control_delete_this_clone", Compiler::StaticType::Bool);
     compiler->beginIfStatement(deleted);
-    compiler->createStopWithoutSync(); // sync happens before the function call
+    compiler->invalidateTarget(); // the sprite doesn't exist anymore
+    compiler->createThreadStop(); // callers should be stopped too
     compiler->endIf();
     return nullptr;
 }

src/engine/compiler.cpp

@@ -705,14 +705,19 @@ void Compiler::createStop()
     impl->builder->createStop();
 }
 
+/*! Creates a stop thread (current script and procedure callers) instruction. */
+void Compiler::createThreadStop()
+{
+    impl->builder->createThreadStop();
+}
+
 /*!
- * Creates a stop script without synchronization instruction.\n
- * Use this if synchronization is not possible at the stop point.
- * \note Only use this when everything is synchronized, e. g. after a function call.
+ * Creates a sprite/stage invalidation point.\n
+ * Use this if synchronization is not possible because the target has been deleted.
  */
-void Compiler::createStopWithoutSync()
+void Compiler::invalidateTarget()
 {
-    impl->builder->createStopWithoutSync();
+    impl->builder->invalidateTarget();
 }
 
 /*! Creates a call to the procedure with the given prototype. */

src/engine/internal/icodebuilder.h

@@ -99,7 +99,9 @@ class ICodeBuilder
         virtual void yield() = 0;
 
         virtual void createStop() = 0;
-        virtual void createStopWithoutSync() = 0;
+        virtual void createThreadStop() = 0;
+
+        virtual void invalidateTarget() = 0;
 
         virtual void createProcedureCall(BlockPrototype *prototype, const Compiler::Args &args) = 0;
 };

src/engine/internal/llvm/instructions/control.cpp

@@ -60,8 +60,12 @@ ProcessResult Control::process(LLVMInstruction *ins)
             ret.next = buildStop(ins);
             break;
 
-        case LLVMInstruction::Type::StopWithoutSync:
-            ret.next = buildStopWithoutSync(ins);
+        case LLVMInstruction::Type::ThreadStop:
+            ret.next = buildThreadStop(ins);
+            break;
+
+        case LLVMInstruction::Type::InvalidateTarget:
+            ret.next = buildInvalidateTarget(ins);
             break;
 
         default:
@@ -337,14 +341,27 @@ LLVMInstruction *Control::buildEndLoop(LLVMInstruction *ins)
 LLVMInstruction *Control::buildStop(LLVMInstruction *ins)
 {
     m_utils.syncVariables();
-    return buildStopWithoutSync(ins);
+
+    m_builder.CreateBr(m_utils.endBranch());
+    llvm::BasicBlock *nextBranch = llvm::BasicBlock::Create(m_utils.llvmCtx(), "", m_utils.function());
+    m_builder.SetInsertPoint(nextBranch);
+
+    return ins->next;
 }
 
-LLVMInstruction *Control::buildStopWithoutSync(LLVMInstruction *ins)
+LLVMInstruction *Control::buildThreadStop(LLVMInstruction *ins)
 {
-    m_builder.CreateBr(m_utils.endBranch());
+    m_utils.syncVariables();
+    m_builder.CreateBr(m_utils.endThreadBranch());
+
     llvm::BasicBlock *nextBranch = llvm::BasicBlock::Create(m_utils.llvmCtx(), "", m_utils.function());
     m_builder.SetInsertPoint(nextBranch);
 
     return ins->next;
 }
+
+LLVMInstruction *Control::buildInvalidateTarget(LLVMInstruction *ins)
+{
+    m_utils.invalidateTarget();
+    return ins->next;
+}

src/engine/internal/llvm/instructions/control.h

@@ -27,7 +27,8 @@ class Control : public InstructionGroup
         LLVMInstruction *buildBeginLoopCondition(LLVMInstruction *ins);
         LLVMInstruction *buildEndLoop(LLVMInstruction *ins);
         LLVMInstruction *buildStop(LLVMInstruction *ins);
-        LLVMInstruction *buildStopWithoutSync(LLVMInstruction *ins);
+        LLVMInstruction *buildThreadStop(LLVMInstruction *ins);
+        LLVMInstruction *buildInvalidateTarget(LLVMInstruction *ins);
 };
 
 } // namespace libscratchcpp::llvmins

src/engine/internal/llvm/instructions/procedures.cpp

@@ -66,9 +66,17 @@ LLVMInstruction *Procedures::buildCallProcedure(LLVMInstruction *ins)
             args.push_back(m_utils.createValue(arg.second));
     }
 
+    // Call the procedure
     llvm::Value *handle = m_builder.CreateCall(m_utils.functions().resolveFunction(name, type), args);
 
+    // Check for end thread sentinel value
+    llvm::BasicBlock *nextBranch = llvm::BasicBlock::Create(llvmCtx, "", function);
+    llvm::Value *endThread = m_builder.CreateICmpEQ(handle, m_utils.threadEndSentinel());
+    m_builder.CreateCondBr(endThread, m_utils.endThreadBranch(), nextBranch);
+    m_builder.SetInsertPoint(nextBranch);
+
     if (!m_utils.warp() && !ins->procedurePrototype->warp()) {
+        // Handle suspend
         llvm::BasicBlock *suspendBranch = llvm::BasicBlock::Create(llvmCtx, "", function);
         llvm::BasicBlock *nextBranch = llvm::BasicBlock::Create(llvmCtx, "", function);
         m_builder.CreateCondBr(m_builder.CreateIsNull(handle), nextBranch, suspendBranch);
@@ -79,6 +87,13 @@ LLVMInstruction *Procedures::buildCallProcedure(LLVMInstruction *ins)
         m_builder.CreateCondBr(done, nextBranch, suspendBranch);
 
         m_builder.SetInsertPoint(nextBranch);
+
+        // The thread could be stopped from the coroutine
+        llvm::BasicBlock *afterResumeBranch = llvm::BasicBlock::Create(llvmCtx, "", function);
+        llvm::Value *isFinished = m_builder.CreateCall(m_utils.functions().resolve_llvm_is_thread_finished(), m_utils.executionContextPtr());
+        m_builder.CreateCondBr(isFinished, m_utils.endThreadBranch(), afterResumeBranch);
+
+        m_builder.SetInsertPoint(afterResumeBranch);
     }
 
     m_utils.reloadVariables();

src/engine/internal/llvm/llvmbuildutils.cpp

@@ -161,8 +161,13 @@ void LLVMBuildUtils::init(llvm::Function *function, BlockPrototype *procedurePro
     reloadVariables();
     reloadLists();
 
-    // Create end branch
+    // Mark the target as valid
+    m_targetValidFlag = m_builder.CreateAlloca(m_builder.getInt1Ty());
+    m_builder.CreateStore(m_builder.getInt1(true), m_targetValidFlag);
+
+    // Create end branches
     m_endBranch = llvm::BasicBlock::Create(m_llvmCtx, "end", m_function);
+    m_endThreadBranch = llvm::BasicBlock::Create(m_llvmCtx, "endThread", m_function);
 }
 
 void LLVMBuildUtils::end(LLVMInstruction *lastInstruction, LLVMRegister *lastConstant)
@@ -184,9 +189,9 @@ void LLVMBuildUtils::end(LLVMInstruction *lastInstruction, LLVMRegister *lastCon
     syncVariables();
     m_builder.CreateBr(m_endBranch);
 
+    // End branch
     m_builder.SetInsertPoint(m_endBranch);
 
-    // End the script function
     llvm::PointerType *pointerType = llvm::PointerType::get(llvm::Type::getInt8Ty(m_llvmCtx), 0);
 
     switch (m_codeType) {
@@ -216,6 +221,33 @@ void LLVMBuildUtils::end(LLVMInstruction *lastInstruction, LLVMRegister *lastCon
                 m_builder.CreateRet(castValue(lastConstant, Compiler::StaticType::Bool));
             break;
     }
+
+    // Thread end branch (stop the entire thread, including procedure callers)
+    m_builder.SetInsertPoint(m_endThreadBranch);
+
+    switch (m_codeType) {
+        case Compiler::CodeType::Script:
+            // Mark the thread as finished
+            m_builder.CreateCall(m_functions.resolve_llvm_mark_thread_as_finished(), { m_executionContextPtr });
+
+            // Return a sentinel value (special pointer) to terminate any procedure callers
+            if (m_warp)
+                m_builder.CreateRet(threadEndSentinel());
+            else if (m_procedurePrototype)
+                m_coroutine->endWithSentinel(threadEndSentinel());
+            else {
+                // There's no need to return the sentinel value in standard scripts because they don't have any callers
+                m_coroutine->end();
+            }
+
+            break;
+
+        case Compiler::CodeType::Reporter:
+        case Compiler::CodeType::HatPredicate:
+            // Procedures cannot be called by these scripts, so we don't have to return the sentinel value
+            m_builder.CreateBr(m_endBranch);
+            break;
+    }
 }
 
 LLVMCompilerContext *LLVMBuildUtils::compilerCtx() const
@@ -323,6 +355,17 @@ llvm::BasicBlock *LLVMBuildUtils::endBranch() const
     return m_endBranch;
 }
 
+llvm::BasicBlock *LLVMBuildUtils::endThreadBranch() const
+{
+    return m_endThreadBranch;
+}
+
+llvm::Value *LLVMBuildUtils::threadEndSentinel() const
+{
+    llvm::PointerType *pointerType = llvm::PointerType::get(llvm::Type::getInt8Ty(m_llvmCtx), 0);
+    return m_builder.CreateIntToPtr(m_builder.getInt64(1), pointerType, "threadEndSentinel");
+}
+
 BlockPrototype *LLVMBuildUtils::procedurePrototype() const
 {
     return m_procedurePrototype;
@@ -401,6 +444,12 @@ LLVMListPtr &LLVMBuildUtils::listPtr(List *list)
 
 void LLVMBuildUtils::syncVariables()
 {
+    llvm::BasicBlock *syncBlock = llvm::BasicBlock::Create(m_llvmCtx, "syncVariables", m_function);
+    llvm::BasicBlock *syncNextBlock = llvm::BasicBlock::Create(m_llvmCtx, "syncVariables.next", m_function);
+    m_builder.CreateCondBr(m_builder.CreateLoad(m_builder.getInt1Ty(), m_targetValidFlag), syncBlock, syncNextBlock);
+
+    m_builder.SetInsertPoint(syncBlock);
+
     // Copy stack variables to the actual variables
     for (auto &[var, varPtr] : m_variablePtrs) {
         llvm::BasicBlock *copyBlock = llvm::BasicBlock::Create(m_llvmCtx, "syncVar", m_function);
@@ -414,6 +463,9 @@ void LLVMBuildUtils::syncVariables()
 
         m_builder.SetInsertPoint(nextBlock);
     }
+
+    m_builder.CreateBr(syncNextBlock);
+    m_builder.SetInsertPoint(syncNextBlock);
 }
 
 void LLVMBuildUtils::reloadVariables()
@@ -444,6 +496,11 @@ void LLVMBuildUtils::reloadLists()
     }
 }
 
+void LLVMBuildUtils::invalidateTarget()
+{
+    m_builder.CreateStore(m_builder.getInt1(false), m_targetValidFlag);
+}
+
 std::vector<LLVMIfStatement> &LLVMBuildUtils::ifStatements()
 {
     return m_ifStatements;

src/engine/internal/llvm/llvmbuildutils.h

@@ -56,6 +56,9 @@ class LIBSCRATCHCPP_TEST_EXPORT LLVMBuildUtils
         size_t stringCount() const;
 
         llvm::BasicBlock *endBranch() const;
+        llvm::BasicBlock *endThreadBranch() const;
+
+        llvm::Value *threadEndSentinel() const;
 
         BlockPrototype *procedurePrototype() const;
         bool warp() const;
@@ -79,6 +82,7 @@ class LIBSCRATCHCPP_TEST_EXPORT LLVMBuildUtils
         void syncVariables();
         void reloadVariables();
         void reloadLists();
+        void invalidateTarget();
 
         std::vector<LLVMIfStatement> &ifStatements();
         std::vector<LLVMLoop> &loops();
@@ -166,6 +170,7 @@ class LIBSCRATCHCPP_TEST_EXPORT LLVMBuildUtils
         llvm::Value *m_functionIdValue = nullptr;
 
         llvm::BasicBlock *m_endBranch = nullptr;
+        llvm::BasicBlock *m_endThreadBranch = nullptr;
 
         llvm::StructType *m_valueDataType = nullptr;
         llvm::StructType *m_stringPtrType = nullptr;
@@ -182,6 +187,7 @@ class LIBSCRATCHCPP_TEST_EXPORT LLVMBuildUtils
         llvm::Value *m_targetVariables = nullptr;
         llvm::Value *m_targetLists = nullptr;
         llvm::Value *m_warpArg = nullptr;
+        llvm::Value *m_targetValidFlag = nullptr;
 
         std::unique_ptr<LLVMCoroutine> m_coroutine;
 

src/engine/internal/llvm/llvmcodebuilder.cpp

@@ -567,9 +567,15 @@ void LLVMCodeBuilder::createStop()
     m_instructions.addInstruction(ins);
 }
 
-void LLVMCodeBuilder::createStopWithoutSync()
+void LLVMCodeBuilder::createThreadStop()
 {
-    auto ins = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::StopWithoutSync, m_loopCondition);
+    auto ins = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::ThreadStop, m_loopCondition);
+    m_instructions.addInstruction(ins);
+}
+
+void LLVMCodeBuilder::invalidateTarget()
+{
+    auto ins = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::InvalidateTarget, m_loopCondition);
     m_instructions.addInstruction(ins);
 }