Patchwork fix PR sanitizer/55617

login
register
mail settings
Submitter Jack Howarth
Date Feb. 3, 2013, 4:57 a.m.
Message ID <20130203045748.GA23486@bromo.med.uc.edu>
Download mbox | patch
Permalink /patch/217727/
State New
Headers show

Comments

Jack Howarth - Feb. 3, 2013, 4:57 a.m.
Currently darwin is unable to utilize libasan with constructors due to the lack of
constructor priority support on that target. The asan_finish_file routine inserts an
essential __asan_init into the array of constructors (via the __mod_init_func section).
However the insertion occurs at the end, and due to the lack of priority support for
constructors, these are executed from the start of the array of constructors on program
startup. This causes code any instrumented code that executes before the __asan_init
call to crash. 
   Since darwin sets...

#undef SUPPORTS_INIT_PRIORITY
#define SUPPORTS_INIT_PRIORITY 0

in gcc/config/darwin.h, all constructors are automatically set to 

#define DEFAULT_INIT_PRIORITY 65535

in gcc/collect2.c. Any code that attempts to set the constructor/destructor priority
on darwin results in a compile time error of "constructor priorities are not supported".
So asan alone should be unique in emitting priorities different from 65535 on darwin.
The attached patch uses a va_gc vector of constructor symbol/priority records to queue
this data as it is generated in calls to machopic_asm_out_constructor. Any instances of
the static constructor with priority 99 emitted by asan are inserted safely in the front
of the vector queue which retains the original order of the remaining constructors in
the queue. The contents of the vector queue are later processed in a new finalize_ctors
routine called from darwin_file_end if necessary. The patch also adds a g++.dg/asan/pr55617.C
test case which is targeted to i?86-*-darwin* and x86_64-*-darwin*.
    The patch reduces the failures observed when running....

make -k check-g++ RUNTESTFLAGS="--target_board=unix'{-fsanitize=address}'"

from 323 to only 85 on darwin (similar to the results on linux). The cov.C testcase also
fails on gcc trunk with -fsanitize=address when recrafted into a dynamic shared library
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55617#c28. This patch eliminates those
crashes. This problem doesn't extend to when the shared library or module is dlopen'd
(which works in stock gcc trunk and with this patch as well).
    The patch has been bootstrap and regression tested on x86_64-apple-darwin12.
Okay for gcc trunk?
         Jack
ps Unfortunately the flag_sort variable is unavailable inside of machopic_asm_out_constructor 
so we have to unconditionally test for priority == 99.
/gcc

2013-02-03  Alexander Potapenko <glider@google.com>
            Jack Howarth  <howarth@bromo.med.uc.edu>

	PR sanitizer/55617
	* config/darwin.c (machopic_asm_out_constructor): Use vector to
	queue constructors while inserting asan static constructors at front.
        (finalize_ctors): New routine to output queued constructors.
	(darwin_file_end): Use finalize_ctors.

/gcc/testsuite

2013-02-03  Alexander Potapenko <glider@google.com>
            Jack Howarth  <howarth@bromo.med.uc.edu>

	PR sanitizer/55617
	* g++.dg/asan/pr55617.C: New test.
Richard Guenther - Feb. 4, 2013, 9:22 a.m.
On Sun, Feb 3, 2013 at 5:57 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
>    Currently darwin is unable to utilize libasan with constructors due to the lack of
> constructor priority support on that target. The asan_finish_file routine inserts an
> essential __asan_init into the array of constructors (via the __mod_init_func section).
> However the insertion occurs at the end, and due to the lack of priority support for
> constructors, these are executed from the start of the array of constructors on program
> startup. This causes code any instrumented code that executes before the __asan_init
> call to crash.
>    Since darwin sets...
>
> #undef SUPPORTS_INIT_PRIORITY
> #define SUPPORTS_INIT_PRIORITY 0
>
> in gcc/config/darwin.h, all constructors are automatically set to
>
> #define DEFAULT_INIT_PRIORITY 65535
>
> in gcc/collect2.c. Any code that attempts to set the constructor/destructor priority
> on darwin results in a compile time error of "constructor priorities are not supported".
> So asan alone should be unique in emitting priorities different from 65535 on darwin.
> The attached patch uses a va_gc vector of constructor symbol/priority records to queue
> this data as it is generated in calls to machopic_asm_out_constructor. Any instances of
> the static constructor with priority 99 emitted by asan are inserted safely in the front
> of the vector queue which retains the original order of the remaining constructors in
> the queue. The contents of the vector queue are later processed in a new finalize_ctors
> routine called from darwin_file_end if necessary. The patch also adds a g++.dg/asan/pr55617.C
> test case which is targeted to i?86-*-darwin* and x86_64-*-darwin*.
>     The patch reduces the failures observed when running....
>
> make -k check-g++ RUNTESTFLAGS="--target_board=unix'{-fsanitize=address}'"
>
> from 323 to only 85 on darwin (similar to the results on linux). The cov.C testcase also
> fails on gcc trunk with -fsanitize=address when recrafted into a dynamic shared library
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55617#c28. This patch eliminates those
> crashes. This problem doesn't extend to when the shared library or module is dlopen'd
> (which works in stock gcc trunk and with this patch as well).
>     The patch has been bootstrap and regression tested on x86_64-apple-darwin12.
> Okay for gcc trunk?

