[PATCH] D124560: [llvm-objdump] Symbolize branch targets and basic block addresses based on the bb-address-map when present in the object file.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:671
+    if (!bb_addr_map) {
+      return createError("Unable to read the bb-addr-map section " +
+                         describe(EF, Sec));
----------------
Errors should use lower-case first letter (and no trailing full stop). Applies below too.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:676
+  }
+  return createError("No bb-addr-map section found!");
+}
----------------
I'd remove the exclamation mark. It feels a little unprofessional to me, if I'm honest: it's not unreasonable for such section to exist apart from anything else.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:991
+    if (!ObjBBAddrMapOrErr) {
+      consumeError(ObjBBAddrMapOrErr.takeError());
+      return;
----------------
It seems like this should at least be a warning?


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1332
 
+    std::unordered_map<uint64_t, BBAddrMap> AddrToBBAddrMap;
+    if (SymbolizeOperands)
----------------
The Programmer's manual says not to use unordererd_map, although I doubt its reasoning is necessarily important here. Would `DenseMap` be more appropriate anyway?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124560