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

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl added a comment.

Thanks for the comments @jhenderson



================
Comment at: llvm/lib/Object/ELF.cpp:643-644
+  }
+  if (ULEBSizeErr)
+    return std::move(ULEBSizeErr);
+  if (!Cur)
----------------
jhenderson wrote:
> 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.
Thanks. I am going to keep the uint32_t restriction. It's not looking bad.


================
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();
----------------
jhenderson wrote:
> 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.
Thanks. You're right.
I am fixing this in two ways:
  # Changing `ReadULEB128AsUInt32` to return zero when `ULEBSizeErr` is set.
  #  Checking `ULEBSizeErr` in the inner loop to prevent from further iteration when the error is set (similar to the cursor error).

After this change, the unit test failed with "Success values must still be checked prior to being destroyed" which made me realize that the error checking order may leave the Cursor error unchecked. So I reversed the checks after the loop to fix it.

I also added a test which exercises the case that you mentioned. Had to encode ULEB128 since our YAML format does not take in `NumBlocks` as input.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/bb-addr-map.test:86
+    Type:   SHT_PROGBITS
+    ShSize: 0x10
+  - Name:   bb_addr_map_2
----------------
jhenderson wrote:
> 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).
You're right. Fixed. (I now understand why we use ShSize for truncating the section).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511