But that does not work across translation units, no?  ISTR collect2 has support
to handle constructor priorities all by itself (at link time,
considering all inputs).
I wonder why darwin cannot use that mechanism to support init priorities?

Richard.

>          Jack
> ps Unfortunately the flag_sort variable is unavailable inside of machopic_asm_out_constructor
> so we have to unconditionally test for priority == 99.
>
Alexander Potapenko - Feb. 4, 2013, 9:35 a.m.
Constructor priorities on Darwin aren't supposed to work across
translation units, see http://llvm.org/bugs/show_bug.cgi?id=12556:

"""
I was told (by Apple folks) that darwin does not support cross-unit constructor
priorities, sorry. This is true for both gcc and llvm-gcc / clang. Within unit
priorities are emulated on darwin (via emission order).

Also, according to Nick Kledzik, constructors are executed on darwin strictly
in link order. So, when you link foo.o and bar.o, then first ctors from foo.o
are executed, then - from bar.o. Maybe this is even documented in some MachO
docs.
"""

Yet it should be possible to delay the constructor emission for a
single TU until all the compiler passes do their job, and then sort
those constructors according to their priorities.

On Mon, Feb 4, 2013 at 1:22 PM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sun, Feb 3, 2013 at 5:57 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
>>    Currently darwin is unable to utilize libasan with constructors due to the lack of
>> constructor priority support on that target. The asan_finish_file routine inserts an
>> essential __asan_init into the array of constructors (via the __mod_init_func section).
>> However the insertion occurs at the end, and due to the lack of priority support for
>> constructors, these are executed from the start of the array of constructors on program
>> startup. This causes code any instrumented code that executes before the __asan_init
>> call to crash.
>>    Since darwin sets...
>>
>> #undef SUPPORTS_INIT_PRIORITY
>> #define SUPPORTS_INIT_PRIORITY 0
>>
>> in gcc/config/darwin.h, all constructors are automatically set to
>>
>> #define DEFAULT_INIT_PRIORITY 65535
>>
>> in gcc/collect2.c. Any code that attempts to set the constructor/destructor priority
>> on darwin results in a compile time error of "constructor priorities are not supported".
>> So asan alone should be unique in emitting priorities different from 65535 on darwin.
>> The attached patch uses a va_gc vector of constructor symbol/priority records to queue
>> this data as it is generated in calls to machopic_asm_out_constructor. Any instances of
>> the static constructor with priority 99 emitted by asan are inserted safely in the front
>> of the vector queue which retains the original order of the remaining constructors in
>> the queue. The contents of the vector queue are later processed in a new finalize_ctors
>> routine called from darwin_file_end if necessary. The patch also adds a g++.dg/asan/pr55617.C
>> test case which is targeted to i?86-*-darwin* and x86_64-*-darwin*.
>>     The patch reduces the failures observed when running....
>>
>> make -k check-g++ RUNTESTFLAGS="--target_board=unix'{-fsanitize=address}'"
>>
>> from 323 to only 85 on darwin (similar to the results on linux). The cov.C testcase also
>> fails on gcc trunk with -fsanitize=address when recrafted into a dynamic shared library
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55617#c28. This patch eliminates those
>> crashes. This problem doesn't extend to when the shared library or module is dlopen'd
>> (which works in stock gcc trunk and with this patch as well).
>>     The patch has been bootstrap and regression tested on x86_64-apple-darwin12.
>> Okay for gcc trunk?
>
> But that does not work across translation units, no?  ISTR collect2 has support
> to handle constructor priorities all by itself (at link time,
> considering all inputs).
> I wonder why darwin cannot use that mechanism to support init priorities?
>
> Richard.
>
>>          Jack
>> ps Unfortunately the flag_sort variable is unavailable inside of machopic_asm_out_constructor
>> so we have to unconditionally test for priority == 99.
>>
Alexander Potapenko - Feb. 4, 2013, 9:37 a.m.
> But that does not work across translation units, no?  ISTR collect2 has support
> to handle constructor priorities all by itself (at link time,
> considering all inputs).
> I wonder why darwin cannot use that mechanism to support init priorities?
>
> Richard.

(resending, sorry for top-posting)
Constructor priorities on Darwin aren't supposed to work across
translation units, see http://llvm.org/bugs/show_bug.cgi?id=12556:

