[Lldb-commits] [PATCH] D78801: [LLDB] Add class WasmProcess for WebAssembly debugging

Greg Clayton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon May 4 20:27:52 PDT 2020


clayborg added a comment.

Interesting approach to DWARF expression evaluation, though it might be simpler to leave the DWARFExpression as it is, and have the plug-in part only handle unknown opcodes. Right now if you want to add just one opcode, you must subclass and make a plug-in instance where 99% of opcodes get forwarded to the original implementation and the one new opcode gets handled by the plug-in. What if the DWARFExpression class was passed to a plug-in that only handles unknown opcodes? Might be fewer code changes and be a bit cleaner. The plug-in would handle only the DW_OP_WASM_location and would still be found/detected if and only if one is encountered?

As for unwinding, I still don't think we need the UnwindWASM class. See my inline comment in "Unwind &Thread::GetUnwinder()" for a bit of reasoning. It is very common for runtimes for languages to support supplying the stack frames for a thread, so this should be built into the lldb_private::Process/lldb_private::Thread classes. For stack unwindind I would suggest adding functions to lldb_private::Thread:

  class Thread {
    /// Check if the runtime supports unwinding call stacks and return true if so.
    ///
    /// If the language runs in a runtime that knows how to unwind the call stack
    /// for a thread, then this function should return true.
    ///
    /// If true is returned, unwinding will use a RuntimeUnwind class that will call
    /// into this class' Thread::GetRuntimeFrame* functions to do the unwinding.
    /// If false is returned, the standard UnwindLLDB will be used where unwinding
    /// will be done using registers, unwind info and other debug info.
    virtual bool HasRuntimeUnwindSupport() {
      return false; // Default to using UnwindLLDB()
    }
    virtual uint32_t GetRuntimeFrameCount() {
      return 0;
   }
   virtual bool GetRuntimeFrameInfoAtIndex(uint32_t frame_idx, lldb::addr_t &cfa, lldb::addr_t &pc, bool &behaves_like_zeroth_frame) {
    return false;
   }
   virtual lldb::RegisterContextSP GetRuntimeFrameRegisterContext(uint32_t frame_idx) {
    return lldb::RegisterContextSP();
   } 

Then either ThreadGDBRemote, or possibly a subclass like ThreadWasm will implement these virtual functions.

For the GetWasmLocal, GetWasmGlobal, GetWasmStackValue, I still think abstracting this into the lldb_private::Process/lldb_private::Thread is the right way to do this. The way you have this right now, there is not way to tell how big the buffer needs to be to fetch any of these local/global/stack values. They all assume 16 bytes is enough.

If we have a runtime that knows about information in a stack frame, function or global, then we should have a way to fetch that  from a process/thread. For example:

  class Process {
    /// Fetch a global variable from the process runtime if this is supported.
    ///
    /// \param var_id A 64 bit value that needs to encode all of the data needed to fetch a
    ///              global variable and should uniquely identify a global variable in a module.
    /// \param buf A buffer that will get the bytes from the global variable. If buf is NULL, then
    ///             size will be returned so the client knows how large the variable is.
    /// \param size The size of the buffer pointed to by "buf". If buf is NULL, this value can be 
    ///              zero to indicate the function should return the actual size of the global variable.
    ///
   /// \returns The actual size of the variable which might be larger that the "size" parameter.
    virtual size_t GetRuntimeGlobalValue(lldb::user_id_t var_id, void *buf, size_t buffer_size) {
      return 0;
    }

We would need to do something similar on the Thread class for locals and stack values:

  class Thread {
    virtual size_t GetRuntimeLocalValue(uint32_t frame_idx, lldb::user_id_t var_id, void *buf, size_t buffer_size) {
      return 0;
    }
    virtual size_t GetRuntimeStackValue(uint32_t frame_idx, lldb::user_id_t var_id, void *buf, size_t buffer_size) {
      return 0;
    }





================
Comment at: lldb/source/Expression/DWARFEvaluator.cpp:327-329
+  case DW_OP_WASM_location:
+    assert(false);
+    break;
----------------
This shouldn't be in here. Remove DW_OP_WASM_location as it is custom.


================
Comment at: lldb/source/Plugins/Process/wasm/WasmProcess.cpp:71-76
+  if (vm_addr < 0x100000000) {
+    if (WasmReadMemory(0 /*frame_index*/, vm_addr, buf, size)) {
+      return size;
+    }
+    return 0;
+  } else
----------------
This seems flaky to me. How are you ever going to get the frame index right unless we encode it into the "vm_addr" using bitfields like we spoke about before. And if we encode it all into the 64 bit address, then we don't need this special read. Seems like we need to figure out if we are going to encode everything into an uint64_t or not. That will be the easiest way to integrate this into LLDB as all memory reads take a "lldb::addr_t" right now (no memory space information). We would change ReadMemory and WriteMemory to start taking a more complex type like:

```
AddressSpecifier {
  lldb::addr_t addr;
  uint64_t segment;
};
```
But that will be a huge change to the LLDB source code that should be done in a separate patch before we do anything here.


================
Comment at: lldb/source/Plugins/Process/wasm/WasmProcess.cpp:185
+
+bool WasmProcess::GetWasmCallStack(std::vector<lldb::addr_t> &call_stack_pcs) {
+  call_stack_pcs.clear();
----------------
This API seems wrong to be on the process. It should be on the ThreadWasm class (if we end up with one). see my main comments for more details.


================
Comment at: lldb/source/Plugins/Process/wasm/WasmProcess.h:20
+//  retrieved from the Wasm engine.
+class WasmProcess : public process_gdb_remote::ProcessGDBRemote {
+public:
----------------
Should be named ProcessWasm to follow all other process classes (if we decide we need to specialize a Wasm process class and not just abstract it into ProcessGDBRRemote).


================
Comment at: lldb/source/Target/Thread.cpp:1857-1863
+  if (!m_unwinder_up) {
+    if (CalculateTarget()->GetArchitecture().GetMachine() ==
+        llvm::Triple::wasm32)
+      m_unwinder_up.reset(new wasm::UnwindWasm(*this));
+    else
+      m_unwinder_up.reset(new UnwindLLDB(*this));
+  }
----------------
If the call stack is available through the Process/Thread interface, I think we should be asking the thread for this information. So this code could be:

```
if (!m_unwinder_up) {
    if (HasRuntimeUnwindSupport())
      m_unwinder_up.reset(new RuntimeUnwind(*this));
    else
      m_unwinder_up.reset(new UnwindLLDB(*this));
  }
```

RuntimeUnwind would be a class that will fetch the stack frame information through the new virtual functions on the Thread class, but only if the virtual Thread::HasRuntimeUnwindSupport() returns true. As new languages and runtimes are added in the future I don't want to see this function look like:

```
if (!m_unwinder_up) {
    if (CalculateTarget()->GetArchitecture().GetMachine() ==
        llvm::Triple::wasm32)
      m_unwinder_up.reset(new wasm::UnwindWasm(*this));
    else if (CalculateTarget()->GetArchitecture().GetMachine() ==
        llvm::Triple::Rust)
      m_unwinder_up.reset(new rust::UnwindRust(*this));
    else if (CalculateTarget()->GetArchitecture().GetMachine() ==
        llvm::Triple::Go)
      m_unwinder_up.reset(new go::UnwindGo(*this));
    else
      m_unwinder_up.reset(new UnwindLLDB(*this));
  }
```

So we should find a way to generalize the stack frames being fetched from the process/thread classes using virtual functions. 

I know this is the way GDB was built (many ifdefs and arch specific detecting code everywhere), but we have plug-ins in LLDB that are there to abstract us from this kind of code. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78801/new/

https://reviews.llvm.org/D78801





More information about the lldb-commits mailing list