Patchwork fix PR 46376, mix LTO, MinGW and virtual base classes

login
register
mail settings
Submitter Rodrigo Rivas
Date Nov. 9, 2010, 4:42 p.m.
Message ID <AANLkTineqPJogG830iO1sagdXQQPxbA2nD7PRrZycEBT@mail.gmail.com>
Download mbox | patch
Permalink /patch/70559/
State New
Headers show

Comments

Rodrigo Rivas - Nov. 9, 2010, 4:42 p.m.
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.
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

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.
Richard Guenther - Nov. 9, 2010, 5 p.m.
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.
>
Rodrigo Rivas - Nov. 9, 2010, 5:27 p.m.
>> 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
Richard Guenther - Nov. 10, 2010, 11:27 a.m.
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
>
Rodrigo Rivas - Nov. 11, 2010, 9:27 a.m.
> I think the patch is ok then.

Well, if we agree it would be nice to see it commited.

Thanks.
Rodrigo
NightStrike - Nov. 11, 2010, 6:57 p.m.
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?
Kai Tietz - Nov. 11, 2010, 8:39 p.m.
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.
Kai Tietz - Nov. 11, 2010, 10:27 p.m.
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 ;)
Rodrigo Rivas - Nov. 11, 2010, 10:56 p.m.
>>> 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
Kai Tietz - Nov. 12, 2010, 9:07 a.m.
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

Patch

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;