[yaml2obj] - Make interface of `NameToIdxMap` class be human friendly and fix users.

[grimar]

Summary

This patch inverses the values returned by addName and
lookup methods of the class mentioned so that they
now return true on success and false on failure.
Also, it does minor code cleanup.

[jhenderson]

I'm happy with this in principle, but I do wonder how you made sure you caught all instances!

tools/yaml2obj/yaml2elf.cpp

78 ↗

(On Diff #196854)

As you're tidying this comment up, please change "asserts" to "Asserts".

400 ↗

(On Diff #196854)

What happens if you specify a section name that doesn't exist? Does this result in an assertion? If so, we should produce an actual error instead in this case.

Similar point may apply below.

[grimar]

In D61190#1482558, @jhenderson wrote:

I do wonder how you made sure you caught all instances!

I am usually using renaming magic for such things. I.e. I renamed addName to addName1, lookup to lookup1 and so on,
then tried to compile and fixed all the places. Then renamed back.

tools/yaml2obj/yaml2elf.cpp

400 ↗

(On Diff #196854)

Oh, I did not notice we do not fail if list a section that do not exist. I think we should check it earlier than here then.
What about landing the following refactoring then? D61322

[grimar]

• Rebased.

tools/yaml2obj/yaml2elf.cpp

78 ↗

(On Diff #196854)

Done. And for Returns above too.

[jhenderson]

LGTM.

tools/yaml2obj/yaml2elf.cpp

78 ↗

(On Diff #196854)

Not sure if those are necessary or not due to doxygen!

llvm/trunk/tools/yaml2obj/yaml2elf.cpp

raw_ostream &getOSAndAlignedOffset(Integer &Offset, unsigned Align) {
     Offset = padToAlignment(Align);
     return OS;
   }
   void writeBlobToStream(raw_ostream &Out) { Out << OS.str(); }
 };
 } // end anonymous namespace
 
 // Used to keep track of section and symbol names, so that in the YAML file
-// sections and symbols can be referenced by name instead of by index.
-namespace {
-class NameToIdxMap {
-  StringMap<int> Map;
-public:
-  /// \returns true if name is already present in the map.
-  bool addName(StringRef Name, unsigned i) {
-    return !Map.insert(std::make_pair(Name, (int)i)).second;
-  }
-  /// \returns true if name is not present in the map
-  bool lookup(StringRef Name, unsigned &Idx) const {
-    StringMap<int>::const_iterator I = Map.find(Name);
-    if (I == Map.end())
-      return true;
-    Idx = I->getValue();
-    return false;
-  }
-  /// asserts if name is not present in the map
-  unsigned get(StringRef Name) const {
-    unsigned Idx = 0;
-    auto missing = lookup(Name, Idx);
-    (void)missing;
-    assert(!missing && "Expected section not found in index");
-    return Idx;
-  }
-  unsigned size() const { return Map.size(); }
-};
+// sections and symbols can be referenced by name instead of by index.
+namespace {
+class NameToIdxMap {
+  StringMap<unsigned> Map;
+
+public:
+  /// \Returns false if name is already present in the map.
+  bool addName(StringRef Name, unsigned Ndx) {
+    return Map.insert({Name, Ndx}).second;
+  }
+  /// \Returns false if name is not present in the map.
+  bool lookup(StringRef Name, unsigned &Idx) const {
+    auto I = Map.find(Name);
+    if (I == Map.end())
+      return false;
+    Idx = I->getValue();
+    return true;
+  }
+  /// Asserts if name is not present in the map.
+  unsigned get(StringRef Name) const {
+    unsigned Idx;
+    if (lookup(Name, Idx))
+      return Idx;
+    assert(false && "Expected section not found in index");
+    return 0;
+  }
+  unsigned size() const { return Map.size(); }
+};
 } // end anonymous namespace
 
 template <class T>
 static size_t arrayDataSize(ArrayRef<T> A) {
   return A.size() * sizeof(T);
 }
 
 template <class T>
for (const auto &YamlPhdr : Doc.ProgramHeaders) {
     Elf_Phdr Phdr;
     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;
+
+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

 
 template <class ELFT>
 void ELFState<ELFT>::setProgramHeaderLayout(std::vector<Elf_Phdr> &PHeaders,
                                             std::vector<Elf_Shdr> &SHeaders) {
   uint32_t PhdrIdx = 0;
   for (auto &YamlPhdr : Doc.ProgramHeaders) {
     Elf_Phdr &PHeader = PHeaders[PhdrIdx++];
 
-    std::vector<Elf_Shdr *> Sections;
-    for (const ELFYAML::SectionName &SecName : YamlPhdr.Sections) {
-      unsigned Index;
-      if (SN2I.lookup(SecName.Section, Index)) {
-        WithColor::error() << "Unknown section referenced: '" << SecName.Section
-                           << "' by program header.\n";
-        exit(1);
+    std::vector<Elf_Shdr *> Sections;
+    for (const ELFYAML::SectionName &SecName : YamlPhdr.Sections) {
+      unsigned Index;
+      if (!SN2I.lookup(SecName.Section, Index)) {
+        WithColor::error() << "Unknown section referenced: '" << SecName.Section
+                           << "' by program header.\n";
+        exit(1);
       }
       Sections.push_back(&SHeaders[Index]);
     }
 
     if (YamlPhdr.Offset) {
       PHeader.p_offset = *YamlPhdr.Offset;
     } else {
       if (YamlPhdr.Sections.size())

 void ELFState<ELFT>::addSymbols(ArrayRef<ELFYAML::Symbol> Symbols,
                                 std::vector<Elf_Sym> &Syms,
                                 const StringTableBuilder &Strtab) {
   for (const auto &Sym : Symbols) {
     Elf_Sym Symbol;
     zero(Symbol);
     if (!Sym.Name.empty())
       Symbol.st_name = Strtab.getOffset(Sym.Name);
-    Symbol.setBindingAndType(Sym.Binding, Sym.Type);
-    if (!Sym.Section.empty()) {
-      unsigned Index;
-      if (SN2I.lookup(Sym.Section, Index)) {
-        WithColor::error() << "Unknown section referenced: '" << Sym.Section
-                           << "' by YAML symbol " << Sym.Name << ".\n";
-        exit(1);
+    Symbol.setBindingAndType(Sym.Binding, Sym.Type);
+    if (!Sym.Section.empty()) {
+      unsigned Index;
+      if (!SN2I.lookup(Sym.Section, Index)) {
+        WithColor::error() << "Unknown section referenced: '" << Sym.Section
+                           << "' by YAML symbol " << Sym.Name << ".\n";
+        exit(1);
       }
       Symbol.st_shndx = Index;
     } else if (Sym.Index) {
       Symbol.st_shndx = *Sym.Index;
     }
     // else Symbol.st_shndex == SHN_UNDEF (== 0), since it was zero'd earlier.
     Symbol.st_value = Sym.Value;
     Symbol.st_other = Sym.Other;
ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
   if (!Section.RelocatableSec.empty() &&
       !convertSectionIndex(SN2I, Section.Name, Section.RelocatableSec, Index))
     return false;
   SHeader.sh_info = Index;
 
   auto &OS = CBA.getOSAndAlignedOffset(SHeader.sh_offset, SHeader.sh_addralign);
 
   for (const auto &Rel : Section.Relocations) {
-    unsigned SymIdx = 0;
-    // If a relocation references a symbol, try to look one up in the symbol
-    // table. If it is not there, treat the value as a symbol index.
-    if (Rel.Symbol && SymN2I.lookup(*Rel.Symbol, SymIdx) &&
-        !to_integer(*Rel.Symbol, SymIdx)) {
-      WithColor::error() << "Unknown symbol referenced: '" << *Rel.Symbol
-                         << "' at YAML section '" << Section.Name << "'.\n";
+    unsigned SymIdx = 0;
+    // If a relocation references a symbol, try to look one up in the symbol
+    // table. If it is not there, treat the value as a symbol index.
+    if (Rel.Symbol && !SymN2I.lookup(*Rel.Symbol, SymIdx) &&
+        !to_integer(*Rel.Symbol, SymIdx)) {
+      WithColor::error() << "Unknown symbol referenced: '" << *Rel.Symbol
+                         << "' at YAML section '" << Section.Name << "'.\n";
       return false;
     }
 
     if (IsRela) {
       Elf_Rela REntry;
       zero(REntry);
       REntry.r_offset = Rel.Offset;
       REntry.r_addend = Rel.Addend;

 template <class ELFT>
 bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
                                          const ELFYAML::Group &Section,
                                          ContiguousBlobAccumulator &CBA) {
   assert(Section.Type == llvm::ELF::SHT_GROUP &&
          "Section type is not SHT_GROUP");
 
   SHeader.sh_entsize = 4;
-  SHeader.sh_size = SHeader.sh_entsize * Section.Members.size();
-
-  unsigned SymIdx;
-  if (SymN2I.lookup(Section.Signature, SymIdx) &&
-      !to_integer(Section.Signature, SymIdx)) {
-    WithColor::error() << "Unknown symbol referenced: '" << Section.Signature
-                       << "' at YAML section '" << Section.Name << "'.\n";
+  SHeader.sh_size = SHeader.sh_entsize * Section.Members.size();
+
+  unsigned SymIdx;
+  if (!SymN2I.lookup(Section.Signature, SymIdx) &&
+      !to_integer(Section.Signature, SymIdx)) {
+    WithColor::error() << "Unknown symbol referenced: '" << Section.Signature
+                       << "' at YAML section '" << Section.Name << "'.\n";
     return false;
   }
   SHeader.sh_info = SymIdx;
 
   raw_ostream &OS =
       CBA.getOSAndAlignedOffset(SHeader.sh_offset, SHeader.sh_addralign);
 
   for (const ELFYAML::SectionOrType &Member : Section.Members) {
bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
   if (Section.Content)
     Section.Content->writeAsBinary(OS);
 
   return true;
 }
 
 template <class ELFT> bool ELFState<ELFT>::buildSectionIndex() {
   for (unsigned i = 0, e = Doc.Sections.size(); i != e; ++i) {
-    StringRef Name = Doc.Sections[i]->Name;
-    DotShStrtab.add(Name);
-    // "+ 1" to take into account the SHT_NULL entry.
-    if (SN2I.addName(Name, i + 1)) {
-      WithColor::error() << "Repeated section name: '" << Name
-                         << "' at YAML section number " << i << ".\n";
-      return false;
+    StringRef Name = Doc.Sections[i]->Name;
+    DotShStrtab.add(Name);
+    // "+ 1" to take into account the SHT_NULL entry.
+    if (!SN2I.addName(Name, i + 1)) {
+      WithColor::error() << "Repeated section name: '" << Name
+                         << "' at YAML section number " << i << ".\n";
+      return false;
     }
   }
 
   auto SecNo = 1 + Doc.Sections.size();
   // Add special sections after input sections, if necessary.
   for (StringRef Name : implicitSectionNames())
-    if (!SN2I.addName(Name, SecNo)) {
+    if (SN2I.addName(Name, SecNo)) {
       // Account for this section, since it wasn't in the Doc
       ++SecNo;
       DotShStrtab.add(Name);
     }
 
   DotShStrtab.finalize();
   return true;
 }
 
 template <class ELFT>
 bool ELFState<ELFT>::buildSymbolIndex(ArrayRef<ELFYAML::Symbol> Symbols) {
   bool GlobalSymbolSeen = false;
   std::size_t I = 0;
   for (const auto &Sym : Symbols) {
     ++I;
 
     StringRef Name = Sym.Name;
     if (Sym.Binding.value == ELF::STB_LOCAL && GlobalSymbolSeen) {
       WithColor::error() << "Local symbol '" + Name +
                                 "' after global in Symbols list.\n";
       return false;
     }
-    if (Sym.Binding.value != ELF::STB_LOCAL)
-      GlobalSymbolSeen = true;
-
-    if (!Name.empty() && SymN2I.addName(Name, I)) {
-      WithColor::error() << "Repeated symbol name: '" << Name << "'.\n";
-      return false;
-    }
+    if (Sym.Binding.value != ELF::STB_LOCAL)
+      GlobalSymbolSeen = true;
+
+    if (!Name.empty() && !SymN2I.addName(Name, I)) {
+      WithColor::error() << "Repeated symbol name: '" << Name << "'.\n";
+      return false;
+    }
   }
   return true;
 }
 
 template <class ELFT> void ELFState<ELFT>::finalizeStrings() {
   // Add the regular symbol names to .strtab section.
   for (const ELFYAML::Symbol &Sym : Doc.Symbols)
     DotStrtab.add(Sym.Name);