Opened 3 years ago

Closed 7 months ago

Last modified 8 days ago

#5435 closed bug (fixed)

GHCi linker should run constructors for linked libraries

Reported by: pumpkin Owned by:
Priority: normal Milestone: 7.8.1
Component: Compiler Version: 7.2.1
Keywords: Cc: howard_b_golden@…, pho@…
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Difficulty: Unknown
Test Case: Blocked By:
Blocking: #7746, #8199 Related Tickets: #3658

Description (last modified by ezyang)

As far as I can tell from my experimentation, any library that contains a function with __attribute__((constructor)) on it won't have its constructor(s) run when loaded, or its destructors run when unloaded. This violates assumptions that some libraries make.

Attachments (5)

0001-Implement-.init-.init_array-support-for-ELF.patch (4.2 KB) - added by ezyang 7 months ago.
ELF support
0002-Implement-initializer-support-for-PEi386.patch (4.2 KB) - added by ezyang 7 months ago.
PEi386 support
0001-Implement-__mod_init_func-for-Mach-O.-Finishes-suppo.patch (5.0 KB) - added by ezyang 7 months ago.
T5435_c_v.o (2.0 KB) - added by simonmar 7 months ago.
T5435_c_dyn.o (2.1 KB) - added by simonmar 7 months ago.

Download all attachments as: .zip

Change History (46)

comment:1 Changed 3 years ago by simonmar

see also #5386

Note that if you put the constructor in a shared library (.so or .dll) then the constructor will be run when the library is loaded, because GHCi invokes the system linker for these.

comment:2 Changed 3 years ago by simonmar

  • Milestone set to 7.4.1
  • Priority changed from normal to high

comment:3 Changed 3 years ago by igloo

See also #3333.

comment:4 Changed 3 years ago by hgolden

  • Cc howard_b_golden@… added

comment:5 Changed 3 years ago by PHO

  • Cc pho@… added

comment:6 Changed 2 years ago by igloo

  • Milestone changed from 7.4.1 to 7.4.2

comment:7 Changed 2 years ago by simonpj

  • Difficulty set to Unknown
  • Milestone changed from 7.4.2 to 7.6.1

There is real work here, to fix our DIY linker to run constructors when we dynamically load static libraries in GHCi. Our ultimate goal is to do dynamic linking by default, so then we wouldn't have any static libraries, and this whole issue would go away.

So we're punting to 7.6.

Simon

comment:8 Changed 20 months ago by simonmar

Would be fixed by #3658.

comment:9 Changed 19 months ago by igloo

  • Blocked By 3658 added

comment:10 Changed 19 months ago by igloo

  • Milestone changed from 7.6.1 to 7.6.2

comment:11 Changed 18 months ago by igloo

  • Milestone changed from 7.6.2 to 7.8.1
  • Priority changed from high to normal

We plan to fix this by switching to dynamic linking

comment:12 Changed 7 months ago by ezyang

  • Description modified (diff)

Note that GHC itself uses constructor (but not destructor) for profiling and foreign exports, so at least the constructor half of this problem is important. I'm attaching implementations for ELF and PEi386; Mach-O coming when I get a Mac to do testing on.

Changed 7 months ago by ezyang

ELF support

Changed 7 months ago by ezyang

PEi386 support

comment:13 Changed 7 months ago by ezyang

These patches could probably be improved by adding .ctors support for ELF, and .init/.init_array for PEi386, but I don't know how to make my GCC on the respective platforms emit those types of sections to test. They might not even be accepted by the system linker; I have no way of telling. I also have pretty reasonable test (essentially a cleaned up version of #8237) which should get added to the test suite.

comment:14 Changed 7 months ago by ezyang

  • Blocking 8199 added

comment:15 Changed 7 months ago by ezyang

  • Blocking 7746 added

comment:16 Changed 7 months ago by simonmar

@ezyang: compile some C++ code with a static constructor?

comment:17 Changed 7 months ago by ezyang

At least my version of GCC still outputs init_array sections. Honestly, I'm pretty surprised MingW GCC still emits constr.

comment:18 Changed 7 months ago by simonmar

@ezyang: patches look good to me, push away. It'll be great to have this fixed, thanks!

comment:19 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In 1935c70f3d7ff77b4990040643d0f282e635b150/testsuite:

Tests for #5435 (init/init_array/constr handling by linker)

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:20 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In e0885adca7cfdcdc2e2ca2bddace122408f76108/ghc:

Implement __mod_init_func for Mach-O. Finishes support for init in #5435.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:21 Changed 7 months ago by ezyang

Also:

