[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:637
+    std::vector<typename Elf_BBAddrMap::BBEntry> BBEntries;
+    for (uint32_t BlockID = 0; Cur && BlockID < NumBlocks; ++BlockID) {
+      uint32_t Offset = ReadULEB128AsUInt32();
----------------
If the ULEB for NumBlocks happened to be 0xffffffffffffffff, I think NumBlocks will be 0xffffffff, which will mean you'll add that many (0, 0, 0) entries to BBEntries. I suspect this is best avoided due to the time and memory cost before you eventually get the reported error.

Adding an extra check to ensure `ULEBSizeErr` isn't bad here is probably necessary.


================
Comment at: llvm/lib/Object/ELF.cpp:643-644
+  }
+  if (ULEBSizeErr)
+    return std::move(ULEBSizeErr);
+  if (!Cur)
----------------
rahmanl wrote:
> jhenderson wrote:
> > I might be mistaken, but won't this cause an assertion due to `Cur` not being checked //and reported// if it is in a bad state? I think it could happen if reading `NumBlocks` produces too large a number, and then a later value is truncated (for example).
> Thanks for the scrutiny. I think this won't happen because `ReadULEB128AsUInt32` always checks ULEBSizeErr and return zero if it's in the error state. However, if we ever read with `Data.getULEB128(Cur)` we may run into the unhandled error problem.
> I added a unit test case to exercise the case that you mentioned, but at this point, I am not sure if the benefit of restricting to uint32_t outweighs the readability cost of this code.
I see, yes. I'd prefer to keep the checks, but if you think it's really making it hard to read, I'm not opposed to reverting to a version without them.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:59
+
+# TRUNCATED: warning: '[[FILE]]': unable to dump SHT_LLVM_BB_ADDR_MAP section with index 1: unable to decode LEB128 at offset [[OFFSET]]: malformed uleb128, extends past end
+
----------------
Would it make sense to show that the second section is still dumped in this case?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:86
+    Type:   SHT_PROGBITS
+    ShSize: 0x10
+  - Name:   bb_addr_map_2
----------------
Why `ShSize`? Do you mean `Size` to give it a non-zero actual size on disk? (`ShSize` overrides only the value in the header - it doesn't impact the section placement).


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:553
   void printCGProfile() override;
+  void printBBAddrMaps() override;
   void printAddrsig() override;
----------------
Here and below, it looks like clang-tidy is unhappy. You probably need to change the base virtual function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511