[Propeller] Make decoding BBAddrMaps trace through relocations

[aidengrossman]

Summary

Currently when using the LLVM tools (eg llvm-readobj, llvm-objdump) to
find information about basic block locations using the propeller tooling
in relocatable object files function addresses are not mapped properly
which causes problems. In llvm-readobj this means that incorrect
function names will be pulled. In llvm-objdum this means that most BBs
won't show up in the output if --symbolize-operands is used. This patch
changes the behavior of decodeBBAddrMap to trace through relocations
to get correct function addresses if it is going through a relocatable
object file. This fixes the behavior in both tools and also other
consumers of decodeBBAddrMap. Some helper functions have been added
in/refactoring done to aid in grabbing BB address map sections now that
in some cases both relocation and BB address map sections need to be
obtained at the same time.

Regression tests moved around/added.

[aidengrossman]

This patch is designed to allow tracing through relocations with the basic block address infrastructure currently in llvm-readobj and llvm-objdump. Currently when using relocatable object files with these tools with basic block sections, the tools almost don't work at all (llvm-objdump with --symbolize-operands will work quite poorly not showing most of the basic block labels), to not having the intended functionality (ie all function addresses in llvm-readobj --bb-addr-map are zero and the name mapping is not right, always mapping to the first function name). This patch fixes that functionality. I'm not completely sure I've taken the right approach here as I'm not too familiar with this section of the code base, so please let me know if there is some common infrastructure or some different approach I should be taking.

[rahmanl]

Thanks for doing this. It has been on our to-do list. @shenhan can share more insights.

Some structural comments.

llvm/lib/Object/ELF.cpp

642–660

Let's see if we can remove this function too, by incorporating the check into the IsMatch predicate.

688

Let's move ELFDumper::getSectionAndRelocations to ELFFile and use it here instead of writing a custom version.

702–707