"""
I was told (by Apple folks) that darwin does not support cross-unit constructor
priorities, sorry. This is true for both gcc and llvm-gcc / clang. Within unit
priorities are emulated on darwin (via emission order).

Also, according to Nick Kledzik, constructors are executed on darwin strictly
in link order. So, when you link foo.o and bar.o, then first ctors from foo.o
are executed, then - from bar.o. Maybe this is even documented in some MachO
docs.
"""

Yet it should be possible to delay the constructor emission for a
single TU until all the compiler passes do their job, and then sort
those constructors according to their priorities.
Jakub Jelinek - Feb. 4, 2013, 9:38 a.m.
On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
> > Okay for gcc trunk?
> 
> But that does not work across translation units, no?  ISTR collect2 has support
> to handle constructor priorities all by itself (at link time,
> considering all inputs).

I wonder why the patch turned from initially at least supporting intra-CU
support for ctor priorities into an ugly hack for asan.  I guess asan
doesn't care too much about inter-CU ctor priorities, it just needs its
ctors to run before anything in the same CU is called (mainly the
__asan_init call), other CUs either won't be asan instrumented, then it
doesn't matter, or will be, but they will have their own __asan_init call.

> I wonder why darwin cannot use that mechanism to support init priorities?

But sure, if collect2 can be used for full init prio support, the better.

	Jakub
Alexander Potapenko - Feb. 4, 2013, 9:55 a.m.
On Mon, Feb 4, 2013 at 1:38 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
>> > Okay for gcc trunk?
>>
>> But that does not work across translation units, no?  ISTR collect2 has support
>> to handle constructor priorities all by itself (at link time,
>> considering all inputs).
>
> I wonder why the patch turned from initially at least supporting intra-CU
> support for ctor priorities into an ugly hack for asan.  I guess asan
> doesn't care too much about inter-CU ctor priorities, it just needs its
> ctors to run before anything in the same CU is called (mainly the
> __asan_init call), other CUs either won't be asan instrumented, then it
> doesn't matter, or will be, but they will have their own __asan_init call.

Yes, I was going to ask the same question.
Since other compile-time instrumentation tools (like ThreadSanitizer)
will benefit from this as well, it's better to provide the intra-CU
support by sorting the list of constructors.
Richard Guenther - Feb. 4, 2013, 10:48 a.m.
On Mon, Feb 4, 2013 at 10:55 AM, Alexander Potapenko <glider@google.com> wrote:
> On Mon, Feb 4, 2013 at 1:38 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
>>> > Okay for gcc trunk?
>>>
>>> But that does not work across translation units, no?  ISTR collect2 has support
>>> to handle constructor priorities all by itself (at link time,
>>> considering all inputs).
>>
>> I wonder why the patch turned from initially at least supporting intra-CU
>> support for ctor priorities into an ugly hack for asan.  I guess asan
>> doesn't care too much about inter-CU ctor priorities, it just needs its
>> ctors to run before anything in the same CU is called (mainly the
>> __asan_init call), other CUs either won't be asan instrumented, then it
>> doesn't matter, or will be, but they will have their own __asan_init call.
>
> Yes, I was going to ask the same question.
> Since other compile-time instrumentation tools (like ThreadSanitizer)
> will benefit from this as well, it's better to provide the intra-CU
> support by sorting the list of constructors.

Which should be done by the middle-end then....?  Not sure if
"have-ctor-priority-support" is properly abstracted.

Richard.
Jack Howarth - Feb. 4, 2013, 2:22 p.m.
On Mon, Feb 04, 2013 at 10:38:29AM +0100, Jakub Jelinek wrote:
> On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
> > > Okay for gcc trunk?
> > 
> > But that does not work across translation units, no?  ISTR collect2 has support
> > to handle constructor priorities all by itself (at link time,
> > considering all inputs).
> 
> I wonder why the patch turned from initially at least supporting intra-CU
> support for ctor priorities into an ugly hack for asan.  I guess asan
> doesn't care too much about inter-CU ctor priorities, it just needs its
> ctors to run before anything in the same CU is called (mainly the
> __asan_init call), other CUs either won't be asan instrumented, then it
> doesn't matter, or will be, but they will have their own __asan_init call.

Jakub,
   I switched to the simple insertion of the asan priorities for two reasons...

1) Mike seemed unconvinced that the single qsort with the proposed sort_ctor_records
of...

+static int
+sort_ctor_records (const void * a, const void * b)
+{
+  const ctor_record *ca = (const ctor_record *)a;
+  const ctor_record *cb = (const ctor_record *)b;
+  if (ca->priority > cb->priority)
+    return 1;
+  if (ca->priority < cb->priority)
+    return -1;
+  if (ca->position > cb->position)
+    return -1;
+  if (ca->position < cb->position)
+    return 1;
+  return 0;
+}

would really be stable in absence of a second call to qsort.

