[obj2yaml] - Refactor how we dump sections. NFCI.

[grimar]

Summary

This is a NFC splitted from D75342.

Previously obj2yaml never dumped a normal SHT_NULL section (i.e. when it is just zeroed)
or non-allocatable SHT_STRTAB/SHT_SYMTAB/SHT_DYNSYM sections.

This patch does not change the output, but it changes the logic so that we now dump these
sections, and them remove them later. It allows us to create and work with our internal representation
of sections, i.e. to work with the vector of Chunks, what looks cleaner.

It is used by D75342 and also should help us to support dumping a content that does not
belong to a section (i.e. to dump some data as Fill chunks).

[jhenderson]

llvm/tools/obj2yaml/elf2yaml.cpp

119–120

Could you explain this case, please, perhaps with a comment in the code?

287

dumpPlaceholderSection maybe? Slightly shorter name.

351

Looks like you have a copy-paste/formatting error around shouldPrintSection().

[grimar]

• Addressed review comments.

llvm/tools/obj2yaml/elf2yaml.cpp

119–120

It is for obj2yaml\elf-null-section.yaml. We have the following YAML description there:

--- !ELF
FileHeader:
  Class:   ELFCLASS64
  Data:    ELFDATA2LSB
  Type:    ET_REL
  Machine: EM_X86_64
Sections:
  - Type: SHT_PROGBITS
    Name: .foo
  - Type:         SHT_NULL
    Name:         ''
    Flags:        [ SHF_ALLOC ]
    Link:         1
    Info:         2
    AddressAlign: 0x3
    Size:         0x4
    EntSize:      0x5
    Address:      0x6

Note: SHT_NULL has empty name and it is the second SHT_NULL section in the object.
(first one is one at index 0, created implicitly).

We expect that obj2yaml produce the following;

# SECOND-SEC-NEXT:   - Type:         SHT_NULL
# SECOND-SEC-NEXT:     Flags:        [ SHF_ALLOC ]
# SECOND-SEC-NEXT:     Address:      0x0000000000000006
# SECOND-SEC-NEXT:     Link:         .foo
# SECOND-SEC-NEXT:     AddressAlign: 0x0000000000000003
# SECOND-SEC-NEXT:     EntSize:      0x0000000000000005
# SECOND-SEC-NEXT:     Content:      '00000000'
# SECOND-SEC-NEXT:     Info:         0x0000000000000002

But now since we started to dump SHT_NULL sections (we remove it later), obj2yaml produces:

- Name: ' [1]'

But it doesn't make sense I think. We should not uniquify empty names. I see no reasons for that.

[jhenderson]

LGTM, with nit.

llvm/tools/obj2yaml/elf2yaml.cpp

119–120

Seems reasonable for now. I doubt we'll have many (any?) cases where we want to reference multiple different sections with empty names.

121

an output -> the output

[MaskRay]

llvm/tools/obj2yaml/elf2yaml.cpp

119–120

After checking STT_SECTION, I think st_name=0 cases should be very rare.

llvm/include/llvm/ObjectYAML/ELFYAML.h

struct Section : public Chunk {
   StringRef Link;
   llvm::yaml::Hex64 AddressAlign;
   Optional<llvm::yaml::Hex64> EntSize;
 
   // Usually sections are not created implicitly, but loaded from YAML.
   // When they are, this flag is used to signal about that.
   bool IsImplicit;
 
+  // Holds the original section index.
+  unsigned OriginalSecNdx;
+
   Section(ChunkKind Kind, bool IsImplicit = false)
       : Chunk(Kind), IsImplicit(IsImplicit) {}
 
   static bool classof(const Chunk *S) { return S->Kind != ChunkKind::Fill; }
 
   // The following members are used to override section fields which is
   // useful for creating invalid objects.
 

llvm/tools/obj2yaml/elf2yaml.cpp

class ELFDumper {
   Expected<ELFYAML::GnuHashSection *> dumpGnuHashSection(const Elf_Shdr *Shdr);
   Expected<ELFYAML::VerdefSection *> dumpVerdefSection(const Elf_Shdr *Shdr);
   Expected<ELFYAML::SymverSection *> dumpSymverSection(const Elf_Shdr *Shdr);
   Expected<ELFYAML::VerneedSection *> dumpVerneedSection(const Elf_Shdr *Shdr);
   Expected<ELFYAML::Group *> dumpGroup(const Elf_Shdr *Shdr);
   Expected<ELFYAML::MipsABIFlags *> dumpMipsABIFlags(const Elf_Shdr *Shdr);
   Expected<ELFYAML::StackSizesSection *>
   dumpStackSizesSection(const Elf_Shdr *Shdr);
+  Expected<ELFYAML::RawContentSection *>
+  dumpPlaceholderSection(const Elf_Shdr *Shdr);
+
+  bool shouldPrintSection(const ELFYAML::Section &S, const Elf_Shdr &SHdr);
 
