| Message ID | AANLkTineqPJogG830iO1sagdXQQPxbA2nD7PRrZycEBT@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 9, 2010 at 5:42 PM, Rodrigo Rivas <rodrigorivascosta@gmail.com> wrote: > Hi! > > The following one-line patch solves the issue reported in the cited PR. > As it happened, the virtual base class needs some thunk functions, and > these in MinGW are not marked weak. > Instead their DECL_COMDAT_GROUP is set, but curiously DECL_COMDAT is still 0. Hm, that sounds like the bug to me. Can you check why this is so? OTOH the DECL_ONE_ONLY check suggests that DECL_COMDAT is not to be trusted. Hmm, that confuses me. Thanks, Richard. > Adding this line solves the problem. > Probably, it also solves PR 45343 and PR 45759 (which would be > duplicates), but that would need checking. > Also, I wonder whether the check of DECL_COMDAT is necessary... > > Regards. > Rodrigo > > diff --git a/gcc/lto-symtab.c b/gcc/lto-symtab.c > index 1d90ab1..b83c75e 100644 > --- a/gcc/lto-symtab.c > +++ b/gcc/lto-symtab.c > @@ -444,6 +444,7 @@ lto_symtab_resolve_replaceable_p (lto_symtab_entry_t e) > { > if (DECL_EXTERNAL (e->decl) > || DECL_COMDAT (e->decl) > + || DECL_ONE_ONLY (e->decl) > || DECL_WEAK (e->decl)) > return true; > > Changelog: > > gcc/ > > 2010-11-09 Rodrigo Rivas Costa <rodrigorivascosta@gmail.com> > > PR lto/46376 > * lto-symtab.c (lto_symtab_resolve_replaceable_p): Use DECL_ONE_ONLY. >
>> Instead their DECL_COMDAT_GROUP is set, but curiously DECL_COMDAT is still 0. > > Hm, that sounds like the bug to me. Can you check why this is so? Yeah, that was what I though at first, but there are several things that made me doubt. First of all: this only fails when using -flto. When linking normally it works, so it must be something related to LTO. Second, this text from tree.h: /* Used in a DECL to indicate that, even if it TREE_PUBLIC, it need not be put out unless it is needed in this translation unit. Entities like this are shared across translation units (like weak entities), but are guaranteed to be generated by any translation unit that needs them, and therefore need not be put out anywhere where they are not needed. DECL_COMDAT is just a hint to the back-end; it is up to front-ends which set this flag to ensure that there will never be any harm, other than bloat, in putting out something which is DECL_COMDAT. */ #define DECL_COMDAT(NODE) \ (DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.comdat_flag) #define DECL_COMDAT_GROUP(NODE) \ (DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.comdat_group) /* Used in TREE_PUBLIC decls to indicate that copies of this DECL in multiple translation units should be merged. */ #define DECL_ONE_ONLY(NODE) (DECL_COMDAT_GROUP (NODE) != NULL_TREE) If we are to believe the comments, the DECL_ONE_ONLY is the one that should be true in the case of a virtual base thunk. Third, there are some other hints to this hidden in comments, here and there. See in varasm.c the comment to decl_replaceable_p. /* A replaceable function or variable is one which may be replaced at link-time with an entirely different definition, provided that the replacement has the same type. For example, functions declared with __attribute__((weak)) on most systems are replaceable. COMDAT functions are not replaceable, since all definitions of the function must be equivalent. It is important that COMDAT functions not be treated as replaceable so that use of C++ template instantiations is not penalized. */ And since my patched function is named lto_symtab_resolve_replaceable_p... well changing COMDAT to TRUE might be seen as risky. And four and last, the function decl_replaceable_p returns FALSE when DECL_COMDAT is TRUE, so... Also, virtual thunk functions pass through "make_decl_one_only" in varasm.c, that uses the MAKE_DECL_ONE_ONLY if defined. I tried to define it for MinGW the same as other targets: #define MAKE_DECL_ONE_ONLY(DECL) (DECL_WEAK (DECL) = 1) This solved the bug, but also made all my vtables disappear! Regards. Rodrigo
On Tue, Nov 9, 2010 at 6:27 PM, Rodrigo Rivas <rodrigorivascosta@gmail.com> wrote: >>> Instead their DECL_COMDAT_GROUP is set, but curiously DECL_COMDAT is still 0. >> >> Hm, that sounds like the bug to me. Can you check why this is so? > > Yeah, that was what I though at first, but there are several things > that made me doubt. > First of all: this only fails when using -flto. When linking normally > it works, so it must be something related to LTO. > Second, this text from tree.h: > > /* Used in a DECL to indicate that, even if it TREE_PUBLIC, it need > not be put out unless it is needed in this translation unit. > Entities like this are shared across translation units (like weak > entities), but are guaranteed to be generated by any translation > unit that needs them, and therefore need not be put out anywhere > where they are not needed. DECL_COMDAT is just a hint to the > back-end; it is up to front-ends which set this flag to ensure > that there will never be any harm, other than bloat, in putting out > something which is DECL_COMDAT. */ > #define DECL_COMDAT(NODE) \ > (DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.comdat_flag) > > #define DECL_COMDAT_GROUP(NODE) \ > (DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.comdat_group) > > /* Used in TREE_PUBLIC decls to indicate that copies of this DECL in > multiple translation units should be merged. */ > #define DECL_ONE_ONLY(NODE) (DECL_COMDAT_GROUP (NODE) != NULL_TREE) > > > If we are to believe the comments, the DECL_ONE_ONLY is the one that > should be true in the case of a virtual base thunk. > > Third, there are some other hints to this hidden in comments, here and > there. See in varasm.c the comment to decl_replaceable_p. > > /* A replaceable function or variable is one which may be replaced > at link-time with an entirely different definition, provided that the > replacement has the same type. For example, functions declared > with __attribute__((weak)) on most systems are replaceable. > > COMDAT functions are not replaceable, since all definitions of the > function must be equivalent. It is important that COMDAT functions > not be treated as replaceable so that use of C++ template > instantiations is not penalized. */ > > And since my patched function is named > lto_symtab_resolve_replaceable_p... well changing COMDAT to TRUE might > be seen as risky. > > And four and last, the function decl_replaceable_p returns FALSE when > DECL_COMDAT is TRUE, so... > > > Also, virtual thunk functions pass through "make_decl_one_only" in > varasm.c, that uses the MAKE_DECL_ONE_ONLY if defined. > I tried to define it for MinGW the same as other targets: > > #define MAKE_DECL_ONE_ONLY(DECL) (DECL_WEAK (DECL) = 1) > > This solved the bug, but also made all my vtables disappear! ;) I think the patch is ok then. Thanks, Richard. > Regards. > Rodrigo >
> I think the patch is ok then.
Well, if we agree it would be nice to see it commited.
Thanks.
Rodrigo
On Thu, Nov 11, 2010 at 4:27 AM, Rodrigo Rivas <rodrigorivascosta@gmail.com> wrote: >> I think the patch is ok then. > > Well, if we agree it would be nice to see it commited. > > Thanks. > Rodrigo > Does that mean you are committing it, or you need someone to?
2010/11/11 NightStrike <nightstrike@gmail.com>: > On Thu, Nov 11, 2010 at 4:27 AM, Rodrigo Rivas > <rodrigorivascosta@gmail.com> wrote: >>> I think the patch is ok then. >> >> Well, if we agree it would be nice to see it commited. >> >> Thanks. >> Rodrigo >> > > Does that mean you are committing it, or you need someone to? > I am just retesting it, and when successful I will commit it for you. Cheers, Kai PS: Checking also for those potential duplicates you mentioned.
2010/11/11 Kai Tietz <ktietz70@googlemail.com>: > 2010/11/11 NightStrike <nightstrike@gmail.com>: >> On Thu, Nov 11, 2010 at 4:27 AM, Rodrigo Rivas >> <rodrigorivascosta@gmail.com> wrote: >>>> I think the patch is ok then. >>> >>> Well, if we agree it would be nice to see it commited. >>> >>> Thanks. >>> Rodrigo >>> >> >> Does that mean you are committing it, or you need someone to? >> > > I am just retesting it, and when successful I will commit it for you. > > Cheers, > Kai > > PS: Checking also for those potential duplicates you mentioned. > > -- > | (\_/) This is Bunny. Copy and paste > | (='.'=) Bunny into your signature to help > | (")_(") him gain world domination > Thanks, your patch solves all three problems (PR lto/46376, PR 45343, and PR 45759). I will apply it tomorrow. Thanks, Kai PS: Using LTO easily busts the available memory of a 32-bit process on Windows targets as I had to notice ;)
>>> Does that mean you are committing it, or you need someone to? >>> I don't have commit permissions. Maybe I should note this on first post, shouldn't I? >> I am just retesting it, and when successful I will commit it for you. Cool! Thank you! > PS: Using LTO easily busts the available memory of a 32-bit process on > Windows targets as I had to notice ;) I didn't notice, but I didn't try to compile any big program. Do you think that the limit in Windows is significantly lower than in Linux? I don't see any reason for this to be the case. Regards. Rodrigo
2010/11/11 Rodrigo Rivas <rodrigorivascosta@gmail.com>: >>>> Does that mean you are committing it, or you need someone to? >>>> > I don't have commit permissions. Maybe I should note this on first > post, shouldn't I? > >>> I am just retesting it, and when successful I will commit it for you. > Cool! Thank you! > >> PS: Using LTO easily busts the available memory of a 32-bit process on >> Windows targets as I had to notice ;) > I didn't notice, but I didn't try to compile any big program. Do you > think that the limit in Windows is significantly lower than in Linux? > I don't see any reason for this to be the case. > > Regards. > Rodrigo > Committed at revision 166645. Kai
diff --git a/gcc/lto-symtab.c b/gcc/lto-symtab.c index 1d90ab1..b83c75e 100644 --- a/gcc/lto-symtab.c +++ b/gcc/lto-symtab.c @@ -444,6 +444,7 @@ lto_symtab_resolve_replaceable_p (lto_symtab_entry_t e) { if (DECL_EXTERNAL (e->decl) || DECL_COMDAT (e->decl) + || DECL_ONE_ONLY (e->decl) || DECL_WEAK (e->decl)) return true;