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

[Paolo Severini via Phabricator via lldb-commits]

paolosev added inline comments.


================
Comment at: lldb/source/Utility/DataExtractor.cpp:914
+/// Extract an unsigned LEB128 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.
----------------
labath wrote:
> It doesn't look like this actually happens, does it? (If max_value is exceeded, the offset will still be updated, right?).
> 
> And overall, I am not very happy with backdooring an api inconsistent with the rest of the DataExtractor (I am aware it was clayborg's idea). Overall, it would probably be better to use the llvm DataExtractor class, which has the Cursor interface designed to solve some of the problems you have here (it can handle EOF, it cannot check the uleb magnitude). And it tries to minimize the number of times you need to error check everything. The usage of it could be something like:
> ```
> llvm::DataExtractor llvm_data = lldb_data.GetAsLLVM();
> llvm::DataExtractor::Cursor c(0);
> unsigned id = llvm_data.GetU8(c);
> unsigned payload_len = llvm_data.GetULEB128(c);
> if (!c)
>   return c.takeError();
> // id and payload_len are valid here
> if (id == 0) {
>   unsigned name_len = llvm_data.GetULEB128(c);
>   SmallVector<uint8_t, 32> name_storage;
>   llvm_data.GetU8(c, name_storage, name_len);
>   if (!c)
>     return c.takeError();
>   // name_len and name valid here
>   StringRef name = toStringRef(makeArrayRef(name_storage));
>   unsigned section_length = ...;
>   m_sect_infos.push_back(...)
> }
> ```
> This won't handle the uleb magnitude check, but these checks seem irrelevant and/or subsumable by other, more useful checks: a) Checking the name length is not necessary, as the code will fail for any names longer 1024 anyway (as that's the amount of data you read); b) instead of `section_len < 2^32` it seems more useful to check that `*offset_ptr + section_len` is less than `2^32`, to make sure we don't wrap the `module_id` part of the "address".
Good points! Changed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71575