[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 a comment.

I don't know the disassembly code well enough to feel comfortable reviewing it or its corresponding testing, without sinking more time into this review than I have available, so someone else better look at those parts at least.



================
Comment at: llvm/include/llvm/Object/ELFObjectFile.h:107
+  /// Returns a vector of all BB address maps in the object file. When
+  //`TextSectionIndex` is specified, only returns the BB address maps
+  // corresponding to the section with that index.
----------------



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1274
+      if (auto *Elf = dyn_cast<ELFObjectFileBase>(Obj)) {
+        // Read the BB-address-map corresponding to this section if present.
+        auto SectionBBAddrMapsOrErr = Elf->readBBAddrMap(Section.getIndex());
----------------



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1277
+        if (!SectionBBAddrMapsOrErr)
+          reportError(SectionBBAddrMapsOrErr.takeError(), Obj->getFileName());
+        for (const auto &FunctionBBAddrMap : *SectionBBAddrMapsOrErr)
----------------
I wonder if this should be a warning rather than an error: it's still possible to disassemble, just not with all the information.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1499
+          if (Iter1 != BBAddrMapLabels.end()) {
+            for (const std::string &Label : Iter1->second)
+              FOS << "<" << Label << ">:\n";
----------------



================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:608
+// Test for the ELFObjectFile::readBBAddrMap API.
+TEST(ELFObjectFileTest, InvalidReadBBAddrMap) {
+  StringRef CommonYamlString(R"(
----------------
Do you have a test case where the TextSectionIndex is specified, but there are no matching sections?

I'd also expect to see checks that the retrieved entries match what was expected, but don't see any of those.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:666
+               "section with index 1: invalid section index: 10");
+  // Linked sections are not check when not targetting a specific text section.
+  DoCheckSucceeds(InvalidLinkedYamlString, None);
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124560