commit 30bf3ed5aae5603116e45a46923b2cfd67757461
Author: Edward Z. Yang <ezyang@mit.edu>
Date:   Thu Sep 5 23:37:37 2013 -0700

    Implement .ctor support for PEi386.
    
    Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

commit 291ec132de6a406b3e70ce4b102907b845c7a60b
Author: Edward Z. Yang <ezyang@mit.edu>
Date:   Fri Aug 30 19:18:28 2013 -0700

    Implement .init/.init_array support for ELF.
    
    Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:22 Changed 7 months ago by simonmar

I'm seeing failures in T5435_v and T5435_dyn here:

=====> T5435_v(normal) 2962 of 3788 [0, 0, 1]
cd ./rts && $MAKE -s --no-print-directory T5435_v    </dev/null >T5435_v.run.stdout 2>T5435_v.run.stderr
Actual stdout output differs from expected:
--- ./rts/T5435_v.stdout        2013-09-14 19:51:59.426916358 +0100
+++ ./rts/T5435_v.run.stdout    2013-09-14 20:54:07.282839857 +0100
@@ -1,3 +1 @@
-initializer1 run
-initializer2 run
 success
*** unexpected failure for T5435_v(normal)

Also you need to make these tests safe to run in parallel, I got this failure with THREADS=2:

