[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

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

400

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

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

Done. And for Returns above too.

[jhenderson]

LGTM.

tools/yaml2obj/yaml2elf.cpp

78

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

tools/yaml2obj/yaml2elf.cpp

public:
   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;
+  StringMap<unsigned> 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 false if name is already present in the map.
+  bool addName(StringRef Name, unsigned Ndx) {
+    return Map.insert({Name, Ndx}).second;
   }
-  /// \returns true if name is not present in the map
+  /// \returns false if name is not present in the map.
   bool lookup(StringRef Name, unsigned &Idx) const {
-    StringMap<int>::const_iterator I = Map.find(Name);
+    auto I = Map.find(Name);
     if (I == Map.end())
-      return true;
-    Idx = I->getValue();
       return false;
+    Idx = I->getValue();
+    return true;
   }
-  /// asserts if name is not present in the map
+  /// 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");
+    if (!lookup(Name, Idx))
+      assert(false && "Expected section not found in index");
     return Idx;
   }
   unsigned size() const { return Map.size(); }
 };
 } // end anonymous namespace
 
 template <class T>
 static size_t arrayDataSize(ArrayRef<T> A) {
for (const auto &YamlPhdr : Doc.ProgramHeaders) {
     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)) {
+  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>
for (auto &YamlPhdr : Doc.ProgramHeaders) {
     } else {
       if (YamlPhdr.Sections.size())
         PHeader.p_offset = UINT32_MAX;
       else
         PHeader.p_offset = 0;
 
       // Find the minimum offset for the program header.
       for (auto SecName : YamlPhdr.Sections) {
-        uint32_t Index = 0;
-        SN2I.lookup(SecName.Section, Index);
-        const auto &SHeader = SHeaders[Index];
+        const auto &SHeader = SHeaders[SN2I.get(SecName.Section)];
         PHeader.p_offset = std::min(PHeader.p_offset, SHeader.sh_offset);
       }
     }
 
     // Find the maximum offset of the end of a section in order to set p_filesz,
     // if not set explicitly.
     if (YamlPhdr.FileSize) {
       PHeader.p_filesz = *YamlPhdr.FileSize;
     } else {
       PHeader.p_filesz = 0;
       for (auto SecName : YamlPhdr.Sections) {
-        uint32_t Index = 0;
-        SN2I.lookup(SecName.Section, Index);
-        const auto &SHeader = SHeaders[Index];
+        const auto &SHeader = SHeaders[SN2I.get(SecName.Section)];
         uint64_t EndOfSection;
         if (SHeader.sh_type == llvm::ELF::SHT_NOBITS)
           EndOfSection = SHeader.sh_offset;
         else
           EndOfSection = SHeader.sh_offset + SHeader.sh_size;
         uint64_t EndOfSegment = PHeader.p_offset + PHeader.p_filesz;
         EndOfSegment = std::max(EndOfSegment, EndOfSection);
         PHeader.p_filesz = EndOfSegment - PHeader.p_offset;
       }
     }
 
     // If not set explicitly, find the memory size by adding the size of
     // sections at the end of the segment. These should be empty (size of zero)
     // and NOBITS sections.
     if (YamlPhdr.MemSize) {
       PHeader.p_memsz = *YamlPhdr.MemSize;
     } else {
       PHeader.p_memsz = PHeader.p_filesz;
       for (auto SecName : YamlPhdr.Sections) {
-        uint32_t Index = 0;
-        SN2I.lookup(SecName.Section, Index);
-        const auto &SHeader = SHeaders[Index];
+        const auto &SHeader = SHeaders[SN2I.get(SecName.Section)];
         if (SHeader.sh_offset == PHeader.p_offset + PHeader.p_filesz)
           PHeader.p_memsz += SHeader.sh_size;
       }
     }
 
     // Set the alignment of the segment to be the same as the maximum alignment
     // of the sections with the same offset so that by default the segment
     // has a valid and sensible alignment.
     if (YamlPhdr.Align) {
       PHeader.p_align = *YamlPhdr.Align;
     } else {
       PHeader.p_align = 1;
       for (auto SecName : YamlPhdr.Sections) {
-        uint32_t Index = 0;
-        SN2I.lookup(SecName.Section, Index);
-        const auto &SHeader = SHeaders[Index];
+        const auto &SHeader = SHeaders[SN2I.get(SecName.Section)];
         if (SHeader.sh_offset == PHeader.p_offset)
           PHeader.p_align = std::max(PHeader.p_align, SHeader.sh_addralign);
       }
     }
   }
 }
 
 template <class ELFT>
 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)) {
+      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;
     }
ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
   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) &&
+    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;
bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
                                          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) &&
+  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 =
bool ELFState<ELFT>::writeSectionContent(Elf_Shdr &SHeader,
   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)) {
+    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;
 }
for (const auto &Sym : Symbols) {
     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)) {
+    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() {