Page MenuHomePhabricator

Add _GLOBAL_OFFSET_TABLE_ symbol to shared libraries
ClosedPublic

Authored by davide on Mar 14 2016, 5:15 PM.

Diff Detail

Event Timeline

davide updated this revision to Diff 50680.Mar 14 2016, 5:15 PM
davide retitled this revision from to Add _GLOBAL_OFFSET_TABLE_ symbol to shared libraries.
davide updated this object.
davide added reviewers: emaste, rafael.
davide added a subscriber: llvm-commits.
emaste edited edge metadata.Mar 14 2016, 6:01 PM

This does fix libcxxabi build / testrun for me.

Note that linking with gnu ld leaves a local absolute symbol for _GLOBAL_OFFSET_TABLE_ in libc++abi.so:

% nm build-nodebug/lib/libc++abi.so | grep 'GLOBAL_OFFSET'      
00000000002592e0 a _GLOBAL_OFFSET_TABLE_

While with this patch lld produces none:

% nm build-lld-selfhost2/lib/libc++abi.so | grep 'GLOBAL_OFFSET'
%

but I don't know that it matters.

ruiu added a subscriber: ruiu.Mar 14 2016, 6:09 PM

Whether we should submit this or not depends on whether it will fix the issue or not, but in case if we do.

ELF/Driver.cpp
345–351

Reorder the conditions in if so that the most important stuff comes first.

// Add default entry symbol. Note that AMDGPU executables has no entry.
if (Config->Entry.empty() && !Config->Shared && !Config->Relocatable &&
    Config->EMachine != EM_AMDGPU)
  Config->Entry = (Config->EMachine == EM_MIPS) ? "__start" : "_start";

Whether we should submit this or not depends on whether it will fix the issue or not

It does fix the issue for me.

(I had one failure in a lld-with-lld test at first, because llvm-dis didn't get built as a dependency. In check-libcxxabi for me two libunwind-related failures and one other unrelated failure remain with this patch.)

rafael edited edge metadata.Mar 15 2016, 8:15 AM
rafael added a subscriber: rafael.

Testcase? :-)

davide updated this revision to Diff 50751.Mar 15 2016, 10:23 AM
davide edited edge metadata.

Address Rui's comments, add a small testcase.

rafael added inline comments.Mar 15 2016, 10:44 AM
ELF/Driver.cpp
351

You don't need the '('

test/ELF/global_offset_table_shared.s
2 ↗(On Diff #50751)

You are not checking that the symbol is not undefined, so this should pass even with your patch reverted.

Rafael LGMT'd this in person. Submitting.

This revision was automatically updated to reflect the committed changes.