[PATCH] D41639: [ELF] Drop unnecessary VersionId setting in scanShlibUndefined

[Rafael Avila de Espindola via llvm-commits]

LGTM

Thanks,
Rafael

Shoaib Meenai via Phabricator <reviews at reviews.llvm.org> writes:

> smeenai created this revision.
> smeenai added reviewers: rafael, ruiu.
> Herald added a subscriber: emaste.
>
> LLD previously used to handle dynamic lists and version scripts in the
> exact same way, even though they have very different semantics for
> shared libraries and subtly different semantics for executables. r315114
> untangled their semantics for executables (building on previous work to
> correct their semantics for shared libraries). With that change, dynamic
> lists won't set the default version to VER_NDX_LOCAL, and so resetting
> the version to VER_NDX_GLOBAL in scanShlibUndefined is unnecessary.
>
> This was causing an issue because version scripts containing `local: *`
> work by setting the default version to VER_NDX_LOCAL, but scanShlibUndefined
> would override this default, and therefore symbols which should have
> been local would end up in the dynamic symbol table, which differs from
> both bfd and gold's behavior. gold silently keeps the symbol hidden in
> such a scenario, whereas bfd issues an error. I prefer bfd's behavior
> and plan to implement that in LLD in a follow-up (and the test case
> added here will be updated accordingly).
>
>
> Repository:
>   rLLD LLVM Linker
>
> https://reviews.llvm.org/D41639
>
> Files:
>   ELF/SymbolTable.cpp
>   test/ELF/shlib-undefined-local.s
>
>
> Index: test/ELF/shlib-undefined-local.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/shlib-undefined-local.s
> @@ -0,0 +1,19 @@
> +# REQUIRES: x86
> +
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-linux-gnu -o %t1.o %S/Inputs/shlib-undefined-ref.s
> +# RUN: ld.lld -shared -o %t.so %t1.o
> +
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-linux-gnu -o %t2.o %s
> +# RUN: echo "{ local: *; };" > %t.script
> +# RUN: ld.lld -version-script %t.script -o %t %t2.o %t.so
> +# RUN: llvm-nm -g %t | FileCheck -allow-empty %s
> +
> +# CHECK-NOT: should_not_be_exported
> +
> +.globl should_not_be_exported
> +should_not_be_exported:
> +	ret
> +
> +.globl _start
> +_start:
> +	ret
> Index: ELF/SymbolTable.cpp
> ===================================================================
> --- ELF/SymbolTable.cpp
> +++ ELF/SymbolTable.cpp
> @@ -598,12 +598,6 @@
>        if (!Sym || !Sym->isDefined())
>          continue;
>        Sym->ExportDynamic = true;
> -
> -      // If -dynamic-list is given, the default version is set to
> -      // VER_NDX_LOCAL, which prevents a symbol to be exported via .dynsym.
> -      // Set to VER_NDX_GLOBAL so the symbol will be handled as if it were
> -      // specified by -dynamic-list.
> -      Sym->VersionId = VER_NDX_GLOBAL;
>      }
>    }
>  }
>
>
> Index: test/ELF/shlib-undefined-local.s
> ===================================================================
> --- /dev/null
> +++ test/ELF/shlib-undefined-local.s
> @@ -0,0 +1,19 @@
> +# REQUIRES: x86
> +
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-linux-gnu -o %t1.o %S/Inputs/shlib-undefined-ref.s
> +# RUN: ld.lld -shared -o %t.so %t1.o
> +
> +# RUN: llvm-mc -filetype=obj -triple=x86_64-linux-gnu -o %t2.o %s
> +# RUN: echo "{ local: *; };" > %t.script
> +# RUN: ld.lld -version-script %t.script -o %t %t2.o %t.so
> +# RUN: llvm-nm -g %t | FileCheck -allow-empty %s
> +
> +# CHECK-NOT: should_not_be_exported
> +
> +.globl should_not_be_exported
> +should_not_be_exported:
> +	ret
> +
> +.globl _start
> +_start:
> +	ret
> Index: ELF/SymbolTable.cpp
> ===================================================================
> --- ELF/SymbolTable.cpp
> +++ ELF/SymbolTable.cpp
> @@ -598,12 +598,6 @@
>        if (!Sym || !Sym->isDefined())
>          continue;
>        Sym->ExportDynamic = true;
> -
> -      // If -dynamic-list is given, the default version is set to
> -      // VER_NDX_LOCAL, which prevents a symbol to be exported via .dynsym.
> -      // Set to VER_NDX_GLOBAL so the symbol will be handled as if it were
> -      // specified by -dynamic-list.
> -      Sym->VersionId = VER_NDX_GLOBAL;
>      }
>    }
>  }