[obj2yaml] - Split sections dumping to a new ELFDumper<ELFT>::dumpSections() method.

[grimar]

Summary

ELFDumper<ELFT>::dump() is too large and deserves splitting.

It is used by https://reviews.llvm.org/D75342

[grimar]

(This helps to simplify another patch I am working on).

[MaskRay]

This change is fine, though I want to see materialized advantage before accepting it.. (On its own, it increases complexity.)

[grimar]

In D75172#1894188, @MaskRay wrote:

On its own, it increases complexity.

By splitting 234 lines method into two I guess .. :)

[grimar]

• Rebase.

[jhenderson]

Personally, I prefer lots of smaller functions, even if they are just chaining into one another, so LGTM (with nit). Might want to let @MaskRay have more time to respond though.

llvm/tools/obj2yaml/elf2yaml.cpp

232

I feel like this is probably a violation of our policy on auto as it's not clear from the line of code what type ChunksOrErr is.

[MaskRay]

llvm/tools/obj2yaml/elf2yaml.cpp

232

Expand to Expected<std::vector<std::unique_ptr<ELFYAML::Chunk>>>

It should fit on one line.

llvm/tools/obj2yaml/elf2yaml.cpp

class ELFDumper {
 
   const object::ELFFile<ELFT> &Obj;
   ArrayRef<Elf_Word> ShndxTable;
 
   Error dumpSymbols(const Elf_Shdr *Symtab,
                     std::vector<ELFYAML::Symbol> &Symbols);
   Error dumpSymbol(const Elf_Sym *Sym, const Elf_Shdr *SymTab,
                    StringRef StrTable, ELFYAML::Symbol &S);
+  Expected<std::vector<std::unique_ptr<ELFYAML::Chunk>>> dumpSections();
   Error dumpCommonSection(const Elf_Shdr *Shdr, ELFYAML::Section &S);
   Error dumpCommonRelocationSection(const Elf_Shdr *Shdr,
                                     ELFYAML::RelocationSection &S);
   template <class RelT>
   Error dumpRelocation(const RelT *Rel, const Elf_Shdr *SymTab,
                        ELFYAML::Relocation &R);
 
   Expected<ELFYAML::AddrsigSection *> dumpAddrsigSection(const Elf_Shdr *Shdr);
template <class ELFT> Expected<ELFYAML::Object *> ELFDumper<ELFT>::dump() {
   }
 
   if (DynSymTab) {
     Y->DynamicSymbols.emplace();
     if (Error E = dumpSymbols(DynSymTab, *Y->DynamicSymbols))
       return std::move(E);
   }
 
+  if (Expected<std::vector<std::unique_ptr<ELFYAML::Chunk>>> ChunksOrErr =
+          dumpSections())
+    Y->Chunks = std::move(*ChunksOrErr);
+  else
+    return ChunksOrErr.takeError();
+
+  return Y.release();
+}
+
+template <class ELFT>
+Expected<std::vector<std::unique_ptr<ELFYAML::Chunk>>>
+ELFDumper<ELFT>::dumpSections() {
+  std::vector<std::unique_ptr<ELFYAML::Chunk>> Ret;
+
   for (const Elf_Shdr &Sec : Sections) {
     switch (Sec.sh_type) {
     case ELF::SHT_DYNAMIC: {
       Expected<ELFYAML::DynamicSection *> SecOrErr = dumpDynamicSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_STRTAB:
     case ELF::SHT_SYMTAB:
     case ELF::SHT_DYNSYM:
       // Do not dump these sections.
       break;
     case ELF::SHT_SYMTAB_SHNDX: {
       Expected<ELFYAML::SymtabShndxSection *> SecOrErr =
           dumpSymtabShndxSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_REL:
     case ELF::SHT_RELA: {
       Expected<ELFYAML::RelocationSection *> SecOrErr = dumpRelocSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_RELR: {
       Expected<ELFYAML::RelrSection *> SecOrErr = dumpRelrSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_GROUP: {
       Expected<ELFYAML::Group *> GroupOrErr = dumpGroup(&Sec);
       if (!GroupOrErr)
         return GroupOrErr.takeError();
-      Y->Chunks.emplace_back(*GroupOrErr);
+      Ret.emplace_back(*GroupOrErr);
       break;
     }
     case ELF::SHT_MIPS_ABIFLAGS: {
       Expected<ELFYAML::MipsABIFlags *> SecOrErr = dumpMipsABIFlags(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_NOBITS: {
       Expected<ELFYAML::NoBitsSection *> SecOrErr = dumpNoBitsSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_NOTE: {
       Expected<ELFYAML::NoteSection *> SecOrErr = dumpNoteSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_HASH: {
       Expected<ELFYAML::HashSection *> SecOrErr = dumpHashSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_GNU_HASH: {
       Expected<ELFYAML::GnuHashSection *> SecOrErr = dumpGnuHashSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_GNU_verdef: {
       Expected<ELFYAML::VerdefSection *> SecOrErr = dumpVerdefSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_GNU_versym: {
       Expected<ELFYAML::SymverSection *> SecOrErr = dumpSymverSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_GNU_verneed: {
       Expected<ELFYAML::VerneedSection *> SecOrErr = dumpVerneedSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_LLVM_ADDRSIG: {
       Expected<ELFYAML::AddrsigSection *> SecOrErr = dumpAddrsigSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_LLVM_LINKER_OPTIONS: {
       Expected<ELFYAML::LinkerOptionsSection *> SecOrErr =
           dumpLinkerOptionsSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_LLVM_DEPENDENT_LIBRARIES: {
       Expected<ELFYAML::DependentLibrariesSection *> SecOrErr =
           dumpDependentLibrariesSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_LLVM_CALL_GRAPH_PROFILE: {
       Expected<ELFYAML::CallGraphProfileSection *> SecOrErr =
           dumpCallGraphProfileSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
       break;
     }
     case ELF::SHT_NULL: {
       // We only dump the SHT_NULL section at index 0 when it
       // has at least one non-null field, because yaml2obj
       // normally creates the zero section at index 0 implicitly.
       if (&Sec == &Sections[0]) {
         const uint8_t *Begin = reinterpret_cast<const uint8_t *>(&Sec);
         const uint8_t *End = Begin + sizeof(Elf_Shdr);
         if (std::find_if(Begin, End, [](uint8_t V) { return V != 0; }) == End)
           break;
       }
       LLVM_FALLTHROUGH;
     }
     default: {
       // Recognize some special SHT_PROGBITS sections by name.
       if (Sec.sh_type == ELF::SHT_PROGBITS) {
         Expected<ELFYAML::Chunk *> SpecialSecOrErr = dumpSpecialSection(&Sec);
         if (!SpecialSecOrErr)
           return SpecialSecOrErr.takeError();
         if (*SpecialSecOrErr) {
-          Y->Chunks.emplace_back(*SpecialSecOrErr);
+          Ret.emplace_back(*SpecialSecOrErr);
           break;
         }
       }
 
       Expected<ELFYAML::RawContentSection *> SecOrErr =
           dumpContentSection(&Sec);
       if (!SecOrErr)
         return SecOrErr.takeError();
-      Y->Chunks.emplace_back(*SecOrErr);
+      Ret.emplace_back(*SecOrErr);
     }
     }
   }
-
-  return Y.release();
+  return std::move(Ret);
 }
 
 template <class ELFT>
 Error ELFDumper<ELFT>::dumpSymbols(const Elf_Shdr *Symtab,
                              std::vector<ELFYAML::Symbol> &Symbols) {
   if (!Symtab)
     return Error::success();