This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Delete an unused special rule from isStaticLinkTimeConstant. NFC
ClosedPublic

Authored by MaskRay on Dec 20 2019, 10:05 PM.

Details

Summary

Weak undefined symbols are preemptible after D71794.

if (sym.isPreemptible)
  return false;
if (!config->isPic)
  return true;
// isPic means includeInDynsym is true after D71794.

...

// We can delete this if because it can never be true.
if (sym.isUndefWeak)
  return true;

Event Timeline

MaskRay created this revision.Dec 20 2019, 10:05 PM
Harbormaster completed remote builds in B42869: Diff 235002.
This revision was not accepted when it landed; it landed in state Needs Review.Jan 8 2020, 9:43 AM
This revision was automatically updated to reflect the committed changes.

*Moved from the commit to here*

Hi. I have a suspicion that this commit might be breaking our build with:

= note: ld.lld: error: relocation R_X86_64_PLT32 cannot refer to absolute symbol: __sanitizer_print_memory_profile
>>> defined in /b/s/w/ir/k/recipe_cleanup/clangd9mmTF/lib/clang/10.0.0/lib/x86_64-unknown-linux-gnu/libclang_rt.asan.a
>>> referenced by sanitizer_common_libcdep.cpp:87 (compiler-rt/lib/sanitizer_common/sanitizer_common_libcdep.cpp:87)
>>>               sanitizer_common_libcdep.cpp.o:(__sanitizer::BackgroundThread(void*)) in archive /b/s/w/ir/k/recipe_cleanup/clangd9mmTF/lib/clang/10.0.0/lib/x86_64-unknown-linux-gnu/libclang_rt.asan.a
clang-10: error: linker command failed with exit code 1 (use -v to see invocation)

Link to build for reference: https://ci.chromium.org/p/fuchsia/builders/ci/fuchsia-x64-debug-clang-subbuild/b8891792394116044160

Running a bisect in the meantime to confirm if this is the faulty patch.

Yup, was able to confirm this is the faulty patch. Would you mind looking into this or reverting in the meantime?

Yup, was able to confirm this is the faulty patch. Would you mind looking into this or reverting in the meantime?

It has already been fixed by commit 375371cc8bff7ba02d0a2203f80de5e640fcadf1.

pcc added a subscriber: pcc.Jan 13 2020, 5:11 PM

Weak undefined symbols are preemptible after 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.

In D71795#1818468, @pcc wrote:

Weak undefined symbols are preemptible after 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.

pcc added a comment.Jan 13 2020, 6:36 PM
In D71795#1818468, @pcc wrote:

Weak undefined symbols are preemptible after 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?

If any follow-ups get landed after the branch, can we make sure they get merged to the branch? Thanks!

In D71795#1818575, @pcc wrote:
In D71795#1818468, @pcc wrote:

Weak undefined symbols are preemptible after 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