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

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added a comment.

I think this is really nice. I have some minor remarks here and there but otherwise this LGTM.



================
Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:88
+
+  if ((module_sp = modules.FindFirstModule(module_spec))) {
+    UpdateLoadedSections(module_sp, link_map_addr, base_addr,
----------------
what about making this if and the one blow a `if (ModuleSP module_sp = ...) { ...; return module_sp; }`. Then you don't need to do the double-parentheses trick and the end of this function can just be `return ModuleSP();` so it is obvious that the end of this function is the error code path.


================
Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:171
+  auto arch = m_process->GetTarget().GetArchitecture();
+  if (arch.GetMachine() != llvm::Triple::wasm32) {
+    return ThreadPlanSP();
----------------
no brackets


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:32
+    const char *magic = reinterpret_cast<const char *>(data_sp->GetBytes());
+    if (strncmp(magic, llvm::wasm::WasmMagic, sizeof(llvm::wasm::WasmMagic)) !=
+        0) {
----------------
aprantl wrote:
> Is a StringRef comparison easier to read here?
(IMHO it is)


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:65
+    data_sp = MapFileData(*file, length, file_offset);
+    if (!data_sp) {
+      return nullptr;
----------------
no brackets


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:79
+    data_sp = MapFileData(*file, length, file_offset);
+    if (!data_sp) {
+      return nullptr;
----------------
No brackets


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:88
+  ArchSpec spec = objfile_up->GetArchitecture();
+  if (spec && objfile_up->SetModulesArchitecture(spec)) {
+    return objfile_up.release();
----------------
no brackets


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:110
+
+// static
+bool ObjectFileWasm::GetVaruint7(DataExtractor &section_header_data,
----------------
I don't think we do these `static` comments usually in LLVM?


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:128
+  uint64_t value = section_header_data.GetULEB128(offset_ptr);
+  if (*offset_ptr == initial_offset || value > uint64_t(1) << 32) {
+    return false;
----------------
Single line if -> no brackets needed.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:172
+        }
+        std::string sect_name(reinterpret_cast<const char *>(name_bytes),
+                              name_len);
----------------
Should also be switched to ConstString if you make the member of the section info a ConstString.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:191
+            }
+            m_symbols_url =
+                ConstString(reinterpret_cast<const char *>(url_bytes), url_len);
----------------
This member is only created here and only used below from what I can see? Also you never compare it against any other strings so it should be a `std::string`.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:292
+  for (SectionInfoCollConstIter it = m_sect_infos.begin();
+       it != m_sect_infos.end(); ++it) {
+    const section_info &sect_info = *it;
----------------
range-based loop


================
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;
----------------
Your `sect_info.name` is already a std::string so comparing here against a ConstString is just a slower and less readable.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:447
+  ModuleSP module_sp(GetModule());
+  if (module_sp) {
+    std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
----------------
early-exit


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:449
+    std::lock_guard<std::recursive_mutex> guard(module_sp->GetMutex());
+    s->Printf("%p: ", static_cast<void *>(this));
+    s->Indent();
----------------
Bonus: You can write this code by directly using `llvm::raw_ostream` by just calling `s->AsRawOstream()` to get the equivalent `raw_ostream`. I migrate all code to LLVM's stream classes so not having more code using lldb::Stream would be nice (but not required to get this patch in).

(same for the Stream code below)


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.h:116
+    uint32_t id;
+    std::string name;
+  } section_info_t;
----------------
This should be a `ConstString`. OR you keep this as a `std::string` and then you move all other ConstString variables that compare against your section names to be just plain `std::string`, `llvm::StringRef` and `llvm::StringSwitch`. But mixing `ConstString` and normal strings brings us the disadvantages of both worlds (hard to read and slow to compare) without any benefits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71575