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

[MaskRay]

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;

[MaskRay]

.

[leonardchan]

*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.

[leonardchan]

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

[MaskRay]

In D71795#1813393, @leonardchan wrote:

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]

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.

[MaskRay]

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]

In D71795#1818555, @MaskRay 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?

[smeenai]

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

[MaskRay]

In D71795#1818575, @pcc wrote:

In D71795#1818555, @MaskRay 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

lld/ELF/Relocations.cpp

static bool isStaticLinkTimeConstant(RelExpr e, RelType type, const Symbol &sym,
   bool relE = isRelExpr(e);
   if (absVal && !relE)
     return true;
   if (!absVal && relE)
     return true;
   if (!absVal && !relE)
     return target->usesOnlyLowPageBits(type);
 
-  // Relative relocation to an absolute value. This is normally unrepresentable,
-  // but if the relocation refers to a weak undefined symbol, we allow it to
-  // resolve to the image base. This is a little strange, but it allows us to
-  // link function calls to such symbols. Normally such a call will be guarded
-  // with a comparison, which will load a zero from the GOT.
-  // Another special case is MIPS _gp_disp symbol which represents offset
-  // between start of a function and '_gp' value and defined as absolute just
-  // to simplify the code.
   assert(absVal && relE);
-  if (sym.isUndefWeak())
-    return true;
 
   // We set the final symbols values for linker script defined symbols later.
   // They always can be computed as a link time constant.
   if (sym.scriptDefined)
       return true;
 
   error("relocation " + toString(type) + " cannot refer to absolute symbol: " +
         toString(sym) + getLocation(s, sym, relOff));