Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[LLD] dtNeeded name should consistent with soNames #72857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CoTinker
Copy link
Contributor

@CoTinker CoTinker commented Nov 20, 2023

If dtNeeded name is inconsistent with soNames, lld can not report error right.

demo

#--- def.s

#--- ref.s
.globl fun
fun:
  bl foo@PLT

#--- main.s
.globl _start
_start:
  bl fun@PLT


$ llvm-mc -filetype=obj -triple=aarch64 main.s -o main.o
$ llvm-mc -filetype=obj -triple=aarch64 def.s -o def.o && ld.lld -shared def.o -o def.so
$ llvm-mc -filetype=obj -triple=aarch64 ref.s -o ref.o && ld.lld -shared ref.o -o ref.so ./def.so

can report error

$ ld.lld main.o ref.so ./def.so

ld.lld: error: undefined reference due to --no-allow-shlib-undefined: foo
>>> referenced by ref.so

dtNeeded name = ./def.so, soName = ./def.so, consistent.

can not report error

$ ld.lld main.o ref.so def.so
dtNeeded name = ./def.so, soName = def.so, inconsistent.

We can see that if dtNeeded name is inconsistent with soNames, lld can not report error right.

for (SharedFile *file : ctx.sharedFiles) {
bool allNeededIsKnown =
llvm::all_of(file->dtNeeded, [&](StringRef needed) {
return symtab.soNames.count(CachedHashStringRef(needed));
});
if (!allNeededIsKnown)
continue;
for (Symbol *sym : file->requiredSymbols) {
if (sym->isUndefined() && !sym->isWeak()) {
diagnose("undefined reference due to --no-allow-shlib-undefined: " +
toString(*sym) + "\n>>> referenced by " + toString(file));
} else if (sym->isDefined() && sym->computeBinding() == STB_LOCAL) {
diagnose("non-exported symbol '" + toString(*sym) + "' in '" +
toString(sym->file) + "' is referenced by DSO '" +
toString(file) + "'");
}
}
}

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 20, 2023

@llvm/pr-subscribers-lld

@llvm/pr-subscribers-lld-elf

Author: Longsheng Mou (CoTinker)

Changes

If dtNeeded name is inconsistent with soNames, lld can not report error right.

demo

libhave.so

clang have.c --shared -fPIC -o libhave.so

// have.h
void foo();

// have.c
#include "have.h"

libuse.so

clang use.c --shared -fPIC -o libuse.so libhave.so

// use.h
#include "have.h"
void fun();

// use.c
#include "use.h"
void fun() {
    foo();
}

main

main.c

#include "use.h"

int main() {
    fun();
}

can report error

clang case/use.c -o exe/libuse.so -lhave -Lexe --shared -fPIC
clang case/main.c -o exe/main.exe -luse -Lexe -lhave
soName=libhave.so needed=libhave.so
image

clang case/use.c -o exe/libuse.so exe/libhave.so --shared -fPIC
clang case/main.c -o exe/main.exe -luse -Lexe exe/libhave.so
soName=exe/libhave.so needed=exe/libhave.so
image

can not report error

clang case/ususe.c -o exe/libuseo exe/libhave.so --shared -fPIC
clang case/main.c -o exe/main.exe -luse -Lexe -lhave
soName=libhave.so needed=exe/libhave.so

clang case/ususe.c -o exe/libuseo -lhave -Lexe --shared -fPIC
clang case/main.c -o exe/main.exe -luse -Lexe exe/libhave.so
soName=exe/libhave.so needed=libhave.so

We can see that if dtNeeded name is inconsistent with soNames, lld can not report error right.

for (SharedFile *file : ctx.sharedFiles) {
bool allNeededIsKnown =
llvm::all_of(file->dtNeeded, [&](StringRef needed) {
return symtab.soNames.count(CachedHashStringRef(needed));
});
if (!allNeededIsKnown)
continue;
for (Symbol *sym : file->requiredSymbols) {
if (sym->isUndefined() && !sym->isWeak()) {
diagnose("undefined reference due to --no-allow-shlib-undefined: " +
toString(*sym) + "\n>>> referenced by " + toString(file));
} else if (sym->isDefined() && sym->computeBinding() == STB_LOCAL) {
diagnose("non-exported symbol '" + toString(*sym) + "' in '" +
toString(sym->file) + "' is referenced by DSO '" +
toString(file) + "'");
}
}
}


Full diff: https://github.com/llvm/llvm-project/pull/72857.diff

1 Files Affected:

  • (modified) lld/ELF/InputFiles.cpp (+2-2)
