Page MenuHomePhabricator

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

Authored by grimar on May 7 2019, 3:39 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.May 7 2019, 3:39 AM
jhenderson added inline comments.May 7 2019, 5:36 AM
tools/llvm-objcopy/ELF/Object.cpp
405 ↗(On Diff #198425)

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

grimar marked an inline comment as done.May 7 2019, 5:41 AM
grimar added inline comments.
tools/llvm-objcopy/ELF/Object.cpp
405 ↗(On Diff #198425)

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 added inline comments.May 7 2019, 5:51 AM
tools/llvm-objcopy/ELF/Object.cpp
405 ↗(On Diff #198425)

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 added inline comments.May 7 2019, 5:58 AM
tools/llvm-objcopy/ELF/Object.cpp
405 ↗(On Diff #198425)

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

arning C4458: declaration of 'size' hides class member

grimar updated this revision to Diff 198464.May 7 2019, 7:12 AM
grimar marked an inline comment as done.
  • Addressed review comment.
tools/llvm-objcopy/ELF/Object.cpp
405 ↗(On Diff #198425)

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.

This revision is now accepted and ready to land.May 7 2019, 7:26 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 8 2019, 12:29 AM