Page MenuHomePhabricator

Fix regression introduced in r205566.

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



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


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.

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.

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.
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() {
  return 0;

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

Probably because each time we scan 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
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.


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).