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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

(I was out of town for like one week. Just coming back.)

The subject may be clearer if you mention `--symbolize-operands` somewhere: this does not affect non `--symbolize-operands` functionality AFAICT.

Use one spelling for `BB address map`. If `BB address map` is canonical, avoid `bb-address-map`. It's also fine to refer to the concept with SHT_LLVM_BB_ADDR_MAP everywhere. (I don't think any object file format will pick up this soon)



================
Comment at: llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-disassemble-symbolize-operands-multi-section.yaml:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-objdump %t -d --symbolize-operands -M intel --no-show-raw-insn --no-leading-addr | \
----------------
Add a file-level comment stating the purpose of the test.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:997
+    return;
+  for (unsigned I = 0; I < Iter->second.BBEntries.size(); ++I) {
+    uint64_t BBAddress = Iter->second.BBEntries[I].Offset + Iter->second.Addr;
----------------
See https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1281
+          AddrToBBAddrMap.try_emplace(FunctionBBAddrMap.Addr,
+                                      std::move(FunctionBBAddrMap));
+      }
----------------
FunctionBBAddrMap is a const reference. `std::move` cannot save a copy.

Use `for (BBAddrMap &FunctionBBAddrMap : ...`


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1447
 
-      std::unordered_map<uint64_t, std::string> AllLabels;
-      if (SymbolizeOperands)
+      DenseMap<uint64_t, std::string> AllLabels;
+      DenseMap<uint64_t, std::vector<std::string>> BBAddrMapLabels;
----------------
The original std::unordered_map<uint64_t, std::string> is better.

Inserting -1 and -2 (tombstone and empty) into a `DenseMap<uint64_t, ...>` leads to an assertion failure. The values are not realistic but we should guard against pathological cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124560