[PATCH] D95511: [llvm-readelf] Support dumping the BB address map section with --bb-addr-map.

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added inline comments.


================
Comment at: llvm/lib/Object/ELF.cpp:379
+    uintX_t Address = static_cast<uintX_t>(Data.getAddress(Cur));
+    uint32_t NumBlocks = Data.getULEB128(Cur);
+    std::vector<typename Elf_BBAddrMap::BBEntry> BBEntries;
----------------
jhenderson wrote:
> `getULEB128` can return `uint64_t`, so I'd use `uint64_t` here and below - it defers truncation of large values to as late as possible, which I think is a good thing. Optionally, also consider an error check that the values aren't greater than max UINT32. A ULEB128 has no defined max size, so if the input format is ULEB128, users might theoretically expect to allow more than 32-bits worth of data. Alternatively, consider using `uin64_t` for the struct member sizes. Values outside that range will be picked up by the ULEB parser and reported, so we don't need to do more then.
Thanks. Based on your suggestion, I plan to read these values as uint64_t and add cast to uint32_t (And add proper error checking). But I noticed that my elf2yaml code had the same problem (reading ULEB-encoded values as uint32_t). So I appreciate if you could please review D95767 before coming back to this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511