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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
