[PATCH] D14090: [ELF2] R_X86_64_COPY relocation implemented

[George Rimar via llvm-commits]

grimar added inline comments.

================
Comment at: ELF/OutputSections.cpp:1002
@@ +1001,3 @@
+    case SymbolBody::SharedKind: {
+      auto *Sym = cast<SharedSymbol<ELFT>>(Body->repl());
+      if (Sym->NeedsCopy)
----------------
ruiu wrote:
> I think you don't want to call repl(). Body->repl() may not be of type SharedSymbol if the same symbol is resolved to something else.
> 
>   if (cast<SharedSymbol<ELFT>>(Body)->NeedsCopy)
>     Outsec = Out<ELFT>::Bss;
>   break;
Fixed.

================
Comment at: ELF/Target.cpp:253
@@ +252,3 @@
+                                      const SymbolBody &S) const {
+
+  if (auto *E = dyn_cast<lld::elf2::ELFSymbolBody<ELF64LE>>(&S))
----------------
ruiu wrote:
> Remove this blank line.
Fixed.

================
Comment at: ELF/Target.cpp:254
@@ +253,3 @@
+
+  if (auto *E = dyn_cast<lld::elf2::ELFSymbolBody<ELF64LE>>(&S))
+    if (E->Sym.getType() != STT_OBJECT)
----------------
ruiu wrote:
> ruiu wrote:
> > Do you need "lld::elf2::" namespace specifiers?
> You can make this a bit shorter.
> 
>   if (Type == R_X86_64_32S || Type == R_X86_64_32 || Type == R_X86_64_PC32)
>     if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(&S))
>       return SS->Sym.getType() == STT_OBJECT;
>   return false;
Fixed.

================
Comment at: ELF/Target.cpp:254-259
@@ +253,8 @@
+
+  if (auto *E = dyn_cast<lld::elf2::ELFSymbolBody<ELF64LE>>(&S))
+    if (E->Sym.getType() != STT_OBJECT)
+      return false;
+  if (!S.isShared())
+    return false;
+  return (Type == R_X86_64_32S || Type == R_X86_64_32 || Type == R_X86_64_PC32);
+}
----------------
grimar wrote:
> ruiu wrote:
> > ruiu wrote:
> > > Do you need "lld::elf2::" namespace specifiers?
> > You can make this a bit shorter.
> > 
> >   if (Type == R_X86_64_32S || Type == R_X86_64_32 || Type == R_X86_64_PC32)
> >     if (auto *SS = dyn_cast<SharedSymbol<ELFT>>(&S))
> >       return SS->Sym.getType() == STT_OBJECT;
> >   return false;
> Fixed.
Fixed.


http://reviews.llvm.org/D14090