The IsRelocatable path should also read the Address (even though it's normally zero). Something like:

uint64_t SectionOffset = Cur.tell();
Address = static_cast<uintX_t>(Data.getAddress(Cur));
if (IsRelocatable) {
  assert(Address == 0); // maybe
  Address += OffsetToRela.at(SectionOffset).r_addend;
}

722

Just define a lambda or a static function for this, and read in Elf_Rela_Range.

724

Let's move this into the caller.

738–741

Change this to assertion (since it's checking something about the code logic).

[shenhan]

Thanks for working on this. You mentioned

This patch changes the behavior of decodeBBAddrMap to trace through relocations to get correct function addresses if it is going through a relocatable object file."

How is this achieved? Each BBAddrMap section has a corresponding .rela section, but that .rela section does not contain real address information (the addresses are only known at linker time). (Most of the time) It has entries of <offset_into_bbaddrmap_section, ".text", addend>. Are you computing function addresses as ".text address as exhibited in the object file + addend"?

[aidengrossman]

How is this achieved? Each BBAddrMap section has a corresponding .rela section, but that .rela section does not contain real address information (the addresses are only known at linker time). (Most of the time) It has entries of <offset_into_bbaddrmap_section, ".text", addend>. Are you computing function addresses as ".text address as exhibited in the object file + addend"?

Sorry. I should've been a bit more clear here. You are correct in that I'm computing the function "addresses" as offsets within the .text section. I've modified decodeBBAddrMap to return the offset of the function from the beginning of the text section so that everything can be resolved properly when addresses aren't available.

Sorry for the delay in addressing reviewer feedback. I should have @rahmanl 's comments addressed tonight or tomorrow so that we can move this patch forward.

[jhenderson]

I've taken a low-level view of the code, as I'm not massively familiar with the higher level stuff. In addition to my various inline comments, please add testing for all the new error/warning paths.

llvm/lib/Object/ELF.cpp

673

Don't use auto here. It's not obvious what the return type is.

680

Are you sure you need the cast here?

694–696

Here and in many other places, don't use braces for single line if statements like this. See https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements.

700

https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

707

728

This probably shouldn't use auto, as the type isn't obvious from the context.

736

Should RelaSec be const & not just const?

744

Again, don't use auto here.

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

6

Why is this unsupported on Windows?

26

This Content is a bit of an opaque blob. Would it be possible and clearer to write this test using assembly instead?

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

5

Again, why is this unsupported on Windows?

81

Again, would it be practical and cleare to write this test using assembly?

llvm/tools/llvm-readobj/ELFDumper.cpp

7095

This warning should provide more context about why it cannot get the section. That context should be available in the error returned by the function you called. Indeed, I wouldn't be surprised if you had a failure in some build configurations because the Error within the Expected has been thrown away without any usage.

Also applies below.

[aidengrossman]

Addressed reviewer feedback, split patch into two to separate the refactoring
of getSectionAndRelocations from the rest of the patch to hopefully keep
things a little bit easier to review.

[aidengrossman]

Thank you everyone for all the feedback. It has been quite helpful. I believe I have addressed most of the feedback given (and probably opened up room for quite a bit more). I still need to add some tests for some of the new failure cases, but they should be relatively sparse after refactoring to use getSectionAndRelocations() instead of rewriting it. I've also split this patch into two (https://reviews.llvm.org/D144783). I tried to get Phabricator to pick it up as a patch stack, but for whatever reason I couldn't get it working.

llvm/lib/Object/ELF.cpp

722

I'm not sure what you mean here by defining a lambda or static function for this. I've moved it into the caller as is.

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

26

The content is definitely quite opaque. From what I can gather from the yaml2obj tests, it is possible to write these tests in assembly (without piping anything around), but all the content has to be in hex with assembly commentary beside it. The current precedent for tests regarding this feature is to use similar opaque blobs (although maybe this should be changed at some point). However, I'm not sure how much value adding assembly would bring. The assembly is essentially the same as the disassembled input that I'm checking later on plus a couple extra irrelevant annotations. For debugging the test case, I think having the expected output would be the most helpful and adding assembly for the input would not add a lot of value. I might be missing something obvious here though, so please let me know if I'm incorrect in one of my points or there's something I haven't thought of.

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

81

This test can have the content of the .text section removed, so I've removed it completely for clarity.

[aidengrossman]

Rebase.

[rahmanl]

Haven't looked at the tests yet. Thanks for the revision.

llvm/lib/Object/ELF.cpp

646

Add a comment for this variable. The name is too general and does not convey what it really is.
Also maybe reword to FunctionAddressTranslaton since this is just about function addresses in SHT_LLVM_BB_ADDR_MAP.

650

Append a context to this error (e.g, "Unable to read the relocation section for section: ....").

651

remove these braces : https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

704

Should we report Error here if the translation entry doesn't exist?

llvm/lib/Object/ELFObjectFile.cpp

697

Let's use reportError here. SHT_LLVM_BB_ADDR_MAP should always link to a text section. We may have to change the `llvm-objdump tests a bit then.

[rahmanl]

llvm/lib/Object/ELFObjectFile.cpp

697

Disregard my previous comment. But is there a reason you are changing the error message?

[aidengrossman]

Rebase.

[aidengrossman]

Address reviewer feedback.

llvm/lib/Object/ELFObjectFile.cpp

697

I think I was messing around with the refactoring at some point and changed it. I've changed it back since there's no reason to change it to this.

[jhenderson]

llvm/lib/Object/ELF.cpp

648

Nit.

Also, since this comment is tied to the following variable, there should not be a blank line between them (there might want to be one before the comment, but that's up to you).

705–710

You could turn this into a plain if without an else if you flip the two parts around, i.e. make the if clause FOTIterator == FunctionOffsetTranslations.end().

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

6

6

Given we have an open ticket for this, I wonder if this should be an XFAIL, not UNSUPPORTED. The idea being that when the issue is fixed, the test will start x-passing, and not be forgotten about.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

6

Also, see above re. XFAIL vs UNSUPPORTED

llvm/tools/llvm-readobj/ELFDumper.cpp

7097

This (currently) returns an Error, so that return value needs capturing and checking.

[aidengrossman]

Address reviewer feedback and rebase.

[aidengrossman]

llvm/tools/llvm-readobj/ELFDumper.cpp

7097

This was originally using the implementation of getSectionAndRelocations from within ELFDumper.cpp that wrapped around the actual implementation, but I've removed that abstraction due to how we're now handling errors and also to improve clarity.

[aidengrossman]

Switch back to UNSUPPORTED on the tests since the original failure was due to
some new behavior in VS2022 and the minimum version for building LLVM is
currently VS2019 (where the test should pass).

[aidengrossman]

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

6

I think we should stick with UNSUPPORTED here because the original failure was due to some change specific to VS2022, and the minimum version to build LLVM is currently VS2019. This means that there might be some buildbots/developers/users building LLVM on VS2019 where this test would unexpectedly pass.

[rahmanl]

llvm/lib/Object/ELFObjectFile.cpp

710

std::nullopt.

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

6

In fact, current build bots still use VS2019 which makes the test succeed on Windows too. The issue with VS2022 was revealed later upon downstream builds with VS2022.

44–55

You can also remove these extra blocks to shorten the test.

80–89

This assembly is irrelevant. right? I mean the object file can be synthesized even if it's not from real code. In that case, I suggest we remove the assembly code.

[aidengrossman]

Use std::nullopt instead of {} when creating an optional. Also went through
elf-bbaddrmap-symbolize-relocatable.yaml to make it only use a single basic
block per function, removed the opaque content binary blob, and also removed
any assembly checks since they're not relevant to what we are trying to test
here.

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

80–89

Right, the assembly is not relevant to the test. I essentially just copied over the test format from the non-relocatable bbaddrmap symbolization test. I've removed the assembly and am now just checking for the basic block labels (which are what was missing in the first place) and this now prevents duplicate functionality between the tests)

[jhenderson]

llvm/lib/Object/ELF.cpp

706

https://llvm.org/docs/CodingStandards.html#error-and-warning-messages

llvm/lib/Object/ELFObjectFile.cpp

710

As far as I understand it, you don't need either - std::optional has a default constructor to initialise as empty. In other words, this line should just be std::optional<Elf_Shdr> RelaSec;

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

6

Not addressed still.

6

Ah, yes, sorry for the noise. It would be nice to have the ability to XFAIL for specific compiler versions, but I don't think that functionality exists in lit.

26

I was referring to making the whole input an assembly file and not using YAML at all. However, with the removal of the opaque blob, that no longer is relevant.

llvm/tools/llvm-readobj/ELFDumper.cpp

7096

Not clang-formatted.

7098

I have a slight concern that this is slightly fragile - if the underlying code changes to emit other kinds of errors, this will fail here. It should probably handle the error base class, in some manner, so that we don't end up with a surprise crash in the future.

[aidengrossman]

Address reviewer feedback.

[aidengrossman]

llvm/tools/llvm-readobj/ELFDumper.cpp

7098

Definitely a valid concern. I fixed up the call sites in ELFDumper.cpp but forgot to do it here. Should be fixed in the latest revision.

[aidengrossman]

Switch from using llvm::MapVector to MapVector.

[aidengrossman]

Rebase.

[rahmanl]

No major comments. Looks good.

llvm/lib/Object/ELFObjectFile.cpp

692

Flip this for more readability, i.e,

if (!TextSectionIndex) return true;

Expected<const Elf_Shdr *> TextSecOrErr = EF.getSection(Sec.sh_link);
...

712

This can be nullptr.

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

102–109

Maybe remove these too.

llvm/tools/llvm-readobj/ELFDumper.cpp

7092

No need to capture anything here.

7109

Change this consistently with the other code.

7111

Again, handle the case where this is nullptr.

[jhenderson]

Code looks broadly fine, but I don't see any testing for the error cases?

llvm/lib/Object/ELF.cpp

707

I wonder if utohexstr would be better here, to give a hex offset?

[aidengrossman]

Address reviewer feedback, add regression tests for new possible failure modes.

llvm/lib/Object/ELF.cpp

707

Thank you for the suggestion. I would think it would definitely be better here to be consistent with the rest of the output from the LLVM tools.

llvm/lib/Object/ELFObjectFile.cpp

712

Added some error handling here and a test case.

[jhenderson]

llvm/lib/Object/ELF.cpp

707

There's a Twine::utohexstr (that I didn't know about), which would be more appropriate here, I think.

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

69

Nit: please add a new line at the end of the file.

72

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

104

There's no particular need for this, is there? Same elsewhere.

118

132

Nit: no new line at EOF.

151

Isn't this the same as the test case two up?

175

?

Is it worth the following test case: an ET_EXEC or ET_DYN file with a .rela.dyn section that doesn't reference a particular section? To show that we don't get this warning in that situation?

[aidengrossman]

Address reviewer feedback.

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

175

Fixed the typo. Thank you. When creating a shared object/exectuable, the addresses for the functions are already populated in the SHT_LLVM_BB_ADDR_MAP` sections, so we don't need to do any relocations. The new paths introduced by this patch also only run on ET_REL files.

[jhenderson]

LGTM, barring typo (and also possibly extra test case, though I'm uncertain about that one).

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

148

Still got a typo :)

175

My thinking is mostly along the lines of "what do we have that ensures that the if (ET_REL) check is actually there?" In other words, if that check disappeared, would this warning potentially pop up?

[aidengrossman]

Address reviewer feedback.

[aidengrossman]

@rahmanl Do you mind reviewing my most recent changes and accepting this (assuming there are no further comments) when you get a chance? Thank you.

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

175

Ah, that logic makes a lot of sense. I've added an ET_DYN test with a .rela.dyn section to test this. Thanks for the suggestion!

[rahmanl]

llvm/lib/Object/ELF.cpp

643

Do not pass arguments by const std::optional& as it will make a copy if we pass it a Elf_Shdr. I suggest we pass by const Elf_Shdr * and let nullptr indicate that the value doesn't exist.

645

Callers may call this function on relocatable sections without providing RelaSec. If assertions are not enabled, we will still get zero addresses for all functions. I think it's better to report error here instead.

In general, assertions should be used to check the invariants of the code, not things like input validation.

648

Here and elsewhere, please use SHT_LLVM_BB_ADDR_MAP when referring to the bb-address-map data. This will make code search easier.

657

Remove braces for this loop.

706

Move this above:

uintX_t Address = static_cast<uintX_t>(Data.getAddress(Cur));
if (!Cur) return Cur.takeError();
if (IsRelocatable) {
   assert(Address == 0);
   auto FOTIterator = FunctionOffsetTranslations.find(SectionOffset);
   if (FOTIterator == FunctionOffsetTranslations.end()) 
             return createError("failed to get relocation data for offset: " +
                          Twine::utohexstr(SectionOffset) + " in section " +
                          describe(*this, Sec));
...

llvm/lib/Object/ELFObjectFile.cpp

719–724

Moving this error handling to decodeBBAddr and passing RelaSec by const Elf_Shdr * will greatly simplify this code:

for (auto const &[Sec, RelocSec] : *SectionRelocMapOrErr) {
    Expected<std::vector<BBAddrMap>> BBAddrMapOrErr =
        EF.decodeBBAddrMap(*Sec, RelocSec);
}

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

12

nit: unnecessary spaces.

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

169

Change to SHT_LLVM_BB_ADDR_MAP.

[jhenderson]

llvm/lib/Object/ELF.cpp

645

Hmm, I think assertions are perfectly valid to use for input validation for function arguments. The question I use to decide on assertion versus error is "can the assertion ever be hit with any of our existing code paths" - in other words, if somebody passed in an arbitrary binary blob to one of our tools that uses this code, could it theoretically get here?

The LLVM coding standard's first example on assert usage actually covers this case. See https://llvm.org/docs/CodingStandards.html#assert-liberally. In that case, the index is passed into the function and validated via an assert.

As an aside, @aidengrossman, you should have a message explaining the assertion (see the coding standards document).

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

12

The spaces should be left there. The idea is to provide an indentation to show that the FileCheck line is a continuation of the previous line (without having to spot that the previous line ends with \. It's a common pattern in many newer tests in the tools (especially ones I've been involved with).

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

168

[rahmanl]

llvm/lib/Object/ELF.cpp

645

Thanks for your clarification @jhenderson. Using assertions for input validation does make sense.

Let me rephrase my opinion. The input validation here is a complex one. It is already being done three times in this code (in the callers ELFObjectFile.cpp:711 , ELFDumper.cpp:7196, and here). The callers already create errors in such cases. I think moving the error handling code here simplifies the code. We don't have to add a message explaining that this function can only be called when RelaSec != std::nullopt || getHeader().e_type != ELF:ET_REL when we already have error handling for the same case in the callers.

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

12

You're right. sorry for the noise.

[rahmanl]

llvm/lib/Object/ELF.cpp

645

Actually, I take my argument back. This function should block calls with assertions. Errors are relevant only in the context of the callers.

651

Here, add
assert(RelaSec && "Trying to read a SHT_LLVM_BB_ADDR_MAP section in a relocatable object file without providing a relocation section.");
And remove the assertion above.

llvm/lib/Object/ELFObjectFile.cpp

719–724

if (IsRelocatable && !RelocSec) return createError(....)
Expected<std::vector<BBAddrMap>> BBAddrMapOrErr =
        EF.decodeBBAddrMap(*Sec, RelocSec);

[aidengrossman]

Address reviewer feedback.

[rahmanl]

Appreciate the work @aidengrossman.

llvm/include/llvm/Object/ELF.h

public:
   getSectionName(const Elf_Shdr &Section,
                  WarningHandler WarnHandler = &defaultWarningHandler) const;
   Expected<StringRef> getSectionName(const Elf_Shdr &Section,
                                      StringRef DotShstrtab) const;
   template <typename T>
   Expected<ArrayRef<T>> getSectionContentsAsArray(const Elf_Shdr &Sec) const;
   Expected<ArrayRef<uint8_t>> getSectionContents(const Elf_Shdr &Sec) const;
   Expected<ArrayRef<uint8_t>> getSegmentContents(const Elf_Phdr &Phdr) const;
-  Expected<std::vector<BBAddrMap>> decodeBBAddrMap(const Elf_Shdr &Sec) const;
+  Expected<std::vector<BBAddrMap>>
+  decodeBBAddrMap(const Elf_Shdr &Sec,
+                  const std::optional<Elf_Shdr> &RelaSec = {}) const;
 
   /// Returns a map from every section matching \p IsMatch to its relocation
   /// section, or \p nullptr if it has no relocation section. This function
   /// returns an error if any of the \p IsMatch calls fail or if it fails to
   /// retrieve the content section of any relocation section.
   Expected<llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *>>
   getSectionAndRelocations(
       std::function<Expected<bool>(const Elf_Shdr &)> IsMatch) const;

llvm/lib/Object/ELF.cpp

return createError("can't map virtual address 0x" +
                        Twine::utohexstr(Phdr.p_offset + Phdr.p_filesz) +
                        ", which is greater than the file size (0x" +
                        Twine::utohexstr(getBufSize()) + ")");
 
   return base() + Offset;
 }
 
 template <class ELFT>
-Expected<std::vector<BBAddrMap>>
-ELFFile<ELFT>::decodeBBAddrMap(const Elf_Shdr &Sec) const {
+Expected<std::vector<BBAddrMap>> ELFFile<ELFT>::decodeBBAddrMap(
+    const Elf_Shdr &Sec, const std::optional<Elf_Shdr> &RelaSec) const {
+  bool IsRelocatable = getHeader().e_type == ELF::ET_REL;
+  assert(!IsRelocatable || (IsRelocatable && RelaSec));
+
+  // This DenseMap maps the offset of each function (the location of the 
+  // reference to the function in the bbaddrmap section) to the addend
+  // (the location of the function in the text section).
+  llvm::DenseMap<uint64_t, uint64_t> FunctionOffsetTranslations;
+  if (IsRelocatable && RelaSec) {
+    Expected<Elf_Rela_Range> Relas = this->relas(*RelaSec);
+    if (!Relas)
+      return createError("unable to read relocations for section " +
+                         describe(*this, Sec) + ": " +
+                         toString(Relas.takeError()));
+    for (Elf_Rela Rela : *Relas) {
+      FunctionOffsetTranslations[Rela.r_offset] = Rela.r_addend;
+    }
+  }
   Expected<ArrayRef<uint8_t>> ContentsOrErr = getSectionContents(Sec);
   if (!ContentsOrErr)
     return ContentsOrErr.takeError();
   ArrayRef<uint8_t> Content = *ContentsOrErr;
   DataExtractor Data(Content, isLE(), ELFT::Is64Bits ? 8 : 4);
   std::vector<BBAddrMap> FunctionEntries;
 
   DataExtractor::Cursor Cur(0);
   Error ULEBSizeErr = Error::success();
   // Helper to extract and decode the next ULEB128 value as uint32_t.
   // Returns zero and sets ULEBSizeErr if the ULEB128 value exceeds the uint32_t
   // limit.
   // Also returns zero if ULEBSizeErr is already in an error state.
   auto ReadULEB128AsUInt32 = [&Data, &Cur, &ULEBSizeErr]() -> uint32_t {
     // Bail out and do not extract data if ULEBSizeErr is already set.
     if (ULEBSizeErr)
       return 0;
     uint64_t Offset = Cur.tell();
     uint64_t Value = Data.getULEB128(Cur);
     if (Value > UINT32_MAX) {
       ULEBSizeErr = createError(
           "ULEB128 value at offset 0x" + Twine::utohexstr(Offset) +
           " exceeds UINT32_MAX (0x" + Twine::utohexstr(Value) + ")");
       return 0;
     }
     return static_cast<uint32_t>(Value);
   };
 
   uint8_t Version = 0;
   while (!ULEBSizeErr && Cur && Cur.tell() < Content.size()) {
     if (Sec.sh_type == ELF::SHT_LLVM_BB_ADDR_MAP) {
       Version = Data.getU8(Cur);
       if (!Cur)
         break;
       if (Version > 2)
         return createError("unsupported SHT_LLVM_BB_ADDR_MAP version: " +
                            Twine(static_cast<int>(Version)));
       Data.getU8(Cur); // Feature byte
     }
+    uint64_t SectionOffset = Cur.tell();
     uintX_t Address = static_cast<uintX_t>(Data.getAddress(Cur));
+    if (IsRelocatable) {
+      assert(Address == 0);
+      auto FOTIterator = FunctionOffsetTranslations.find(SectionOffset);
+      if (FOTIterator == FunctionOffsetTranslations.end())
+        return createError("Failed to get relocation data for offset: " +
+                           Twine(SectionOffset) + " in section " +
+                           describe(*this, Sec));
+        Address = FOTIterator->second;
+    }
     uint32_t NumBlocks = ReadULEB128AsUInt32();
     std::vector<BBAddrMap::BBEntry> BBEntries;
     uint32_t PrevBBEndOffset = 0;
     for (uint32_t BlockIndex = 0;
          !ULEBSizeErr && Cur && (BlockIndex < NumBlocks); ++BlockIndex) {
       uint32_t ID = Version >= 2 ? ReadULEB128AsUInt32() : BlockIndex;
       uint32_t Offset = ReadULEB128AsUInt32();
       uint32_t Size = ReadULEB128AsUInt32();
       uint32_t Metadata = ReadULEB128AsUInt32();
       if (Version >= 1) {
         // Offset is calculated relative to the end of the previous BB.
         Offset += PrevBBEndOffset;
         PrevBBEndOffset = Offset + Size;
       }
       BBEntries.push_back({ID, Offset, Size, Metadata});
     }
     FunctionEntries.push_back({Address, std::move(BBEntries)});
   }
   // Either Cur is in the error state, or ULEBSizeError is set (not both), but
   // we join the two errors here to be safe.
   if (!Cur || ULEBSizeErr)
     return joinErrors(Cur.takeError(), std::move(ULEBSizeErr));
   return FunctionEntries;
 }
 
 template <class ELFT>
 Expected<
     llvm::MapVector<const typename ELFT::Shdr *, const typename ELFT::Shdr *>>
 ELFFile<ELFT>::getSectionAndRelocations(
     std::function<Expected<bool>(const Elf_Shdr &)> IsMatch) const {
   llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> SecToRelocMap;
   for (const Elf_Shdr &Sec : cantFail(this->sections())) {
     Expected<bool> DoesSectionMatch = IsMatch(Sec);
     if (!DoesSectionMatch) {
       return DoesSectionMatch.takeError();
       continue;
     }
     if (*DoesSectionMatch) {
       if (SecToRelocMap.insert(std::make_pair(&Sec, (const Elf_Shdr *)nullptr))
               .second)
         continue;
     }

llvm/lib/Object/ELFObjectFile.cpp

ELFObjectFileBase::getPltAddresses() const {
   }
   return Result;
 }
 
 template <class ELFT>
 Expected<std::vector<BBAddrMap>> static readBBAddrMapImpl(
     const ELFFile<ELFT> &EF, std::optional<unsigned> TextSectionIndex) {
   using Elf_Shdr = typename ELFT::Shdr;
+  bool IsRelocatable = EF.getHeader().e_type == ELF::ET_REL;
   std::vector<BBAddrMap> BBAddrMaps;
+
   const auto &Sections = cantFail(EF.sections());
-  for (const Elf_Shdr &Sec : Sections) {
+  auto IsMatch = [&](const Elf_Shdr &Sec) -> Expected<bool> {
     if (Sec.sh_type != ELF::SHT_LLVM_BB_ADDR_MAP &&
         Sec.sh_type != ELF::SHT_LLVM_BB_ADDR_MAP_V0)
-      continue;
+      return false;
     if (TextSectionIndex) {
       Expected<const Elf_Shdr *> TextSecOrErr = EF.getSection(Sec.sh_link);
       if (!TextSecOrErr)
         return createError("unable to get the linked-to section for " +
                            describe(EF, Sec) + ": " +
                            toString(TextSecOrErr.takeError()));
       if (*TextSectionIndex != std::distance(Sections.begin(), *TextSecOrErr))
-        continue;
+        return false;
     }
-    Expected<std::vector<BBAddrMap>> BBAddrMapOrErr = EF.decodeBBAddrMap(Sec);
+    return true;
+  };
+
+  Expected<llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *>>
+      SectionRelocMapOrErr = EF.getSectionAndRelocations(IsMatch);
+  if (!SectionRelocMapOrErr)
+    return SectionRelocMapOrErr.takeError();
+
+  for (auto const &[Sec, RelocSec] : *SectionRelocMapOrErr) {
+    std::optional<Elf_Shdr> RelaSec = std::nullopt;
+    if (IsRelocatable)
+      RelaSec = *RelocSec;
+    Expected<std::vector<BBAddrMap>> BBAddrMapOrErr =
+        EF.decodeBBAddrMap(*Sec, RelaSec);
     if (!BBAddrMapOrErr)
-      return createError("unable to read " + describe(EF, Sec) + ": " +
+      return createError("unable to read " + describe(EF, *Sec) + ": " +
                          toString(BBAddrMapOrErr.takeError()));
     std::move(BBAddrMapOrErr->begin(), BBAddrMapOrErr->end(),
               std::back_inserter(BBAddrMaps));
   }
   return BBAddrMaps;
 }
 
 template <class ELFT>
 static Expected<std::vector<VersionEntry>>
 readDynsymVersionsImpl(const ELFFile<ELFT> &EF,
                        ELFObjectFileBase::elf_symbol_iterator_range Symbols) {
   using Elf_Shdr = typename ELFT::Shdr;
   const Elf_Shdr *VerSec = nullptr;
   const Elf_Shdr *VerNeedSec = nullptr;
   const Elf_Shdr *VerDefSec = nullptr;
   // The user should ensure sections() can't fail here.

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-disassemble-symbolize-operands.yaml

 ## Test that in the presence of SHT_LLVM_BB_ADDR_MAP sections,
 ## --symbolize-operands can display <BB*> labels.
 
 ## Fails on windows (https://github.com/llvm/llvm-project/issues/60013).
 # UNSUPPORTED: system-windows
 
 ## Executable object file.
-# RUN: yaml2obj --docnum=1 -DTYPE=ET_EXEC -DFOO_ADDR=0x4000 -DBAR_ADDR=0x5000 %s -o %t1
+# RUN: yaml2obj --docnum=1 -DFOO_ADDR=0x4000 -DBAR_ADDR=0x5000 %s -o %t1
 # RUN: llvm-objdump %t1 -d --symbolize-operands -M intel --no-show-raw-insn --no-leading-addr | \
 # RUN:   FileCheck %s -DSYM=symbol --match-full-lines --check-prefixes=INTEL
 # RUN: llvm-objdump %t1 -d --symbolize-operands -M att --no-show-raw-insn --no-leading-addr | \
 # RUN:   FileCheck %s -DSYM=symbol --match-full-lines --check-prefixes=ATT
 
-## Relocatable object file.
-# RUN: yaml2obj --docnum=1 -DTYPE=ET_REL -DFOO_ADDR=0x0 -DBAR_ADDR=0x0 %s -o %t2
-# RUN: llvm-objdump %t2 -d --symbolize-operands -M intel --no-show-raw-insn --no-leading-addr | \
-# RUN:   FileCheck %s -DSYM=foo+0x200c --match-full-lines --check-prefix=INTEL
-# RUN: llvm-objdump %t2 -d --symbolize-operands -M att --no-show-raw-insn --no-leading-addr | \
-# RUN:   FileCheck %s -DSYM=foo+0x200c --match-full-lines --check-prefix=ATT
-
 ## Executable object file with a single SHT_LLVM_BB_ADDR_MAP for multiple text sections.
 # RUN: yaml2obj --docnum=2 %s -o %t3
 # RUN: llvm-objdump %t3 -d --symbolize-operands -M intel --no-show-raw-insn --no-leading-addr | \
 # RUN:   FileCheck %s -DSYM=symbol --match-full-lines --check-prefixes=INTEL
 # RUN: llvm-objdump %t3 -d --symbolize-operands -M att --no-show-raw-insn --no-leading-addr | \
 # RUN:   FileCheck %s -DSYM=symbol --match-full-lines --check-prefixes=ATT
 
 ## Expect to find the branch and basic block labels and global variable name.

 ## This object file contains a separate text section and SHT_LLVM_BB_ADDR_MAP
 ## section for each of the two functions foo and bar.
 ## This is used to test --symbolize-operands on reloctable and non-relocotable
 ## object files.
 --- !ELF
 FileHeader:
   Class:   ELFCLASS64
   Data:    ELFDATA2LSB
-  Type:    [[TYPE]]
+  Type:    ET_EXEC
   Machine: EM_X86_64
 Sections:
   - Name:    .text.foo
     Type:    SHT_PROGBITS
     Address: [[FOO_ADDR]]
     Flags:   [SHF_ALLOC, SHF_EXECINSTR]
     Content: '503b0505200000907d02ebf5c3'
   - Name:    .text.bar

llvm/test/tools/llvm-objdump/X86/elf-bbaddrmap-symbolize-relocatable.yaml

+## Test that in the presence of SHT_LLVM_BB_ADDR_MAP sections,
+## --symbolize-operands can display <BB*> labels properly in a relocatable
+## object file.
+
+# Fails on windows (https://github.com/llvm/llvm-project/issues/60013).
+# UNSUPPORTED: system-windows
+
+## Relocatable Object file.
+# RUN: yaml2obj %s -o %t1
+# RUN: llvm-objdump %t1 -d --symbolize-operands -M att --no-show-raw-insn --no-leading-addr | \
+# RUN:   FileCheck %s -DSYM=symbol --match-full-lines
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+  SectionHeaderStringTable: .strtab
+Sections:
+  - Name:    .text
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+    Content: FFFF
+  - Name:    .llvm_bb_addr_map
+    Type:    SHT_LLVM_BB_ADDR_MAP
+    Link:    .text
+    Entries:
+      - Version: 2
+        BBEntries:
+          - ID:              0
+            AddressOffset:   0x0
+            Size:            0x1
+            Metadata:        0x1
+      - Version: 2
+        BBEntries:
+          - ID:              0
+            AddressOffset:   0x0
+            Size:            0x1
+            Metadata:        0x1
+  - Name:  .rela.llvm_bb_addr_map
+    Type:  SHT_RELA
+    Flags: [ SHF_INFO_LINK ]
+    Link:  .symtab
+    Info:  .llvm_bb_addr_map
+    Relocations:
+      - Offset: 0x2
+        Symbol: .text
+        Type:   R_X86_64_64
+      - Offset: 0x11
+        Symbol: .text
+        Type:   R_X86_64_64
+        Addend: 1
+Symbols:
+  - Name:    a
+    Section: .text
+    Value:   0x0
+  - Name:    c
+    Section: .text
+    Value:   0x1
+  - Name:    .text
+    Type:    STT_SECTION
+    Section: .text
+
+# CHECK:      <a>:
+# CHECK-NEXT: <BB0>:
+# CHECK:      <c>:
+# CHECK-NEXT: <BB0>:

llvm/test/tools/llvm-readobj/ELF/bb-addr-map-relocatable.test

+## This test checks how we handle the --bb-addr-map option on relocatable
+## object files.
+
+## Fails on windows (https://github.com/llvm/llvm-project/issues/60013).
+# UNSUPPORTED: system-windows
+
+# RUN: yaml2obj %s -o %t1.o
+# RUN: llvm-readobj %t1.o --bb-addr-map | FileCheck %s
+
+# CHECK:      BBAddrMap [
+# CHECK-NEXT:   Function {
+# CHECK-NEXT:     At: 0x0
+# CHECK-NEXT:     Name: <?>
+# CHECK-NEXT:     BB entries [
+# CHECK-NEXT:       {
+# CHECK-NEXT:         ID: 0
+# CHECK-NEXT:         Offset: 0x0
+# CHECK-NEXT:         Size: 0xF
+# CHECK-NEXT:         HasReturn: Yes
+# CHECK-NEXT:         HasTailCall: No
+# CHECK-NEXT:         IsEHPad: No
+# CHECK-NEXT:         CanFallThrough: No
+# CHECK-NEXT:       }
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT:   Function {
+# CHECK-NEXT:     At: 0x10
+# CHECK-NEXT:     Name: <?>
+# CHECK-NEXT:     BB entries [
+# CHECK-NEXT:       {
+# CHECK-NEXT:         ID: 0
+# CHECK-NEXT:         Offset: 0x0
+# CHECK-NEXT:         Size: 0x11
+# CHECK-NEXT:         HasReturn: No
+# CHECK-NEXT:         HasTailCall: No
+# CHECK-NEXT:         IsEHPad: No
+# CHECK-NEXT:         CanFallThrough: Yes
+# CHECK-NEXT:       }
+# CHECK-NEXT:       {
+# CHECK-NEXT:         ID: 1
+# CHECK-NEXT:         Offset: 0x11
+# CHECK-NEXT:         Size: 0xE
+# CHECK-NEXT:         HasReturn: No
+# CHECK-NEXT:         HasTailCall: No
+# CHECK-NEXT:         IsEHPad: No
+# CHECK-NEXT:         CanFallThrough: No
+# CHECK-NEXT:       }
+# CHECK-NEXT:       {
+# CHECK-NEXT:         ID: 2
+# CHECK-NEXT:         Offset: 0x1F
+# CHECK-NEXT:         Size: 0x6
+# CHECK-NEXT:         HasReturn: No
+# CHECK-NEXT:         HasTailCall: No
+# CHECK-NEXT:         IsEHPad: No
+# CHECK-NEXT:         CanFallThrough: Yes
+# CHECK-NEXT:       }
+# CHECK-NEXT:       {
+# CHECK-NEXT:         ID: 3
+# CHECK-NEXT:         Offset: 0x25
+# CHECK-NEXT:         Size: 0x5
+# CHECK-NEXT:         HasReturn: Yes
+# CHECK-NEXT:         HasTailCall: No
+# CHECK-NEXT:         IsEHPad: No
+# CHECK-NEXT:         CanFallThrough: No
+# CHECK-NEXT:       }
+# CHECK-NEXT:     ]
+# CHECK-NEXT:   }
+# CHECK-NEXT: ]
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS64
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_X86_64
+  SectionHeaderStringTable: .strtab
+Sections:
+  - Name:    .text
+    Type:    SHT_PROGBITS
+    Flags:   [ SHF_ALLOC, SHF_EXECINSTR ]
+  - Name:    .llvm_bb_addr_map
+    Type:    SHT_LLVM_BB_ADDR_MAP
+    Link:    .text
+    Entries:
+      - Version: 2
+        BBEntries:
+          - ID:              0
+            AddressOffset:   0x0
+            Size:            0xF
+            Metadata:        0x1
+      - Version: 2
+        BBEntries:
+          - ID:              0
+            AddressOffset:   0x0
+            Size:            0x11
+            Metadata:        0x8
+          - ID:              1
+            AddressOffset:   0x0
+            Size:            0xE
+            Metadata:        0x0
+          - ID:              2
+            AddressOffset:   0x0
+            Size:            0x6
+            Metadata:        0x8
+          - ID:              3
+            AddressOffset:   0x0
+            Size:            0x5
+            Metadata:        0x1
+  - Name:  .rela.llvm_bb_addr_map
+    Type:  SHT_RELA
+    Flags: [ SHF_INFO_LINK ]
+    Link:  .symtab
+    Info:  .llvm_bb_addr_map
+    Relocations:
+      - Offset: 0x2
+        Symbol: .text
+        Type:   R_X86_64_64
+      - Offset: 0x11
+        Symbol: .text
+        Type:   R_X86_64_64
+        Addend: 16
+Symbols:
+  - Name:    a
+    Section: .text
+    Value:   0x0
+  - Name:    c
+    Section: .text
+    Value:   0x10
+  - Name:    .text
+    Type:    STT_SECTION
+    Section: .text

llvm/tools/llvm-readobj/ELFDumper.cpp

for (uint32_t I = 0, Size = CGProfileOrErr->size(); I != Size; ++I) {
       }
       W.printNumber("Weight", CGPE.cgp_weight);
     }
   }
 }
 
 template <class ELFT> void LLVMELFDumper<ELFT>::printBBAddrMaps() {
   bool IsRelocatable = this->Obj.getHeader().e_type == ELF::ET_REL;
-  for (const Elf_Shdr &Sec : cantFail(this->Obj.sections())) {
-    if (Sec.sh_type != SHT_LLVM_BB_ADDR_MAP &&
-        Sec.sh_type != SHT_LLVM_BB_ADDR_MAP_V0) {
-      continue;
-    }
+  using Elf_Shdr = typename ELFT::Shdr;
+  auto IsMatch = [&](const Elf_Shdr &Sec) -> bool {
+    return Sec.sh_type == ELF::SHT_LLVM_BB_ADDR_MAP ||
+           Sec.sh_type == ELF::SHT_LLVM_BB_ADDR_MAP_V0;
+  };
+  Expected<llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *>> SecRelocMapOrErr = this->Obj.getSectionAndRelocations(IsMatch);
+  if(!SecRelocMapOrErr) {
+    handleAllErrors(SecRelocMapOrErr.takeError(), [&](StringError &SE) {
+      this->reportUniqueWarning(SE.getMessage());
+    });
+    return;
+  for (auto const& [Sec, RelocSec] : *SecRelocMapOrErr) {
     std::optional<const Elf_Shdr *> FunctionSec;
     if (IsRelocatable)
       FunctionSec =
-          unwrapOrError(this->FileName, this->Obj.getSection(Sec.sh_link));
+          unwrapOrError(this->FileName, this->Obj.getSection(Sec->sh_link));
     ListScope L(W, "BBAddrMap");
+    std::optional<Elf_Shdr> RelaSec = {};
+    if(IsRelocatable)
+      RelaSec = *RelocSec;
     Expected<std::vector<BBAddrMap>> BBAddrMapOrErr =
-        this->Obj.decodeBBAddrMap(Sec);
+        this->Obj.decodeBBAddrMap(*Sec, RelaSec);
     if (!BBAddrMapOrErr) {
-      this->reportUniqueWarning("unable to dump " + this->describe(Sec) + ": " +
+      this->reportUniqueWarning("unable to dump " + this->describe(*Sec) + ": " +
                                 toString(BBAddrMapOrErr.takeError()));
       continue;
     }
     for (const BBAddrMap &AM : *BBAddrMapOrErr) {
       DictScope D(W, "Function");
       W.printHex("At", AM.Addr);
       SmallVector<uint32_t> FuncSymIndex =
           this->getSymbolIndexesForFunctionAddress(AM.Addr, FunctionSec);
       std::string FuncName = "<?>";
       if (FuncSymIndex.empty())
         this->reportUniqueWarning(
             "could not identify function symbol for address (0x" +
-            Twine::utohexstr(AM.Addr) + ") in " + this->describe(Sec));
+            Twine::utohexstr(AM.Addr) + ") in " + this->describe(*Sec));
       else
         FuncName = this->getStaticSymbolName(FuncSymIndex.front());
       W.printString("Name", FuncName);
 
       ListScope L(W, "BB entries");
       for (const BBAddrMap::BBEntry &BBE : AM.BBEntries) {
         DictScope L(W);
         W.printNumber("ID", BBE.ID);