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

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added a comment.

Sounds exciting. My comments are all about formatting and coding style, if someone has something technical to say, too that would be appreciated.



================
Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:1
+//===-- DynamicLoaderWasmDYLD.cpp --------------------------------*- C++
+//-*-===//
----------------
1. This hangs over the line
2. a -*- C++ -*- is only necessary for .h files where C vs. C++ is ambiguous



================
Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:77
+// This method should be called for each Wasm module loaded in the debuggee,
+// with base_addr = 0x{module_id}|00000000 (for example 0x0000000100000000).
+
----------------
This should be a doxygen comment.


================
Comment at: lldb/source/Plugins/DynamicLoader/WASM-DYLD/DynamicLoaderWasmDYLD.cpp:132
+              // offset in the Wasm module.
+              if (section_sp->GetName() == "code") {
+                if (m_process->GetTarget().SetSectionLoadAddress(
----------------
Can you convert this to early exits? The deep nesting makes the control flow difficult to read.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:1
+//===-- ObjectFileWasm.cpp -------------------------------- -*- C++ -*-===//
+//
----------------
ditto


================
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) {
----------------
Is a StringRef comparison easier to read here?


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:72
+  assert(data_sp);
+  if (!ValidateModuleHeader(data_sp)) {
+    return nullptr;
----------------
Do you want any form error logging/handling here?


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:76
+
+  // Update the data to contain the entire file if it doesn't already
+  if (data_sp->GetByteSize() < length) {
----------------
Please use full sentences in comments with a trailing `.`.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:99
+                                                 addr_t header_addr) {
+  if (ValidateModuleHeader(data_sp)) {
+    std::unique_ptr<ObjectFileWasm> objfile_up(
----------------
early-exitify?


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:150
+  // - the actual contents.
+  if (GetVaruint7(section_header_data, &offset, &section_id) &&
+      GetVaruint32(section_header_data, &offset, &payload_len)) {
----------------
Again early exits would make this easier to read.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.cpp:394
+// These 64-bit addresses will be used to request code ranges for a specific
+// module from the WebAssembly engine.
+
----------------
doxygen comment.


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.h:46
+
+  // PluginInterface protocol
+  ConstString GetPluginName() override { return GetPluginNameStatic(); }
----------------
FYI you can group doxygen comments like this:
```
/// PluginInterface protocol
/// \{
...
/// \}
```


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.h:102
+
+  static bool GetVaruint7(DataExtractor &section_header_data,
+                          lldb::offset_t *offset_ptr, uint8_t *result);
----------------
`llvm::Optional<uint8_t> GetVaruint7()`


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.h:104
+                          lldb::offset_t *offset_ptr, uint8_t *result);
+  static bool GetVaruint32(DataExtractor &section_header_data,
+                           lldb::offset_t *offset_ptr, uint32_t *result);
----------------
same here


================
Comment at: lldb/source/Plugins/ObjectFile/WASM/ObjectFileWasm.h:119
+
+  void DumpSectionHeader(Stream *s, const section_info_t &sh);
+  void DumpSectionHeaders(Stream *s);
----------------
doxygen comments on all (most) non-override function would be useful.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71575