 public:
   ELFDumper(const object::ELFFile<ELFT> &O);
   Expected<ELFYAML::Object *> dump();
 };
 
 }
 
 template <class ELFT>
 ELFDumper<ELFT>::ELFDumper(const object::ELFFile<ELFT> &O)
     : Obj(O) {}
 
 template <class ELFT>
 Expected<StringRef>
 ELFDumper<ELFT>::getUniquedSectionName(const Elf_Shdr *Sec) {
   unsigned SecIndex = Sec - &Sections[0];
   assert(&Sections[SecIndex] == Sec);
   if (!SectionNames[SecIndex].empty())
     return SectionNames[SecIndex];
 
   auto NameOrErr = Obj.getSectionName(Sec);
   if (!NameOrErr)
     return NameOrErr;
   StringRef Name = *NameOrErr;
+  // In some specific cases we might have more than one section without a
+  // name (sh_name == 0). It normally doesn't happen, but when we have this case
+  // it doesn't make sense to uniquify their names and add noise to an output.
+  if (Name.empty())
+    return "";
+
   std::string &Ret = SectionNames[SecIndex];
 
   auto It = UsedSectionNames.insert({Name, 0});
   if (!It.second)
     Ret = (Name + " [" + Twine(++It.first->second) + "]").str();
   else
     Ret = std::string(Name);
   return Ret;
if (SymTab->sh_type == ELF::SHT_SYMTAB) {
     else
       SymbolNames[Index] = std::string(Name);
     return SymbolNames[Index];
   }
 
   return Name;
 }
 
+template <class ELFT>
+bool ELFDumper<ELFT>::shouldPrintSection(const ELFYAML::Section &S,
+                                         const Elf_Shdr &SHdr) {
+  // We only print 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 (S.Type == ELF::SHT_NULL && (&SHdr == &Sections[0])) {
+    const uint8_t *Begin = reinterpret_cast<const uint8_t *>(&SHdr);
+    const uint8_t *End = Begin + sizeof(Elf_Shdr);
+    return std::find_if(Begin, End, [](uint8_t V) { return V != 0; }) != End;
+  }
+
+  // Normally we use "Symbols:" and "DynamicSymbols:" to describe contents of
+  // symbol tables. We also build and emit corresponding string tables
+  // implicitly. But sometimes it is important to preserve positions and virtual
+  // addresses of allocatable sections, e.g. for creating program headers.
+  // Generally we are trying to reduce noise in the YAML output. Because
+  // of that we do not print non-allocatable versions of such sections and
+  // assume they are placed at the end.
+  if (S.Type == ELF::SHT_STRTAB || S.Type == ELF::SHT_SYMTAB ||
+      S.Type == ELF::SHT_DYNSYM)
+    return S.Flags.getValueOr(ELFYAML::ELF_SHF(0)) & ELF::SHF_ALLOC;
+
+  return true;
+}
+
 template <class ELFT> Expected<ELFYAML::Object *> ELFDumper<ELFT>::dump() {
   auto Y = std::make_unique<ELFYAML::Object>();
 
   // Dump header. We do not dump SHEntSize, SHOff, SHNum and SHStrNdx fields.
   // When not explicitly set, the values are set by yaml2obj automatically
   // and there is no need to dump them here.
   Y->Header.Class = ELFYAML::ELF_ELFCLASS(Obj.getHeader()->getFileClass());
   Y->Header.Data = ELFYAML::ELF_ELFDATA(Obj.getHeader()->getDataEncoding());
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
+  // We dump all sections first. It is simple and allows us to verify that all
+  // sections are valid and also to generalize the code. But we are not going to
+  // keep all of them in the final output (see comments for
+  // 'shouldPrintSection()'). Undesired chunks will be removed later.
+  Expected<std::vector<std::unique_ptr<ELFYAML::Chunk>>> ChunksOrErr =
+      dumpSections();
+  if (!ChunksOrErr)
     return ChunksOrErr.takeError();
 
+  std::vector<std::unique_ptr<ELFYAML::Chunk>> Chunks = std::move(*ChunksOrErr);
+  llvm::erase_if(Chunks, [this](const std::unique_ptr<ELFYAML::Chunk> &C) {
+    const ELFYAML::Section &S = cast<ELFYAML::Section>(*C.get());
+    return !shouldPrintSection(S, Sections[S.OriginalSecNdx]);
+  });
+
+  Y->Chunks = std::move(Chunks);
   return Y.release();
 }
 
 template <class ELFT>