2) Once I realized that darwin sets the default priority of constructors to
DEFAULT_INIT_PRIORITY 65535, the desired sorting method seemed rather unclear.
I assume we need to really sort these so that the priorities from 
MAX_INIT_PRIORITY-1 through 0 appear first in the queue and then those with
MAX_INIT_PRIORITY, right? It isn't obvious how we can achieve that in
sort_ctor_record with a single pass through qsort.
               Jack

> 
> > I wonder why darwin cannot use that mechanism to support init priorities?
> 
> But sure, if collect2 can be used for full init prio support, the better.
> 
> 	Jakub
Jack Howarth - Feb. 4, 2013, 2:28 p.m.
On Mon, Feb 04, 2013 at 09:22:27AM -0500, Jack Howarth wrote:
> On Mon, Feb 04, 2013 at 10:38:29AM +0100, Jakub Jelinek wrote:
> > On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
> > > > Okay for gcc trunk?
> > > 
> > > But that does not work across translation units, no?  ISTR collect2 has support
> > > to handle constructor priorities all by itself (at link time,
> > > considering all inputs).
> > 
> > I wonder why the patch turned from initially at least supporting intra-CU
> > support for ctor priorities into an ugly hack for asan.  I guess asan
> > doesn't care too much about inter-CU ctor priorities, it just needs its
> > ctors to run before anything in the same CU is called (mainly the
> > __asan_init call), other CUs either won't be asan instrumented, then it
> > doesn't matter, or will be, but they will have their own __asan_init call.
> 
> Jakub,
>    I switched to the simple insertion of the asan priorities for two reasons...
> 
> 1) Mike seemed unconvinced that the single qsort with the proposed sort_ctor_records
> of...
> 
> +static int
> +sort_ctor_records (const void * a, const void * b)
> +{
> +  const ctor_record *ca = (const ctor_record *)a;
> +  const ctor_record *cb = (const ctor_record *)b;
> +  if (ca->priority > cb->priority)
> +    return 1;
> +  if (ca->priority < cb->priority)
> +    return -1;
> +  if (ca->position > cb->position)
> +    return -1;
> +  if (ca->position < cb->position)
> +    return 1;
> +  return 0;
> +}
> 
> would really be stable in absence of a second call to qsort.
> 
> 2) Once I realized that darwin sets the default priority of constructors to
> DEFAULT_INIT_PRIORITY 65535, the desired sorting method seemed rather unclear.
> I assume we need to really sort these so that the priorities from 
> MAX_INIT_PRIORITY-1 through 0 appear first in the queue and then those with
> MAX_INIT_PRIORITY, right? It isn't obvious how we can achieve that in
> sort_ctor_record with a single pass through qsort.
>                Jack

Nevermind. I now see sorting priorities from lower to higher throughout is correct. I'll
repost the originally proposed patch for qsort later.
            Jack

> 
> > 
> > > I wonder why darwin cannot use that mechanism to support init priorities?
> > 
> > But sure, if collect2 can be used for full init prio support, the better.
> > 
> > 	Jakub
Jakub Jelinek - Feb. 4, 2013, 2:44 p.m.
On Mon, Feb 04, 2013 at 09:22:27AM -0500, Jack Howarth wrote:
>    I switched to the simple insertion of the asan priorities for two reasons...
> 
> 1) Mike seemed unconvinced that the single qsort with the proposed sort_ctor_records
> of...
> 
> +static int
> +sort_ctor_records (const void * a, const void * b)
> +{
> +  const ctor_record *ca = (const ctor_record *)a;
> +  const ctor_record *cb = (const ctor_record *)b;
> +  if (ca->priority > cb->priority)
> +    return 1;
> +  if (ca->priority < cb->priority)
> +    return -1;
> +  if (ca->position > cb->position)
> +    return -1;

Obviously this should have been return 1;

> +  if (ca->position < cb->position)
> +    return 1;

and this return -1;

> +  return 0;
> +}
> 
> would really be stable in absence of a second call to qsort.

Ugh, how can that not be stable?  position is different in every vector
entry, so even the return 0; case above would happen only if qsort
(incorrectly) called it with two same pointers.  So, the second and any
further calls to qsort with the same comparison function in this case
necessarily don't change anything in the array (ok, unless you have more
than 4billion ctors and overflow position, or unless your OS has a buggy
qsort (which wouldn't surprise me for Darwin)).

> 2) Once I realized that darwin sets the default priority of constructors to
> DEFAULT_INIT_PRIORITY 65535, the desired sorting method seemed rather unclear.
> I assume we need to really sort these so that the priorities from 
> MAX_INIT_PRIORITY-1 through 0 appear first in the queue and then those with
> MAX_INIT_PRIORITY, right? It isn't obvious how we can achieve that in
> sort_ctor_record with a single pass through qsort.

??  You simply sort by priority ascending, and for same priorities, by
position ascending.

	Jakub
