Page MenuHomePhabricator

Fix regression introduced in r205566.
ClosedPublic

Authored by ruiu on May 12 2014, 4:34 PM.

Details

Summary

In r205566, I made a change to Resolver so that Resolver revisit
only archive files in --start-group and --end-group pair. That's
not correct, as it also has to revisit DSO files.

This patch is to fix the issue. I once tried to just revisit DSO
files in a group, but it made Resolver to fall in an infinite loop.
I needed to fix the bug that Resolver loops over a group even if
no undefined symbols are newly created in the group.

Added a test to demonstrate the fix. I confirmed that it succeeded
before r205566, failed after r205566, and is ok with this patch.

Diff Detail

Repository
rL LLVM

Event Timeline

ruiu updated this revision to Diff 9329.May 12 2014, 4:34 PM
ruiu retitled this revision from to Fix regression introduced in r205566..
ruiu updated this object.
ruiu edited the test plan for this revision. (Show Details)
ruiu added a reviewer: atanasyan.
ruiu added a subscriber: Unknown Object (MLST).
atanasyan accepted this revision.May 13 2014, 12:12 AM
atanasyan edited edge metadata.

LGTM with some minor notes.

include/lld/Driver/GnuLdInputGraph.h
81 ↗(On Diff #9329)

This phrase only if the node is an archive library does not correspond to the current code. Now we reset the next file index for shared libraries too.

lib/Core/Resolver.cpp
65 ↗(On Diff #9329)

It is not good to always print undefAdded: ... in debug mode. Let's remove this debug output or surround it by the DEBUG_WITH_TYPE macros.

182 ↗(On Diff #9329)

The comment Returns true is inconsistent. The Resolver::doDefinedAtom() method has void return type.

This revision is now accepted and ready to land.May 13 2014, 12:12 AM
shankarke added inline comments.
lib/Core/Resolver.cpp
66–68 ↗(On Diff #9329)

it looks like notifyProgress is called only if there is a undefined atom added, can we do it in doUndefinedAtom ? This way there would be no need to change much.

atanasyan requested changes to this revision.May 13 2014, 7:43 AM
atanasyan edited edge metadata.

It looks like there is a bug in this patch. The lld linker hangs when build a.out executable in the following test case.

% cat main.c
int foo();

int main() {
  return foo();
}

% cat foo.c
int bar();

int foo() {
  return bar();
}

% cat bar.c
void func();

int bar() {
  return 0;
}

int bar1() {
  func();
  return 0;
}

% gcc -c main.c foo.c
% gcc -c -fPIC bar.c
% lld -flavor gnu -target x86_64 -shared --noinhibit-exec -o libbar.so bar.o
% lld -flavor gnu -target x86_64 -e main -o a.out main.o --start-group libbar.so foo.o --end-group

Probably because each time we scan libbar.so we find the undefined symbol func and call the notifyProgress() and go to the next scan.

This revision now requires changes to proceed.May 13 2014, 7:43 AM
ruiu updated this revision to Diff 9358.May 13 2014, 12:14 PM
ruiu edited edge metadata.
  • fix the infinite loop bug that Simon pointed out
ruiu added a comment.May 13 2014, 12:18 PM

The infinite loop bug was not caused by the notifyProgress call, but because we accidentally created a loop in _replacedAtoms. When the undefined atom "func" is added to the symbol table second time, it's replacement atom (in _replacedAtoms in SymbolTable.cpp) is set to itself. It's easily fixable. PTAL.

ruiu added inline comments.May 13 2014, 12:20 PM
lib/Core/Resolver.cpp
66–68 ↗(On Diff #9329)

I don't want to call notifyProgress in doUndefinedAtom. If we do, notifyProgress() would be called for each undefined atom. That's not wrong but would be slow. I want call it only once.

atanasyan accepted this revision.May 14 2014, 2:29 AM
atanasyan edited edge metadata.

LGTM

This revision is now accepted and ready to land.May 14 2014, 2:29 AM
ruiu closed this revision.May 19 2014, 7:24 AM
ruiu updated this revision to Diff 9561.

Closed by commit rL208797 (authored by @ruiu).