[PATCH] D71575: [LLDB] Add ObjectFileWasm plugin for WebAssembly debugging

[Greg Clayton via Phabricator via llvm-commits]

clayborg added inline comments.


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:34
+/// Checks whether the data buffer starts with a valid Wasm module header.
+static bool ValidateModuleHeader(const DataBufferSP &data_sp) {
+  if (!data_sp || data_sp->GetByteSize() < kWasmHeaderSize)
----------------
I typically would put "data_sp" into a DataExtractor and extract a uint32_t and then check the decoded value with something like:

```
static bool ValidateModuleHeader(DataExtractor &data, uint64_t *offset_ptr) {
  auto magic = data.GetU32(offset_ptr);
  if (magic == WASM_MAGIC)
    return true;
  if (magic == WASM_CIGAM) {
    // Set byte order in DataExtractor
    data.SetByteOrder(data.GetByteOrder() == eByteOrderBig ? eByteOrderLittle : eByteOrderBig);
    return true;
  }
  return false;
}
```

This function expects a DataExtractor to be passed in that has "data_sp" inside of it with the host endian set as the byte order. It will set the byte order correctly. It also expects to have two uint32_t macros defined: WASM_MAGIC and WASM_CIGAM. These contain the non byte swapped and the byte swapped magic values. Easy to replace those with real definitions from else where (llvm::file_magic::wasm_object? Not sure of the type of this though, seemed like a StringRef).


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:44
+
+  uint32_t version = llvm::support::endian::read32le(Ptr);
+  return version == llvm::wasm::WasmVersion;
----------------
use DataExtractor::GetU32()? Or is the byte order always little endian for wasm object files?


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:187-195
+    const uint8_t *name_bytes = section_header_data.PeekData(offset, *name_len);
+    // If a custom section has a name longer than the allocated buffer or longer
+    // than the data left in the image, ignore this section.
+    if (!name_bytes)
+      return false;
+
+    llvm::StringRef sect_name(reinterpret_cast<const char *>(name_bytes),
----------------
All these lines can use the GetCStr with a length:
```
ConstString sect_name(data.GetCStr(&offset, *name_len));
if (!sect_name)
  return false;
```


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:199
+    m_sect_infos.push_back(section_info{*offset_ptr + offset, section_length,
+                                        *section_id, ConstString(sect_name)});
+    offset += section_length;
----------------
remove ConstString constructor here if we switch code above as suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71575