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
§ion_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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