Jack Howarth - Feb. 4, 2013, 3:14 p.m.
On Mon, Feb 04, 2013 at 03:44:04PM +0100, Jakub Jelinek wrote:
> On Mon, Feb 04, 2013 at 09:22:27AM -0500, Jack Howarth wrote:
> >    I switched to the simple insertion of the asan priorities for two reasons...
> > 
> > 1) Mike seemed unconvinced that the single qsort with the proposed sort_ctor_records
> > of...
> > 
> > +static int
> > +sort_ctor_records (const void * a, const void * b)
> > +{
> > +  const ctor_record *ca = (const ctor_record *)a;
> > +  const ctor_record *cb = (const ctor_record *)b;
> > +  if (ca->priority > cb->priority)
> > +    return 1;
> > +  if (ca->priority < cb->priority)
> > +    return -1;
> > +  if (ca->position > cb->position)
> > +    return -1;
> 
> Obviously this should have been return 1;
> 
> > +  if (ca->position < cb->position)
> > +    return 1;
> 
> and this return -1;
> 
> > +  return 0;
> > +}
> > 
> > would really be stable in absence of a second call to qsort.
> 
> Ugh, how can that not be stable?  position is different in every vector
> entry, so even the return 0; case above would happen only if qsort
> (incorrectly) called it with two same pointers.  So, the second and any
> further calls to qsort with the same comparison function in this case
> necessarily don't change anything in the array (ok, unless you have more
> than 4billion ctors and overflow position, or unless your OS has a buggy
> qsort (which wouldn't surprise me for Darwin)).

Actually don't we need...

static int
sort_ctor_records (const void * a, const void * b)
{
  const ctor_record *ca = (const ctor_record *)a;
  const ctor_record *cb = (const ctor_record *)b;
  if (ca->priority > cb->priority)
    return 1;
  if (ca->priority < cb->priority)
    return -1;
  if ((ca->priority == cb->priority) && (ca->position > cb->position))
    return 1;
  if ((ca->priority == cb->priority) && (ca->position < cb->position))
    return -1;
  return 0;
}

so that the last two checks only sort the original positions of constructors for
the same priority?

> 
> > 2) Once I realized that darwin sets the default priority of constructors to
> > DEFAULT_INIT_PRIORITY 65535, the desired sorting method seemed rather unclear.
> > I assume we need to really sort these so that the priorities from 
> > MAX_INIT_PRIORITY-1 through 0 appear first in the queue and then those with
> > MAX_INIT_PRIORITY, right? It isn't obvious how we can achieve that in
> > sort_ctor_record with a single pass through qsort.
> 
> ??  You simply sort by priority ascending, and for same priorities, by
> position ascending.
> 
> 	Jakub
Mike Stump - Feb. 4, 2013, 6:55 p.m.
On Feb 4, 2013, at 6:22 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
>   I switched to the simple insertion of the asan priorities for two reasons...
> 
> 1) Mike seemed unconvinced that the single qsort with the proposed sort_ctor_records
> of…

> would really be stable in absence of a second call to qsort.

This is wrong; a mis-understanding of my point.  Since the latest patch is correct with respect to this point, it doesn't matter.
Mike Stump - Feb. 4, 2013, 7:39 p.m.
On Feb 4, 2013, at 1:38 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
>>> Okay for gcc trunk?
>> 
>> But that does not work across translation units, no?  ISTR collect2 has support
>> to handle constructor priorities all by itself (at link time,
>> considering all inputs).
> 
> I wonder why the patch turned from initially at least supporting intra-CU
> support for ctor priorities into an ugly hack for asan.  I guess asan
> doesn't care too much about inter-CU ctor priorities, it just needs its
> ctors to run before anything in the same CU is called (mainly the
> __asan_init call), other CUs either won't be asan instrumented, then it
> doesn't matter, or will be, but they will have their own __asan_init call.
> 
>> I wonder why darwin cannot use that mechanism to support init priorities?
> 
> But sure, if collect2 can be used for full init prio support, the better.

It would be nice if someone contributed full init_priority support…  I'd be happy to review that. A good patch for that would add it to clang for darwin, and have gcc use that same mechanism so that we can interoperate nicely.  Absent interoperability…  I think it would be annoying, as then you have to have a binary incompatibility to fix the one that is wrong.
Alexander Potapenko - Feb. 6, 2013, 2:23 p.m.
I can't see how full init_priority support can work without proper aid
from ld and/or the dynamic linker. According to the Apple people,
those don't treat the cross-module priorities properly, so there's
little that can be done on the compiler side.

