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

login
register
mail settings
Submitter Kai Tietz
Date March 22, 2013, 8:44 a.m.
Message ID <CAEwic4Y5mMhokud4T-ZtA3GAHeWiQsGXfq=goo-2fj9Qm3GHSA@mail.gmail.com>
Download mbox | patch
Permalink /patch/229922/
State New
Headers show

Comments

Kai Tietz - March 22, 2013, 8:44 a.m.
Hi,

this change is actual used by cygwin and is required for upcoming x64
cygwin target.

ChangeLog

2013-03-22  Kai Tietz  <ktietz@redhat.com>

	* config/i386/cygming-crtbegin.c (__register_frame_info): Make weak.
	(__deregister_frame_info): Likewise.

Tested for i686-pc-cygwin, and x86_64-pc-cygwin.  I will apply this
code tomorrow if there are no objections by other
Windows-target-maintainers.

Regards,
Kai
Dave Korn - March 22, 2013, 11:58 p.m.
On 22/03/2013 08:44, Kai Tietz wrote:
> Hi,
> 
> this change is actual used by cygwin and is required for upcoming x64
> cygwin target.
> 
> ChangeLog
> 
> 2013-03-22  Kai Tietz  <ktietz@redhat.com>
> 
> 	* config/i386/cygming-crtbegin.c (__register_frame_info): Make weak.
> 	(__deregister_frame_info): Likewise.
> 
> Tested for i686-pc-cygwin, and x86_64-pc-cygwin.  I will apply this
> code tomorrow if there are no objections by other
> Windows-target-maintainers.


  I don't think the ChangeLog entry is right; it doesn't make the declarations
weak, it supplies definitions.

  Also, can you explain the motivation for this change?  I don't see how it's
going to work right; from what I remember, we don't have weak definitions in
PE-COFF, just weak references.  How does the correct definition get chosen
when we may have two definitions in a final link?

    cheers,
      DaveK
Kai Tietz - March 23, 2013, 12:08 a.m.
2013/3/23 Dave Korn <dave.korn.cygwin@gmail.com>:
> On 22/03/2013 08:44, Kai Tietz wrote:
>> Hi,
>>
>> this change is actual used by cygwin and is required for upcoming x64
>> cygwin target.
>>
>> ChangeLog
>>
>> 2013-03-22  Kai Tietz  <ktietz@redhat.com>
>>
>>       * config/i386/cygming-crtbegin.c (__register_frame_info): Make weak.
>>       (__deregister_frame_info): Likewise.
>>
>> Tested for i686-pc-cygwin, and x86_64-pc-cygwin.  I will apply this
>> code tomorrow if there are no objections by other
>> Windows-target-maintainers.
>
>
>   I don't think the ChangeLog entry is right; it doesn't make the declarations
> weak, it supplies definitions.
>
>   Also, can you explain the motivation for this change?  I don't see how it's
> going to work right; from what I remember, we don't have weak definitions in
> PE-COFF, just weak references.  How does the correct definition get chosen
> when we may have two definitions in a final link?

Well, weak undefs are possible with pe-coff.  We ran into that by
porting cygwin to x64.
But you are right that pe-coff doesn't support undefines (weak or
none-weak) within final-link, so for a weak we need always a default
implementation.  This we added here.

If in a different TU a none-weak implementation is present, ld will
resolve that as usual.

>     cheers,
>       DaveK

Cheers,
Kai
Dave Korn - March 23, 2013, 12:23 a.m.
On 23/03/2013 00:08, Kai Tietz wrote:
> 2013/3/23 Dave Korn <dave.korn.cygwin@gmail.com>:

>>   Also, can you explain the motivation for this change?  I don't see how it's
>> going to work right; from what I remember, we don't have weak definitions in
>> PE-COFF, just weak references.  How does the correct definition get chosen
>> when we may have two definitions in a final link?
> 
> Well, weak undefs are possible with pe-coff.  We ran into that by
> porting cygwin to x64.
> But you are right that pe-coff doesn't support undefines (weak or
> none-weak) within final-link, so for a weak we need always a default
> implementation.  This we added here.

  I thought it does (support weak undefines within final link).  Weak
references with no definition resolve to zero, no?

    cheers,
      DaveK