Stderr:
/usr/bin/ld: T5435.o: invalid string offset 4519 >= 1388 for section `.strtab'
/usr/bin/ld: final link failed: Nonrepresentable section on output
collect2: ld returned 1 exit status
make[3]: *** [T5435_v] Error 1

comment:23 Changed 7 months ago by ezyang

  • Status changed from new to infoneeded

I fixed the thread safety issue, but I'll need more help reproducing the error. What happens if you run make T5435_v; can you post all the output?

comment:24 Changed 7 months ago by simonmar

$ make T5435_v 
rm -f T5435_c_v.o T5435_v
'/playpen/validate/inplace/bin/ghc-stage2' -fforce-recomp -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts  -v0 -c T5435.c -o T5435_c_v.o
'/playpen/validate/inplace/bin/ghc-stage2' -fforce-recomp -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts  -v0 T5435.hs -o T5435_v
./T5435_v T5435_c_v.o
success
$ make T5435_dyn
rm -f T5435_c_dyn.o T5435_dyn
'/playpen/validate/inplace/bin/ghc-stage2' -fforce-recomp -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts  -v0 -dynamic -fPIC -c T5435.c -o T5435_c_dyn.o
'/playpen/validate/inplace/bin/ghc-stage2' -fforce-recomp -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts  -v0 -dynamic -fPIC T5435.hs -o T5435_dyn
./T5435_dyn T5435_c_dyn.o
success

comment:25 Changed 7 months ago by ezyang

See, what I don't understand is why the dynamic case isn't working; it shouldn't be exercising any of our code. OK, what happens when you run this set of commands?

ezyang@javelin:~/Dev/ghc-build-july/testsuite/tests/rts$ objdump -h T5435_c_dyn.o

T5435_c_dyn.o:     file format elf64-x86-64

Sections:
Idx Name          Size      VMA               LMA               File off  Algn
  0 .text         00000048  0000000000000000  0000000000000000  00000040  2**2
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
  1 .data         00000000  0000000000000000  0000000000000000  00000088  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  2 .bss          00000000  0000000000000000  0000000000000000  00000088  2**2
                  ALLOC
  3 .rodata       00000022  0000000000000000  0000000000000000  00000088  2**0
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  4 .init_array   00000010  0000000000000000  0000000000000000  000000b0  2**3
                  CONTENTS, ALLOC, LOAD, RELOC, DATA
  5 .comment      0000002b  0000000000000000  0000000000000000  000000c0  2**0
                  CONTENTS, READONLY
  6 .note.GNU-stack 00000000  0000000000000000  0000000000000000  000000eb  2**0
                  CONTENTS, READONLY
  7 .eh_frame     00000058  0000000000000000  0000000000000000  000000f0  2**3
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
ezyang@javelin:~/Dev/ghc-build-july/testsuite/tests/rts$ objdump -s -j .init_array T5435_c_dyn.o

T5435_c_dyn.o:     file format elf64-x86-64

Contents of section .init_array:
 0000 00000000 00000000 00000000 00000000  ................
ezyang@javelin:~/Dev/ghc-build-july/testsuite/tests/rts$ objdump -r -j .init_array T5435_c_dyn.o

T5435_c_dyn.o:     file format elf64-x86-64

RELOCATION RECORDS FOR [.init_array]:
OFFSET           TYPE              VALUE 
0000000000000000 R_X86_64_64       .text
0000000000000008 R_X86_64_64       .text+0x0000000000000024



comment:26 Changed 7 months ago by simonmar

I have .ctors:

> objdump --all-headers T5435_c_dyn.o

T5435_c_dyn.o:     file format elf64-x86-64
T5435_c_dyn.o
architecture: i386:x86-64, flags 0x00000011:
HAS_RELOC, HAS_SYMS
start address 0x0000000000000000

Sections:
Idx Name          Size      VMA               LMA               File off  Algn
  0 .text         00000048  0000000000000000  0000000000000000  00000040  2**2
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, CODE
  1 .data         00000000  0000000000000000  0000000000000000  00000088  2**2
                  CONTENTS, ALLOC, LOAD, DATA
  2 .bss          00000000  0000000000000000  0000000000000000  00000088  2**2
                  ALLOC
  3 .rodata       00000022  0000000000000000  0000000000000000  00000088  2**0
                  CONTENTS, ALLOC, LOAD, READONLY, DATA
  4 .ctors        00000010  0000000000000000  0000000000000000  000000b0  2**3
                  CONTENTS, ALLOC, LOAD, RELOC, DATA
  5 .comment      0000002b  0000000000000000  0000000000000000  000000c0  2**0
                  CONTENTS, READONLY
  6 .note.GNU-stack 00000000  0000000000000000  0000000000000000  000000eb  2**0
                  CONTENTS, READONLY
  7 .eh_frame     00000058  0000000000000000  0000000000000000  000000f0  2**3
                  CONTENTS, ALLOC, LOAD, RELOC, READONLY, DATA
SYMBOL TABLE:
0000000000000000 l    df *ABS*  0000000000000000 T5435.c
0000000000000000 l    d  .text  0000000000000000 .text
0000000000000000 l    d  .data  0000000000000000 .data
0000000000000000 l    d  .bss   0000000000000000 .bss
0000000000000000 l    d  .rodata        0000000000000000 .rodata
0000000000000000 l     F .text  0000000000000024 initializer1
0000000000000000 l    d  .ctors 0000000000000000 .ctors
0000000000000024 l     F .text  0000000000000024 initializer2
0000000000000000 l    d  .note.GNU-stack        0000000000000000 .note.GNU-stack
0000000000000000 l    d  .eh_frame      0000000000000000 .eh_frame
0000000000000000 l    d  .comment       0000000000000000 .comment
0000000000000000         *UND*  0000000000000000 _GLOBAL_OFFSET_TABLE_
0000000000000000         *UND*  0000000000000000 puts
0000000000000000         *UND*  0000000000000000 stdout
0000000000000000         *UND*  0000000000000000 fflush


RELOCATION RECORDS FOR [.text]:
OFFSET           TYPE              VALUE 
0000000000000007 R_X86_64_PC32     .rodata-0x0000000000000004
000000000000000c R_X86_64_PLT32    puts-0x0000000000000004
0000000000000013 R_X86_64_GOTPCREL  stdout-0x0000000000000004
000000000000001e R_X86_64_PLT32    fflush-0x0000000000000004
000000000000002b R_X86_64_PC32     .rodata+0x000000000000000d
0000000000000030 R_X86_64_PLT32    puts-0x0000000000000004
0000000000000037 R_X86_64_GOTPCREL  stdout-0x0000000000000004
0000000000000042 R_X86_64_PLT32    fflush-0x0000000000000004


RELOCATION RECORDS FOR [.ctors]:
OFFSET           TYPE              VALUE 
0000000000000000 R_X86_64_64       .text
0000000000000008 R_X86_64_64       .text+0x0000000000000024


RELOCATION RECORDS FOR [.eh_frame]:
OFFSET           TYPE              VALUE 
0000000000000020 R_X86_64_PC32     .text
0000000000000040 R_X86_64_PC32     .text+0x0000000000000024

comment:27 Changed 7 months ago by ezyang

Oh, well, that would explain the static case; I never implemented ctors because I wasn't sure if anyone had a version of GCC which still emitted them. Can you attach the object file to this bug report so I can figure out what the correct flags to test for are?

Actually, I know what's going on with the dynamic case too.

Changed 7 months ago by simonmar

Changed 7 months ago by simonmar

comment:28 Changed 7 months ago by simonmar

Files attached, thanks for looking into this.

comment:29 Changed 7 months ago by ezyang

While fixing this, I found a few more bugs, which are going to be a bit annoying to fix:

  • The current dynamic test is really a test for loading a static library that happens to be PIC'd. We need to explicitly use addDLL to load a shared library. In fact, we can run a statically linked executable and still load a DLL.
  • But we can't reuse this test for Windows, because on Linux/Mac? OS X, addDLL requires a file extension, while on Windows, we must omit it. So we need two tests here.
  • Also, on Windows, when I pass -dynamic when building the C DLL causes errors like C:/Users/ezyang/Dev/ghc-init/inplace/mingw/bin/ld.exe: cannot find -lHSbase-4.7.0.0-ghc7.7.20130905; no problem, just omit the flag. But this is harmless on Linux, so maybe there is a bug here.
  • With the dynamic test working, I found that actually, ctors is reversed, so I need to revert the behavior back to the reversed version. (I had it that way originally, but flipped it due to the buggy test case)
  • Then I found out that, actually, the order MingW GCC compiles __attribute__(constructor) to run is reverse of how it compiles it on Linux. I think this might be a bug, but I couldn't find anywhere where GCC said that they were going to run in any specific order. But this means to keep the test cases consistent, we need to manually lay out the ctors section ourself. No problem, so we need two tests: one for a C compilation, and one for a assembly compilation (with all of the various ways an initializer can be spelled). Obviously this doesn't apply to Mach-O.
  • Then I wondered, "Well, what happens if there are both ctors and init_array and init; is there a prescribed order these should run in, or is it just the order they show up in the sections?" Well, the dynamic linker will guidance here.
  • I also tried to induce ordering with priority, but I found out that GCC compiles constructor priority by tacking on a number to the name of the ctors section; I didn't write support for that but maybe I should.
  • And then Anders pointed out to me that in C++, the order of constructors in the compilation unit is guaranteed, and it seems unlikely GCC would use priority to manage that, so that would be an easy way to find out if MingW GCC was buggy; but if it is buggy, what can we do?

comment:30 Changed 7 months ago by ezyang

Just checked: MingW g++ only generates a single entry in ctors per compilation unit, so they'll get it right.

comment:31 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In 85a9e2468dc74b9e5ccde0dd61be86219fd323a2/ghc:

Run ctors initializers backwards, see #5435.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:32 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In 7520b9775cf57b61208f68a7233449763007ee2f/testsuite:

T5435 test improvements, see #5435 for details.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:33 Changed 7 months ago by ezyang

OK, pushed some partial fixes, next step:

  1. Implement assembly-based test for different section types
  1. Implement ctors support on Linux
  1. Implement init/init_array support on Windows

Skipping priority sections for now.

comment:34 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In 3c313be6aeacda3c704c114e3477083bad7432a1/testsuite:

New and improved tests for #5435. Linux only at the moment.

Added a new test Makefile variable $(dllext), which is
instantiated to .dll or .so or .dylib depending on your platform.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:35 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In b3c5baa2663b183a9b77a8daf10303a36b99bcc7/testsuite:

Implement assembly test for #5435 in Windows.

Fixes HostOS bug, where the define was not being set.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:36 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In 28e921b2954ef0afe08c9f96d779fb845e513421/testsuite:

Finish up asm test #5435 for Mac OS X

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:37 Changed 7 months ago by ezyang

  • Blocked By 3658 removed
  • Resolution set to fixed
  • Status changed from infoneeded to closed

OK, all done. (PS: Windows system linker does NOT honor init_array sections, so I didn't implement it.)

To repeat: file a new ticket for destructors if you need 'em.

comment:38 Changed 7 months ago by simonmar

Great work Edward!

comment:39 Changed 7 months ago by Edward Z. Yang <ezyang@…>

In 4b22a8b57b1791d0a596869040eee8856fc9bb7d/testsuite:

Add missing Windows stdout/stderr output for #5435.

Signed-off-by: Edward Z. Yang <ezyang@mit.edu>

comment:40 Changed 7 months ago by Simon Marlow <marlowsd@…>

In 1908195261ccf16b0f3d2e77ebd5cd40c9e29cbc/ghc:

Fix linker_unload now that we are running constructors in the linker (#8291)

See also #5435.

Now we have to remember the the StablePtrs that get created by the
module initializer so that we can free them again in unloadObj().

comment:41 Changed 8 days ago by Edward Z. Yang <ezyang@…>

In f8e12e2b396e0c475e1403ab8ac3fc4d63c1681e/ghc:

Fix #5435, adding new test config check_stdout.

check_stdout(f) allows you to override the test framework's
diff based output checking with another mechanism.  f is
a function which takes two arguments: the first is the
filename containing the observed stdout, the second is the
normaliser that would have been applied (in case you want
to read, normalise, and then do something.)

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
Note: See TracTickets for help on using tickets.