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