[PATCH] D71795: [ELF] Delete an unused special rule from isStaticLinkTimeConstant. NFC

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In D71795#1818555 <https://reviews.llvm.org/D71795#1818555>, @MaskRay wrote:

> In D71795#1818468 <https://reviews.llvm.org/D71795#1818468>, @pcc wrote:
>
> > > Weak undefined symbols are preemptible after D71794 <https://reviews.llvm.org/D71794>.
> >
> > What about hidden weak undefined symbols? We now reject the following input:
> >
> >    cat foo.cpp
> >    __attribute__((weak, visibility("hidden"))) void f();
> >   
> >    void g() {
> >      if (f) f();
> >    }
> >    $ clang -shared -fuse-ld=lld foo.cpp
> >    ld.lld: error: relocation R_X86_64_PLT32 cannot refer to absolute symbol: f()
> >   >>> defined in /tmp/foo-014d89.o
> >   >>> referenced by foo.cpp
> >   >>>               /tmp/foo-014d89.o:(g())
> >    clang: error: linker command failed with exit code 1 (use -v to see invocation)
> >   
> >
> > This seems like a reasonable thing to want to do. I actually hit this problem in a slightly different case (`--exclude-libs` demoted the weak undef to hidden) which we could fix like so:
> >
> >   diff --git a/lld/ELF/Driver.cpp b/lld/ELF/Driver.cpp
> >   index 19598f52cd4..54c033303f6 100644
> >   --- a/lld/ELF/Driver.cpp
> >   +++ b/lld/ELF/Driver.cpp
> >   @@ -1374,7 +1374,7 @@ static void excludeLibs(opt::InputArgList &args) {
> >        if (!file->archiveName.empty())
> >          if (all || libs.count(path::filename(file->archiveName)))
> >            for (Symbol *sym : file->getSymbols())
> >   -          if (!sym->isLocal() && sym->file == file)
> >   +          if (!sym->isUndefined() && !sym->isLocal() && sym->file == file)
> >                sym->versionId = VER_NDX_LOCAL;
> >      };
> >   
> >
> >
> > But I'm somewhat more concerned about the explicit hidden case.
>
>
> This patch looks good. Only defined symbols should have versions. I think the logic will also be closer to GNU ld:
>
>   // bfd/elflink.c
>   	  /* If this symbol has default visibility and the user has
>   	     requested we not re-export it, then mark it as hidden.  */
>   	  if (!bfd_is_und_section (sec)
>   	      && !dynamic
>   	      && abfd->no_export
>   	      && ELF_ST_VISIBILITY (isym->st_other) != STV_INTERNAL)
>   	    isym->st_other = (STV_HIDDEN
>   			      | (isym->st_other & ~ELF_ST_VISIBILITY (-1)));
>
>
> If you haven't started working on this yet, I can send a patch.


Again, note that it wouldn't fix the explicit hidden case, but I suppose it could be seen as somewhat orthogonal. Could you send the patch, please?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71795/new/

https://reviews.llvm.org/D71795