+Expected<ELFYAML::RawContentSection *>
+ELFDumper<ELFT>::dumpPlaceholderSection(const Elf_Shdr *Shdr) {
+  auto S = std::make_unique<ELFYAML::RawContentSection>();
+  if (Error E = dumpCommonSection(Shdr, *S.get()))
+    return std::move(E);
+  return S.release();
+}
+
+template <class ELFT>
 Expected<std::vector<std::unique_ptr<ELFYAML::Chunk>>>
 ELFDumper<ELFT>::dumpSections() {
   std::vector<std::unique_ptr<ELFYAML::Chunk>> Ret;
   auto Add = [&](Expected<ELFYAML::Chunk *> SecOrErr) -> Error {
     if (!SecOrErr)
       return SecOrErr.takeError();
     Ret.emplace_back(*SecOrErr);
     return Error::success();
case ELF::SHT_LLVM_LINKER_OPTIONS:
       return [this](const Elf_Shdr *S) { return dumpLinkerOptionsSection(S); };
     case ELF::SHT_LLVM_DEPENDENT_LIBRARIES:
       return [this](const Elf_Shdr *S) {
         return dumpDependentLibrariesSection(S);
       };
     case ELF::SHT_LLVM_CALL_GRAPH_PROFILE:
       return
           [this](const Elf_Shdr *S) { return dumpCallGraphProfileSection(S); };
+    case ELF::SHT_STRTAB:
+    case ELF::SHT_SYMTAB:
+    case ELF::SHT_DYNSYM:
+      // The contents of these sections are described by other parts of the YAML
+      // file. But we still want to dump them, because their properties can be
+      // important. See comments for 'shouldPrintSection()' for more details.
+      return [this](const Elf_Shdr *S) { return dumpPlaceholderSection(S); };
     default:
       return nullptr;
     }
   };
 
   for (const Elf_Shdr &Sec : Sections) {
     // We have dedicated dumping functions for most of the section types.
     // Try to use one of them first.
     if (std::function<Expected<ELFYAML::Chunk *>(const Elf_Shdr *)> DumpFn =
             GetDumper(Sec.sh_type)) {
       if (Error E = Add(DumpFn(&Sec)))
         return std::move(E);
       continue;
     }
 
-    if (Sec.sh_type == ELF::SHT_STRTAB || Sec.sh_type == ELF::SHT_SYMTAB ||
-        Sec.sh_type == ELF::SHT_DYNSYM) {
-      // The contents of these sections are described by other parts of the YAML
-      // file. We still dump them so that their positions in the section header
-      // table are correctly recorded. We only dump allocatable section because
-      // their positions and addresses are important, e.g. for creating program
-      // headers. Some sections, like .symtab or .strtab normally are not
-      // allocatable and do not have virtual addresses. We want to avoid noise
-      // in the YAML output and assume that they are placed at the end.
-      if (Sec.sh_flags & ELF::SHF_ALLOC) {
-        auto S = std::make_unique<ELFYAML::RawContentSection>();
-        if (Error E = dumpCommonSection(&Sec, *S.get()))
-          return std::move(E);
-        if (Error E = Add(S.release()))
-          return std::move(E);
-      }
-      continue;
-    }
-
-    if (Sec.sh_type == 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)
-          continue;
-      }
-    }
-
     // Recognize some special SHT_PROGBITS sections by name.
     if (Sec.sh_type == ELF::SHT_PROGBITS) {
       auto NameOrErr = getUniquedSectionName(&Sec);
       if (!NameOrErr)
         return NameOrErr.takeError();
 
       if (ELFYAML::StackSizesSection::nameMatches(*NameOrErr)) {
         if (Error E = Add(dumpStackSizesSection(&Sec)))
if (Shdr->sh_flags)
     S.Flags = static_cast<ELFYAML::ELF_SHF>(Shdr->sh_flags);
   if (Shdr->sh_addr)
     S.Address = static_cast<uint64_t>(Shdr->sh_addr);
   S.AddressAlign = Shdr->sh_addralign;
 
   if (Shdr->sh_entsize != getDefaultShEntSize<ELFT>(S.Type))
     S.EntSize = static_cast<llvm::yaml::Hex64>(Shdr->sh_entsize);
 
+  S.OriginalSecNdx = Shdr - &Sections[0];
+
   auto NameOrErr = getUniquedSectionName(Shdr);
   if (!NameOrErr)
     return NameOrErr.takeError();
   S.Name = NameOrErr.get();
 
   if (Shdr->sh_link != ELF::SHN_UNDEF) {
     auto LinkSection = Obj.getSection(Shdr->sh_link);
     if (!LinkSection)