[PATCH] D35724: [ELF] - Fix missing relocation when linking executable with --unresolved-symbols=ignore-all

[Rafael Avila de Espindola via llvm-commits]

LGTM

Thanks,
Rafael

George Rimar via Phabricator <reviews at reviews.llvm.org> writes:

> grimar updated this revision to Diff 108331.
> grimar added a comment.
>
> - Addressed review comments.
> - Added testcase with protected symbol. In that case we fail to link it with  --unresolved-symbols=ignore-all. That behavior is natural and matches both bfd and gold. See no reasons to change it atm.
>
>
> https://reviews.llvm.org/D35724
>
> Files:
>   ELF/Symbols.cpp
>   test/ELF/executable-undefined-ignoreall.s
>   test/ELF/executable-undefined-protected-ignoreall.s
>   test/ELF/no-inhibit-exec.s
>
>
> Index: test/ELF/no-inhibit-exec.s
> ===================================================================
> --- test/ELF/no-inhibit-exec.s
> +++ test/ELF/no-inhibit-exec.s
> @@ -2,11 +2,15 @@
>  # RUN: not ld.lld %t -o %t2
>  # RUN: ld.lld %t --noinhibit-exec -o %t2
>  # RUN: llvm-objdump -d %t2 | FileCheck %s
> +# RUN: llvm-readobj -r %t2 | FileCheck %s --check-prefix=RELOC
>  # REQUIRES: x86
>  
>  # CHECK: Disassembly of section .text:
>  # CHECK-NEXT: _start
> -# CHECK-NEXT: 201000: {{.*}} callq -2101253
> +# CHECK-NEXT: 201000: {{.*}} callq 0
> +
> +# RELOC: Relocations [
> +# RELOC: ]
>  
>  # next code will not link without noinhibit-exec flag
>  # because of undefined symbol _bar
> Index: test/ELF/executable-undefined-protected-ignoreall.s
> ===================================================================
> --- test/ELF/executable-undefined-protected-ignoreall.s
> +++ test/ELF/executable-undefined-protected-ignoreall.s
> @@ -0,0 +1,8 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
> +# RUN: not ld.lld %t -o %tout --unresolved-symbols=ignore-all -pie 2>&1 | FileCheck %s
> +# CHECK: error: undefined symbol: foo
> +
> +.protected foo
> +_start:
> +callq foo at PLT
> Index: test/ELF/executable-undefined-ignoreall.s
> ===================================================================
> --- test/ELF/executable-undefined-ignoreall.s
> +++ test/ELF/executable-undefined-ignoreall.s
> @@ -0,0 +1,13 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
> +# RUN: ld.lld %t -o %tout --unresolved-symbols=ignore-all -pie
> +# RUN: llvm-readobj -r %tout | FileCheck %s
> +
> +# CHECK:      Relocations [
> +# CHECK-NEXT:   Section ({{.*}}) .rela.plt {
> +# CHECK-NEXT:     0x2018 R_X86_64_JUMP_SLOT foo 0x0
> +# CHECK-NEXT:   }
> +# CHECK-NEXT: ]
> +
> +_start:
> +callq foo at PLT
> Index: ELF/Symbols.cpp
> ===================================================================
> --- ELF/Symbols.cpp
> +++ ELF/Symbols.cpp
> @@ -141,18 +141,20 @@
>    if (isShared())
>      return !NeedsCopy && !NeedsPltAddr;
>  
> -  // That's all that can be preempted in a non-DSO.
> -  if (!Config->Shared)
> -    return false;
> -
>    // Only symbols that appear in dynsym can be preempted.
>    if (!symbol()->includeInDynsym())
>      return false;
>  
>    // Only default visibility symbols can be preempted.
>    if (symbol()->Visibility != STV_DEFAULT)
>      return false;
>  
> +  // Undefined external symbols in a non-DSO usually reported by linker and link
> +  // fails, but together with --unresolved-symbols=ignore-all link succeeds and
> +  // runtime linker should take care about them during execution.
> +  if (!Config->Shared)
> +    return isUndefined();
> +
>    // -Bsymbolic means that definitions are not preempted.
>    if (Config->Bsymbolic || (Config->BsymbolicFunctions && isFunc()))
>      return !isDefined();
> @@ -357,8 +359,11 @@
>  bool Symbol::includeInDynsym() const {
>    if (computeBinding() == STB_LOCAL)
>      return false;
> -  return ExportDynamic || body()->isShared() ||
> -         (body()->isUndefined() && Config->Shared);
> +  if (ExportDynamic || body()->isShared())
> +    return true;
> +  if (!body()->isUndefined())
> +    return false;
> +  return Config->Shared || !body()->symbol()->isWeak();
>  }
>  
>  // Print out a log message for --trace-symbol.
>
>
> Index: test/ELF/no-inhibit-exec.s
> ===================================================================
> --- test/ELF/no-inhibit-exec.s
> +++ test/ELF/no-inhibit-exec.s
> @@ -2,11 +2,15 @@
>  # RUN: not ld.lld %t -o %t2
>  # RUN: ld.lld %t --noinhibit-exec -o %t2
>  # RUN: llvm-objdump -d %t2 | FileCheck %s
> +# RUN: llvm-readobj -r %t2 | FileCheck %s --check-prefix=RELOC
>  # REQUIRES: x86
>  
>  # CHECK: Disassembly of section .text:
>  # CHECK-NEXT: _start
> -# CHECK-NEXT: 201000: {{.*}} callq -2101253
> +# CHECK-NEXT: 201000: {{.*}} callq 0
> +
> +# RELOC: Relocations [
> +# RELOC: ]
>  
>  # next code will not link without noinhibit-exec flag
>  # because of undefined symbol _bar
> Index: test/ELF/executable-undefined-protected-ignoreall.s
> ===================================================================
> --- test/ELF/executable-undefined-protected-ignoreall.s
> +++ test/ELF/executable-undefined-protected-ignoreall.s
> @@ -0,0 +1,8 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
> +# RUN: not ld.lld %t -o %tout --unresolved-symbols=ignore-all -pie 2>&1 | FileCheck %s
> +# CHECK: error: undefined symbol: foo
> +
> +.protected foo
> +_start:
> +callq foo at PLT
> Index: test/ELF/executable-undefined-ignoreall.s
> ===================================================================
> --- test/ELF/executable-undefined-ignoreall.s
> +++ test/ELF/executable-undefined-ignoreall.s
> @@ -0,0 +1,13 @@
> +# REQUIRES: x86
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t
> +# RUN: ld.lld %t -o %tout --unresolved-symbols=ignore-all -pie
> +# RUN: llvm-readobj -r %tout | FileCheck %s
> +
> +# CHECK:      Relocations [
> +# CHECK-NEXT:   Section ({{.*}}) .rela.plt {
> +# CHECK-NEXT:     0x2018 R_X86_64_JUMP_SLOT foo 0x0
> +# CHECK-NEXT:   }
> +# CHECK-NEXT: ]
> +
> +_start:
> +callq foo at PLT
> Index: ELF/Symbols.cpp
> ===================================================================
> --- ELF/Symbols.cpp
> +++ ELF/Symbols.cpp
> @@ -141,18 +141,20 @@
>    if (isShared())
>      return !NeedsCopy && !NeedsPltAddr;
>  
> -  // That's all that can be preempted in a non-DSO.
> -  if (!Config->Shared)
> -    return false;
> -
>    // Only symbols that appear in dynsym can be preempted.
>    if (!symbol()->includeInDynsym())
>      return false;
>  
>    // Only default visibility symbols can be preempted.
>    if (symbol()->Visibility != STV_DEFAULT)
>      return false;
>  
> +  // Undefined external symbols in a non-DSO usually reported by linker and link
> +  // fails, but together with --unresolved-symbols=ignore-all link succeeds and
> +  // runtime linker should take care about them during execution.
> +  if (!Config->Shared)
> +    return isUndefined();
> +
>    // -Bsymbolic means that definitions are not preempted.
>    if (Config->Bsymbolic || (Config->BsymbolicFunctions && isFunc()))
>      return !isDefined();
> @@ -357,8 +359,11 @@
>  bool Symbol::includeInDynsym() const {
>    if (computeBinding() == STB_LOCAL)
>      return false;
> -  return ExportDynamic || body()->isShared() ||
> -         (body()->isUndefined() && Config->Shared);
> +  if (ExportDynamic || body()->isShared())
> +    return true;
> +  if (!body()->isUndefined())
> +    return false;
> +  return Config->Shared || !body()->symbol()->isWeak();
>  }
>  
>  // Print out a log message for --trace-symbol.