[Lldb-commits] [PATCH] D71575: [LLDB] Add initial support for WebAssembly debugging
Sam Clegg via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Dec 16 15:57:15 PST 2019
sbc100 added a comment.
I don't know the lldb codebase, but from a webassembly perspective this looks promising.
I suppose we are long way from having a webassebly VM that exports that correct wire protocol to actually test this?
================
Comment at: lldb/include/lldb/Utility/ArchSpec.h:191
+ eCore_wasm32,
kNumCores,
----------------
Add another newline below to the follow the existing grouping pattern?
================
Comment at: lldb/source/API/SystemInitializerFull.cpp:73
#include "Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.h"
+#include "Plugins/ObjectFile/WASM/ObjectFileWasm.h"
#include "Plugins/OperatingSystem/Python/OperatingSystemPython.h"
----------------
Can you name this directory "wasm" rather than "WASM" since its not an acronym.
================
Comment at: lldb/source/API/SystemInitializerFull.cpp:179
ObjectFilePECOFF::Initialize();
+ wasm::ObjectFileWasm::Initialize();
----------------
Why is the namespace needed here for wasm but not the other three above.. seems inconsistent.
================
Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.h:1
+//===-- DynamicLoaderWasmDYLD.h ------------------------------*- C++ -*-===//
+//
----------------
Again, avoid WASM in the directory name. I suppose "Wasm" would also be acceptable, but I'm trying to push for "wasm" or "WebAssembly".
================
Comment at: lldb/source/Utility/ArchSpec.cpp:107
+ {eByteOrderLittle, 4, 2, 4, llvm::Triple::arm, ArchSpec::eCore_arm_armv8l,
+ "armv8l"},
{eByteOrderLittle, 4, 4, 4, llvm::Triple::aarch64_32,
----------------
Is this just clang format being greedy?
================
Comment at: lldb/unittests/ObjectFile/WASM/CMakeLists.txt:11
+ )
+
----------------
trailing newline
================
Comment at: lldb/unittests/ObjectFile/WASM/TestObjectFileWasm.cpp:2
+//===-- TestObjectFileWasm.cpp -----------------------------------*- C++
+//-*-===//
+//
----------------
somethings up with the ling wrapping here.
================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:314
EM_BPF = 247, // Linux kernel bpf virtual machine
+ EM_WASM = 248, // WebAssembly
};
----------------
This seems like an odd place to add this, given that are not using or relying on ELF anywhere. Does this make sense?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71575/new/
https://reviews.llvm.org/D71575
More information about the lldb-commits
mailing list