diff --git a/lld/ELF/InputFiles.cpp b/lld/ELF/InputFiles.cpp
index 7ac737dab0519a8..05b9dc2d5474170 100644
--- a/lld/ELF/InputFiles.cpp
+++ b/lld/ELF/InputFiles.cpp
@@ -1439,7 +1439,7 @@ template <class ELFT> void SharedFile::parse() {
       uint64_t val = dyn.getVal();
       if (val >= this->stringTable.size())
         fatal(toString(this) + ": invalid DT_NEEDED entry");
-      dtNeeded.push_back(this->stringTable.data() + val);
+      dtNeeded.push_back(path::filename(this->stringTable.data() + val));
     } else if (dyn.d_tag == DT_SONAME) {
       uint64_t val = dyn.getVal();
       if (val >= this->stringTable.size())
@@ -1452,7 +1452,7 @@ template <class ELFT> void SharedFile::parse() {
   DenseMap<CachedHashStringRef, SharedFile *>::iterator it;
   bool wasInserted;
   std::tie(it, wasInserted) =
-      symtab.soNames.try_emplace(CachedHashStringRef(soName), this);
+      symtab.soNames.try_emplace(CachedHashStringRef(path::filename(soName)), this);
 
   // If a DSO appears more than once on the command line with and without
   // --as-needed, --no-as-needed takes precedence over --as-needed because a

Copy link

github-actions bot commented Nov 20, 2023

✅ With the latest revision this PR passed the C/C++ code formatter.

@CoTinker
Copy link
Contributor Author

@MaskRay

@smithp35
Copy link
Collaborator

I'm finding your examples a bit hard to follow. Can you clarify a few things:

  • Did you mean have.c to just be a declaration? There is no foo in the dynamic symbol.
  • What is ususe.c? Is it just use.c renamed?
  • Is -o exe/libuseo a mistake -luse won't pick that up as it looks for libuse.a or libuse.so
  • Please can you write a test for your patch? Look for examples in lld/ELF/tests

@CoTinker
Copy link
Contributor Author

I'm finding your examples a bit hard to follow. Can you clarify a few things:

  • Did you mean have.c to just be a declaration? There is no foo in the dynamic symbol.
  • What is ususe.c? Is it just use.c renamed?
  • Is -o exe/libuseo a mistake -luse won't pick that up as it looks for libuse.a or libuse.so
  • Please can you write a test for your patch? Look for examples in lld/ELF/tests
  1. YES,the foo function is declared but not implemented.
  2. That's my fault, I wrote it wrong.
  3. No, actually in case 3, dtneeded name is exe/libhave.so, but soName is libhave.so in -lhave, so there are inconsistent. In case 4, dtneeded name is libhave.so, but soName is exe/libhave.so, alse inconsistent.
  4. I'll try it.

@CoTinker CoTinker force-pushed the soName branch 2 times, most recently from 92dc3ac to 69b001a Compare November 21, 2023 04:40
If dtNeeded name is inconsistent with soNames, lld can not report error right.
@MaskRay
Copy link
Member

MaskRay commented Nov 21, 2023

I suspect that you have an incorrect use case, but the case is not clearly described and I cannot verify.
This inconsistency fault is likely at the user side when a shared object does not have DT_SONAME (https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#sonamename)

FYI I place all needed files in a directory and run cd dir; mkreproduce . > /tmp/rep.sh to create cat <<eof and echo instructions that other people can easily reproduce. (e.g. https://maskray.me/blog/2020-11-26-all-about-symbol-versioning#version-script is an example using the script to prepare for input files)

#!/bin/zsh
dry_run=
if [[ $1 == --dry-run ]]; then dry_run=1; shift; fi

f() {
  for f in $1/*(ND.^x); do
    [[ ${f##*/} =~ '^\.' ]] && continue
    if [[ -n $dry_run ]]; then
      echo $f
    elif [[ $(wc -l < $f) == 1 ]]; then
      echo "echo '$(<$f)' > $f"
    else
      if [[ ${f##*/} == Makefile ]]; then
        printf '%s\n' "sed 's/^        /\\t/' > $f <<'eof'"
      else
        echo "cat > $f <<'eof'"
      fi
      cat $f
      echo eof
    fi
  done
  for d in $1/*(ND/); do
    [[ $d =~ '^\.' ]] && continue
    echo mkdir -p $d
    f $d
  done
}

f ${1:-.}
clang case/use.c -o exe/libuse.so -lhave -Lexe --shared -fPIC

The -o libuse.so instruction does not mention how you switched the directory, so the example is difficult to follow.

@CoTinker
Copy link
Contributor Author

I suspect that you have an incorrect use case, but the case is not clearly described and I cannot verify. This inconsistency fault is likely at the user side when a shared object does not have DT_SONAME (https://maskray.me/blog/2020-11-15-explain-gnu-linker-options#sonamename)

FYI I place all needed files in a directory and run cd dir; mkreproduce . > /tmp/rep.sh to create cat <<eof and echo instructions that other people can easily reproduce. (e.g. https://maskray.me/blog/2020-11-26-all-about-symbol-versioning#version-script is an example using the script to prepare for input files)

#!/bin/zsh
dry_run=
if [[ $1 == --dry-run ]]; then dry_run=1; shift; fi

f() {
  for f in $1/*(ND.^x); do
    [[ ${f##*/} =~ '^\.' ]] && continue
    if [[ -n $dry_run ]]; then
      echo $f
    elif [[ $(wc -l < $f) == 1 ]]; then
      echo "echo '$(<$f)' > $f"
    else
      if [[ ${f##*/} == Makefile ]]; then
        printf '%s\n' "sed 's/^        /\\t/' > $f <<'eof'"
      else
        echo "cat > $f <<'eof'"
      fi
      cat $f
      echo eof
    fi
  done
  for d in $1/*(ND/); do
    [[ $d =~ '^\.' ]] && continue
    echo mkdir -p $d
    f $d
  done
}

f ${1:-.}
clang case/use.c -o exe/libuse.so -lhave -Lexe --shared -fPIC

The -o libuse.so instruction does not mention how you switched the directory, so the example is difficult to follow.

Thank you, the core problem is probably shared object does not have DT_SONAME.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants