This is an archive of the discontinued LLVM Phabricator instance.

[ELF] - Create _DYNAMIC symbol for dynamic output
ClosedPublic

Authored by grimar on Feb 25 2016, 6:57 AM.

Diff Detail

Event Timeline

grimar updated this revision to Diff 49055.Feb 25 2016, 6:57 AM
grimar retitled this revision from to [ELF] - Create _DYNAMIC symbol for dynamic output.
grimar updated this object.
grimar added reviewers: ruiu, rafael.
grimar added subscribers: llvm-commits, grimar.
emaste added a subscriber: emaste.Feb 25 2016, 7:07 AM

I'm seeing strange test failures in my self-hosted lld testrun with this patch applied.

Test process is:

  1. cd 1st stage build dir, configured with FreeBSD host compiler (Clang 3.4.1) and ld.bfd (2.17.50)
  2. ninja lld
  3. ninja check-lld
  4. cd 2nd stage build dir, configured with FreeBSD host compiler (Clang 3.4.1) and ld.lld from (2)
  5. ninja lld
  6. ninja check-lld

(3) passes, (6) fails in strange ways, for example:

********************
FAIL: lld :: COFF/entrylib.ll (33 of 1003)
******************** TEST 'lld :: COFF/entrylib.ll' FAILED ********************
Script:
--
llvm-as -o /tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.obj /tank/emaste/src/llvm/tools/lld/test/COFF/entrylib.ll
rm -f /tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.lib
llvm-ar cru /tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.lib /tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.obj
/tank/emaste/src/llvm/build-lld-selfhost2/./bin/lld-link /out:/tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.exe /entry:main /tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.lib
--
Exit Code: 134

Command Output (stderr):
--
warning: /machine is not specified. x64 is assumed.
Assertion failed: (Str && "StringRef cannot be built from a NULL argument"), function StringRef, file ../include/llvm/ADT/StringRef.h, line 73.
#0 0x0000000000f3fd8e llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/tank/emaste/src/llvm/build-lld-selfhost2/bin/lld+0xf3fd8e)
#1 0x0000000000f40199 PrintStackTraceSignalHandler(void*) (/tank/emaste/src/llvm/build-lld-selfhost2/bin/lld+0xf40199)
#2 0x0000000000f3ca97 llvm::sys::RunSignalHandlers(void) (/tank/emaste/src/llvm/build-lld-selfhost2/bin/lld+0xf3ca97)
#3 0x0000000000f405cc SignalHandler(int) (/tank/emaste/src/llvm/build-lld-selfhost2/bin/lld+0xf405cc)
#4 0x00000008046f695a handle_signal /tank/emaste/src/git-stable-10/lib/libthr/thread/thr_sig.c:249:0
#5 0x00000008046f6158 thr_sighandler /tank/emaste/src/git-stable-10/lib/libthr/thread/thr_sig.c:189:0
/tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.script: line 4: 85785 Abort trap              (core dumped) /tank/emaste/src/llvm/build-lld-selfhost2/./bin/lld-link /out:/tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.exe /entry:main /tank/emaste/src/llvm/build-lld-selfhost2/tools/lld/test/COFF/Output/entrylib.ll.tmp.lib

--
ruiu edited edge metadata.Feb 25 2016, 11:20 AM

The code looks good, but please take a look at the test failure that Ed reported.

I'm seeing strange test failures in my self-hosted lld testrun with this patch applied.

Hello Ed,

I just checked twice that able to self host with this patch. I am using the latest source code available for llvm,clang and lld.

Stage 1 (output to /home/umb/LLVM/build):
At first I configured the build using:
-DCMAKE_BUILD_TYPE=Release -DLLVM_PARALLEL_COMPILE_JOBS=8 -DLLVM_ENABLE_THREADS=true
It shows:

  • The C compiler identification is GNU 4.9.2
  • The CXX compiler identification is GNU 4.9.2

...
ninja lld, ninja check-lld works fine here.

Stage 2 (output to /home/umb/LLVM/selfhost):
Then I am configuring it for self-hosting using the binaries from stage 1:
-DCMAKE_CXX_FLAGS="-std=c++11 -fuse-ld=lld -Wl,--gc-sections" -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_FLAGS=-fuse-ld=lld -DCMAKE_C_COMPILER=/home/umb/LLVM/build/bin/clang -DLLVM_PARALLEL_COMPILE_JOBS=8 -DLLVM_ENABLE_THREADS=true -DCMAKE_CXX_COMPILER=/home/umb/LLVM/build/bin/clang++
It shows:

  • The C compiler identification is Clang 3.9.0
  • The CXX compiler identification is Clang 3.9.0

and uses lld from stage 1 for linkage.
...
I have no troubles with ninja check-lld at this stage either:

umb@ubuntu:~/LLVM/selfhost$ ninja check-lld
[1/1] Running lld test suite
-- Testing: 1105 tests, 8 threads --
Testing: 0 .. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
Testing Time: 4.64s
  Expected Passes    : 1094
  Expected Failures  : 1
  Unsupported Tests  : 10
umb@ubuntu:~/LLVM/selfhost$

I looked at differences of bfd/gold/lld output and found the next:

gold output:

Num:    Value          Size Type    Bind   Vis      Ndx Name
1: 00000000000011e0   176 OBJECT  LOCAL  HIDDEN     5 _DYNAMIC

bfd output:

6: 00000000002001a8     0 OBJECT  LOCAL  DEFAULT    5 _DYNAMIC

lld output:

1: 0000000000001000     0 NOTYPE  LOCAL  HIDDEN     5 _DYNAMIC

It seems nobody remembers why _DYNAMIC was made as object in binutils:
https://sourceware.org/ml/binutils/2002-03/msg00360.html
I am not sure if we should make it STT_OBJECT as there are seems to be no
real reasons for that.

Also there is no reason to make it hidden as bfd does not do that. Making
it default simplies the code a bit.

About why it is no able to selfhost for you - I have no ideas now, any suggestions are welcome.

grimar added a comment.EditedFeb 26 2016, 3:49 AM

At the same time I am not sure should _DYNAMIC have default visibility ? There is no real need in that either as it is used only for linking dynamic linker.
So gold behavior is also fine here.

I tried repeating the test with a new checkout, and it seems the first failure occurs with llvm-tblgen:

FAILED: cd /tank/emaste/src/llvm-lld/build-lld/include/llvm/IR && /tank/emaste/src/llvm-lld/build-lld/bin/llvm-tblgen -gen-attrs -I /tank/emaste/src/llvm-lld/include/llvm/IR -I /tank/emaste/src/llvm-lld/lib/Target -I /tank/emaste/src/llvm-lld/include /tank/emaste/src/llvm-lld/include/llvm/IR/Attributes.td -o /tank/emaste/src/llvm-lld/build-lld/include/llvm/IR/Attributes.inc.tmp
Assertion failed: (hasOptions() && "No options specified!"), function ParseCommandLineOptions, file ../lib/Support/CommandLine.cpp, line 816.
ninja: build stopped: subcommand failed.

A FreeBSD/llvm-tblgen issue was reported in PR 25539, but I have not been able to reproduce previously (prior to trying this patch).

I now see what change this patch introduces:

@@ -1,5 +1,5 @@
 
-build-unpatched-lld/bin/llvm-tblgen:     file format elf64-x86-64-freebsd
+build-lld/bin/llvm-tblgen:     file format elf64-x86-64-freebsd
 
 Disassembly of section .text:
 
@@ -79,7 +79,7 @@
 
 #ifdef GCRT
        atexit(_mcleanup);
-   9c05b:      b8 00 00 00 00          mov    $0x0,%eax
+   9c05b:      b8 10 01 54 00          mov    $0x540110,%eax
    9c060:      48 85 c0                test   %rax,%rax
    9c063:      74 0a                   je     9c06f <_start+0x6f>
        monstartup(&eprol, &etext);
@@ -95,7 +95,7 @@
        size_t array_size, n;
 
        if (&_DYNAMIC != NULL)
-   9c074:      b8 00 00 00 00          mov    $0x0,%eax
+   9c074:      b8 10 01 54 00          mov    $0x540110,%eax
    9c079:      48 85 c0                test   %rax,%rax
    9c07c:      0f 85 df 00 00 00       jne    9c161 <_start+0x161>
                return;

That is, the patch fixes _DYNAMIC and it's now being set properly. So there's probably not a bug in this patch, but rather by fixing this we expose a different bug that affects FreeBSD.

That is, the patch fixes _DYNAMIC and it's now being set properly. So there's probably not a bug in this patch, but rather by fixing this we expose a different bug that affects FreeBSD.

So I am not sure what will be correct then: can we commit this even if it is introduces such regression issues on BSD ? Or we need to fix them first ?
(Selfhosting under Ubuntu works fine with this.)

emaste added a subscriber: davide.Feb 28 2016, 8:17 PM

can we commit this even if it is introduces such regression issues on BSD

I think we should; I think it must have somehow been working by accident before (without _DYNAMIC). We just need to find and fix the real bug that this now exposes. @davide, what do you think?

rafael edited edge metadata.Feb 29 2016, 7:03 AM
rafael added a subscriber: rafael.

I will try to reproduce the issue.

It should be safe to land this now.

rafael accepted this revision.Mar 1 2016, 5:59 AM
rafael edited edge metadata.

lgtm

This revision is now accepted and ready to land.Mar 1 2016, 5:59 AM
emaste added a comment.Mar 1 2016, 6:57 AM

Thanks @rafael, looks good here now.

This revision was automatically updated to reflect the committed changes.