[ELF] Move getSectionAndRelocations to ELF.cpp from ELFDumper.cpp

[aidengrossman]

Summary

This refactoring will allow for this utility function to be used in
other places in the codebase outside of the llvm-readobj tool.

[aidengrossman]

This is the refactoring of getSectionsAndRelocations requested in https://reviews.llvm.org/D143841. I've put it in a separate patch to try and keep things a little bit more manageable. I still need to add some test cases (probably unit tests) for the failure modes since I've changed it around quite a bit (new failure modes + older failure modes would only report a text warning rather than returning an Error). Other than that though, this patch should be good to go for review.

[aidengrossman]

Add unit tests for GetSectionAndRelocations.

[aidengrossman]

Remove erroneously committed core dump.

[rahmanl]

llvm/lib/Object/ELF.cpp

727–735

How about reporting warnings here and changing SecToRelcMap type to llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>> so you can put the errors there? This way you can just let the clients do the error handling on their own.

[jhenderson]

Code essentially looks good. I haven't had time to go through the tests yet, but if you plan on changing/expanding them anyway, I might as well wait until those are ready.

llvm/lib/Object/ELF.cpp

713

I'm not sure this should be called IsSuccess, if I'm honest, since it's not a boolean. I'd name it something as simples as Errors or the like.

730–733

I believe this can just be as the inline edit shows (reformatting will be required still).

739

This will cause a failure in certain configurations, because IsSuccess is not being consumed in any way.

Is there a reason this and the other return statement in the loop don't simply use joinErrors like everything else?

[jhenderson]

llvm/lib/Object/ELF.cpp

727–735

This is a library, so we shouldn't be reporting warnings directly here (see also my lightning talk :) https://www.youtube.com/watch?v=YSEY4pg1YB0). Or do you mean something else?

[rahmanl]

llvm/include/llvm/Object/ELF.h

397

Let's not add complexity here. IsMatch should be a simple function which returns true or false.

[rahmanl]

llvm/include/llvm/Object/ELF.h

397

If adopting the approach I mentioned below, we can keep the return value as Expected<bool>> here and just capture the error and store that in the map.

llvm/lib/Object/ELF.cpp

727–735

Makes sense. No need to report the warning here. My main suggestion is to avoid both returning by parameter and by return value. So I guess in this case we can return by llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>>. This way, the client will need to consume the error only when accessing the corresponding map entry. WDYT?

[aidengrossman]

llvm/include/llvm/Object/ELF.h

397

See my comment below on the other approach. I think that returning an Expected<bool> here doesn't add too much complexity, and it allows for errors to propagate up through the library code so that callers can handle errors depending upon their specific requirements. Otherwise we either have to assert (might not be a bad design decision in the only caller of this function returning errors through the matcher though) or report an error/warning within the library code, which I'd prefer to avoid.

llvm/lib/Object/ELF.cpp

727–735

Changing to llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>> wouldn't really capture the failure modes that are present here. We either can't get the section linked to by the Rel/Rela's SH_INFO which means we can't associate it with any specific relocated section, or the IsMatch call fails which also means we likely can't associate it with any content section within the map.

739

Thank you for catching this. I've changed to joinErrors (not sure why I didn't use it here).

[aidengrossman]

Address reviewer feedback.

[aidengrossman]

Remove some left over if statement brackets.

[rahmanl]

llvm/lib/Object/ELF.cpp

727–735

I see what you mean. My concern with the current logic is that the function is very hard to explain (because of double return by parameter and by return value) and use. Sections with IsMatch=true and with failed rela sections, will be mapped to nullptr, but their failure will be returned.

How about Expected<llvm::MapVector<const Elf_Shdr *, Expected<const Elf_Shdr *>>> ? If any of the IsMatch calls fail, then we outright return it. Otherwise, we fill in the map with all the matched sections. If any matched section's rela section cannot be captured, then the failure will be in the map. WDYT?

[aidengrossman]

llvm/lib/Object/ELF.cpp

727–735

