[Lldb-commits] [lldb] [lldb] Use the correct Wasm register context (PR #162942)

[via lldb-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-lldb

Author: Jonas Devlieghere (JDevlieghere)

<details>
<summary>Changes</summary>

When implementing the WebAssembly Process Plugin, I initially added WasmGDBRemoteRegisterContext as a placeholder in the UnwindWasm TU.

After implementing RegisterContextWasm (#<!-- -->151056), I forgot to switch over to the new implementation. This meant we were using the wrong register context for all frames, except frame 0. The register context deals with the virtual registers, so this only becomes an issue when trying to evaluate a `DW_OP_WASM_location`, which is why this went unnoticed.

This PR removes the WasmGDBRemoteRegisterContext placeholder and updates UnwindWasm to use RegisterContextWasm instead for all frames.

In terms of testing, I considered updating TestWasm but that would require adding even more state to the already complicated GDB stub. This doesn't scale and my focus over the next weeks/months will be coming up with a comprehensive testing strategy that involves running (a subset of the) API tests under a Wasm runtime, which will cover this and much more.

rdar://159297244

---
Full diff: https://github.com/llvm/llvm-project/pull/162942.diff


3 Files Affected:

- (modified) lldb/source/Plugins/Process/wasm/RegisterContextWasm.cpp (+1-1) 
- (modified) lldb/source/Plugins/Process/wasm/RegisterContextWasm.h (+2-1) 
- (modified) lldb/source/Plugins/Process/wasm/UnwindWasm.cpp (+4-18) 


``````````diff
diff --git a/lldb/source/Plugins/Process/wasm/RegisterContextWasm.cpp b/lldb/source/Plugins/Process/wasm/RegisterContextWasm.cpp
index b4681718cb688..2e0207696a0ec 100644
--- a/lldb/source/Plugins/Process/wasm/RegisterContextWasm.cpp
+++ b/lldb/source/Plugins/Process/wasm/RegisterContextWasm.cpp
@@ -22,7 +22,7 @@ using namespace lldb_private::process_gdb_remote;
 using namespace lldb_private::wasm;
 
 RegisterContextWasm::RegisterContextWasm(
-    wasm::ThreadWasm &thread, uint32_t concrete_frame_idx,
+    ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
     GDBRemoteDynamicRegisterInfoSP reg_info_sp)
     : GDBRemoteRegisterContext(thread, concrete_frame_idx, reg_info_sp, false,
                                false) {}
diff --git a/lldb/source/Plugins/Process/wasm/RegisterContextWasm.h b/lldb/source/Plugins/Process/wasm/RegisterContextWasm.h
index 7e63eb85bc75c..6ca31e530d14d 100644
--- a/lldb/source/Plugins/Process/wasm/RegisterContextWasm.h
+++ b/lldb/source/Plugins/Process/wasm/RegisterContextWasm.h
@@ -10,6 +10,7 @@
 #define LLDB_SOURCE_PLUGINS_PROCESS_WASM_REGISTERCONTEXTWASM_H
 
 #include "Plugins/Process/gdb-remote/GDBRemoteRegisterContext.h"
+#include "Plugins/Process/gdb-remote/ThreadGDBRemote.h"
 #include "ThreadWasm.h"
 #include "Utility/WasmVirtualRegisters.h"
 #include "lldb/lldb-private-types.h"
@@ -34,7 +35,7 @@ class RegisterContextWasm
     : public process_gdb_remote::GDBRemoteRegisterContext {
 public:
   RegisterContextWasm(
-      wasm::ThreadWasm &thread, uint32_t concrete_frame_idx,
+      process_gdb_remote::ThreadGDBRemote &thread, uint32_t concrete_frame_idx,
       process_gdb_remote::GDBRemoteDynamicRegisterInfoSP reg_info_sp);
 
   ~RegisterContextWasm() override;
diff --git a/lldb/source/Plugins/Process/wasm/UnwindWasm.cpp b/lldb/source/Plugins/Process/wasm/UnwindWasm.cpp
index 99845dd2918d0..319c5e262aaf9 100644
--- a/lldb/source/Plugins/Process/wasm/UnwindWasm.cpp
+++ b/lldb/source/Plugins/Process/wasm/UnwindWasm.cpp
@@ -9,6 +9,7 @@
 #include "UnwindWasm.h"
 #include "Plugins/Process/gdb-remote/ThreadGDBRemote.h"
 #include "ProcessWasm.h"
+#include "RegisterContextWasm.h"
 #include "ThreadWasm.h"
 #include "lldb/Utility/LLDBLog.h"
 #include "lldb/Utility/Log.h"
@@ -18,21 +19,6 @@ using namespace lldb_private;
 using namespace process_gdb_remote;
 using namespace wasm;
 
-class WasmGDBRemoteRegisterContext : public GDBRemoteRegisterContext {
-public:
-  WasmGDBRemoteRegisterContext(ThreadGDBRemote &thread,
-                               uint32_t concrete_frame_idx,
-                               GDBRemoteDynamicRegisterInfoSP &reg_info_sp,
-                               uint64_t pc)
-      : GDBRemoteRegisterContext(thread, concrete_frame_idx, reg_info_sp, false,
-                                 false) {
-    // Wasm does not have a fixed set of registers but relies on a mechanism
-    // named local and global variables to store information such as the stack
-    // pointer. The only actual register is the PC.
-    PrivateSetRegisterValue(0, pc);
-  }
-};
-
 lldb::RegisterContextSP
 UnwindWasm::DoCreateRegisterContextForFrame(lldb_private::StackFrame *frame) {
   if (m_frames.size() <= frame->GetFrameIndex())
@@ -43,9 +29,9 @@ UnwindWasm::DoCreateRegisterContextForFrame(lldb_private::StackFrame *frame) {
   ProcessWasm *wasm_process =
       static_cast<ProcessWasm *>(thread->GetProcess().get());
 
-  return std::make_shared<WasmGDBRemoteRegisterContext>(
-      *gdb_thread, frame->GetConcreteFrameIndex(),
-      wasm_process->GetRegisterInfo(), m_frames[frame->GetFrameIndex()]);
+  return std::make_shared<RegisterContextWasm>(*gdb_thread,
+                                               frame->GetConcreteFrameIndex(),
+                                               wasm_process->GetRegisterInfo());
 }
 
 uint32_t UnwindWasm::DoGetFrameCount() {

``````````

</details>


https://github.com/llvm/llvm-project/pull/162942