[yaml2obj] Move redundant statements into a separate static function

[jhenderson]

tools/yaml2obj/yaml2elf.cpp

229 ↗

(On Diff #176402)

"check" implies to me that it's just validating something. However, this function does more than that (it sets the Index variable too). I'd suggest renaming the function convertSectionIndex or similar.

230 ↗

(On Diff #176402)

SecField to me implies the name of a section field, e.g. "sh_link". I'd suggest renaming SecField to IndexSrc or similar, and Index to IndexDest. The two are closely related, so having similar names makes sense to me.

278 ↗

(On Diff #176402)

This isn't a section index lookup. It's a symbol lookup. Note that the message is different.

[jhenderson]

tools/yaml2obj/yaml2elf.cpp

278 ↗

(On Diff #176402)

By the way, were there no test failures because of this? If there were none, I'd suggest adding a test for this particular case, again in a separate change.

[Higuoxing]

tools/yaml2obj/yaml2elf.cpp

278 ↗

(On Diff #176402)

Sure, I'll add one.

[Higuoxing]

tools/yaml2obj/yaml2elf.cpp

278 ↗

(On Diff #176402)

I noticed this problem, because they are all of type NameToIdxMap.
I did not noticed the error hints are different, I'll fix them, thanks for your reviewing

[jhenderson]

tools/yaml2obj/yaml2elf.cpp

239 ↗

(On Diff #176583)

As this is only used the once, I don't think that it is useful to pull this case out into a separate function. I could be persuaded otherwise though.

[Higuoxing]

I just want to make the logic inside initSectionHeaders simple. I could wait until there are two places that needs simplify ...

Thank you @jhenderson, for your kind reviewing :)

[jhenderson]

LGTM.

llvm/trunk/tools/yaml2obj/yaml2elf.cpp

for (const auto &YamlPhdr : Doc.ProgramHeaders) {
     Phdr.p_type = YamlPhdr.Type;
     Phdr.p_flags = YamlPhdr.Flags;
     Phdr.p_vaddr = YamlPhdr.VAddr;
     Phdr.p_paddr = YamlPhdr.PAddr;
     PHeaders.push_back(Phdr);
   }
 }
 
+static bool convertSectionIndex(NameToIdxMap &SN2I, StringRef SecName,
+                                StringRef IndexSrc, unsigned &IndexDest) {
+  if (SN2I.lookup(IndexSrc, IndexDest) && !to_integer(IndexSrc, IndexDest)) {
+    WithColor::error() << "Unknown section referenced: '" << IndexSrc
+                       << "' at YAML section '" << SecName << "'.\n";
+    return false;
+  }
+  return true;
+}
+
 template <class ELFT>
 bool ELFState<ELFT>::initSectionHeaders(std::vector<Elf_Shdr> &SHeaders,
                                         ContiguousBlobAccumulator &CBA) {
   // Ensure SHN_UNDEF entry is present. An all-zero section header is a
   // valid SHN_UNDEF entry since SHT_NULL == 0.
   Elf_Shdr SHeader;
   zero(SHeader);
   SHeaders.push_back(SHeader);
 
   for (const auto &Sec : Doc.Sections) {
     zero(SHeader);
     SHeader.sh_name = DotShStrtab.getOffset(Sec->Name);
     SHeader.sh_type = Sec->Type;
     SHeader.sh_flags = Sec->Flags;
     SHeader.sh_addr = Sec->Address;
     SHeader.sh_addralign = Sec->AddressAlign;
 
     if (!Sec->Link.empty()) {
       unsigned Index;
-      if (SN2I.lookup(Sec->Link, Index) && !to_integer(Sec->Link, Index)) {
-        WithColor::error() << "Unknown section referenced: '" << Sec->Link
-                           << "' at YAML section '" << Sec->Name << "'.\n";
+      if (!convertSectionIndex(SN2I, Sec->Name, Sec->Link, Index))
         return false;
-      }
       SHeader.sh_link = Index;
     }
 
     if (auto S = dyn_cast<ELFYAML::RawContentSection>(Sec.get()))
       writeSectionContent(SHeader, *S, CBA);
     else if (auto S = dyn_cast<ELFYAML::RelocationSection>(Sec.get())) {
       if (S->Link.empty())
         // For relocation section set link to .symtab by default.
         SHeader.sh_link = getDotSymTabSecNo();
 
       unsigned Index;
-      if (SN2I.lookup(S->Info, Index) && !to_integer(S->Info, Index)) {
-        WithColor::error() << "Unknown section referenced: '" << S->Info
-                           << "' at YAML section '" << S->Name << "'.\n";
+      if (!convertSectionIndex(SN2I, S->Name, S->Info, Index))
         return false;
-      }
       SHeader.sh_info = Index;
-
       if (!writeSectionContent(SHeader, *S, CBA))
         return false;
     } else if (auto S = dyn_cast<ELFYAML::Group>(Sec.get())) {
       unsigned SymIdx;
       if (SymN2I.lookup(S->Info, SymIdx) && !to_integer(S->Info, SymIdx)) {
         WithColor::error() << "Unknown symbol referenced: '" << S->Info
                            << "' at YAML section '" << S->Name << "'.\n";
         return false;
bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
 
   auto &OS = CBA.getOSAndAlignedOffset(SHeader.sh_offset, SHeader.sh_addralign);
 
   for (auto member : Section.Members) {
     Elf_Word SIdx;
     unsigned int sectionIndex = 0;
     if (member.sectionNameOrType == "GRP_COMDAT")
       sectionIndex = llvm::ELF::GRP_COMDAT;
-    else if (SN2I.lookup(member.sectionNameOrType, sectionIndex) &&
-             !to_integer(member.sectionNameOrType, sectionIndex)) {
-      WithColor::error() << "Unknown section referenced: '"
-                         << member.sectionNameOrType << "' at YAML section' "
-                         << Section.Name << "\n";
+    else if (!convertSectionIndex(SN2I, Section.Name, member.sectionNameOrType,
+                                  sectionIndex))
       return false;
-    }
     SIdx = sectionIndex;
     OS.write((const char *)&SIdx, sizeof(SIdx));
   }
   return true;
 }
 
 template <class ELFT>
 bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,