diff mbox

Fix PR libitm/55693

Message ID CAKQMxzRzbLas2gVz=77m=uLRzGz8U5hzZTpZubEpUiqtaEbQ5A@mail.gmail.com
State New
Headers show

Commit Message

Patrick Marlier Feb. 6, 2013, 4:37 p.m. UTC 
Hi Jack,

Thanks for having a look at this.

However I don't understand why you need this:



It seems you just add a macro TM_DESTRUCTOR which is the same as
ENDFILE_SPEC. Maybe I missed something (I am updating my svn)...

Also why HAVE_ELF_STYLE_WEAKREF is not defined for MacOS version where
weak ref works? It could be better to define function to dummy only if
HAVE_ELF_STYLE_WEAKREF is not set (and not testing the MacOS version).

If we want to remove support for Xcode 4.2 (buggy linker), I think you
should also have a look at the old commit to undo it (I remember a
change in libitm which add a #ifdef MACH).

Thanks.

--
Patrick

Comments

Jack Howarth Feb. 6, 2013, 5:20 p.m. UTC | #1 
On Wed, Feb 06, 2013 at 05:37:12PM +0100, Patrick Marlier wrote:
> Hi Jack,
> 
> Thanks for having a look at this.
> 
> However I don't understand why you need this:
> 
> Index: gcc/config/i386/darwin.h
> ===================================================================
> --- gcc/config/i386/darwin.h (revision 195764)
> +++ gcc/config/i386/darwin.h (working copy)
> @@ -131,8 +131,7 @@ extern int darwin_emit_branch_islands;
>    "%{Ofast|ffast-math|funsafe-math-optimizations:crtfastmath.o%s} \
>     %{mpc32:crtprec32.o%s} \
>     %{mpc64:crtprec64.o%s} \
> -   %{mpc80:crtprec80.o%s} \
> -   %{fgnu-tm: -lcrttme.o}"
> +   %{mpc80:crtprec80.o%s}" TM_DESTRUCTOR
> 
>  #undef SUBTARGET_EXTRA_SPECS
>  #define SUBTARGET_EXTRA_SPECS                                   \
> Index: gcc/config/darwin.h
> ===================================================================
> --- gcc/config/darwin.h (revision 195764)
> +++ gcc/config/darwin.h (working copy)
> @@ -363,7 +363,8 @@ extern GTY(()) int darwin_ms_struct;
>    %{shared-libgcc:%:version-compare(< 10.5 mmacosx-version-min= crt3.o%s)}"
> 
>  /* We want a destructor last in the list.  */
> -#define ENDFILE_SPEC "%{fgnu-tm: -lcrttme.o}"
> +#define TM_DESTRUCTOR "%{fgnu-tm: -lcrttme.o}"
> +#define ENDFILE_SPEC TM_DESTRUCTOR
> 
>  #define DARWIN_EXTRA_SPECS \
>    { "darwin_crt1", DARWIN_CRT1_SPEC }, \
> 
> 
> It seems you just add a macro TM_DESTRUCTOR which is the same as
> ENDFILE_SPEC. Maybe I missed something (I am updating my svn)...

Patrick,
   This was the patch Iain proposed off-list and I just cleaned up the comments.
I believe that he added the additional definition of TM_DESTRUCTOR so that 
it could be used to replace the explicit instance of %{fgnu-tm: -lcrttme.o}
in the definition of ENDFILE_SPEC in gcc/config/i386/darwin.h.

> 
> Also why HAVE_ELF_STYLE_WEAKREF is not defined for MacOS version where
> weak ref works? It could be better to define function to dummy only if
> HAVE_ELF_STYLE_WEAKREF is not set (and not testing the MacOS version).

I assume you are referring to the change...

 /* Provide dummy functions to satisfy linkage for versions of the Darwin 
    tool-chain that that can't handle undefined weak refs at the link stage.
-   ??? Define these dummy functions only when !HAVE_ELF_STYLE_WEAKREF. */
+   Don't define for 10.6 or later with faster weak-symbol coalescing. */

As far as I can tell that was only a TODO type note in the comments and
was never implemented. Using !HAVE_ELF_STYLE_WEAKREF as the conditional
to avoid defining the dummy functions won't help at all. The problem here
has nothing to do with HAVE_ELF_STYLE_WEAKREF but is the fact that 
the introduction of faster c++ weak-symbol coalescing prevents dyld from
looking in libstdc++ for the __cxa* symbols if weak symbols for them
exist in linked libgcc/config/darwin-crt-tm.o. This problem also exists
for Xcode 3.2.6 which doesn't HAVE_ELF_STYLE_WEAKREF. So the correct fix
is what Iain proposed of defining the dummy functions only when the
__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ < 10.6.

> 
> If we want to remove support for Xcode 4.2 (buggy linker), I think you
> should also have a look at the old commit to undo it (I remember a
> change in libitm which add a #ifdef MACH).

I don't see the point. The libitm.c++/eh-1.C execution test case is broken
current gcc trunk on Xcode 4.2 up to 4.4. With the patch, it remains broken (as you 
there are some linker bugs you just can hack around).
             Jack

> 
> Thanks.
> 
> --
> Patrick
Iain Sandoe Feb. 6, 2013, 5:28 p.m. UTC | #2 
On 6 Feb 2013, at 17:20, Jack Howarth wrote:

> On Wed, Feb 06, 2013 at 05:37:12PM +0100, Patrick Marlier wrote:
>> Hi Jack,
>> 
>> Thanks for having a look at this.
>> 
>> However I don't understand why you need this:
>> 
>> Index: gcc/config/i386/darwin.h
>> ===================================================================
>> --- gcc/config/i386/darwin.h (revision 195764)
>> +++ gcc/config/i386/darwin.h (working copy)
>> @@ -131,8 +131,7 @@ extern int darwin_emit_branch_islands;
>>   "%{Ofast|ffast-math|funsafe-math-optimizations:crtfastmath.o%s} \
>>    %{mpc32:crtprec32.o%s} \
>>    %{mpc64:crtprec64.o%s} \
>> -   %{mpc80:crtprec80.o%s} \
>> -   %{fgnu-tm: -lcrttme.o}"
>> +   %{mpc80:crtprec80.o%s}" TM_DESTRUCTOR
>> 
>> #undef SUBTARGET_EXTRA_SPECS
>> #define SUBTARGET_EXTRA_SPECS                                   \
>> Index: gcc/config/darwin.h
>> ===================================================================
>> --- gcc/config/darwin.h (revision 195764)
>> +++ gcc/config/darwin.h (working copy)
>> @@ -363,7 +363,8 @@ extern GTY(()) int darwin_ms_struct;
>>   %{shared-libgcc:%:version-compare(< 10.5 mmacosx-version-min= crt3.o%s)}"
>> 
>> /* We want a destructor last in the list.  */
>> -#define ENDFILE_SPEC "%{fgnu-tm: -lcrttme.o}"
>> +#define TM_DESTRUCTOR "%{fgnu-tm: -lcrttme.o}"
>> +#define ENDFILE_SPEC TM_DESTRUCTOR
>> 
>> #define DARWIN_EXTRA_SPECS \
>>   { "darwin_crt1", DARWIN_CRT1_SPEC }, \
>> 
>> 
>> It seems you just add a macro TM_DESTRUCTOR which is the same as
>> ENDFILE_SPEC. Maybe I missed something (I am updating my svn)...
> 
> Patrick,
>   This was the patch Iain proposed off-list and I just cleaned up the comments.
> I believe that he added the additional definition of TM_DESTRUCTOR so that 
> it could be used to replace the explicit instance of %{fgnu-tm: -lcrttme.o}
> in the definition of ENDFILE_SPEC in gcc/config/i386/darwin.h.

Correct - it was non-obvious to have a second instance embedded in the sub-dir.
(but I'm not going to complain if that change is removed).

==

Apropos support.

I believe our policy is to support the latest "official" XCode from Apple for each version of Darwin.

In some cases (Darwins < 9) the last issued XCode will not build GCC and it's neccessary to build odcctools and bootstrap via an older GCC.

but otherwise I don't see a point in trying to support anything that's an 'unofficial developer' release of XCode.

Iain
Jack Howarth Feb. 6, 2013, 6:37 p.m. UTC | #3 
On Wed, Feb 06, 2013 at 05:28:33PM +0000, Iain Sandoe wrote:
> 
> On 6 Feb 2013, at 17:20, Jack Howarth wrote:
> 
> > On Wed, Feb 06, 2013 at 05:37:12PM +0100, Patrick Marlier wrote:
> >> Hi Jack,
> >> 
> >> Thanks for having a look at this.
> >> 
> >> However I don't understand why you need this:
> >> 
> >> Index: gcc/config/i386/darwin.h
> >> ===================================================================
> >> --- gcc/config/i386/darwin.h (revision 195764)
> >> +++ gcc/config/i386/darwin.h (working copy)
> >> @@ -131,8 +131,7 @@ extern int darwin_emit_branch_islands;
> >>   "%{Ofast|ffast-math|funsafe-math-optimizations:crtfastmath.o%s} \
> >>    %{mpc32:crtprec32.o%s} \
> >>    %{mpc64:crtprec64.o%s} \
> >> -   %{mpc80:crtprec80.o%s} \
> >> -   %{fgnu-tm: -lcrttme.o}"
> >> +   %{mpc80:crtprec80.o%s}" TM_DESTRUCTOR
> >> 
> >> #undef SUBTARGET_EXTRA_SPECS
> >> #define SUBTARGET_EXTRA_SPECS                                   \
> >> Index: gcc/config/darwin.h
> >> ===================================================================
> >> --- gcc/config/darwin.h (revision 195764)
> >> +++ gcc/config/darwin.h (working copy)
> >> @@ -363,7 +363,8 @@ extern GTY(()) int darwin_ms_struct;
> >>   %{shared-libgcc:%:version-compare(< 10.5 mmacosx-version-min= crt3.o%s)}"
> >> 
> >> /* We want a destructor last in the list.  */
> >> -#define ENDFILE_SPEC "%{fgnu-tm: -lcrttme.o}"
> >> +#define TM_DESTRUCTOR "%{fgnu-tm: -lcrttme.o}"
> >> +#define ENDFILE_SPEC TM_DESTRUCTOR
> >> 
> >> #define DARWIN_EXTRA_SPECS \
> >>   { "darwin_crt1", DARWIN_CRT1_SPEC }, \
> >> 
> >> 
> >> It seems you just add a macro TM_DESTRUCTOR which is the same as
> >> ENDFILE_SPEC. Maybe I missed something (I am updating my svn)...
> > 
> > Patrick,
> >   This was the patch Iain proposed off-list and I just cleaned up the comments.
> > I believe that he added the additional definition of TM_DESTRUCTOR so that 
> > it could be used to replace the explicit instance of %{fgnu-tm: -lcrttme.o}
> > in the definition of ENDFILE_SPEC in gcc/config/i386/darwin.h.
> 
> Correct - it was non-obvious to have a second instance embedded in the sub-dir.
> (but I'm not going to complain if that change is removed).
> 
> ==
> 
> Apropos support.
> 
> I believe our policy is to support the latest "official" XCode from Apple for each version of Darwin.

Iain,
   Do remember that Xcode 4.x on Snow Leopard is the odd duck here. Those releases were the
only Xcode releases that Apple required to be explicitly purchased at a cost from the App Store.
Also, those releases were only briefly available from the App Store (as they were removed as
soon as Lion was released). So it is no longer the latest "official" Xcode for Snow Leopard
(unless you have a paid developer account). I suspect this episode will never be repeated and had
more to do with the novelty of the App Store at the time. Apple has been very good with Xcode
releases under Lion (starting with 4.2 up to the current 4.6 release) all being free.
           Jack

> 
> In some cases (Darwins < 9) the last issued XCode will not build GCC and it's neccessary to build odcctools and bootstrap via an older GCC.
> 
> but otherwise I don't see a point in trying to support anything that's an 'unofficial developer' release of XCode.

It's really neither fish nor fowl as Apple did release it 'officially' but only for a brief period of time.

> 
> Iain
Patrick Marlier Feb. 6, 2013, 8:15 p.m. UTC | #4 
On Wed, Feb 6, 2013 at 6:28 PM, Iain Sandoe <iain@codesourcery.com> wrote:
>
> On 6 Feb 2013, at 17:20, Jack Howarth wrote:
>
>> On Wed, Feb 06, 2013 at 05:37:12PM +0100, Patrick Marlier wrote:
>>> Hi Jack,
>>>
>>> Thanks for having a look at this.
>>>
>>> However I don't understand why you need this:
>>>
>>> Index: gcc/config/i386/darwin.h
>>> ===================================================================
>>> --- gcc/config/i386/darwin.h (revision 195764)
>>> +++ gcc/config/i386/darwin.h (working copy)
>>> @@ -131,8 +131,7 @@ extern int darwin_emit_branch_islands;
>>>   "%{Ofast|ffast-math|funsafe-math-optimizations:crtfastmath.o%s} \
>>>    %{mpc32:crtprec32.o%s} \
>>>    %{mpc64:crtprec64.o%s} \
>>> -   %{mpc80:crtprec80.o%s} \
>>> -   %{fgnu-tm: -lcrttme.o}"
>>> +   %{mpc80:crtprec80.o%s}" TM_DESTRUCTOR
>>>
>>> #undef SUBTARGET_EXTRA_SPECS
>>> #define SUBTARGET_EXTRA_SPECS                                   \
>>> Index: gcc/config/darwin.h
>>> ===================================================================
>>> --- gcc/config/darwin.h (revision 195764)
>>> +++ gcc/config/darwin.h (working copy)
>>> @@ -363,7 +363,8 @@ extern GTY(()) int darwin_ms_struct;
>>>   %{shared-libgcc:%:version-compare(< 10.5 mmacosx-version-min= crt3.o%s)}"
>>>
>>> /* We want a destructor last in the list.  */
>>> -#define ENDFILE_SPEC "%{fgnu-tm: -lcrttme.o}"
>>> +#define TM_DESTRUCTOR "%{fgnu-tm: -lcrttme.o}"
>>> +#define ENDFILE_SPEC TM_DESTRUCTOR
>>>
>>> #define DARWIN_EXTRA_SPECS \
>>>   { "darwin_crt1", DARWIN_CRT1_SPEC }, \
>>>
>>>
>>> It seems you just add a macro TM_DESTRUCTOR which is the same as
>>> ENDFILE_SPEC. Maybe I missed something (I am updating my svn)...
>>
>> Patrick,
>>   This was the patch Iain proposed off-list and I just cleaned up the comments.
>> I believe that he added the additional definition of TM_DESTRUCTOR so that
>> it could be used to replace the explicit instance of %{fgnu-tm: -lcrttme.o}
>> in the definition of ENDFILE_SPEC in gcc/config/i386/darwin.h.
>
> Correct - it was non-obvious to have a second instance embedded in the sub-dir.
> (but I'm not going to complain if that change is removed).

I had a look at Iain's patch from the PR but some changes are missing
in the proposed patch to give TM_DESTRUCTOR a sense.
So either you add changes for files: gcc/config/darwin10.h
libgcc/config.host libgcc/config/t-darwin to this patch or you create
another patch with changes to gcc/config/darwin10.h libgcc/config.host
libgcc/config/t-darwin gcc/config/darwin.h gcc/config/i386/darwin.h.

About this old patch
http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00851.html, I though that
"&& !defined (__MACH__)" should be changed but it seems the best
way...

Thanks for the precision about XCode.

Note that I cannot approve anything. So the patch must be approved by
a GCC master ;)
--
Patrick
Jack Howarth Feb. 6, 2013, 11:19 p.m. UTC | #5 
On Wed, Feb 06, 2013 at 09:15:20PM +0100, Patrick Marlier wrote:
> About this old patch
> http://gcc.gnu.org/ml/gcc-patches/2012-02/msg00851.html, I though that
> "&& !defined (__MACH__)" should be changed but it seems the best
> way...

Actually on reflection this conditional may be why the proposed patch doesn't
work under 10.6 with Xcode 4.2. That configuration, unlike Xcode 4.5.2/4.6
on 10.7/10.8, doesn't cause HAVE_ELF_STYLE_WEAKREF to be defined and the
testcase was failing with undefined symbols for _ZdlPv. Since Xcode 4.2
has an undefined dynamic lookup bug and Iain said the patch worked with
Xcode 3.2.6 on 10.6, I didn't explore further. I can test tomorrow removing
the "&& !defined (__MACH__)" from libitm/eh_cpp.cc and libitm/alloc_cpp.cc
to see if that fixes the Xcode 4.2/10.6 combination. However, from the
original comments added to the code, this likely will cause the problem
with undefined weak refs being improperly handled at link to time to return
for older Xcode's. Do we know exactly which Xcode releases show that issue?
        Jack

> 
> Thanks for the precision about XCode.
> 
> Note that I cannot approve anything. So the patch must be approved by
> a GCC master ;)
> --
> Patrick
diff mbox

Patch

Index: gcc/config/i386/darwin.h
===================================================================
--- gcc/config/i386/darwin.h (revision 195764)
+++ gcc/config/i386/darwin.h (working copy)
@@ -131,8 +131,7 @@  extern int darwin_emit_branch_islands;
   "%{Ofast|ffast-math|funsafe-math-optimizations:crtfastmath.o%s} \
    %{mpc32:crtprec32.o%s} \
    %{mpc64:crtprec64.o%s} \
-   %{mpc80:crtprec80.o%s} \
-   %{fgnu-tm: -lcrttme.o}"
+   %{mpc80:crtprec80.o%s}" TM_DESTRUCTOR

 #undef SUBTARGET_EXTRA_SPECS
 #define SUBTARGET_EXTRA_SPECS                                   \
Index: gcc/config/darwin.h
===================================================================
--- gcc/config/darwin.h (revision 195764)
+++ gcc/config/darwin.h (working copy)
@@ -363,7 +363,8 @@  extern GTY(()) int darwin_ms_struct;
   %{shared-libgcc:%:version-compare(< 10.5 mmacosx-version-min= crt3.o%s)}"

 /* We want a destructor last in the list.  */
-#define ENDFILE_SPEC "%{fgnu-tm: -lcrttme.o}"
+#define TM_DESTRUCTOR "%{fgnu-tm: -lcrttme.o}"
+#define ENDFILE_SPEC TM_DESTRUCTOR

 #define DARWIN_EXTRA_SPECS \
   { "darwin_crt1", DARWIN_CRT1_SPEC }, \