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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I think this is pretty good for a first pass, but I would like to see this split up into (at least) three patches (one for each plugin). That way, we can properly focus on each plugin. For instance, I'm pretty sure that the object file and symbol vendor changes are fully testable. The dynamic loader stuff may or may not be, but I don't want to discuss that yet to avoid too many parallel threads going on.



================
Comment at: lldb/source/API/SystemInitializerFull.cpp:179
   ObjectFilePECOFF::Initialize();
+  wasm::ObjectFileWasm::Initialize();
 
----------------
sbc100 wrote:
> Why is the namespace needed here for wasm but not the other three above.. seems inconsistent.
Some of the older code puts "plugins" into the default namespace, but lately we've started to put new plugins into their own namespaces. However, most of the old plugins have not been migrated yet.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:299
+      section_type = eSectionTypeCode;
+    else if (g_sect_name_dwarf_debug_abbrev == sect_info.name.c_str())
+      section_type = eSectionTypeDWARFDebugAbbrev;
----------------
teemperor wrote:
> Your `sect_info.name` is already a std::string so comparing here against a ConstString is just a slower and less readable.
ELF and PECOFF code has been already converted to use StringSwitch for this stuff. I'd do the same here.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:314
   EM_BPF = 247,           // Linux kernel bpf virtual machine
+  EM_WASM = 248,          // WebAssembly
 };
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71575