| Message ID | 20130221132136.GZ1215@tucnak.zalov.cz |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 21, 2013 at 02:21:36PM +0100, Jakub Jelinek wrote: > Here is a different fix, so libasan.so will not have .preinit_array, but > libasan.a will have it. Ideally, that hunk should go into a separate > source file (asan_preinit.cc ?), be just compiled into an object file, > rather than shared library and the compiler driver should include it > explicitly in the link. BTW, if you move it into asan_preinit.cc (or whatever other name) separate file, as the Makefiles aren't shared, it is fine if for clang you choose to put it into clang libasan.a, and gcc can do something different. Jakub
On Thu, Feb 21, 2013 at 5:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, Feb 21, 2013 at 05:15:51PM +0400, Konstantin Serebryany wrote: >> This commit breaks the build if the BFD linker is used (I have gold on >> my box, so I missed it). >> >> Short repro: >> % cat preinit.cc >> void foo() {} >> __attribute__((section(".preinit_array"))) void (*xxx)(void) = foo; >> % g++ preinit.cc -shared # gold >> % sudo apt-get remove binutils-gold >> ... >> % g++ preinit.cc -shared # bfd >> /usr/bin/ld: /tmp/cc4GVflE.o: .preinit_array section is not allowed in DSO >> /usr/bin/ld: failed to set dynamic section sizes: Nonrepresentable >> section on output >> collect2: ld returned 1 exit status >> % >> >> Can we stop building the asan-rt as DSO and leave only the static >> variant (as in clang)? > > No, IMNSHO it is desirable to support also that. It may cause I more trouble (we've seen a couple of bugs already) then do good. Anyway, we can get rid of this later. > > Here is a different fix, so libasan.so will not have .preinit_array, but > libasan.a will have it. Ideally, that hunk should go into a separate > source file (asan_preinit.cc ?), be just compiled into an object file, > rather than shared library and the compiler driver should include it > explicitly in the link. > > The used attribute is there because, as it isn't (or shouldn't) be exported > out of the library, if libasan was built with LTO, it could very well be > optimized away. > > 2013-02-21 Jakub Jelinek <jakub@redhat.com> > > * asan/asan_rtl.cc (__asan_preinit): Don't add if PIC macro is > defined. Add used attribute. > > --- libsanitizer/asan/asan_rtl.cc.jj 2013-02-21 14:10:41.000000000 +0100 > +++ libsanitizer/asan/asan_rtl.cc 2013-02-21 14:16:28.985547506 +0100 > @@ -520,11 +520,11 @@ void __asan_init() { > } > } > > -#if ASAN_USE_PREINIT_ARRAY > +#if ASAN_USE_PREINIT_ARRAY && !defined (PIC) > // On Linux, we force __asan_init to be called before anyone else > // by placing it into .preinit_array section. > // FIXME: do we have anything like this on Mac? > - __attribute__((section(".preinit_array"))) > + __attribute__((section(".preinit_array"), used)) > void (*__asan_preinit)(void) =__asan_init; > #elif defined(_WIN32) && defined(_DLL) > // On Windows, when using dynamic CRT (/MD), we can put a pointer Thanks! May I ask you to commit this to gcc (I have to run away now)? I'll put this into upstream (maybe with asan_preinit.cc as you suggest) tomorrow. --kcc > > > Jakub
On Thu, Feb 21, 2013 at 05:26:30PM +0400, Konstantin Serebryany wrote:
> May I ask you to commit this to gcc (I have to run away now)?
I'll get it through bootstrap/regtest cycle first, commit if that succeeds.
Jakub
Jakub, thanks again for cleaning up my mess. Here is a question regarding your fix: > -#if ASAN_USE_PREINIT_ARRAY > +#if ASAN_USE_PREINIT_ARRAY && !defined (PIC) The PIC macro is an artifact of the GCC build system and is not directly related the the -fPIC flag? As I can see, in the gcc build we compile all of asan sources twice: w/ and w/o "-fPIC -DPIC". If I move the preinit_array to a separate file (asan_preinit.cc), will we need to have two builds? I think we can just build all objects once with -fPIC and then not link asan_preinit.o into libasan.so In clang we build the static libasan with -fPIC, it doesn't hurt. Anyway, I've just committed http://llvm.org/viewvc/llvm-project?rev=175871&view=rev with asan_preinit.cc --kcc On Thu, Feb 21, 2013 at 5:26 PM, Konstantin Serebryany <konstantin.s.serebryany@gmail.com> wrote: > On Thu, Feb 21, 2013 at 5:21 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> On Thu, Feb 21, 2013 at 05:15:51PM +0400, Konstantin Serebryany wrote: >>> This commit breaks the build if the BFD linker is used (I have gold on >>> my box, so I missed it). >>> >>> Short repro: >>> % cat preinit.cc >>> void foo() {} >>> __attribute__((section(".preinit_array"))) void (*xxx)(void) = foo; >>> % g++ preinit.cc -shared # gold >>> % sudo apt-get remove binutils-gold >>> ... >>> % g++ preinit.cc -shared # bfd >>> /usr/bin/ld: /tmp/cc4GVflE.o: .preinit_array section is not allowed in DSO >>> /usr/bin/ld: failed to set dynamic section sizes: Nonrepresentable >>> section on output >>> collect2: ld returned 1 exit status >>> % >>> >>> Can we stop building the asan-rt as DSO and leave only the static >>> variant (as in clang)? >> >> No, IMNSHO it is desirable to support also that. > > It may cause I more trouble (we've seen a couple of bugs already) then do good. > Anyway, we can get rid of this later. > >> >> Here is a different fix, so libasan.so will not have .preinit_array, but >> libasan.a will have it. Ideally, that hunk should go into a separate >> source file (asan_preinit.cc ?), be just compiled into an object file, >> rather than shared library and the compiler driver should include it >> explicitly in the link. >> >> The used attribute is there because, as it isn't (or shouldn't) be exported >> out of the library, if libasan was built with LTO, it could very well be >> optimized away. >> >> 2013-02-21 Jakub Jelinek <jakub@redhat.com> >> >> * asan/asan_rtl.cc (__asan_preinit): Don't add if PIC macro is >> defined. Add used attribute. >> >> --- libsanitizer/asan/asan_rtl.cc.jj 2013-02-21 14:10:41.000000000 +0100 >> +++ libsanitizer/asan/asan_rtl.cc 2013-02-21 14:16:28.985547506 +0100 >> @@ -520,11 +520,11 @@ void __asan_init() { >> } >> } >> >> -#if ASAN_USE_PREINIT_ARRAY >> +#if ASAN_USE_PREINIT_ARRAY && !defined (PIC) >> // On Linux, we force __asan_init to be called before anyone else >> // by placing it into .preinit_array section. >> // FIXME: do we have anything like this on Mac? >> - __attribute__((section(".preinit_array"))) >> + __attribute__((section(".preinit_array"), used)) >> void (*__asan_preinit)(void) =__asan_init; >> #elif defined(_WIN32) && defined(_DLL) >> // On Windows, when using dynamic CRT (/MD), we can put a pointer > > Thanks! > May I ask you to commit this to gcc (I have to run away now)? > I'll put this into upstream (maybe with asan_preinit.cc as you > suggest) tomorrow. > > --kcc > >> >> >> Jakub
On Fri, Feb 22, 2013 at 11:53:39AM +0400, Konstantin Serebryany wrote: > Jakub, thanks again for cleaning up my mess. > > Here is a question regarding your fix: > > -#if ASAN_USE_PREINIT_ARRAY > > +#if ASAN_USE_PREINIT_ARRAY && !defined (PIC) > > The PIC macro is an artifact of the GCC build system and is not > directly related the the -fPIC flag? The PIC macro is an artifact of using libtool, which takes care of building everything twice, once with -fPIC -DPIC, once without. If we set up our Makefiles so that asan_preinit.cc isn't build as part of libasan.la (the libtool library), but just as an object, then the PIC guard isn't needed. Jakub
--- libsanitizer/asan/asan_rtl.cc.jj 2013-02-21 14:10:41.000000000 +0100 +++ libsanitizer/asan/asan_rtl.cc 2013-02-21 14:16:28.985547506 +0100 @@ -520,11 +520,11 @@ void __asan_init() { } } -#if ASAN_USE_PREINIT_ARRAY +#if ASAN_USE_PREINIT_ARRAY && !defined (PIC) // On Linux, we force __asan_init to be called before anyone else // by placing it into .preinit_array section. // FIXME: do we have anything like this on Mac? - __attribute__((section(".preinit_array"))) + __attribute__((section(".preinit_array"), used)) void (*__asan_preinit)(void) =__asan_init; #elif defined(_WIN32) && defined(_DLL) // On Windows, when using dynamic CRT (/MD), we can put a pointer