This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Fix missing relocation when linking executable with --unresolved-symbols=ignore-all
ClosedPublic

Authored by grimar on Jul 21 2017, 6:24 AM.

Diff Detail

Event Timeline

grimar created this revision.Jul 21 2017, 6:24 AM
grimar planned changes to this revision.Jul 21 2017, 6:47 AM
grimar updated this revision to Diff 107669.Jul 21 2017, 6:55 AM
grimar edited the summary of this revision. (Show Details)
  • Removed too agressive change about --noinhibit-exec behavior.
ruiu edited edge metadata.Jul 21 2017, 3:00 PM

Looks like what you described in the commit message is different from what this patch actually does. This patch is to handle the --noinhibit_exec option which wasn't handled before.

In D35724#817642, @ruiu wrote:

Looks like what you described in the commit message is different from what this patch actually does.

Logic supporting what is desctibed in commit message was implemented in includeInDynsym(),
but I had to change how we handle --noinhibit_exec because few out testcase started to fail.

This patch is to handle the --noinhibit_exec option which wasn't handled before.

Actually we already handled it, but in a local way, it affected undefined symbols policy:

static UnresolvedPolicy getUnresolvedSymbolPolicy(opt::InputArgList &Args) {
  // -noinhibit-exec or -r imply some default values.
  if (Args.hasArg(OPT_noinhibit_exec))
    return UnresolvedPolicy::WarnAll;

I'll try to split --noinhibit exec change to a different patch.

Splitted --noinhibit_exec change to D35793.

ruiu added a comment.Jul 24 2017, 10:15 AM

Please remove the code for -noinhibit-exec from this patch.

grimar updated this revision to Diff 108026.Jul 25 2017, 2:41 AM
  • Removed --noinhibit-exec relative code. It was splitted to D35793.

Notes: without D35793 this fails 2 testcases currently.

ruiu added inline comments.Jul 25 2017, 12:36 PM
ELF/Symbols.cpp
152

Why do you have to move this code?

365–374

Does this order matter?

grimar updated this revision to Diff 108247.Jul 26 2017, 4:26 AM
  • Addressed review comments.
ELF/Symbols.cpp
152

Previously for !Config->Shared we always returned false and it was early exit,
but if we want to handle undefined symbols case we need to move it and we can not exit
early anymore.

For example we should not return that undefined symbol is preemtible if it is hidden
(in that case includeInDynsym() should filter it out, that is required for hidden-vis-shared.s).

There are many other tests with other cases which would reasonable fail if I do not move this piece.

365–374

A kind of. What I tried to do here is preserve original logic, but add code that is equal to:

if (!Config->Shared && canBeExternal() && !body()->symbol()->isWeak())
  return true;

I just separated conditions and tried to exit early when possible. I reduced it by 2 lines in fresh diff.

grimar updated this revision to Diff 108331.Jul 26 2017, 11:23 AM
  • Addressed review comments.
  • Added testcase with protected symbol. In that case we fail to link it with --unresolved-symbols=ignore-all. That behavior is natural and matches both bfd and gold. See no reasons to change it atm.
ruiu accepted this revision.Jul 26 2017, 1:20 PM

LGTM with this comment.

ELF/Symbols.cpp
152–154

Undefined symbols in non-DSOs are usually just an error, so it usually doesn't matter whether we return true or false here. However, if -unresolved-symbols=ignore-all is specified, undefined symbols in executables are automatically exported so that the runtime linker can try to resolve them. In that case, they is preemptible. So, we return true for an undefined symbol in case the option is specified.

This revision is now accepted and ready to land.Jul 26 2017, 1:20 PM
This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Jul 27 2017, 2:35 AM
grimar planned changes to this revision.Jul 27 2017, 2:36 AM
grimar updated this revision to Diff 108628.Jul 28 2017, 5:00 AM
  • Updated to support testcase from r309329 (weak-undef-export.s)
This revision is now accepted and ready to land.Jul 28 2017, 5:00 AM
grimar requested review of this revision.Jul 28 2017, 5:00 AM
grimar edited edge metadata.
ruiu added inline comments.Jul 28 2017, 10:35 AM
test/ELF/no-inhibit-exec.s
11–12

This doesn't seem to make much sense. Did you forgot to add -NEXT?

grimar updated this revision to Diff 108900.Jul 31 2017, 4:48 AM
  • Addressed comment: fixed testcase.
test/ELF/no-inhibit-exec.s
11–12

Thats true, thanks !

grimar updated this revision to Diff 109075.Aug 1 2017, 3:58 AM
  • Rebased, addressed comments.
This revision was automatically updated to reflect the committed changes.