Page MenuHomePhabricator

[ELF] - Allow relocation to a weak undefined symbol when -z notext is given.
ClosedPublic

Authored by grimar on Dec 23 2017, 3:31 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Dec 23 2017, 3:31 AM

This LGTM, I had a similar change in my repo while trying to fix PR35720.

ELF/Relocations.cpp
607 ↗(On Diff #128076)

I think this comment was previously a logical continuation of the "is simply writable" comment, and the "Or," and ", too." don't quite really follow with the reordering, maybe just "If we are allowed ... a dynamic relocation as we want."

grimar updated this revision to Diff 128095.Dec 23 2017, 9:34 PM
  • Updated comment.
ELF/Relocations.cpp
607 ↗(On Diff #128076)

Thanks, Ed !

Conflicts after rL321430. I tried a straightforward rebase (moving the if ((S.Flags & SHF_WRITE) || !Config->ZText) block below if (Sym.isUndefWeak()) but test/ELF/retain-und.s (from rL293093) fails.

Conflicts after rL321430. I tried a straightforward rebase (moving the if ((S.Flags & SHF_WRITE) || !Config->ZText) block below if (Sym.isUndefWeak()) but test/ELF/retain-und.s (from rL293093) fails.

That testcase looks close to what this patch trying to fix. It contains weak undef.
Both gold and bfd of v2.29.51.20171006 does not produce relocation for that testcase.

It seems to me we should fix that testcase as well. I'll rebase this patch now.

grimar updated this revision to Diff 128151.Dec 26 2017, 12:30 AM
  • Rebased.
grimar planned changes to this revision.Dec 26 2017, 12:35 AM

Ah no. Actually gold and bfd of v2.29.51.20171006 produce relocation when -shared is given (I tested without). And do not produce it for executable.
Looking again.

Ok, I think GNU linkers do correct thing. We also should produce the relocation for week undefined when building shared library.
Imagine next code:

dso.c:

extern __attribute__((visibility("default"), weak)) unsigned long long foo();
unsigned long long func() {
  return &foo;
}
unsigned long long run() {
 return func();
}

main.c:

unsigned long long run();

unsigned long long foo() {
 return 1;
}

int main() {
 return run() > 0 ? 1 : 0;
}

If we do not produce the relocation for week undefined foo when linkind DSO, code would work incorrectly:

~/LLVM/build/bin/clang -o dso dso.c -shared -fuse-ld=lld -O0 -z notext
~/LLVM/build/bin/clang -o main main.c dso -fuse-ld=lld -O0
./main 
echo $?
0

Output should be 1.

I'll update the patch and testcase.

grimar updated this revision to Diff 128158.Dec 26 2017, 2:55 AM
  • Updated in according to previous comments.

The original FreeBSD test case (a 'hello world' linked with lld -znotext) works with this change applied.

emaste added inline comments.Dec 26 2017, 6:27 AM
ELF/Relocations.cpp
593 ↗(On Diff #128158)

minor nit, "and we are producing an executable"

emaste accepted this revision.Dec 26 2017, 1:33 PM

LGTM

This revision is now accepted and ready to land.Dec 26 2017, 1:33 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.