[llvm-objcopy] - Fix for "Bug 41775 - SymbolTableSection::addSymbol - shadow variable names"

[grimar]

Summary

This is a fix for https://bugs.llvm.org/show_bug.cgi?id=41775,

I checked that we do not actually need this line,
since we always call removeSectionReferences which calls removeSymbols which
updates the Size:

Error SymbolTableSection::removeSymbols(
    function_ref<bool(const Symbol &)> ToRemove) {
  Symbols.erase(
      std::remove_if(std::begin(Symbols) + 1, std::end(Symbols),
                     [ToRemove](const SymPtr &Sym) { return ToRemove(*Sym); }),
      std::end(Symbols));
  Size = Symbols.size() * EntrySize;
  assignIndices();
  return Error::success();
}

But I think it worth to fix this assignment instead of removing it,
that allows to relax the dependencies.

[jhenderson]

tools/llvm-objcopy/ELF/Object.cpp

405

Maybe a better fix would be to rename this parameter to SymbolSize, to avoid any future confusion?

[grimar]

tools/llvm-objcopy/ELF/Object.cpp

405

But it will be not consistent with Type, Value and others. Doesn't seem we want to
rename all of those to SymbolType, SymbolValue etc?

[jhenderson]

tools/llvm-objcopy/ELF/Object.cpp

405

If another person comes along and writes something to do with the section size in this function, then I could easily see them make the same mistake. Also, at least on Visual Studio with high warning levels, the compiler emits warnings for shadowed variables (and this would have highlighted this issue here), so we should allow people to build with high warning levels by fixing this. I don't mind the name being slightly inconsistent, if it makes the code less fragile. I also am not strongly against SymbolType, SymbolValue etc, if you feel consistency is important.

[MaskRay]

tools/llvm-objcopy/ELF/Object.cpp

405

LLVM_ENABLE_WARNINGS defaults to on => /W4 for MSVC. This shadowing emits a warning:

arning C4458: declaration of 'size' hides class member

[grimar]

• Addressed review comment.

tools/llvm-objcopy/ELF/Object.cpp

405

Well. For some reason I do not see this warning on my MSVS2017 when compiling this file.
/W4 was set by default for me and I did not touch/set LLVM_ENABLE_WARNINGS, i.e. it is also should be On.

But OK, if that can show a warning, I am fine to rename to SymbolSize.

[jhenderson]

LGTM.

tools/llvm-objcopy/ELF/Object.cpp

void SymbolTableSection::assignIndices() {
   uint32_t Index = 0;
   for (auto &Sym : Symbols)
     Sym->Index = Index++;
 }
 
 void SymbolTableSection::addSymbol(Twine Name, uint8_t Bind, uint8_t Type,
                                    SectionBase *DefinedIn, uint64_t Value,
                                    uint8_t Visibility, uint16_t Shndx,
                                    uint64_t Size) {
   Symbol Sym;
   Sym.Name = Name.str();
   Sym.Binding = Bind;
   Sym.Type = Type;
   Sym.DefinedIn = DefinedIn;
   if (DefinedIn != nullptr)
     DefinedIn->HasSymbol = true;
   if (DefinedIn == nullptr) {
     if (Shndx >= SHN_LORESERVE)
       Sym.ShndxType = static_cast<SymbolShndxType>(Shndx);
     else
       Sym.ShndxType = SYMBOL_SIMPLE_INDEX;
   }
   Sym.Value = Value;
   Sym.Visibility = Visibility;
   Sym.Size = Size;
   Sym.Index = Symbols.size();
   Symbols.emplace_back(llvm::make_unique<Symbol>(Sym));
-  Size += this->EntrySize;
+  this->Size += this->EntrySize;
 }
 
 Error SymbolTableSection::removeSectionReferences(
     bool AllowBrokenLinks,
     function_ref<bool(const SectionBase *)> ToRemove) {
   if (ToRemove(SectionIndexTable))
     SectionIndexTable = nullptr;
   if (ToRemove(SymbolNames)) {