| Message ID | CAEwic4Y5mMhokud4T-ZtA3GAHeWiQsGXfq=goo-2fj9Qm3GHSA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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