On Mon, Feb 4, 2013 at 11:39 PM, Mike Stump <mikestump@comcast.net> wrote:
> On Feb 4, 2013, at 1:38 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
>>>> Okay for gcc trunk?
>>>
>>> But that does not work across translation units, no?  ISTR collect2 has support
>>> to handle constructor priorities all by itself (at link time,
>>> considering all inputs).
>>
>> I wonder why the patch turned from initially at least supporting intra-CU
>> support for ctor priorities into an ugly hack for asan.  I guess asan
>> doesn't care too much about inter-CU ctor priorities, it just needs its
>> ctors to run before anything in the same CU is called (mainly the
>> __asan_init call), other CUs either won't be asan instrumented, then it
>> doesn't matter, or will be, but they will have their own __asan_init call.
>>
>>> I wonder why darwin cannot use that mechanism to support init priorities?
>>
>> But sure, if collect2 can be used for full init prio support, the better.
>
> It would be nice if someone contributed full init_priority support…  I'd be happy to review that. A good patch for that would add it to clang for darwin, and have gcc use that same mechanism so that we can interoperate nicely.  Absent interoperability…  I think it would be annoying, as then you have to have a binary incompatibility to fix the one that is wrong.



--
Alexander Potapenko
Software Engineer
Google Moscow
Jack Howarth - Feb. 6, 2013, 3:05 p.m.
On Wed, Feb 06, 2013 at 06:23:50PM +0400, Alexander Potapenko wrote:
> I can't see how full init_priority support can work without proper aid
> from ld and/or the dynamic linker. According to the Apple people,
> those don't treat the cross-module priorities properly, so there's
> little that can be done on the compiler side.
> 
> On Mon, Feb 4, 2013 at 11:39 PM, Mike Stump <mikestump@comcast.net> wrote:
> > On Feb 4, 2013, at 1:38 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> >> On Mon, Feb 04, 2013 at 10:22:48AM +0100, Richard Biener wrote:
> >>>> Okay for gcc trunk?
> >>>
> >>> But that does not work across translation units, no?  ISTR collect2 has support
> >>> to handle constructor priorities all by itself (at link time,
> >>> considering all inputs).
> >>
> >> I wonder why the patch turned from initially at least supporting intra-CU
> >> support for ctor priorities into an ugly hack for asan.  I guess asan
> >> doesn't care too much about inter-CU ctor priorities, it just needs its
> >> ctors to run before anything in the same CU is called (mainly the
> >> __asan_init call), other CUs either won't be asan instrumented, then it
> >> doesn't matter, or will be, but they will have their own __asan_init call.
> >>
> >>> I wonder why darwin cannot use that mechanism to support init priorities?
> >>
> >> But sure, if collect2 can be used for full init prio support, the better.
> >
> > It would be nice if someone contributed full init_priority support…  I'd be happy to review that. A good patch for that would add it to clang for darwin, and have gcc use that same mechanism so that we can interoperate nicely.  Absent interoperability…  I think it would be annoying, as then you have to have a binary incompatibility to fix the one that is wrong.
> 
> 

Alexander,
   I never claimed full init priority support however FSF gcc on darwin currently
has no init priority support at all. Since Mike wanted to sort the destructors as
well as the constructors and this achieves usable intra-module init priority support
for FSF gcc darwin, I don't see why we don't take advantage of it. Especially 
considering that the constructors and destructors will now always be sorted anyway.
        Jack
ps We will have one advantage over clang's init priority support as we can use -flto
to combine all of the code modules (outside of libraries) into a single one for the
sorting of constructors/destructors. This allows the g++.dg/special/conpr-3.C execution
test case to operate properly on darwin with -flto. Again, remember that clang currently
at least supports init priority on a intra-module level. I am just trying to leverage
the sorting of constructors/destructors that we added for asan to achive the same
level of functionality in FSF gcc on darwin.

> 
> --
> Alexander Potapenko
> Software Engineer
> Google Moscow
Alexander Potapenko - Feb. 6, 2013, 3:14 p.m.
> Alexander,
>    I never claimed full init priority support however FSF gcc on darwin currently
> has no init priority support at all. Since Mike wanted to sort the destructors as
> well as the constructors and this achieves usable intra-module init priority support
> for FSF gcc darwin, I don't see why we don't take advantage of it. Especially
> considering that the constructors and destructors will now always be sorted anyway.
>         Jack
> ps We will have one advantage over clang's init priority support as we can use -flto
> to combine all of the code modules (outside of libraries) into a single one for the
> sorting of constructors/destructors. This allows the g++.dg/special/conpr-3.C execution
> test case to operate properly on darwin with -flto. Again, remember that clang currently
> at least supports init priority on a intra-module level. I am just trying to leverage
> the sorting of constructors/destructors that we added for asan to achive the same
> level of functionality in FSF gcc on darwin.
>

Jack,
I understand and fully support your desire for intra-module ctor/dtor priority.
My comment was meant to reply to Mike (sorry for top-posting it,
again), who, as far as I understood him, wanted to see full
init_priority support on Darwin, which IIUC can't be implemented
without the proper linker support. LTO may help as a bandaid, but I
don't think this solution scales well enough yet.

