[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/docs/CommandGuide/llvm-readelf.rst:37
+
+ Display the contents of the BB address map section(s), which contains the
+ address of each function, along with the relative offset of its basic blocks.
----------------
jhenderson wrote:
> Spell out "BB". I assume it is "basic block"? Also, as this is only implemented for LLVM output style, I'd mention that in the documentation. Also, you say "section(s)", implying there might be more than one. However, the code only supports dumping a single section per object file. Either your code or your documentation needs updating!
You're right. I am changing the code to support dumping multiple BB address map sections.


================
Comment at: llvm/lib/Object/ELF.cpp:643-644
+  }
+  if (ULEBSizeErr)
+    return std::move(ULEBSizeErr);
+  if (!Cur)
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95511