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

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

I would suggest removing GetVaruint7 and GetVaruint32 and adding "llvm::Optional<uint8_t> DataExtractor::GetULEB128(uint64_t *offset_ptr, uint64_t max_value);" as mentioned in inlined comments.



================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:49
+  lldb::offset_t initial_offset = *offset_ptr;
+  uint64_t value = section_header_data.GetULEB128(offset_ptr);
+  if (*offset_ptr == initial_offset || value > 127)
----------------
Is it ok if we consume more than 1 byte here?  What is the offset points to a larger ULEB, are we ok with advancing the offset by multiple bytes or should we back it up and return llvm::None?

This might be a good candidate to add to DataExtractor directly as:

```
/// Extract a ULEB128 number with a specified max value. If the extracted value exceeds 
/// "max_value" the offset will be left unchanged and llvm::None will be returned.
llvm::Optional<uint8_t> DataExtractor::GetULEB128(uint64_t *offset_ptr, uint64_t max_value);
```

There are many places where we extract a uint64_t, but only need a uint16_t (like in the DWARF parser where all DW_TAG_XXXX, DW_AT_XXX and DW_FORM_XXX values must only be uint16_t values but are encoded as ULEB128 values. So this could be used elsewhere if we do put it into DataExtractor.


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:162-163
+  // - the actual contents.
+  llvm::Optional<uint8_t> section_id =
+      GetVaruint7(section_header_data, &offset);
+  if (!section_id)
----------------
Is this a one byte section ID or is it a ULEB? Not sure why it would be encoded as a ULEB if it is always one byte? IF this really is just a one byte value, then replace with:

```
uint8_t section_id = data.GetU8(&offset);
```


================
Comment at: lldb/source/Plugins/ObjectFile/wasm/ObjectFileWasm.cpp:59
+/// Reads a LEB128 variable-length unsigned integer, limited to 32 bits.
+static llvm::Optional<uint32_t> GetVaruint32(DataExtractor &section_header_data,
+                                             lldb::offset_t *offset_ptr) {
----------------
remove if we add:
```
llvm::Optional<uint8_t> DataExtractor::GetULEB128(uint64_t *offset_ptr, uint64_t max_value);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71575