Alex
Jack Howarth - Feb. 6, 2013, 3:31 p.m.
On Wed, Feb 06, 2013 at 07:14:07PM +0400, Alexander Potapenko wrote:
> > Alexander,
> >    I never claimed full init priority support however FSF gcc on darwin currently
> > has no init priority support at all. Since Mike wanted to sort the destructors as
> > well as the constructors and this achieves usable intra-module init priority support
> > for FSF gcc darwin, I don't see why we don't take advantage of it. Especially
> > considering that the constructors and destructors will now always be sorted anyway.
> >         Jack
> > ps We will have one advantage over clang's init priority support as we can use -flto
> > to combine all of the code modules (outside of libraries) into a single one for the
> > sorting of constructors/destructors. This allows the g++.dg/special/conpr-3.C execution
> > test case to operate properly on darwin with -flto. Again, remember that clang currently
> > at least supports init priority on a intra-module level. I am just trying to leverage
> > the sorting of constructors/destructors that we added for asan to achive the same
> > level of functionality in FSF gcc on darwin.
> >
> 
> Jack,
> I understand and fully support your desire for intra-module ctor/dtor priority.
> My comment was meant to reply to Mike (sorry for top-posting it,
> again), who, as far as I understood him, wanted to see full
> init_priority support on Darwin, which IIUC can't be implemented
> without the proper linker support. LTO may help as a bandaid, but I
> don't think this solution scales well enough yet.
> 
> Alex

Alex,
   I have already opened a radr://13149612, "inter module constructor/destructor priority
support needed on darwin", and pinged the darwin linker developer on this issue. 
         Jack
ps Once we have init priority enabled on darwin in gcc trunk, I plan to open a radar about
the absence of support collating this at -O4 on clang. That should be easier to address and 
might get Apple looking at the state of their init priority support again. IHMO, it would
be a start if they at least support inter-module priority support in clang's internal assembler.
I've been lobbying Apple for sometime to provide a stand-alone gas replacement based on
clang's internal assembler so we would eventually get such fixes through that.
Ian Taylor - Feb. 6, 2013, 3:36 p.m.
On Wed, Feb 6, 2013 at 7:14 AM, Alexander Potapenko <glider@google.com> wrote:
>
> I understand and fully support your desire for intra-module ctor/dtor priority.
> My comment was meant to reply to Mike (sorry for top-posting it,
> again), who, as far as I understood him, wanted to see full
> init_priority support on Darwin, which IIUC can't be implemented
> without the proper linker support. LTO may help as a bandaid, but I
> don't think this solution scales well enough yet.

It could be done using collect2.  Though it would be a bit tedious to write.

Or it could perhaps be done via a linker plugin--the Darwin linker
supports plugins.  I don't know if the Darwin linker has the required
plugin support, but the gold linker does.

Or we could just bug the Darwin guys to support it in the linker, it's
clearly a useful feature, and they do have linker support.

Ian
Jack Howarth - Feb. 6, 2013, 3:44 p.m.
On Wed, Feb 06, 2013 at 07:36:06AM -0800, Ian Lance Taylor wrote:
> On Wed, Feb 6, 2013 at 7:14 AM, Alexander Potapenko <glider@google.com> wrote:
> >
> > I understand and fully support your desire for intra-module ctor/dtor priority.
> > My comment was meant to reply to Mike (sorry for top-posting it,
> > again), who, as far as I understood him, wanted to see full
> > init_priority support on Darwin, which IIUC can't be implemented
> > without the proper linker support. LTO may help as a bandaid, but I
> > don't think this solution scales well enough yet.
> 
> It could be done using collect2.  Though it would be a bit tedious to write.

Ian,
    Don't you need assembler support for the constructor init priorities to be
called in the correct order when they reside in static or shared libraries?
I couldn't puzzle out how collect2 can help with that issue.
            Jack

> 
> Or it could perhaps be done via a linker plugin--the Darwin linker
> supports plugins.  I don't know if the Darwin linker has the required
> plugin support, but the gold linker does.
> 
> Or we could just bug the Darwin guys to support it in the linker, it's
> clearly a useful feature, and they do have linker support.
> 
> Ian
Ian Taylor - Feb. 6, 2013, 4:22 p.m.
On Wed, Feb 6, 2013 at 7:44 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
> On Wed, Feb 06, 2013 at 07:36:06AM -0800, Ian Lance Taylor wrote:
>> On Wed, Feb 6, 2013 at 7:14 AM, Alexander Potapenko <glider@google.com> wrote:
>> >
>> > I understand and fully support your desire for intra-module ctor/dtor priority.
>> > My comment was meant to reply to Mike (sorry for top-posting it,
>> > again), who, as far as I understood him, wanted to see full
>> > init_priority support on Darwin, which IIUC can't be implemented
>> > without the proper linker support. LTO may help as a bandaid, but I
>> > don't think this solution scales well enough yet.
>>
>> It could be done using collect2.  Though it would be a bit tedious to write.
>
> Ian,
>     Don't you need assembler support for the constructor init priorities to be
> called in the correct order when they reside in static or shared libraries?
> I couldn't puzzle out how collect2 can help with that issue.

