Debugger: Restructure amber::debug interfaces

Remove the need for callbacks, which unnecessarily compilated the parsing.

src/amberscript/parser.cc

@@ -1816,7 +1816,7 @@ Result Parser::ParseDebug() {
     return res;
   }
 
-  auto dbg = MakeUnique<debug::Script>();
+  auto dbg = debug::Script::Create();
   for (auto token = tokenizer_->NextToken();; token = tokenizer_->NextToken()) {
     if (token->IsEOL())
       continue;
@@ -1841,11 +1841,6 @@ Result Parser::ParseDebug() {
 }
 
 Result Parser::ParseDebugThread(debug::Events* dbg) {
-  Result result;
-  auto parseThread = [&](debug::Thread* thread) {
-    result = ParseDebugThreadBody(thread);
-  };
-
   auto token = tokenizer_->NextToken();
   if (token->AsString() == "GLOBAL_INVOCATION_ID") {
     uint32_t invocation[3] = {};
@@ -1855,19 +1850,33 @@ Result Parser::ParseDebugThread(debug::Events* dbg) {
         return Result("expected invocation index");
       invocation[i] = token->AsUint32();
     }
+
+    auto thread = debug::ThreadScript::Create();
+    auto result = ParseDebugThreadBody(thread.get());
+    if (!result.IsSuccess()) {
+      return result;
+    }
+
     dbg->BreakOnComputeGlobalInvocation(invocation[0], invocation[1],
-                                        invocation[2], parseThread);
+                                        invocation[2], thread);
   } else if (token->AsString() == "VERTEX_INDEX") {
     token = tokenizer_->NextToken();
     if (!token->IsInteger())
       return Result("expected vertex index");
     auto vertex_index = token->AsUint32();
-    dbg->BreakOnVertexIndex(vertex_index, parseThread);
+
+    auto thread = debug::ThreadScript::Create();
+    auto result = ParseDebugThreadBody(thread.get());
+    if (!result.IsSuccess()) {
+      return result;
+    }
+
+    dbg->BreakOnVertexIndex(vertex_index, thread);
   } else {
     return Result("expected GLOBAL_INVOCATION_ID or VERTEX_INDEX");
   }
 
-  return result;
+  return Result();
 }
 
 Result Parser::ParseDebugThreadBody(debug::Thread* thread) {

src/debug.cc

@@ -14,8 +14,10 @@
 
 #include "src/debug.h"
 
+#include <functional>
 #include <memory>
 #include <string>
+#include <vector>
 
 #include "src/make_unique.h"
 
@@ -24,15 +26,44 @@ namespace debug {
 
 namespace {
 
-// ThreadScript is an implementation of amber::debug::Thread that records all
-// calls made on it, which can be later replayed using ThreadScript::Run().
-class ThreadScript : public Thread {
+class ScriptImpl : public Script {
  public:
-  void Run(Thread* thread) {
+  void Run(Events* e) const override {
+    for (auto f : sequence_) {
+      f(e);
+    }
+  }
+
+  void BreakOnComputeGlobalInvocation(
+      uint32_t x,
+      uint32_t y,
+      uint32_t z,
+      const std::shared_ptr<const ThreadScript>& thread) override {
+    sequence_.emplace_back([=](Events* events) {
+      events->BreakOnComputeGlobalInvocation(x, y, z, thread);
+    });
+  }
+
+  void BreakOnVertexIndex(
+      uint32_t index,
+      const std::shared_ptr<const ThreadScript>& thread) override {
+    sequence_.emplace_back(
+        [=](Events* events) { events->BreakOnVertexIndex(index, thread); });
+  }
+
+ private:
+  using Event = std::function<void(Events*)>;
+  std::vector<Event> sequence_;
+};
+
+class ThreadScriptImpl : public ThreadScript {
+ public:
+  void Run(Thread* thread) const override {
     for (auto f : sequence_) {
       f(thread);
     }
   }
+
   // Thread compliance
   void StepOver() override {
     sequence_.emplace_back([](Thread* t) { t->StepOver(); });
@@ -78,37 +109,12 @@ class ThreadScript : public Thread {
 Thread::~Thread() = default;
 Events::~Events() = default;
 
-void Script::Run(Events* e) {
-  for (auto f : sequence_) {
-    f(e);
-  }
+std::unique_ptr<Script> Script::Create() {
+  return MakeUnique<ScriptImpl>();
 }
 
-void Script::BreakOnComputeGlobalInvocation(uint32_t x,
-                                            uint32_t y,
-                                            uint32_t z,
-                                            const OnThread& callback) {
-  auto script = std::make_shared<ThreadScript>();
-  callback(script.get());  // Record
-
-  sequence_.emplace_back([=](Events* events) {
-    events->BreakOnComputeGlobalInvocation(x, y, z, [=](Thread* thread) {
-      script->Run(thread);  // Replay
-    });
-  });
-}
-
-void Script::BreakOnVertexIndex(uint32_t index, const OnThread& callback) {
-  // std::make_shared is used here instead of MakeUnique as std::function is
-  // copyable, and cannot capture move-only values.
-  auto script = std::make_shared<ThreadScript>();
-  callback(script.get());  // Record
-
-  sequence_.emplace_back([=](Events* events) {
-    events->BreakOnVertexIndex(index, [=](Thread* thread) {
-      script->Run(thread);  // Replay
-    });
-  });
+std::shared_ptr<ThreadScript> ThreadScript::Create() {
+  return std::make_shared<ThreadScriptImpl>();
 }
 
 }  // namespace debug

src/debug.h

@@ -18,15 +18,18 @@
 #include <stdint.h>
 
 #include <functional>
+#include <memory>
 #include <string>
-#include <vector>
 
 #include "amber/result.h"
 
 /// amber::debug holds the types used for testing a graphics debugger.
 namespace amber {
 namespace debug {
 
+// Forward declaration.
+class ThreadScript;
+
 /// Location holds a file path and a 1-based line number.
 struct Location {
   std::string file;
@@ -75,48 +78,50 @@ class Thread {
 /// Events is the interface used to control the debugger.
 class Events {
  public:
-  using OnThread = std::function<void(Thread*)>;
-
   virtual ~Events();
 
   /// BreakOnComputeGlobalInvocation instructs the debugger to set a breakpoint
   /// at the start of the compute shader program for the invocation with the
-  /// global invocation identifier [|x|, |y|, |z|]. The |amber::debug::Thread|*
-  /// parameter to the |OnThread| callback is used to control and verify the
-  /// debugger behavior for the given thread.
-  virtual void BreakOnComputeGlobalInvocation(uint32_t x,
-                                              uint32_t y,
-                                              uint32_t z,
-                                              const OnThread&) = 0;
+  /// global invocation identifier [|x|, |y|, |z|], and run the |ThreadScript|
+  /// once the breakpoint is hit.
+  virtual void BreakOnComputeGlobalInvocation(
+      uint32_t x,
+      uint32_t y,
+      uint32_t z,
+      const std::shared_ptr<const ThreadScript>&) = 0;
 
   /// BreakOnVertexIndex instructs the debugger to set a breakpoint at the start
   /// of the vertex shader program for the invocation with the vertex index
-  /// [index]. The |amber::debug::Thread|* parameter to the |OnThread| callback
-  /// is used to control and verify the debugger behavior for the given thread.
-  virtual void BreakOnVertexIndex(uint32_t index, const OnThread&) = 0;
+  /// [index], and run the |ThreadScript| once the breakpoint is hit.
+  virtual void BreakOnVertexIndex(
+      uint32_t index,
+      const std::shared_ptr<const ThreadScript>&) = 0;
 };
 
-/// Script is an implementation of the |amber::debug::Events| interface, and is
+/// Script is an specialization of the |amber::debug::Events| interface, and is
 /// used to record all the calls made on it, which can be later replayed with
 /// |Script::Run|.
 class Script : public Events {
  public:
   /// Run replays all the calls made to the |Script| on the given |Events|
-  /// parameter, including calls made to any |amber::debug::Thread|s passed to
-  /// |OnThread| callbacks.
-  void Run(Events*);
+  /// parameter.
+  virtual void Run(Events*) const = 0;
 
-  // Events compliance
-  void BreakOnComputeGlobalInvocation(uint32_t x,
-                                      uint32_t y,
-                                      uint32_t z,
-                                      const OnThread&) override;
+  // Create constructs and returns a new Script.
+  static std::unique_ptr<Script> Create();
+};
 
-  void BreakOnVertexIndex(uint32_t index, const OnThread&) override;
+/// ThreadScript is a specialization of the |amber::debug::Thread| interface,
+/// and is used to record all the calls made on it, which can be later replayed
+/// with |ThreadScript::Run|.
+class ThreadScript : public Thread {
+ public:
+  /// Run replays all the calls made to the |ThreadScript| on the given |Thread|
+  /// parameter.
+  virtual void Run(Thread*) const = 0;
 
- private:
-  using Event = std::function<void(Events*)>;
-  std::vector<Event> sequence_;
+  // Create constructs and returns a new ThreadScript.
+  static std::shared_ptr<ThreadScript> Create();
 };
 
 }  // namespace debug

src/vulkan/engine_vulkan_debugger.cc

@@ -495,15 +495,15 @@ class Thread : public debug::Thread {
   Thread(std::shared_ptr<dap::Session> session,
          int threadId,
          int lane,
-         const debug::Events::OnThread& callback)
+         std::shared_ptr<const debug::ThreadScript> script)
       : threadId_(threadId),
         lane_(lane),
         client_(session, [this](const std::string& err) { OnError(err); }) {
     // The thread script runs concurrently with other debugger thread scripts.
     // Run on a separate amber thread.
-    thread_ = std::thread([this, callback] {
-      callback(this);  // Begin running the thread script.
-      done_.Signal();  // Signal when done.
+    thread_ = std::thread([this, script] {
+      script->Run(this);  // Begin running the thread script.
+      done_.Signal();     // Signal when done.
     });
   }
 
@@ -773,17 +773,19 @@ class EngineVulkan::VkDebugger : public Engine::Debugger {
       uint32_t x,
       uint32_t y,
       uint32_t z,
-      const debug::Events::OnThread& callback) override {
+      const std::shared_ptr<const debug::ThreadScript>& script) override {
     std::unique_lock<std::mutex> lock(threads_mutex_);
-    pendingThreads_.emplace(GlobalInvocationId{x, y, z}, callback);
+    pendingThreads_.emplace(GlobalInvocationId{x, y, z}, script);
   };
 
-  void BreakOnVertexIndex(uint32_t index, const OnThread& callback) override {
+  void BreakOnVertexIndex(
+      uint32_t index,
+      const std::shared_ptr<const debug::ThreadScript>& script) override {
     InvocationKey::Data data;
     data.vertexId = index;
     auto key = InvocationKey{InvocationKey::Type::kVertexIndex, data};
     std::unique_lock<std::mutex> lock(threads_mutex_);
-    pendingThreads_.emplace(key, callback);
+    pendingThreads_.emplace(key, script);
   }
 
  private:
@@ -799,16 +801,15 @@ class EngineVulkan::VkDebugger : public Engine::Debugger {
     std::unique_lock<std::mutex> lock(threads_mutex_);
     for (auto it = pendingThreads_.begin(); it != pendingThreads_.end(); it++) {
       auto& key = it->first;
-      auto& callback = it->second;
+      auto& script = it->second;
       switch (key.type) {
         case InvocationKey::Type::kGlobalInvocationId: {
           auto invocation_id = key.data.globalInvocationId;
           int lane;
           if (FindGlobalInvocationId(threadId, invocation_id, &lane)) {
             DEBUGGER_LOG("Breakpoint hit: GetGlobalInvocationId: [%d, %d, %d]",
                          invocation_id.x, invocation_id.y, invocation_id.z);
-            auto thread =
-                MakeUnique<Thread>(session_, threadId, lane, callback);
+            auto thread = MakeUnique<Thread>(session_, threadId, lane, script);
             runningThreads_.emplace_back(std::move(thread));
             pendingThreads_.erase(it);
             return;
@@ -820,8 +821,7 @@ class EngineVulkan::VkDebugger : public Engine::Debugger {
           int lane;
           if (FindVertexIndex(threadId, vertex_id, &lane)) {
             DEBUGGER_LOG("Breakpoint hit: VertexId: %d", vertex_id);
-            auto thread =
-                MakeUnique<Thread>(session_, threadId, lane, callback);
+            auto thread = MakeUnique<Thread>(session_, threadId, lane, script);
             runningThreads_.emplace_back(std::move(thread));
             pendingThreads_.erase(it);
             return;
@@ -934,9 +934,10 @@ class EngineVulkan::VkDebugger : public Engine::Debugger {
     error_ += error;
   }
 
-  using PendingThreadsMap = std::unordered_map<InvocationKey,
-                                               debug::Script::OnThread,
-                                               InvocationKey::Hash>;
+  using PendingThreadsMap =
+      std::unordered_map<InvocationKey,
+                         std::shared_ptr<const debug::ThreadScript>,
+                         InvocationKey::Hash>;
   using ThreadVector = std::vector<std::unique_ptr<Thread>>;
   std::shared_ptr<dap::Session> session_;
   std::mutex threads_mutex_;