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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ELF.cpp:624
+  // uint32_t limit.
+  // Also returns zero if ULEBSizeErr is already in in error state.
+  auto ReadULEB128AsUInt32 = [&Data, &Cur, &ULEBSizeErr]() -> uint32_t {
----------------



================
Comment at: llvm/lib/Object/ELF.cpp:644
+    std::vector<typename Elf_BBAddrMap::BBEntry> BBEntries;
+    for (uint32_t BlockID = 0; !ULEBSizeErr && Cur && (BlockID < NumBlocks); ++BlockID) {
+      uint32_t Offset = ReadULEB128AsUInt32();
----------------
clang-format is complaining.

> Had to encode ULEB128 since our YAML format does not take in NumBlocks as input.

Sounds like this should be added to the YAML input support, so that you can write a custom NumBlocks value that doesn't match the real number of blocks. We do similar things for ELF sections for example, where you can override the implicit value with a value of your own choice which gets written to the field.


================
Comment at: llvm/lib/Object/ELF.cpp:652-659
+  // ULEBSizeErr is always checked before the cursor in the loop above, which
+  // makes it possible for the cursor error to be left unchecked.
+  // Here, we reverse the order and check the cursor first to avoid the
+  // unhandled error problem.
+  if (!Cur)
+    return std::move(Cur.takeError());
+  if (ULEBSizeErr)
----------------
This is probably fine, but feels a little fragile. How about using `joinErrors` to combine the two errors into a single one that can be reported, as suggested in the inline edit? That also avoids any risk of problems in the event the code changes such that they could both be in a bad state. `joinErrors` of an `Error::success()` and a non-success `Error` is safe and equivalent to just returning the actual `Error`, if I remember correctly.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:583
+
+  // Synthetize a test which encodes an out-of-range value (0x70fffffff) for
+  // the number of blocks.
----------------
Consider instead making the suggested yaml2obj change first, and then using that for this test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511