2013/4/9 Dave Korn <dave.korn.cyg...@gmail.com>:
> On 22/03/2013 08:44, Kai Tietz wrote:
>
>> 2013-03-22 Kai Tietz <kti...@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
