This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Don't emit dynamic relocations with weak undef in writable sections
ClosedPublic

Authored by MaskRay on Jun 7 2019, 3:48 AM.

Details

Summary

In processRelocAux(), our handling of 1) link-time constant and 2) weak
undef is the same, so put them together to simplify the logic.

This moves the weak undef code around. The result is that:

For a writable section, we will not emit dynamic relocations for weak undefined
symbols.

Since this simplifies the logic, improves consistency with the readonly
case, and also seems to match what GNU linkers do, so this is probably
good to do.

The condition !Config->Shared was there probably because it is common
for a -shared link not to specify full dependencies. Keep it now but we
may revisit the decision in the future.

gABI says:

The behavior of weak symbols in areas not specified by this document is
implementation defined. Weak symbols are intended primarily for use in
system software. Applications using weak symbols are unreliable since
changes in the runtime environment might cause the execution to fail.

With this change, we can simplify another place
Symbol::includeInDynsym() (see D59549) a bit.

Event Timeline

MaskRay created this revision.Jun 7 2019, 3:48 AM
grimar added inline comments.Jun 7 2019, 3:56 AM
test/ELF/weak-undef-rw.s
10

What is GNU linkers behavior in this case?

MaskRay updated this revision to Diff 203529.Jun 7 2019, 3:57 AM
MaskRay edited the summary of this revision. (Show Details)

Emphasize the benefits

MaskRay marked an inline comment as done.Jun 7 2019, 3:59 AM
MaskRay added inline comments.
test/ELF/weak-undef-rw.s
10

GNU linkers don't emit dynamic relocations for executables, so this change seems to improves compatibility.

ruiu added a comment.Jun 7 2019, 4:18 AM

Does this mean that we previously emit a dynamic relocation for a weak undefined symbol that couldn't have been resolved at link time, and this patch changes the behavior so that weak undefined symbols must be resolved to some symbol at link time. Am I correct?

If so, that change seems a little risky, as it will silently change a program behavior. If the following three conditions are met, a program will behave differently.

  1. An object file contains a weak undefined symbol
  2. No DSOs define the weak undefined symbol at link time
  3. But some DSO defines the weak undefined symbol at run time

I'd think that the above conditions are minor, and you generally shouldn't rely on that kind of edge cases, though.

Ultimately, it seems like it is ultimately a programming language and ELF's failure that they underrepresents the difference between global variables defined in other modules and global variables defined in the same module. The ELF's model tries to hide the difference between static linking and dynamic linking, but the difference leaks at various places like this. We have only one type of "weak" undefined type, but in dynamic linking, a symbol is given two chances to be resolved (static link time and run time), so it is hard to say what is a correct behavior.

ruiu added inline comments.Jun 7 2019, 4:19 AM
test/ELF/weak-undef-rw.s
10

Hm, if this is the same as GNU linker, I believe it's a safe change in practice.

MaskRay updated this revision to Diff 203535.Jun 7 2019, 4:41 AM

In weak-undef-rw.s, mention this matches ld.bfd and gold

I've tested this internally. This is a safe change.

ruiu added a comment.Jun 9 2019, 10:46 PM

Was my previous comment correct? (I'm not suggesting any change at the moment but I'd like to make sure that we are on the same page.)

Your previous comment is correct.

  1. An object file contains a weak undefined symbol
  1. No DSOs define the weak undefined symbol at link time
  1. But some DSO defines the weak undefined symbol at run time

Currently:

  • If !CanWrite, no dynamic relocation is created (cannot be preempted by a runtime DSO).
  • If CanWrite, a dynamic relocation is created (can be preempted by a runtime DSO).

This patch makes it:

  • If !CanWrite, no dynamic relocation is created
  • If CanWrite, no dynamic relocation is created

This also appears to be what ld.bfd and gold do.

Shall we give it a try?

ruiu accepted this revision.Jun 14 2019, 6:32 AM

Yes, let's land this. LGTM

This revision is now accepted and ready to land.Jun 14 2019, 6:32 AM
This revision was automatically updated to reflect the committed changes.