[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/docs/CommandGuide/llvm-objdump.rst:229
 
+  When bb-address-map section is present (i.e., object file is built with `-fbasic-block-sections=labels`) labels are retrieved from that section instead.
+
----------------



================
Comment at: llvm/include/llvm/Object/ELFTypes.h:816
+
+    bool operator==(const BBEntry &Other) const {
+      return Offset == Other.Offset && Size == Other.Size &&
----------------
Did you consider using `std::memcmp` here instead of checking each individual field separately. The problem with the current code is that if you add a new field, it'll be easy to forget to add it in the comparison. On the other hand, `std::memcmp` would prevent adding non-POD fields, like `std::vector` etc, so may not be appropriate. Up to you.


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:689
+  // Check that when no bb-address-map section is found for a text section,
+  // we return empty result.
+  DoCheckSucceeds(CommonYamlString, /*TextSectionIndex=*/2, {});
----------------



================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:717
+               "uleb128, extends past end");
+  // Check that when we can read the other section's bb-address-maps which are
+  // valid.
----------------
I think?


================
Comment at: llvm/unittests/Object/ELFObjectFileTest.cpp:608
+// Test for the ELFObjectFile::readBBAddrMap API.
+TEST(ELFObjectFileTest, InvalidReadBBAddrMap) {
+  StringRef CommonYamlString(R"(
----------------
rahmanl wrote:
> jhenderson wrote:
> > 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.
> > Do you have a test case where the TextSectionIndex is specified, but there are no matching sections?
> 
> Would you please clarify?
> 
> I do have tests for when the requested `TextSectionIndex` doesn't match with `sh_link` of the bb-address-map sections. The call to `readBBAddrMap` would succeed and return an empty vector in such case -- indicating that no BB-address-map was found for that section. Do you suggest I return Error in this case? 
> 
> Or do you mean a test for when the requested `TextSectionIndex` is not a valid text section?
> 
> > I'd also expect to see checks that the retrieved entries match what was expected, but don't see any of those.
> 
> Sure. I added additional cases and also support for equality of `BBAddrMap`s.
> I do have tests for when the requested TextSectionIndex doesn't match with sh_link of the bb-address-map sections. The call to readBBAddrMap would succeed and return an empty vector in such case -- indicating that no BB-address-map was found for that section. Do you suggest I return Error in this case?

No, what you've added in this latest version is what I wanted.

Aside: the test name is no longer really valid.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124560