Returning an Expected<llvm::MapVector<const Elf_Shdr*, const Elf_Shdr*>> should work pretty well and would definitely make the function easier to understand. Having the value of the MapVector also be an Expected would make things slightly messy though I would think since we would only be creating errors when adding content sections, and then destroying the errors when associating a relocation section (which also means the value of the error has to be checked). Returning a std::optional as the value seems like it should work well though since it captures exactly the information that we want (address to section or the fact that there's no section). And we should be able to just continue propagating all of the errors as done currently.

[rahmanl]

llvm/lib/Object/ELF.cpp

727–735

since we would only be creating errors when adding content sections, and then destroying the errors when associating a relocation section (which also means the value of the error has to be checked)

I am not following the logic. Why do we need to insert initial entries in the map? We insert an entry in the map iff IsMatch(Sec) && (Sec.sh_type == ELF::SHT_RELA || Sec.sh_type == ELF::SHT_REL)) and the value of the entry will either contain the relocations section or Error if it can't be retrieved. These errors will of course need to be consumed by the caller (which is the intention of making this a library call). I can see that the current callers (printCGProfile and printRelocatableStackSizes) both already iterate over the entire map. So they can easily report warnings upon consuming errors and it will be the same for our case. We don't need to keep another wrapper around this function ELFDumper<ELFT>::getSectionAndRelocations that applies a specific error handling. We can simply let the callers choose what to do with the errors.

Clients can also ignore the errors which will trigger assertion error if -DLLVM_ENABLE_ABI_BREAKING_CHECKS=true IIUC.

[aidengrossman]

llvm/lib/Object/ELF.cpp

727–735

The function loops through all of the sections and starts by seeing if the current section matches (ie it is a matching content section). If so, it inserts a pointer to the section into the map with the relocation section entered in as nullptr to be filled in later. If the section, doesn't match, but is a relocation section, the section linked to by SH_INFO is checked. That section will already be in the map if the relocation section follows the content section (which I believe is normally the case).

If the section is a rel/rela section, there are two failure cases. We either fail to get the relocated section and then can't even associate it with a content/relocated section to put it into the map, or the call to IsMatch with the linked to content section fails where I also don't think it would really be correct to put anything into the map, which suggests to me that the only error we'd be putting in an Expected<const Elf_Shdr*> map value would be a "not found" error, which has the consequences mentioned earlier.

We don't necessarily have to insert the content section pointer with a nullptr to the relocation section if we come across the content section first, but changing that around would mean that content sections that don't have relocation sections don't get put in.

Hopefully my logic makes sense here. If it doesn't, or I'm misunderstanding what you're trying to say, please let me know. Thank you for the feedback so far.

[jhenderson]

llvm/lib/Object/ELF.cpp

717

You've still got a return here in the middle of the loop. Intentional or not?

727–735

I'd strongly advise against a Map that contains Expected<T> entries, because it means at some point, you have to iterate over all the entries to consume all the errors. Whilst you might do that already in the current usage, future users of the code might well just want to get an individual section that is in this vector, and will run into problems later on. I think it would be better to just bundle all errors into one with joinErrors that is then returned in the form of Expected<MapVector<...>> or similar.

[rahmanl]

llvm/lib/Object/ELF.cpp

727–735

This makes sense. Also I was also under the wrong impression that there are errors associated to every matching content section while errors are associated to the rela sections for which we can't get the content section (which is a serious error. Surprised to see that the initial code considers those warnings). Matching sections which don't have a rela section simply get nullptr. Then we can adopt Jame's suggestion of just returning Expected<MapVector<const Elf_Shdr *, const Elf_Shdr *>>.

[aidengrossman]

Address some reviewer feedback, particularly moving from return by parameter to
return by value, returning an Expected<...> that will contain errors if there
is a failure. This does produce a slight functional change in
getSectionAndRelocations() because instead of returning a partially
incomplete map if there are any errors, we are now not returning any map and
just returning the errors. This doesn't break any test cases and doesn't seem
like a bad change of behavior to me, but it is something to note.

[aidengrossman]

Marked some inline comments as done. I've also gotten rid of the getSectionAndRelocations wrapper inside of ELFDumper.cpp because it was somewhat confusing in regards to the naming and also is less compatible with how we're now treating error handling. Again, this now does change some behavior as we're now failing earlier, so maybe this should be noted somewhere.

[aidengrossman]

Continue after getting an error so we aren't trying to get a value from the
Expected<...> object and refactor some of the test code.

[rahmanl]

Thank you. Looks clean.

llvm/include/llvm/Object/ELF.h