Kai Tietz - March 23, 2013, 12:27 a.m.
2013/3/23 Dave Korn <dave.korn.cygwin@gmail.com>:
> On 23/03/2013 00:08, Kai Tietz wrote:
>> 2013/3/23 Dave Korn <dave.korn.cygwin@gmail.com>:
>
>>>   Also, can you explain the motivation for this change?  I don't see how it's
>>> going to work right; from what I remember, we don't have weak definitions in
>>> PE-COFF, just weak references.  How does the correct definition get chosen
>>> when we may have two definitions in a final link?
>>
>> Well, weak undefs are possible with pe-coff.  We ran into that by
>> porting cygwin to x64.
>> But you are right that pe-coff doesn't support undefines (weak or
>> none-weak) within final-link, so for a weak we need always a default
>> implementation.  This we added here.
>
>   I thought it does (support weak undefines within final link).  Weak
> references with no definition resolve to zero, no?
>
>     cheers,
>       DaveK
>

No, actual they aren't zero value, they are reported as undefined
symbol, which seems to me from perpective of pe-coff weak definition
the right thing to do.

Cheers,
Kai
Dave Korn - March 23, 2013, 12:41 a.m.
On 23/03/2013 00:27, Kai Tietz wrote:
> 2013/3/23 Dave Korn <dave.korn.cygwin@gmail.com>:
>> On 23/03/2013 00:08, Kai Tietz wrote:
>>> 2013/3/23 Dave Korn <dave.korn.cygwin@gmail.com>:
>>>>   Also, can you explain the motivation for this change?  I don't see how it's
>>>> going to work right; from what I remember, we don't have weak definitions in
>>>> PE-COFF, just weak references.  How does the correct definition get chosen
>>>> when we may have two definitions in a final link?
>>> Well, weak undefs are possible with pe-coff.  We ran into that by
>>> porting cygwin to x64.
>>> But you are right that pe-coff doesn't support undefines (weak or
>>> none-weak) within final-link, so for a weak we need always a default
>>> implementation.  This we added here.
>>   I thought it does (support weak undefines within final link).  Weak
>> references with no definition resolve to zero, no?

> No, actual they aren't zero value, they are reported as undefined
> symbol, which seems to me from perpective of pe-coff weak definition
> the right thing to do.

  I think something's going wrong.  From the PE-COFF specs:

> “Weak externals” are a mechanism for object files that allows flexibility
> at link time. A module can contain an unresolved external symbol (sym1),
> but it can also include an auxiliary record that indicates that if sym1 is
> not present at link time, another external symbol (sym2) is used to resolve
> references instead. If a definition of sym1 is linked, then an external
> reference to the symbol is resolved normally. If a definition of sym1 is
> not linked, then all references to the weak external for sym1 refer to sym2
> instead. The external symbol, sym2, must always be linked; typically, it is
> defined in the module that contains the weak reference to sym1.

  In gcc/binutils, what we do is automatically provide an aux record (sym2)
with the value *ABS* zero.  If a definition (strong, because we only have
strong definitions in PE-COFF) of the weakly-referenced symbol is provided,
that overrides the aux value of zero, but otherwise the zero value is used.

  IIRC, there's a binutils bugzilla entry about things not going well if two
strong definitions are provided to resolve a weak external.  I'll try and dig
it up.

    cheers,
      DaveK
Dave Korn - April 9, 2013, 5:58 a.m.
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.

    cheers,
      DaveK
Kai Tietz - April 9, 2013, 10:37 a.m.
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
Dave Korn - April 9, 2013, 5:33 p.m.
On 09/04/2013 11:37, Kai Tietz wrote:

> Hmm, well in standard-case you are right.  But well there is still a
> chance that GetProcAddress returns NULL-pointer ...  

  How would that actually happen?  Removing any of those functions from libgcc
or libjava would be a very serious ABI breakage; we're never going to do that.
 (And even if we do, the version number of the DLL would have to change and
LoadLibrary wouldn't return anything, unless we changed the shared lib string,
which would be the appropriate time to re-add a check.)  It's not critical,
but it's wasted code.

    cheers,
      DaveK

Patch

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)
+{}

+TARGET_ATTRIBUTE_WEAK void *
+__deregister_frame_info (__attribute__((unused)) const void *p)
+{ return (void*) 0; }
+#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