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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

In D71795#1818575 <https://reviews.llvm.org/D71795#1818575>, @pcc wrote:

> 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?


D72681 <https://reviews.llvm.org/D72681>


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71795