397

I think a function comment here will be very helpful. Something like:

Returns a map from every section matching \p IsMatch to its relocation section, or \p nullptr if it has no relocation section. Returns error if any of the \p IsMatch calls fail or if it fails to retrieve the content section of any relocation section.

llvm/lib/Object/ELF.cpp

722

Use braces on the outer if to avoid a potential dangling else situation.

747

remove these braces.

llvm/tools/llvm-readobj/ELFDumper.cpp

6230

Can we turn these into failures without breaking any tests? I think early return is not consistent with reporting warnings anymore.

[rahmanl]

llvm/tools/llvm-readobj/ELFDumper.cpp

6230

Alternatively, report a warning similar to
reportWarning("unable to print stack size: " + StackSizeRelocMapOrErr.takeError().getMessage())

[aidengrossman]

Made getSectionAndRelocations fail fast, adjusted tests to match, address
other reviewer feedback.

[aidengrossman]

llvm/tools/llvm-readobj/ELFDumper.cpp

6230

I've changed the function to fail early (since before the latest revision I just had it looping through to find all the errors which doesn't make sense). This breaks one test that had the same duplicate failure which I've fixed.

[jhenderson]

llvm/include/llvm/Object/ELF.h

404

Maybe a dumb question, but why are we qualifying MapVector with the llvm namespace here and elsewhere?

llvm/lib/Object/ELF.cpp

718–719

Do you want to try again with these two lines ;)

I'm assuming one of the two lines is spurious...

732–735

Same comment as above.

740–741

And again.

llvm/test/tools/llvm-readobj/ELF/stack-sizes.test

874 ↗

