Patchwork r196201 - in /trunk: gcc/ChangeLog gcc/config/i...

login
register
mail settings
Submitter Jakub Jelinek
Date Feb. 21, 2013, 1:21 p.m.
Message ID <20130221132136.GZ1215@tucnak.zalov.cz>
Download mbox | patch
Permalink /patch/222287/
State New
Headers show

Comments

Jakub Jelinek - Feb. 21, 2013, 1:21 p.m.
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.

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.



	Jakub
Jakub Jelinek - Feb. 21, 2013, 1:25 p.m.
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
Konstantin Serebryany - Feb. 21, 2013, 1:26 p.m.
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
Jakub Jelinek - Feb. 21, 2013, 1:27 p.m.
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
Konstantin Serebryany - Feb. 22, 2013, 7:53 a.m.
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
Jakub Jelinek - Feb. 22, 2013, 8:50 a.m.
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

Patch

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