[Lldb-commits] [PATCH] D71575: [LLDB] Add initial support for WebAssembly debugging

[Paolo Severini via Phabricator via lldb-commits]

paolosev marked 43 inline comments as done.
paolosev added a comment.

Thanks for all the comments! I am updating the code following your suggestions.
Next step will be to split this into three distinct patches, as suggested by Pavel.



================
Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:1
+//===-- DynamicLoaderWasmDYLD.cpp --------------------------------*- C++
+//-*-===//
----------------
aprantl wrote:
> 1. This hangs over the line
> 2. a -*- C++ -*- is only necessary for .h files where C vs. C++ is ambiguous
> 
Fixed, but I see the -*- C++ -*- in all files.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:1
+//===-- ObjectFileWasm.cpp -------------------------------- -*- C++ -*-===//
+//
----------------
aprantl wrote:
> ditto
But all the C++ files seem to have "-*- C++ -*-===//" in the first line.


================
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,
----------------
sbc100 wrote:
> Is this just clang format being greedy?
Yes, it was edited by clang format.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:314
   EM_BPF = 247,           // Linux kernel bpf virtual machine
+  EM_WASM = 248,          // WebAssembly
 };
----------------
labath wrote:
> sbc100 wrote:
> > This seems like an odd place to add this, given that are not using or relying on ELF anywhere.  Does this make sense?
> Indeed, that looks very unexpected, and begs an explanation.
Oops... my mistake. This is a relic from an old version where I expected Wasm stripped debug symbol might also come in an ELF file. Removed.


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

https://reviews.llvm.org/D71575