(On Diff #502497)

It would be nice to not lost the second warning, since it's for a different section, and therefore indicates that there's more than one problem. I think you'd have to use joinErrors in your loop, to gather all errors into a single one, and then handle them at the caller end, individually.

llvm/tools/llvm-readobj/ELFDumper.cpp

6229

7060

[aidengrossman]

Address reviewer feedback.

llvm/include/llvm/Object/ELF.h

404

Not a dumb question. For some reason I completely forgot about the fact that we're either inside a namspace llvm {} block or using namespace llvm;. This should be fixed with the latest revision.

llvm/test/tools/llvm-readobj/ELF/stack-sizes.test

874 ↗

(On Diff #502497)

Fixed by moving back to using joinErrors within the getSectionAndRelocations() function. This also addresses a couple of your other comments since I forgot to remove the continue; statements left behind there.

[aidengrossman]

Fix uses of llvm::MapVector that I didn't catch last time.

[rahmanl]

Thanks for your patience with the code review. And please wait for James's approval too.

llvm/tools/llvm-readobj/ELFDumper.cpp

7065

Just noticed this loop doesn't check that the map value is not nullptr.

If you're willing to fix it, please add something similar:

// Warn about call graph profile sections without a relocation section.
if (!CGRelSection) {
  reportWarning(createError("SHT_LLVM_CALL_GRAPH_PROFILE (" + describe(*CGSection) +
                            ") does not have a corresponding "
                            "relocation section"),
                FileName);
  continue;
}

llvm/unittests/Object/ELFObjectFileTest.cpp

892 ↗

(On Diff #502551)

remove brace for single-line if statements per LLVM coding style.

[jhenderson]

llvm/tools/llvm-readobj/ELFDumper.cpp

7060

This code looks to be untested.

llvm/unittests/Object/ELFObjectFileTest.cpp

837 ↗

(On Diff #502551)

[aidengrossman]

Address reviewer feedback.

llvm/tools/llvm-readobj/ELFDumper.cpp

7065

It does, just within the getSymbolIndices() function. Lines 7008 and 7025 in the revision you submitted this comment on.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/call-graph-profile.test

355 ↗

(On Diff #503079)

I'm not conviced we should mention the function name here - it could easily rot. It could just say "failed to get a section associated with a relocation section" or something to that effect.

357 ↗

(On Diff #503079)

The YAML is usually after the actual test case that it is used in.

367 ↗

(On Diff #503079)

Do you actually need the symbols?

376 ↗

(On Diff #503079)

Nit: no new line at EOF.

[aidengrossman]

Address reviewer feedback.

[aidengrossman]

Update inline comments.

[jhenderson]

Looks good, barring one nit.

llvm/unittests/Object/ELFObjectFileTest.cpp

837 ↗

(On Diff #503528)

[aidengrossman]

Address reviewer feedback.

[aidengrossman]

Thank you everyone for the feedback and catching all of my mistakes.

llvm/include/llvm/Object/ELF.h

 //===- ELF.h - ELF object file implementation -------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file declares the ELFFile template class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_OBJECT_ELF_H
 #define LLVM_OBJECT_ELF_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/Object/ELFTypes.h"
 #include "llvm/Object/Error.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Error.h"
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;
+  Error getSectionAndRelocations(
+      std::function<Expected<bool>(const Elf_Shdr &)> IsMatch,
+      llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> &SecToRelocMap) const;
 
   void createFakeSections();
 };
 
 using ELF32LEFile = ELFFile<ELF32LE>;
 using ELF64LEFile = ELFFile<ELF64LE>;
 using ELF32BEFile = ELFFile<ELF32BE>;
 using ELF64BEFile = ELFFile<ELF64BE>;
 
 template <class ELFT>
 inline Expected<const typename ELFT::Shdr *>
 getSection(typename ELFT::ShdrRange Sections, uint32_t Index) {
   if (Index >= Sections.size())
     return createError("invalid section index: " + Twine(Index));

llvm/lib/Object/ELF.cpp

ELFFile<ELFT>::decodeBBAddrMap(const Elf_Shdr &Sec) const {
   }
   // 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>
+Error ELFFile<ELFT>::getSectionAndRelocations(
+    std::function<Expected<bool>(const Elf_Shdr &)> IsMatch,
+    llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> &SecToRelocMap) const {
+  Error IsSuccess = Error::success();
+  for (const Elf_Shdr &Sec : cantFail(this->sections())) {
+    Expected<bool> DoesSectionMatch = IsMatch(Sec);
+    if(!DoesSectionMatch)
+      return joinErrors(std::move(IsSuccess), DoesSectionMatch.takeError());
+    if (*DoesSectionMatch)
+      if (SecToRelocMap.insert(std::make_pair(&Sec, (const Elf_Shdr *)nullptr))
+              .second)
+        continue;
+
+    if (Sec.sh_type != ELF::SHT_RELA && Sec.sh_type != ELF::SHT_REL)
+      continue;
+
+    Expected<const Elf_Shdr *> RelSecOrErr = this->getSection(Sec.sh_info);
+    if (!RelSecOrErr) {
+      IsSuccess = joinErrors(
+          std::move(IsSuccess),
+          make_error<StringError>(std::error_code(),
+                                  describe(*this, Sec) +
+                                      ": failed to get a relocated section: " +
+              toString(RelSecOrErr.takeError())));
+      continue;
+    }
+    const Elf_Shdr *ContentsSec = *RelSecOrErr;
+    Expected<bool> DoesRelTargetMatch = IsMatch(*ContentsSec);
+    if(!DoesRelTargetMatch)
+      return DoesRelTargetMatch.takeError();
+    if (*DoesRelTargetMatch)
+      SecToRelocMap[ContentsSec] = &Sec;
+  }
+  return IsSuccess;
+}
+
 template class llvm::object::ELFFile<ELF32LE>;
 template class llvm::object::ELFFile<ELF32BE>;
 template class llvm::object::ELFFile<ELF64LE>;
 template class llvm::object::ELFFile<ELF64BE>;

llvm/tools/llvm-readobj/ELFDumper.cpp

virtual void printStackSizeEntry(uint64_t Size,
                                    ArrayRef<std::string> FuncNames) = 0;
 
   void printRelocatableStackSizes(std::function<void()> PrintHeader);
   void printNonRelocatableStackSizes(std::function<void()> PrintHeader);
 
   /// Retrieves sections with corresponding relocation sections based on
   /// IsMatch.
   void getSectionAndRelocations(
-      std::function<bool(const Elf_Shdr &)> IsMatch,
+      std::function<Expected<bool>(const Elf_Shdr &)> IsMatch,
       llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> &SecToRelocMap);
 
   const object::ELFObjectFile<ELFT> &ObjF;
   const ELFFile<ELFT> &Obj;
   StringRef FileName;
 
   Expected<DynRegionInfo> createDRI(uint64_t Offset, uint64_t Size,
                                     uint64_t EntSize) {
while (Offset < Contents.size()) {
                                   Data, &Offset))
         break;
     }
   }
 }
 
 template <class ELFT>
 void ELFDumper<ELFT>::getSectionAndRelocations(
-    std::function<bool(const Elf_Shdr &)> IsMatch,
+    std::function<Expected<bool>(const Elf_Shdr &)> IsMatch,
     llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> &SecToRelocMap) {
-  for (const Elf_Shdr &Sec : cantFail(Obj.sections())) {
-    if (IsMatch(Sec))
-      if (SecToRelocMap.insert(std::make_pair(&Sec, (const Elf_Shdr *)nullptr))
-              .second)
-        continue;
-
-    if (Sec.sh_type != ELF::SHT_RELA && Sec.sh_type != ELF::SHT_REL)
-      continue;
-
-    Expected<const Elf_Shdr *> RelSecOrErr = Obj.getSection(Sec.sh_info);
-    if (!RelSecOrErr) {
-      reportUniqueWarning(describe(Sec) +
-                          ": failed to get a relocated section: " +
-                          toString(RelSecOrErr.takeError()));
-      continue;
-    }
-    const Elf_Shdr *ContentsSec = *RelSecOrErr;
-    if (IsMatch(*ContentsSec))
-      SecToRelocMap[ContentsSec] = &Sec;
-  }
+  Error RecoverableErrors = Obj.getSectionAndRelocations(IsMatch, SecToRelocMap);
+  handleAllErrors(std::move(RecoverableErrors), [&](const StringError &SE) {
+    reportUniqueWarning(SE.getMessage());
+  });
 }
 
 template <class ELFT>
 void ELFDumper<ELFT>::printRelocatableStackSizes(
     std::function<void()> PrintHeader) {
   // Build a map between stack size sections and their corresponding relocation
   // sections.
   llvm::MapVector<const Elf_Shdr *, const Elf_Shdr *> StackSizeRelocMap;
   auto IsMatch = [&](const Elf_Shdr &Sec) -> bool {
     StringRef SectionName;
     if (Expected<StringRef> NameOrErr = Obj.getSectionName(Sec))
       SectionName = *NameOrErr;
     else
       consumeError(NameOrErr.takeError());
 
     return SectionName == ".stack_sizes";
   };
   getSectionAndRelocations(IsMatch, StackSizeRelocMap);
 
   for (const auto &StackSizeMapEntry : StackSizeRelocMap) {
     PrintHeader();
     const Elf_Shdr *StackSizesELFSec = StackSizeMapEntry.first;
     const Elf_Shdr *RelocSec = StackSizeMapEntry.second;
 
     // Warn about stack size sections without a relocation section.
     if (!RelocSec) {
       reportWarning(createError(".stack_sizes (" + describe(*StackSizesELFSec) +
                                 ") does not have a corresponding "
                                 "relocation section"),
                     FileName);
       continue;
     }
template <class ELFT> void LLVMELFDumper<ELFT>::printCGProfile() {
   auto IsMatch = [](const Elf_Shdr &Sec) -> bool {
     return Sec.sh_type == ELF::SHT_LLVM_CALL_GRAPH_PROFILE;
   };
   this->getSectionAndRelocations(IsMatch, SecToRelocMap);
 
   for (const auto &CGMapEntry : SecToRelocMap) {
     const Elf_Shdr *CGSection = CGMapEntry.first;
     const Elf_Shdr *CGRelSection = CGMapEntry.second;
 
     Expected<ArrayRef<Elf_CGProfile>> CGProfileOrErr =
         this->Obj.template getSectionContentsAsArray<Elf_CGProfile>(*CGSection);
     if (!CGProfileOrErr) {
       this->reportUniqueWarning(
           "unable to load the SHT_LLVM_CALL_GRAPH_PROFILE section: " +
           toString(CGProfileOrErr.takeError()));
       return;
     }
 
     SmallVector<uint32_t, 128> SymbolIndices;
     bool UseReloc =
         getSymbolIndices<ELFT>(CGRelSection, this->Obj, this, SymbolIndices);
     if (UseReloc && SymbolIndices.size() != CGProfileOrErr->size() * 2) {