fix error handling with threads

bcdonovan · May 24, 2024 · 5c97859 · 5c97859
1 parent f710001
commit 5c97859
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 5 deletions.
diff --git a/lib/Arguments/Arguments.cpp b/lib/Arguments/Arguments.cpp
@@ -30,8 +30,11 @@
 
 #include <deque>
 #include <fstream>
+#include <functional>
 #include <iostream>
 #include <memory>
+#include <mutex>
+#include <optional>
 #include <sstream>
 #include <string>
 #include <sys/types.h>
@@ -67,6 +70,19 @@ llvm::Error updateParameters(qssc::payload::PatchablePayload *payload,
   bool errorSet = false;
   llvm::Error firstError = llvm::Error::success();
 
+  // the onDiagnastic method used to emit diagnostics to python
+  // is not thread safe
+  // setup of local callback to capture the highest level diagnostic
+  // and re-emit from the main thread if threading is being used
+  std::optional<qssc::Diagnostic> localDiagValue = std::nullopt;
+  std::optional<DiagnosticCallback> const localCallback =
+      std::optional(std::function([&](const Diagnostic &diag) {
+        if (!localDiagValue.has_value() ||
+            localDiagValue.value().severity < diag.severity) {
+          localDiagValue = diag;
+        }
+      }));
+
   uint numThreads = 0;
   for (const auto &[binaryName, patchPoints] : sig.patchPointsByBinary) {
 
@@ -85,8 +101,11 @@ llvm::Error updateParameters(qssc::payload::PatchablePayload *payload,
 
     auto &binaryData = binaryDataOrErr.get();
 
+    // onDiagnostic callback is not thread safe
+    auto localDiagnostic = (enableThreads) ? localCallback : onDiagnostic;
+
     auto binary = std::shared_ptr<BindArgumentsImplementation>(
-        factory.create(binaryData, onDiagnostic));
+        factory.create(binaryData, localDiagnostic));
     binary->setTreatWarningsAsErrors(treatWarningsAsErrors);
 
     if (enableThreads) {
@@ -105,12 +124,12 @@ llvm::Error updateParameters(qssc::payload::PatchablePayload *payload,
           return;
 
         for (auto const &patchPoint : patchPoints) {
-
           auto err = binary->patch(patchPoint, arguments);
           if (err && !errorSet) {
             const std::lock_guard<std::mutex> lock(errorMutex);
             firstError = std::move(err);
             errorSet = true;
+            return;
           }
         }
       });
@@ -128,8 +147,21 @@ llvm::Error updateParameters(qssc::payload::PatchablePayload *payload,
 
     binaries.clear();
 
-    if (errorSet)
-      return firstError;
+    if (errorSet || localDiagValue.has_value()) {
+      // emit error or warning via onDiagnostic if
+      // one was set
+      auto *diagnosticCallback =
+          onDiagnostic.has_value() ? &onDiagnostic.value() : nullptr;
+      if (diagnosticCallback && localDiagValue.has_value())
+        (*diagnosticCallback)(localDiagValue.value());
+      // possibly return the error
+      auto minLevel =
+          (treatWarningsAsErrors) ? Severity::Info : Severity::Warning;
+      if (localDiagValue.has_value() &&
+          (localDiagValue.value().severity > minLevel)) {
+        return firstError;
+      }
+    }
   }
 
   return llvm::Error::success();

diff --git a/releasenotes/notes/link-threading-d295374d01595205.yaml b/releasenotes/notes/link-threading-d295374d01595205.yaml
@@ -2,4 +2,4 @@
 features:
   - |
     Adds `number_of_threads` parameter to qss_compiler.link_file
-    interface to control number of threads used during linking. 
+    interface to control number of threads used during linking.