collect2 sees the whole link.  It can gather all the constructors
together and sort them.

Ian
Mike Stump - Feb. 6, 2013, 8:33 p.m.
On Feb 6, 2013, at 6:23 AM, Alexander Potapenko <glider@google.com> wrote:
> I can't see how full init_priority support can work without proper aid
> from ld and/or the dynamic linker.

I can, but, I don't see that that matters much.  It is a mere matter of software.  Rough sketch, define an encoding of the priority into the object, then define a process by which that data is respected during startup.  The encoding is arbitrary and doesn't much matter.  Once defined, the software to use that encoding is trivial.  The only hanging point is the realization that one needs to hook into the normal startup sequence.  The compiler can do this by having an object in the link that appears first (first to run ctors) that runs all the high priority ctors from its actor.  Now, personally, I favor having the vendor add support first, so that we get a defined abi to adhere to, but that isn't strictly required.  We can drive the abi, by defining it and submitted changes to clang to adhere to it.  Once in and released, I'd not expect the abi to be changed post that, that would then become the de-facto api, and de-jure, once documented.

> According to the Apple people,
> those don't treat the cross-module priorities properly, so there's
> little that can be done on the compiler side.

This is mistaken.
Mike Stump - Feb. 6, 2013, 8:35 p.m.
On Feb 6, 2013, at 7:44 AM, Jack Howarth <howarth@bromo.med.uc.edu> wrote:
>    Don't you need assembler support for the constructor init priorities to be
> called in the correct order when they reside in static or shared libraries?

No.  It can be done without assembler support.

Patch

Index: gcc/config/darwin.c
===================================================================
--- gcc/config/darwin.c	(revision 195686)
+++ gcc/config/darwin.c	(working copy)
@@ -83,6 +83,13 @@  along with GCC; see the file COPYING3.  
    kernel) the stubs might still be required, and this will be set true.  */
 int darwin_emit_branch_islands = false;
 
+typedef struct GTY(()) ctor_record {
+  rtx symbol;
+  int priority;		/* constructor priority */
+} ctor_record;
+
+static GTY(()) vec<ctor_record, va_gc> *ctors = NULL;
+
 /* A flag to determine whether we are running c++ or obj-c++.  This has to be
    settable from non-c-family contexts too (i.e. we can't use the c_dialect_
    functions).  */
@@ -1708,15 +1715,32 @@  machopic_select_rtx_section (enum machin
 void
 machopic_asm_out_constructor (rtx symbol, int priority ATTRIBUTE_UNUSED)
 {
+  ctor_record new_elt = {symbol, priority};
+  if (priority == 99)
+    vec_safe_insert(ctors, 0, new_elt); 
+  else
+    vec_safe_push (ctors, new_elt);
+
+  if (! MACHOPIC_INDIRECT)
+    fprintf (asm_out_file, ".reference .constructors_used\n");
+}
+
+static void 
+finalize_ctors()
+{
+  unsigned int i;
+  ctor_record *elt;
+ 
   if (MACHOPIC_INDIRECT)
     switch_to_section (darwin_sections[mod_init_section]);
   else
     switch_to_section (darwin_sections[constructor_section]);
-  assemble_align (POINTER_SIZE);
-  assemble_integer (symbol, POINTER_SIZE / BITS_PER_UNIT, POINTER_SIZE, 1);
 
-  if (! MACHOPIC_INDIRECT)
-    fprintf (asm_out_file, ".reference .constructors_used\n");
+  FOR_EACH_VEC_SAFE_ELT (ctors, i, elt)
+    {
+      assemble_align (POINTER_SIZE);
+      assemble_integer (elt->symbol, POINTER_SIZE / BITS_PER_UNIT, POINTER_SIZE, 1);
+    }
 }
 
 void
@@ -2762,6 +2786,8 @@  darwin_file_start (void)
 void
 darwin_file_end (void)
 {
+  if (!vec_safe_is_empty (ctors))
+    finalize_ctors();
   machopic_finish (asm_out_file);
   if (strcmp (lang_hooks.name, "GNU C++") == 0)
     {
--- /dev/null	2013-02-02 10:53:51.000000000 -0500
+++ gcc/testsuite/g++.dg/asan/pr55617.C	2013-02-02 10:22:17.000000000 -0500
@@ -0,0 +1,8 @@ 
+// { dg-do run { target { i?86-*-darwin* x86_64-*-darwin* } } }
+
+struct c18 { 
+  virtual void bar() { }
+};
+c18 ret;
+int main () {
+}