Re: [patch libgcc]: Adjust cygming-crtbegin code to use weak

[Kai Tietz <ktietz70 at googlemail dot com>]

2013/4/9 Dave Korn <dave.korn.cygwin@gmail.com>:
> On 22/03/2013 08:44, Kai Tietz wrote:
>
>> 2013-03-22  Kai Tietz  <ktietz@redhat.com>
>>
>>       * config/i386/cygming-crtbegin.c (__register_frame_info): Make weak.
>>       (__deregister_frame_info): Likewise.
>
>     Hi Kai,
>
>   I read your explanation of the problem relating to x86-64 memory models over
> on the Cygwin dev list, and that explained your motivation for making this
> change; I see why it's not easy to get an *ABS* 0 reference there.  So,
> providing dummy versions of the functions makes perfect sense to me, and
> certainly won't cause problems for i686.  (I did a lot of testing, and the
> only problem I found is that a weak definition has to be provided on the
> linker command line *after* the file that contains the weak-with-zero-default
> definition if it is to override that; in the case here however we're going to
> be overriding the weak-with-default by a strong function declaration, so that
> issue does not arise.)
>
>   I still have a comment or two about the patch itself:
>
>> Index: libgcc/config/i386/cygming-crtbegin.c
>> ===================================================================
>> --- libgcc/config/i386/cygming-crtbegin.c     (Revision 196898)
>> +++ libgcc/config/i386/cygming-crtbegin.c     (Arbeitskopie)
>> @@ -46,15 +46,33 @@ see the files COPYING3 and COPYING.RUNTIME respect
>>  #define LIBGCJ_SONAME "libgcj_s.dll"
>>  #endif
>>
>> -
>> +#if DWARF2_UNWIND_INFO
>>  /* Make the declarations weak.  This is critical for
>>     _Jv_RegisterClasses because it lives in libgcj.a  */
>> -extern void __register_frame_info (const void *, struct object *)
>> +extern void __register_frame_info (__attribute__((unused)) const void *,
>> +                                __attribute__((unused)) struct object *)
>>                                  TARGET_ATTRIBUTE_WEAK;
>> -extern void *__deregister_frame_info (const void *)
>> +extern void *__deregister_frame_info (__attribute__((unused)) const void *)
>>                                     TARGET_ATTRIBUTE_WEAK;
>> -extern void _Jv_RegisterClasses (const void *) TARGET_ATTRIBUTE_WEAK;
>> +TARGET_ATTRIBUTE_WEAK void
>> +__register_frame_info (__attribute__((unused)) const void *p,
>> +                    __attribute__((unused)) struct object *o)
>> +{}
>
>   Braces should go on separate lines I think.
>
>> +TARGET_ATTRIBUTE_WEAK void *
>> +__deregister_frame_info (__attribute__((unused)) const void *p)
>> +{ return (void*) 0; }
>
>   Certainly here.
>
>> +#endif /* DWARF2_UNWIND_INFO */
>> +
>> +#if TARGET_USE_JCR_SECTION
>> +extern void _Jv_RegisterClasses (__attribute__((unused)) const void *)
>> +  TARGET_ATTRIBUTE_WEAK;
>> +
>> +TARGET_ATTRIBUTE_WEAK void
>> +_Jv_RegisterClasses (__attribute__((unused)) const void *p)
>> +{}
>> +#endif /* TARGET_USE_JCR_SECTION */
>> +
>>  #if defined(HAVE_LD_RO_RW_SECTION_MIXING)
>>  # define EH_FRAME_SECTION_CONST const
>>  #else
>
>   Also, now that you've provided a default weak definition of the functions in
> the file itself, it's no longer possible for the function pointer variables
> (register_frame_fn, register_class_fn, deregister_frame_fn) to be zero, so you
> should remove the if () tests on them and just call them unconditionally.

Hmm, well in standard-case you are right.  But well there is still a
chance that GetProcAddress returns NULL-pointer ...  I think we should
keep the if-check.

>     cheers,
>       DaveK
>

Kai