Patchwork [LTO] add externally_visible attribute when necessary with -fwhole-program and resolution file.

login
register
mail settings
Submitter Bingfeng Mei
Date June 10, 2010, 10:03 a.m.
Message ID <7FB04A5C213E9943A72EE127DB74F0ADA66675B582@SJEXCHCCR02.corp.ad.broadcom.com>
Download mbox | patch
Permalink /patch/55188/
State New
Headers show

Comments

Bingfeng Mei - June 10, 2010, 10:03 a.m.
Richard,

I tried to set externally_visible flags directly for cgraph node & varbool node.
Unfortunately, these flags seem to be set and used elsewhere too. The values
set in lto-symtab.c cannot be reliably passed to ipa.c (function_and_variable_visibility),
where the flags are actually re-computed.
 
      if (cgraph_externally_visible_p (node, whole_program))
        {
	  gcc_assert (!node->global.inlined_to);
	  node->local.externally_visible = true;
	}
      else
	node->local.externally_visible = false;

The flag for bar2 function is true before this piece of code even I doesn't set
it in lto-symtab.c. 

For now, I think attribute is still cleaner solution though
not cheap. The following is the updated patch.

Cheers,
Bingfeng

2010-06-10  Bingfeng Mei <bmei@broadcom.com>

	* lto-symbtab.c (lto_symtab_merge_decls_1): Add externally_visible
	attribute for declaration of LDPR_PREVAILING_DEF when compiling with
	-fwhole-program 
	(lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY for 
	internal resolver
        
> -----Original Message-----
> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> Sent: 09 June 2010 16:26
> To: Bingfeng Mei
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
> > I added an attribute because -fwhole-program/externally_visible is
> handled in ipa.c
> >
> > ...
> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES (node-
> >decl)))
> >    return true;
> > ...
> >
> > Adding attribute seems cleaner than changing flags, otherwise I need
> to change
> > handling in ipa.c as well.
> 
> True, but there is an externally_visible flag in cgraph_node,
> so I guess that attribute lookup is bogus.
> 
> Richard.
> 
> > Bingfeng
> >
> >> -----Original Message-----
> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> Sent: 09 June 2010 16:02
> >> To: Bingfeng Mei
> >> Cc: gcc-patches@gcc.gnu.org
> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> necessary with -fwhole-program and resolution file.
> >>
> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei <bmei@broadcom.com>
> wrote:
> >> > Hi,
> >> > This patch addresses issue discussed in
> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
> >> >
> >> > With the patch, any declaration which is resolved as
> >> LDPR_PREVAILING_DEF
> >> > and compiled with -fwhole-program is annotated with
> >> > attribute "externally_visible" if it doesn't exist already.
> >> > This eliminates the error-prone process of manual annotation
> >> > of the attribute when compiling mixed LTO/non-LTO applications.
> >> >
> >> > For the following test files:
> >> > a.c
> >> >
> >> > #include <string.h>
> >> > #include <stdio.h>
> >> > extern int foo(int);
> >> > /* Called by b.c, should not be optimized by -fwhole-program */
> >> > void bar()
> >> > {
> >> >  printf("bar\n");
> >> > }
> >> > /* Not used by others, should be optimized out by -fwhole-
> program*/
> >> > void bar2()
> >> > {
> >> >  printf("bar2\n");
> >> > }
> >> > extern int src[], dst[];
> >> > int vvvvvv;
> >> > int main()
> >> > {
> >> >  int ret;
> >> >  vvvvvv = 12;
> >> >  ret = foo(20);
> >> >  bar2();
> >> >  memcpy(dst, src, 100);
> >> >  return ret + 3;
> >> > }
> >> >
> >> > b.c
> >> >
> >> > #include <stdio.h>
> >> > int src[100];
> >> > int dst[100];
> >> > extern int vvvvvv;
> >> > extern void bar();
> >> > int foo(int c)
> >> > {
> >> >  printf("Hello world: %d\n", c);
> >> >  bar();
> >> >  return 1000 + vvvvvv;
> >> > }
> >> >
> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
> >> > ar cru libb.a b.o
> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-linker-
> plugin
> >> -o f -fwhole-program
> >> >
> >> > The code is compiled and linked correctly. bar & vvvvvv don't
> become
> >> static function
> >> > and cause link errors, whereas bar2 is inlined as expected.
> >> >
> >> >
> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >> >
> >> > Cheers,
> >> > Bingfeng Mei
> >> >
> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
> >> >
> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
> >> externally_visible
> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
> >> compiling with
> >> >        -fwhole-program
> >> >
> >> >
> >> > Index: lto-symtab.c
> >> >
> ===================================================================
> >> > --- lto-symtab.c        (revision 160463)
> >> > +++ lto-symtab.c        (working copy)
> >> > @@ -476,7 +476,19 @@
> >> >
> >> >   /* If the chain is already resolved there is nothing else to
> >> do.  */
> >> >   if (e->resolution != LDPR_UNKNOWN)
> >> > -    return;
> >> > +    {
> >> > +      /* Add externally_visible attribute for declaration of
> >> LDPR_PREVAILING_DEF */
> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
> flag_whole_program)
> >> > +        {
> >> > +          if (!lookup_attribute ("externally_visible",
> >> DECL_ATTRIBUTES (e->decl)))
> >> > +            {
> >> > +              DECL_ATTRIBUTES (e->decl)
> >> > +                = tree_cons (get_identifier
> ("externally_visible"),
> >> NULL_TREE,
> >> > +                             DECL_ATTRIBUTES (e->decl));
> >>
> >> I don't think we need to add an attribute here but we can play
> >> with some cgraph flags which is cheaper.
> >>
> >> Also I think this isn't really correct - not everything that
> prevails
> >> needs to be externally visible (in fact, you seem to simply
> >> remove the effect of -fwhole-program completely).
> >>
> >> A testcase that should still work is
> >>
> >> t1.c:
> >> void foo(void) { bar (); }
> >> t2.c
> >> extern void foo (void);
> >> void bar (void) {}
> >> void eliminate_me (void) {}
> >> int main()
> >> {
> >>    foo();
> >> }
> >>
> >> and eliminate_me should still be eliminated with -fwhole-program
> >> if you do
> >>
> >> gcc -c t1.c
> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
> >>
> >> Thus, the resolution file probably does not have the information
> >> you need (a list of references from outside of the LTO file set).
> >>
> >> Richard.
> >
> >
> >
Richard Guenther - June 10, 2010, 10:12 a.m.
On Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
> Richard,
>
> I tried to set externally_visible flags directly for cgraph node & varbool node.
> Unfortunately, these flags seem to be set and used elsewhere too. The values
> set in lto-symtab.c cannot be reliably passed to ipa.c (function_and_variable_visibility),
> where the flags are actually re-computed.
>
>      if (cgraph_externally_visible_p (node, whole_program))
>        {
>          gcc_assert (!node->global.inlined_to);
>          node->local.externally_visible = true;
>        }
>      else
>        node->local.externally_visible = false;
>
> The flag for bar2 function is true before this piece of code even I doesn't set
> it in lto-symtab.c.

I believe this is an error - Honza knows the code and will probably
comment.

> For now, I think attribute is still cleaner solution though
> not cheap. The following is the updated patch.

Thanks - I believe this is valuable but want to hear comments
from Honza as we shouldn't need to use attributes here.

Richard.

> Cheers,
> Bingfeng
>
> 2010-06-10  Bingfeng Mei <bmei@broadcom.com>
>
>        * lto-symbtab.c (lto_symtab_merge_decls_1): Add externally_visible
>        attribute for declaration of LDPR_PREVAILING_DEF when compiling with
>        -fwhole-program
>        (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY for
>        internal resolver
>
> Index: lto-symtab.c
> ===================================================================
> --- lto-symtab.c        (revision 160463)
> +++ lto-symtab.c        (working copy)
> @@ -530,11 +530,15 @@
>     return;
>
>  found:
> -  if (TREE_CODE (prevailing->decl) == VAR_DECL
> -      && TREE_READONLY (prevailing->decl))
> -    prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
> -  else
> -    prevailing->resolution = LDPR_PREVAILING_DEF;
> +  /* If current lto files represent the whole program,
> +    it is correct to use LDPR_PREVALING_DEF_IRONLY.
> +    If current lto files are part of whole program, internal
> +    resolver doesn't know if it is LDPR_PREVAILING_DEF
> +    or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to
> +    using -fwhole-program. Otherwise, it doesn't
> +    matter using either LDPR_PREVAILING_DEF or
> +    LDPR_PREVAILING_DEF_IRONLY */
> +  prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
>  }
>
>  /* Merge all decls in the symbol table chain to the prevailing decl and
> @@ -698,6 +702,18 @@
>       && TREE_CODE (prevailing->decl) != VAR_DECL)
>     prevailing->next = NULL;
>
> +
> +  /* Add externally_visible attribute for declaration of LDPR_PREVAILING_DEF */
> +  if (prevailing->resolution == LDPR_PREVAILING_DEF && flag_whole_program)
> +    {
> +      if (!lookup_attribute ("externally_visible",
> +                             DECL_ATTRIBUTES (prevailing->decl)))
> +        {
> +          DECL_ATTRIBUTES (prevailing->decl)
> +            = tree_cons (get_identifier ("externally_visible"), NULL_TREE,
> +                         DECL_ATTRIBUTES (prevailing->decl));
> +        }
> +    }
>   return 1;
>  }
>
>> -----Original Message-----
>> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> Sent: 09 June 2010 16:26
>> To: Bingfeng Mei
>> Cc: gcc-patches@gcc.gnu.org
>> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> necessary with -fwhole-program and resolution file.
>>
>> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
>> > I added an attribute because -fwhole-program/externally_visible is
>> handled in ipa.c
>> >
>> > ...
>> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES (node-
>> >decl)))
>> >    return true;
>> > ...
>> >
>> > Adding attribute seems cleaner than changing flags, otherwise I need
>> to change
>> > handling in ipa.c as well.
>>
>> True, but there is an externally_visible flag in cgraph_node,
>> so I guess that attribute lookup is bogus.
>>
>> Richard.
>>
>> > Bingfeng
>> >
>> >> -----Original Message-----
>> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> >> Sent: 09 June 2010 16:02
>> >> To: Bingfeng Mei
>> >> Cc: gcc-patches@gcc.gnu.org
>> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> >> necessary with -fwhole-program and resolution file.
>> >>
>> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei <bmei@broadcom.com>
>> wrote:
>> >> > Hi,
>> >> > This patch addresses issue discussed in
>> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
>> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
>> >> >
>> >> > With the patch, any declaration which is resolved as
>> >> LDPR_PREVAILING_DEF
>> >> > and compiled with -fwhole-program is annotated with
>> >> > attribute "externally_visible" if it doesn't exist already.
>> >> > This eliminates the error-prone process of manual annotation
>> >> > of the attribute when compiling mixed LTO/non-LTO applications.
>> >> >
>> >> > For the following test files:
>> >> > a.c
>> >> >
>> >> > #include <string.h>
>> >> > #include <stdio.h>
>> >> > extern int foo(int);
>> >> > /* Called by b.c, should not be optimized by -fwhole-program */
>> >> > void bar()
>> >> > {
>> >> >  printf("bar\n");
>> >> > }
>> >> > /* Not used by others, should be optimized out by -fwhole-
>> program*/
>> >> > void bar2()
>> >> > {
>> >> >  printf("bar2\n");
>> >> > }
>> >> > extern int src[], dst[];
>> >> > int vvvvvv;
>> >> > int main()
>> >> > {
>> >> >  int ret;
>> >> >  vvvvvv = 12;
>> >> >  ret = foo(20);
>> >> >  bar2();
>> >> >  memcpy(dst, src, 100);
>> >> >  return ret + 3;
>> >> > }
>> >> >
>> >> > b.c
>> >> >
>> >> > #include <stdio.h>
>> >> > int src[100];
>> >> > int dst[100];
>> >> > extern int vvvvvv;
>> >> > extern void bar();
>> >> > int foo(int c)
>> >> > {
>> >> >  printf("Hello world: %d\n", c);
>> >> >  bar();
>> >> >  return 1000 + vvvvvv;
>> >> > }
>> >> >
>> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
>> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
>> >> > ar cru libb.a b.o
>> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-linker-
>> plugin
>> >> -o f -fwhole-program
>> >> >
>> >> > The code is compiled and linked correctly. bar & vvvvvv don't
>> become
>> >> static function
>> >> > and cause link errors, whereas bar2 is inlined as expected.
>> >> >
>> >> >
>> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
>> >> >
>> >> > Cheers,
>> >> > Bingfeng Mei
>> >> >
>> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
>> >> >
>> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
>> >> externally_visible
>> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
>> >> compiling with
>> >> >        -fwhole-program
>> >> >
>> >> >
>> >> > Index: lto-symtab.c
>> >> >
>> ===================================================================
>> >> > --- lto-symtab.c        (revision 160463)
>> >> > +++ lto-symtab.c        (working copy)
>> >> > @@ -476,7 +476,19 @@
>> >> >
>> >> >   /* If the chain is already resolved there is nothing else to
>> >> do.  */
>> >> >   if (e->resolution != LDPR_UNKNOWN)
>> >> > -    return;
>> >> > +    {
>> >> > +      /* Add externally_visible attribute for declaration of
>> >> LDPR_PREVAILING_DEF */
>> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
>> flag_whole_program)
>> >> > +        {
>> >> > +          if (!lookup_attribute ("externally_visible",
>> >> DECL_ATTRIBUTES (e->decl)))
>> >> > +            {
>> >> > +              DECL_ATTRIBUTES (e->decl)
>> >> > +                = tree_cons (get_identifier
>> ("externally_visible"),
>> >> NULL_TREE,
>> >> > +                             DECL_ATTRIBUTES (e->decl));
>> >>
>> >> I don't think we need to add an attribute here but we can play
>> >> with some cgraph flags which is cheaper.
>> >>
>> >> Also I think this isn't really correct - not everything that
>> prevails
>> >> needs to be externally visible (in fact, you seem to simply
>> >> remove the effect of -fwhole-program completely).
>> >>
>> >> A testcase that should still work is
>> >>
>> >> t1.c:
>> >> void foo(void) { bar (); }
>> >> t2.c
>> >> extern void foo (void);
>> >> void bar (void) {}
>> >> void eliminate_me (void) {}
>> >> int main()
>> >> {
>> >>    foo();
>> >> }
>> >>
>> >> and eliminate_me should still be eliminated with -fwhole-program
>> >> if you do
>> >>
>> >> gcc -c t1.c
>> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
>> >>
>> >> Thus, the resolution file probably does not have the information
>> >> you need (a list of references from outside of the LTO file set).
>> >>
>> >> Richard.
>> >
>> >
>> >
>
>
>
Bingfeng Mei - June 10, 2010, 2:51 p.m.
Richard,
I figured out what happen with externally_visible flags in cgraph node/vnode.

In ipa.c (function_and_varialbe_visibility), the externally_visible flags are set to
true when compile with -flto because cgraph_externally_visible_p always returns true as
whole_program is false (regardless -fwhole-program option is set or not, which is 
always ignored in the first phase of LTO compilation). 

      if (cgraph_externally_visible_p (node, whole_program))
        {
	  gcc_assert (!node->global.inlined_to);
	  node->local.externally_visible = true;
	}

Then the flags are packed/unpacked from LTO byte code. That's why the externally_visible
flag is set to true for "bar2" function before executing my patch. 



Another issue is following code (your patch on 22/5). The vvvvvv
 variable in my example is going to be resolved by internal
 resolver as LDPR_PREVAILING_DEF_IRONLY instead of 
LDPR_PREVAILING_DEF by resolution file. Could you
Elaborate what is the exact issue here? 

	  /* The LTO plugin for gold doesn't handle common symbols
	     properly.  Let us choose manually.  */
	  if (DECL_COMMON (e->decl))
	    e->resolution = LDPR_UNKNOWN;

Thanks,
Bingfeng
> -----Original Message-----
> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> Sent: 10 June 2010 11:12
> To: Bingfeng Mei
> Cc: gcc-patches@gcc.gnu.org; Jan Hubicka
> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> On Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com>
> wrote:
> > Richard,
> >
> > I tried to set externally_visible flags directly for cgraph node &
> varbool node.
> > Unfortunately, these flags seem to be set and used elsewhere too. The
> values
> > set in lto-symtab.c cannot be reliably passed to ipa.c
> (function_and_variable_visibility),
> > where the flags are actually re-computed.
> >
> >      if (cgraph_externally_visible_p (node, whole_program))
> >        {
> >          gcc_assert (!node->global.inlined_to);
> >          node->local.externally_visible = true;
> >        }
> >      else
> >        node->local.externally_visible = false;
> >
> > The flag for bar2 function is true before this piece of code even I
> doesn't set
> > it in lto-symtab.c.
> 
> I believe this is an error - Honza knows the code and will probably
> comment.
> 
> > For now, I think attribute is still cleaner solution though
> > not cheap. The following is the updated patch.
> 
> Thanks - I believe this is valuable but want to hear comments
> from Honza as we shouldn't need to use attributes here.
> 
> Richard.
> 
> > Cheers,
> > Bingfeng
> >
> > 2010-06-10  Bingfeng Mei <bmei@broadcom.com>
> >
> >        * lto-symbtab.c (lto_symtab_merge_decls_1): Add
> externally_visible
> >        attribute for declaration of LDPR_PREVAILING_DEF when
> compiling with
> >        -fwhole-program
> >        (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY
> for
> >        internal resolver
> >
> > Index: lto-symtab.c
> > ===================================================================
> > --- lto-symtab.c        (revision 160463)
> > +++ lto-symtab.c        (working copy)
> > @@ -530,11 +530,15 @@
> >     return;
> >
> >  found:
> > -  if (TREE_CODE (prevailing->decl) == VAR_DECL
> > -      && TREE_READONLY (prevailing->decl))
> > -    prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
> > -  else
> > -    prevailing->resolution = LDPR_PREVAILING_DEF;
> > +  /* If current lto files represent the whole program,
> > +    it is correct to use LDPR_PREVALING_DEF_IRONLY.
> > +    If current lto files are part of whole program, internal
> > +    resolver doesn't know if it is LDPR_PREVAILING_DEF
> > +    or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to
> > +    using -fwhole-program. Otherwise, it doesn't
> > +    matter using either LDPR_PREVAILING_DEF or
> > +    LDPR_PREVAILING_DEF_IRONLY */
> > +  prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
> >  }
> >
> >  /* Merge all decls in the symbol table chain to the prevailing decl
> and
> > @@ -698,6 +702,18 @@
> >       && TREE_CODE (prevailing->decl) != VAR_DECL)
> >     prevailing->next = NULL;
> >
> > +
> > +  /* Add externally_visible attribute for declaration of
> LDPR_PREVAILING_DEF */
> > +  if (prevailing->resolution == LDPR_PREVAILING_DEF &&
> flag_whole_program)
> > +    {
> > +      if (!lookup_attribute ("externally_visible",
> > +                             DECL_ATTRIBUTES (prevailing->decl)))
> > +        {
> > +          DECL_ATTRIBUTES (prevailing->decl)
> > +            = tree_cons (get_identifier ("externally_visible"),
> NULL_TREE,
> > +                         DECL_ATTRIBUTES (prevailing->decl));
> > +        }
> > +    }
> >   return 1;
> >  }
> >
> >> -----Original Message-----
> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> Sent: 09 June 2010 16:26
> >> To: Bingfeng Mei
> >> Cc: gcc-patches@gcc.gnu.org
> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> necessary with -fwhole-program and resolution file.
> >>
> >> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com>
> wrote:
> >> > I added an attribute because -fwhole-program/externally_visible is
> >> handled in ipa.c
> >> >
> >> > ...
> >> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES
> (node-
> >> >decl)))
> >> >    return true;
> >> > ...
> >> >
> >> > Adding attribute seems cleaner than changing flags, otherwise I
> need
> >> to change
> >> > handling in ipa.c as well.
> >>
> >> True, but there is an externally_visible flag in cgraph_node,
> >> so I guess that attribute lookup is bogus.
> >>
> >> Richard.
> >>
> >> > Bingfeng
> >> >
> >> >> -----Original Message-----
> >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> >> Sent: 09 June 2010 16:02
> >> >> To: Bingfeng Mei
> >> >> Cc: gcc-patches@gcc.gnu.org
> >> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> >> necessary with -fwhole-program and resolution file.
> >> >>
> >> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei <bmei@broadcom.com>
> >> wrote:
> >> >> > Hi,
> >> >> > This patch addresses issue discussed in
> >> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
> >> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
> >> >> >
> >> >> > With the patch, any declaration which is resolved as
> >> >> LDPR_PREVAILING_DEF
> >> >> > and compiled with -fwhole-program is annotated with
> >> >> > attribute "externally_visible" if it doesn't exist already.
> >> >> > This eliminates the error-prone process of manual annotation
> >> >> > of the attribute when compiling mixed LTO/non-LTO applications.
> >> >> >
> >> >> > For the following test files:
> >> >> > a.c
> >> >> >
> >> >> > #include <string.h>
> >> >> > #include <stdio.h>
> >> >> > extern int foo(int);
> >> >> > /* Called by b.c, should not be optimized by -fwhole-program */
> >> >> > void bar()
> >> >> > {
> >> >> >  printf("bar\n");
> >> >> > }
> >> >> > /* Not used by others, should be optimized out by -fwhole-
> >> program*/
> >> >> > void bar2()
> >> >> > {
> >> >> >  printf("bar2\n");
> >> >> > }
> >> >> > extern int src[], dst[];
> >> >> > int vvvvvv;
> >> >> > int main()
> >> >> > {
> >> >> >  int ret;
> >> >> >  vvvvvv = 12;
> >> >> >  ret = foo(20);
> >> >> >  bar2();
> >> >> >  memcpy(dst, src, 100);
> >> >> >  return ret + 3;
> >> >> > }
> >> >> >
> >> >> > b.c
> >> >> >
> >> >> > #include <stdio.h>
> >> >> > int src[100];
> >> >> > int dst[100];
> >> >> > extern int vvvvvv;
> >> >> > extern void bar();
> >> >> > int foo(int c)
> >> >> > {
> >> >> >  printf("Hello world: %d\n", c);
> >> >> >  bar();
> >> >> >  return 1000 + vvvvvv;
> >> >> > }
> >> >> >
> >> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
> >> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
> >> >> > ar cru libb.a b.o
> >> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-linker-
> >> plugin
> >> >> -o f -fwhole-program
> >> >> >
> >> >> > The code is compiled and linked correctly. bar & vvvvvv don't
> >> become
> >> >> static function
> >> >> > and cause link errors, whereas bar2 is inlined as expected.
> >> >> >
> >> >> >
> >> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >> >> >
> >> >> > Cheers,
> >> >> > Bingfeng Mei
> >> >> >
> >> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
> >> >> >
> >> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
> >> >> externally_visible
> >> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
> >> >> compiling with
> >> >> >        -fwhole-program
> >> >> >
> >> >> >
> >> >> > Index: lto-symtab.c
> >> >> >
> >> ===================================================================
> >> >> > --- lto-symtab.c        (revision 160463)
> >> >> > +++ lto-symtab.c        (working copy)
> >> >> > @@ -476,7 +476,19 @@
> >> >> >
> >> >> >   /* If the chain is already resolved there is nothing else to
> >> >> do.  */
> >> >> >   if (e->resolution != LDPR_UNKNOWN)
> >> >> > -    return;
> >> >> > +    {
> >> >> > +      /* Add externally_visible attribute for declaration of
> >> >> LDPR_PREVAILING_DEF */
> >> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
> >> flag_whole_program)
> >> >> > +        {
> >> >> > +          if (!lookup_attribute ("externally_visible",
> >> >> DECL_ATTRIBUTES (e->decl)))
> >> >> > +            {
> >> >> > +              DECL_ATTRIBUTES (e->decl)
> >> >> > +                = tree_cons (get_identifier
> >> ("externally_visible"),
> >> >> NULL_TREE,
> >> >> > +                             DECL_ATTRIBUTES (e->decl));
> >> >>
> >> >> I don't think we need to add an attribute here but we can play
> >> >> with some cgraph flags which is cheaper.
> >> >>
> >> >> Also I think this isn't really correct - not everything that
> >> prevails
> >> >> needs to be externally visible (in fact, you seem to simply
> >> >> remove the effect of -fwhole-program completely).
> >> >>
> >> >> A testcase that should still work is
> >> >>
> >> >> t1.c:
> >> >> void foo(void) { bar (); }
> >> >> t2.c
> >> >> extern void foo (void);
> >> >> void bar (void) {}
> >> >> void eliminate_me (void) {}
> >> >> int main()
> >> >> {
> >> >>    foo();
> >> >> }
> >> >>
> >> >> and eliminate_me should still be eliminated with -fwhole-program
> >> >> if you do
> >> >>
> >> >> gcc -c t1.c
> >> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
> >> >>
> >> >> Thus, the resolution file probably does not have the information
> >> >> you need (a list of references from outside of the LTO file set).
> >> >>
> >> >> Richard.
> >> >
> >> >
> >> >
> >
> >
> >
Richard Guenther - June 10, 2010, 4:11 p.m.
On Thu, Jun 10, 2010 at 4:51 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
> Richard,
> I figured out what happen with externally_visible flags in cgraph node/vnode.
>
> In ipa.c (function_and_varialbe_visibility), the externally_visible flags are set to
> true when compile with -flto because cgraph_externally_visible_p always returns true as
> whole_program is false (regardless -fwhole-program option is set or not, which is
> always ignored in the first phase of LTO compilation).
>
>      if (cgraph_externally_visible_p (node, whole_program))
>        {
>          gcc_assert (!node->global.inlined_to);
>          node->local.externally_visible = true;
>        }
>
> Then the flags are packed/unpacked from LTO byte code. That's why the externally_visible
> flag is set to true for "bar2" function before executing my patch.
>
>
>
> Another issue is following code (your patch on 22/5). The vvvvvv
>  variable in my example is going to be resolved by internal
>  resolver as LDPR_PREVAILING_DEF_IRONLY instead of
> LDPR_PREVAILING_DEF by resolution file. Could you
> Elaborate what is the exact issue here?

Gold does not keep track of which symbol from which object
file it will end up using for commons but instead simply picks
the first one.  We rely on the fact that we get the one with
the largest size - with gold we even can end up with
a non-prevailing one.  And there's some version of gold which doesn't
resolve commons at all.

So we need to run through our own resolution code here.

An example is

extern int i;
--
int i;

where gold can claim that 'extern int i' was prevailing.

>          /* The LTO plugin for gold doesn't handle common symbols
>             properly.  Let us choose manually.  */
>          if (DECL_COMMON (e->decl))
>            e->resolution = LDPR_UNKNOWN;
>
> Thanks,
> Bingfeng
>> -----Original Message-----
>> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> Sent: 10 June 2010 11:12
>> To: Bingfeng Mei
>> Cc: gcc-patches@gcc.gnu.org; Jan Hubicka
>> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> necessary with -fwhole-program and resolution file.
>>
>> On Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com>
>> wrote:
>> > Richard,
>> >
>> > I tried to set externally_visible flags directly for cgraph node &
>> varbool node.
>> > Unfortunately, these flags seem to be set and used elsewhere too. The
>> values
>> > set in lto-symtab.c cannot be reliably passed to ipa.c
>> (function_and_variable_visibility),
>> > where the flags are actually re-computed.
>> >
>> >      if (cgraph_externally_visible_p (node, whole_program))
>> >        {
>> >          gcc_assert (!node->global.inlined_to);
>> >          node->local.externally_visible = true;
>> >        }
>> >      else
>> >        node->local.externally_visible = false;
>> >
>> > The flag for bar2 function is true before this piece of code even I
>> doesn't set
>> > it in lto-symtab.c.
>>
>> I believe this is an error - Honza knows the code and will probably
>> comment.
>>
>> > For now, I think attribute is still cleaner solution though
>> > not cheap. The following is the updated patch.
>>
>> Thanks - I believe this is valuable but want to hear comments
>> from Honza as we shouldn't need to use attributes here.
>>
>> Richard.
>>
>> > Cheers,
>> > Bingfeng
>> >
>> > 2010-06-10  Bingfeng Mei <bmei@broadcom.com>
>> >
>> >        * lto-symbtab.c (lto_symtab_merge_decls_1): Add
>> externally_visible
>> >        attribute for declaration of LDPR_PREVAILING_DEF when
>> compiling with
>> >        -fwhole-program
>> >        (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY
>> for
>> >        internal resolver
>> >
>> > Index: lto-symtab.c
>> > ===================================================================
>> > --- lto-symtab.c        (revision 160463)
>> > +++ lto-symtab.c        (working copy)
>> > @@ -530,11 +530,15 @@
>> >     return;
>> >
>> >  found:
>> > -  if (TREE_CODE (prevailing->decl) == VAR_DECL
>> > -      && TREE_READONLY (prevailing->decl))
>> > -    prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
>> > -  else
>> > -    prevailing->resolution = LDPR_PREVAILING_DEF;
>> > +  /* If current lto files represent the whole program,
>> > +    it is correct to use LDPR_PREVALING_DEF_IRONLY.
>> > +    If current lto files are part of whole program, internal
>> > +    resolver doesn't know if it is LDPR_PREVAILING_DEF
>> > +    or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to
>> > +    using -fwhole-program. Otherwise, it doesn't
>> > +    matter using either LDPR_PREVAILING_DEF or
>> > +    LDPR_PREVAILING_DEF_IRONLY */
>> > +  prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
>> >  }
>> >
>> >  /* Merge all decls in the symbol table chain to the prevailing decl
>> and
>> > @@ -698,6 +702,18 @@
>> >       && TREE_CODE (prevailing->decl) != VAR_DECL)
>> >     prevailing->next = NULL;
>> >
>> > +
>> > +  /* Add externally_visible attribute for declaration of
>> LDPR_PREVAILING_DEF */
>> > +  if (prevailing->resolution == LDPR_PREVAILING_DEF &&
>> flag_whole_program)
>> > +    {
>> > +      if (!lookup_attribute ("externally_visible",
>> > +                             DECL_ATTRIBUTES (prevailing->decl)))
>> > +        {
>> > +          DECL_ATTRIBUTES (prevailing->decl)
>> > +            = tree_cons (get_identifier ("externally_visible"),
>> NULL_TREE,
>> > +                         DECL_ATTRIBUTES (prevailing->decl));
>> > +        }
>> > +    }
>> >   return 1;
>> >  }
>> >
>> >> -----Original Message-----
>> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> >> Sent: 09 June 2010 16:26
>> >> To: Bingfeng Mei
>> >> Cc: gcc-patches@gcc.gnu.org
>> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> >> necessary with -fwhole-program and resolution file.
>> >>
>> >> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com>
>> wrote:
>> >> > I added an attribute because -fwhole-program/externally_visible is
>> >> handled in ipa.c
>> >> >
>> >> > ...
>> >> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES
>> (node-
>> >> >decl)))
>> >> >    return true;
>> >> > ...
>> >> >
>> >> > Adding attribute seems cleaner than changing flags, otherwise I
>> need
>> >> to change
>> >> > handling in ipa.c as well.
>> >>
>> >> True, but there is an externally_visible flag in cgraph_node,
>> >> so I guess that attribute lookup is bogus.
>> >>
>> >> Richard.
>> >>
>> >> > Bingfeng
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> >> >> Sent: 09 June 2010 16:02
>> >> >> To: Bingfeng Mei
>> >> >> Cc: gcc-patches@gcc.gnu.org
>> >> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> >> >> necessary with -fwhole-program and resolution file.
>> >> >>
>> >> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei <bmei@broadcom.com>
>> >> wrote:
>> >> >> > Hi,
>> >> >> > This patch addresses issue discussed in
>> >> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
>> >> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
>> >> >> >
>> >> >> > With the patch, any declaration which is resolved as
>> >> >> LDPR_PREVAILING_DEF
>> >> >> > and compiled with -fwhole-program is annotated with
>> >> >> > attribute "externally_visible" if it doesn't exist already.
>> >> >> > This eliminates the error-prone process of manual annotation
>> >> >> > of the attribute when compiling mixed LTO/non-LTO applications.
>> >> >> >
>> >> >> > For the following test files:
>> >> >> > a.c
>> >> >> >
>> >> >> > #include <string.h>
>> >> >> > #include <stdio.h>
>> >> >> > extern int foo(int);
>> >> >> > /* Called by b.c, should not be optimized by -fwhole-program */
>> >> >> > void bar()
>> >> >> > {
>> >> >> >  printf("bar\n");
>> >> >> > }
>> >> >> > /* Not used by others, should be optimized out by -fwhole-
>> >> program*/
>> >> >> > void bar2()
>> >> >> > {
>> >> >> >  printf("bar2\n");
>> >> >> > }
>> >> >> > extern int src[], dst[];
>> >> >> > int vvvvvv;
>> >> >> > int main()
>> >> >> > {
>> >> >> >  int ret;
>> >> >> >  vvvvvv = 12;
>> >> >> >  ret = foo(20);
>> >> >> >  bar2();
>> >> >> >  memcpy(dst, src, 100);
>> >> >> >  return ret + 3;
>> >> >> > }
>> >> >> >
>> >> >> > b.c
>> >> >> >
>> >> >> > #include <stdio.h>
>> >> >> > int src[100];
>> >> >> > int dst[100];
>> >> >> > extern int vvvvvv;
>> >> >> > extern void bar();
>> >> >> > int foo(int c)
>> >> >> > {
>> >> >> >  printf("Hello world: %d\n", c);
>> >> >> >  bar();
>> >> >> >  return 1000 + vvvvvv;
>> >> >> > }
>> >> >> >
>> >> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
>> >> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
>> >> >> > ar cru libb.a b.o
>> >> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-linker-
>> >> plugin
>> >> >> -o f -fwhole-program
>> >> >> >
>> >> >> > The code is compiled and linked correctly. bar & vvvvvv don't
>> >> become
>> >> >> static function
>> >> >> > and cause link errors, whereas bar2 is inlined as expected.
>> >> >> >
>> >> >> >
>> >> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
>> >> >> >
>> >> >> > Cheers,
>> >> >> > Bingfeng Mei
>> >> >> >
>> >> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
>> >> >> >
>> >> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
>> >> >> externally_visible
>> >> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
>> >> >> compiling with
>> >> >> >        -fwhole-program
>> >> >> >
>> >> >> >
>> >> >> > Index: lto-symtab.c
>> >> >> >
>> >> ===================================================================
>> >> >> > --- lto-symtab.c        (revision 160463)
>> >> >> > +++ lto-symtab.c        (working copy)
>> >> >> > @@ -476,7 +476,19 @@
>> >> >> >
>> >> >> >   /* If the chain is already resolved there is nothing else to
>> >> >> do.  */
>> >> >> >   if (e->resolution != LDPR_UNKNOWN)
>> >> >> > -    return;
>> >> >> > +    {
>> >> >> > +      /* Add externally_visible attribute for declaration of
>> >> >> LDPR_PREVAILING_DEF */
>> >> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
>> >> flag_whole_program)
>> >> >> > +        {
>> >> >> > +          if (!lookup_attribute ("externally_visible",
>> >> >> DECL_ATTRIBUTES (e->decl)))
>> >> >> > +            {
>> >> >> > +              DECL_ATTRIBUTES (e->decl)
>> >> >> > +                = tree_cons (get_identifier
>> >> ("externally_visible"),
>> >> >> NULL_TREE,
>> >> >> > +                             DECL_ATTRIBUTES (e->decl));
>> >> >>
>> >> >> I don't think we need to add an attribute here but we can play
>> >> >> with some cgraph flags which is cheaper.
>> >> >>
>> >> >> Also I think this isn't really correct - not everything that
>> >> prevails
>> >> >> needs to be externally visible (in fact, you seem to simply
>> >> >> remove the effect of -fwhole-program completely).
>> >> >>
>> >> >> A testcase that should still work is
>> >> >>
>> >> >> t1.c:
>> >> >> void foo(void) { bar (); }
>> >> >> t2.c
>> >> >> extern void foo (void);
>> >> >> void bar (void) {}
>> >> >> void eliminate_me (void) {}
>> >> >> int main()
>> >> >> {
>> >> >>    foo();
>> >> >> }
>> >> >>
>> >> >> and eliminate_me should still be eliminated with -fwhole-program
>> >> >> if you do
>> >> >>
>> >> >> gcc -c t1.c
>> >> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
>> >> >>
>> >> >> Thus, the resolution file probably does not have the information
>> >> >> you need (a list of references from outside of the LTO file set).
>> >> >>
>> >> >> Richard.
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>
>
>
Bingfeng Mei - June 10, 2010, 4:23 p.m.
Actually, I got correct results from gold
x.c
extern int i;

y.c
int i;

~/work/install-x86/bin/gcc  y.o x.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps
x.res:
2
y.o 1
78 PREVAILING_DEF_IRONLY i
x.o 0

~/work/install-x86/bin/gcc  x.o y.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps
y.res
2
x.o 0
y.o 1
78 PREVAILING_DEF_IRONLY i


I am using binutils-2.20-51.

Bingfeng
> -----Original Message-----
> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> Sent: 10 June 2010 17:12
> To: Bingfeng Mei
> Cc: gcc-patches@gcc.gnu.org; Jan Hubicka
> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> necessary with -fwhole-program and resolution file.
> 
> On Thu, Jun 10, 2010 at 4:51 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
> > Richard,
> > I figured out what happen with externally_visible flags in cgraph
> node/vnode.
> >
> > In ipa.c (function_and_varialbe_visibility), the externally_visible
> flags are set to
> > true when compile with -flto because cgraph_externally_visible_p
> always returns true as
> > whole_program is false (regardless -fwhole-program option is set or
> not, which is
> > always ignored in the first phase of LTO compilation).
> >
> >      if (cgraph_externally_visible_p (node, whole_program))
> >        {
> >          gcc_assert (!node->global.inlined_to);
> >          node->local.externally_visible = true;
> >        }
> >
> > Then the flags are packed/unpacked from LTO byte code. That's why the
> externally_visible
> > flag is set to true for "bar2" function before executing my patch.
> >
> >
> >
> > Another issue is following code (your patch on 22/5). The vvvvvv
> >  variable in my example is going to be resolved by internal
> >  resolver as LDPR_PREVAILING_DEF_IRONLY instead of
> > LDPR_PREVAILING_DEF by resolution file. Could you
> > Elaborate what is the exact issue here?
> 
> Gold does not keep track of which symbol from which object
> file it will end up using for commons but instead simply picks
> the first one.  We rely on the fact that we get the one with
> the largest size - with gold we even can end up with
> a non-prevailing one.  And there's some version of gold which doesn't
> resolve commons at all.
> 
> So we need to run through our own resolution code here.
> 
> An example is
> 
> extern int i;
> --
> int i;
> 
> where gold can claim that 'extern int i' was prevailing.
> 
> >          /* The LTO plugin for gold doesn't handle common symbols
> >             properly.  Let us choose manually.  */
> >          if (DECL_COMMON (e->decl))
> >            e->resolution = LDPR_UNKNOWN;
> >
> > Thanks,
> > Bingfeng
> >> -----Original Message-----
> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> Sent: 10 June 2010 11:12
> >> To: Bingfeng Mei
> >> Cc: gcc-patches@gcc.gnu.org; Jan Hubicka
> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> necessary with -fwhole-program and resolution file.
> >>
> >> On Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com>
> >> wrote:
> >> > Richard,
> >> >
> >> > I tried to set externally_visible flags directly for cgraph node &
> >> varbool node.
> >> > Unfortunately, these flags seem to be set and used elsewhere too.
> The
> >> values
> >> > set in lto-symtab.c cannot be reliably passed to ipa.c
> >> (function_and_variable_visibility),
> >> > where the flags are actually re-computed.
> >> >
> >> >      if (cgraph_externally_visible_p (node, whole_program))
> >> >        {
> >> >          gcc_assert (!node->global.inlined_to);
> >> >          node->local.externally_visible = true;
> >> >        }
> >> >      else
> >> >        node->local.externally_visible = false;
> >> >
> >> > The flag for bar2 function is true before this piece of code even
> I
> >> doesn't set
> >> > it in lto-symtab.c.
> >>
> >> I believe this is an error - Honza knows the code and will probably
> >> comment.
> >>
> >> > For now, I think attribute is still cleaner solution though
> >> > not cheap. The following is the updated patch.
> >>
> >> Thanks - I believe this is valuable but want to hear comments
> >> from Honza as we shouldn't need to use attributes here.
> >>
> >> Richard.
> >>
> >> > Cheers,
> >> > Bingfeng
> >> >
> >> > 2010-06-10  Bingfeng Mei <bmei@broadcom.com>
> >> >
> >> >        * lto-symbtab.c (lto_symtab_merge_decls_1): Add
> >> externally_visible
> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
> >> compiling with
> >> >        -fwhole-program
> >> >        (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY
> >> for
> >> >        internal resolver
> >> >
> >> > Index: lto-symtab.c
> >> >
> ===================================================================
> >> > --- lto-symtab.c        (revision 160463)
> >> > +++ lto-symtab.c        (working copy)
> >> > @@ -530,11 +530,15 @@
> >> >     return;
> >> >
> >> >  found:
> >> > -  if (TREE_CODE (prevailing->decl) == VAR_DECL
> >> > -      && TREE_READONLY (prevailing->decl))
> >> > -    prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
> >> > -  else
> >> > -    prevailing->resolution = LDPR_PREVAILING_DEF;
> >> > +  /* If current lto files represent the whole program,
> >> > +    it is correct to use LDPR_PREVALING_DEF_IRONLY.
> >> > +    If current lto files are part of whole program, internal
> >> > +    resolver doesn't know if it is LDPR_PREVAILING_DEF
> >> > +    or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to
> >> > +    using -fwhole-program. Otherwise, it doesn't
> >> > +    matter using either LDPR_PREVAILING_DEF or
> >> > +    LDPR_PREVAILING_DEF_IRONLY */
> >> > +  prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
> >> >  }
> >> >
> >> >  /* Merge all decls in the symbol table chain to the prevailing
> decl
> >> and
> >> > @@ -698,6 +702,18 @@
> >> >       && TREE_CODE (prevailing->decl) != VAR_DECL)
> >> >     prevailing->next = NULL;
> >> >
> >> > +
> >> > +  /* Add externally_visible attribute for declaration of
> >> LDPR_PREVAILING_DEF */
> >> > +  if (prevailing->resolution == LDPR_PREVAILING_DEF &&
> >> flag_whole_program)
> >> > +    {
> >> > +      if (!lookup_attribute ("externally_visible",
> >> > +                             DECL_ATTRIBUTES (prevailing->decl)))
> >> > +        {
> >> > +          DECL_ATTRIBUTES (prevailing->decl)
> >> > +            = tree_cons (get_identifier ("externally_visible"),
> >> NULL_TREE,
> >> > +                         DECL_ATTRIBUTES (prevailing->decl));
> >> > +        }
> >> > +    }
> >> >   return 1;
> >> >  }
> >> >
> >> >> -----Original Message-----
> >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> >> Sent: 09 June 2010 16:26
> >> >> To: Bingfeng Mei
> >> >> Cc: gcc-patches@gcc.gnu.org
> >> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
> >> >> necessary with -fwhole-program and resolution file.
> >> >>
> >> >> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com>
> >> wrote:
> >> >> > I added an attribute because -fwhole-program/externally_visible
> is
> >> >> handled in ipa.c
> >> >> >
> >> >> > ...
> >> >> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES
> >> (node-
> >> >> >decl)))
> >> >> >    return true;
> >> >> > ...
> >> >> >
> >> >> > Adding attribute seems cleaner than changing flags, otherwise I
> >> need
> >> >> to change
> >> >> > handling in ipa.c as well.
> >> >>
> >> >> True, but there is an externally_visible flag in cgraph_node,
> >> >> so I guess that attribute lookup is bogus.
> >> >>
> >> >> Richard.
> >> >>
> >> >> > Bingfeng
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
> >> >> >> Sent: 09 June 2010 16:02
> >> >> >> To: Bingfeng Mei
> >> >> >> Cc: gcc-patches@gcc.gnu.org
> >> >> >> Subject: Re: [PATCH, LTO] add externally_visible attribute
> when
> >> >> >> necessary with -fwhole-program and resolution file.
> >> >> >>
> >> >> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei
> <bmei@broadcom.com>
> >> >> wrote:
> >> >> >> > Hi,
> >> >> >> > This patch addresses issue discussed in
> >> >> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
> >> >> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
> >> >> >> >
> >> >> >> > With the patch, any declaration which is resolved as
> >> >> >> LDPR_PREVAILING_DEF
> >> >> >> > and compiled with -fwhole-program is annotated with
> >> >> >> > attribute "externally_visible" if it doesn't exist already.
> >> >> >> > This eliminates the error-prone process of manual annotation
> >> >> >> > of the attribute when compiling mixed LTO/non-LTO
> applications.
> >> >> >> >
> >> >> >> > For the following test files:
> >> >> >> > a.c
> >> >> >> >
> >> >> >> > #include <string.h>
> >> >> >> > #include <stdio.h>
> >> >> >> > extern int foo(int);
> >> >> >> > /* Called by b.c, should not be optimized by -fwhole-program
> */
> >> >> >> > void bar()
> >> >> >> > {
> >> >> >> >  printf("bar\n");
> >> >> >> > }
> >> >> >> > /* Not used by others, should be optimized out by -fwhole-
> >> >> program*/
> >> >> >> > void bar2()
> >> >> >> > {
> >> >> >> >  printf("bar2\n");
> >> >> >> > }
> >> >> >> > extern int src[], dst[];
> >> >> >> > int vvvvvv;
> >> >> >> > int main()
> >> >> >> > {
> >> >> >> >  int ret;
> >> >> >> >  vvvvvv = 12;
> >> >> >> >  ret = foo(20);
> >> >> >> >  bar2();
> >> >> >> >  memcpy(dst, src, 100);
> >> >> >> >  return ret + 3;
> >> >> >> > }
> >> >> >> >
> >> >> >> > b.c
> >> >> >> >
> >> >> >> > #include <stdio.h>
> >> >> >> > int src[100];
> >> >> >> > int dst[100];
> >> >> >> > extern int vvvvvv;
> >> >> >> > extern void bar();
> >> >> >> > int foo(int c)
> >> >> >> > {
> >> >> >> >  printf("Hello world: %d\n", c);
> >> >> >> >  bar();
> >> >> >> >  return 1000 + vvvvvv;
> >> >> >> > }
> >> >> >> >
> >> >> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
> >> >> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
> >> >> >> > ar cru libb.a b.o
> >> >> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-
> linker-
> >> >> plugin
> >> >> >> -o f -fwhole-program
> >> >> >> >
> >> >> >> > The code is compiled and linked correctly. bar & vvvvvv
> don't
> >> >> become
> >> >> >> static function
> >> >> >> > and cause link errors, whereas bar2 is inlined as expected.
> >> >> >> >
> >> >> >> >
> >> >> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> >> >> >> >
> >> >> >> > Cheers,
> >> >> >> > Bingfeng Mei
> >> >> >> >
> >> >> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
> >> >> >> >
> >> >> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
> >> >> >> externally_visible
> >> >> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
> >> >> >> compiling with
> >> >> >> >        -fwhole-program
> >> >> >> >
> >> >> >> >
> >> >> >> > Index: lto-symtab.c
> >> >> >> >
> >> >>
> ===================================================================
> >> >> >> > --- lto-symtab.c        (revision 160463)
> >> >> >> > +++ lto-symtab.c        (working copy)
> >> >> >> > @@ -476,7 +476,19 @@
> >> >> >> >
> >> >> >> >   /* If the chain is already resolved there is nothing else
> to
> >> >> >> do.  */
> >> >> >> >   if (e->resolution != LDPR_UNKNOWN)
> >> >> >> > -    return;
> >> >> >> > +    {
> >> >> >> > +      /* Add externally_visible attribute for declaration
> of
> >> >> >> LDPR_PREVAILING_DEF */
> >> >> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
> >> >> flag_whole_program)
> >> >> >> > +        {
> >> >> >> > +          if (!lookup_attribute ("externally_visible",
> >> >> >> DECL_ATTRIBUTES (e->decl)))
> >> >> >> > +            {
> >> >> >> > +              DECL_ATTRIBUTES (e->decl)
> >> >> >> > +                = tree_cons (get_identifier
> >> >> ("externally_visible"),
> >> >> >> NULL_TREE,
> >> >> >> > +                             DECL_ATTRIBUTES (e->decl));
> >> >> >>
> >> >> >> I don't think we need to add an attribute here but we can play
> >> >> >> with some cgraph flags which is cheaper.
> >> >> >>
> >> >> >> Also I think this isn't really correct - not everything that
> >> >> prevails
> >> >> >> needs to be externally visible (in fact, you seem to simply
> >> >> >> remove the effect of -fwhole-program completely).
> >> >> >>
> >> >> >> A testcase that should still work is
> >> >> >>
> >> >> >> t1.c:
> >> >> >> void foo(void) { bar (); }
> >> >> >> t2.c
> >> >> >> extern void foo (void);
> >> >> >> void bar (void) {}
> >> >> >> void eliminate_me (void) {}
> >> >> >> int main()
> >> >> >> {
> >> >> >>    foo();
> >> >> >> }
> >> >> >>
> >> >> >> and eliminate_me should still be eliminated with -fwhole-
> program
> >> >> >> if you do
> >> >> >>
> >> >> >> gcc -c t1.c
> >> >> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
> >> >> >>
> >> >> >> Thus, the resolution file probably does not have the
> information
> >> >> >> you need (a list of references from outside of the LTO file
> set).
> >> >> >>
> >> >> >> Richard.
> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >> >
> >
> >
> >
Richard Guenther - June 11, 2010, 9:15 a.m.
On Thu, Jun 10, 2010 at 6:23 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
> Actually, I got correct results from gold
> x.c
> extern int i;
>
> y.c
> int i;
>
> ~/work/install-x86/bin/gcc  y.o x.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps
> x.res:
> 2
> y.o 1
> 78 PREVAILING_DEF_IRONLY i
> x.o 0
>
> ~/work/install-x86/bin/gcc  x.o y.o -O2 -fuse-linker-plugin -o f -flto -fwhole-program -save-temps
> y.res
> 2
> x.o 0
> y.o 1
> 78 PREVAILING_DEF_IRONLY i
>
>
> I am using binutils-2.20-51.

Yes, the above was a bug that was actually fixed.  The issue that
it chooses a random fortran common block and not the largest
one still prevails.

We might consider reverting the change on the trunk (it's certainly
safe on the branch and makes more things work there).  But it
would also be nice for either gold or the linker-plugin being fixed
for the size issue.  (I think you run into the issue when building
SPEC 2k6, you might also find a closed bugzilla that reports it)

Richard.

> Bingfeng
>> -----Original Message-----
>> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> Sent: 10 June 2010 17:12
>> To: Bingfeng Mei
>> Cc: gcc-patches@gcc.gnu.org; Jan Hubicka
>> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> necessary with -fwhole-program and resolution file.
>>
>> On Thu, Jun 10, 2010 at 4:51 PM, Bingfeng Mei <bmei@broadcom.com> wrote:
>> > Richard,
>> > I figured out what happen with externally_visible flags in cgraph
>> node/vnode.
>> >
>> > In ipa.c (function_and_varialbe_visibility), the externally_visible
>> flags are set to
>> > true when compile with -flto because cgraph_externally_visible_p
>> always returns true as
>> > whole_program is false (regardless -fwhole-program option is set or
>> not, which is
>> > always ignored in the first phase of LTO compilation).
>> >
>> >      if (cgraph_externally_visible_p (node, whole_program))
>> >        {
>> >          gcc_assert (!node->global.inlined_to);
>> >          node->local.externally_visible = true;
>> >        }
>> >
>> > Then the flags are packed/unpacked from LTO byte code. That's why the
>> externally_visible
>> > flag is set to true for "bar2" function before executing my patch.
>> >
>> >
>> >
>> > Another issue is following code (your patch on 22/5). The vvvvvv
>> >  variable in my example is going to be resolved by internal
>> >  resolver as LDPR_PREVAILING_DEF_IRONLY instead of
>> > LDPR_PREVAILING_DEF by resolution file. Could you
>> > Elaborate what is the exact issue here?
>>
>> Gold does not keep track of which symbol from which object
>> file it will end up using for commons but instead simply picks
>> the first one.  We rely on the fact that we get the one with
>> the largest size - with gold we even can end up with
>> a non-prevailing one.  And there's some version of gold which doesn't
>> resolve commons at all.
>>
>> So we need to run through our own resolution code here.
>>
>> An example is
>>
>> extern int i;
>> --
>> int i;
>>
>> where gold can claim that 'extern int i' was prevailing.
>>
>> >          /* The LTO plugin for gold doesn't handle common symbols
>> >             properly.  Let us choose manually.  */
>> >          if (DECL_COMMON (e->decl))
>> >            e->resolution = LDPR_UNKNOWN;
>> >
>> > Thanks,
>> > Bingfeng
>> >> -----Original Message-----
>> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> >> Sent: 10 June 2010 11:12
>> >> To: Bingfeng Mei
>> >> Cc: gcc-patches@gcc.gnu.org; Jan Hubicka
>> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> >> necessary with -fwhole-program and resolution file.
>> >>
>> >> On Thu, Jun 10, 2010 at 12:03 PM, Bingfeng Mei <bmei@broadcom.com>
>> >> wrote:
>> >> > Richard,
>> >> >
>> >> > I tried to set externally_visible flags directly for cgraph node &
>> >> varbool node.
>> >> > Unfortunately, these flags seem to be set and used elsewhere too.
>> The
>> >> values
>> >> > set in lto-symtab.c cannot be reliably passed to ipa.c
>> >> (function_and_variable_visibility),
>> >> > where the flags are actually re-computed.
>> >> >
>> >> >      if (cgraph_externally_visible_p (node, whole_program))
>> >> >        {
>> >> >          gcc_assert (!node->global.inlined_to);
>> >> >          node->local.externally_visible = true;
>> >> >        }
>> >> >      else
>> >> >        node->local.externally_visible = false;
>> >> >
>> >> > The flag for bar2 function is true before this piece of code even
>> I
>> >> doesn't set
>> >> > it in lto-symtab.c.
>> >>
>> >> I believe this is an error - Honza knows the code and will probably
>> >> comment.
>> >>
>> >> > For now, I think attribute is still cleaner solution though
>> >> > not cheap. The following is the updated patch.
>> >>
>> >> Thanks - I believe this is valuable but want to hear comments
>> >> from Honza as we shouldn't need to use attributes here.
>> >>
>> >> Richard.
>> >>
>> >> > Cheers,
>> >> > Bingfeng
>> >> >
>> >> > 2010-06-10  Bingfeng Mei <bmei@broadcom.com>
>> >> >
>> >> >        * lto-symbtab.c (lto_symtab_merge_decls_1): Add
>> >> externally_visible
>> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
>> >> compiling with
>> >> >        -fwhole-program
>> >> >        (lto_symtab_resolve_symbols) Use LDPR_PREVAILING_DEF_IRONLY
>> >> for
>> >> >        internal resolver
>> >> >
>> >> > Index: lto-symtab.c
>> >> >
>> ===================================================================
>> >> > --- lto-symtab.c        (revision 160463)
>> >> > +++ lto-symtab.c        (working copy)
>> >> > @@ -530,11 +530,15 @@
>> >> >     return;
>> >> >
>> >> >  found:
>> >> > -  if (TREE_CODE (prevailing->decl) == VAR_DECL
>> >> > -      && TREE_READONLY (prevailing->decl))
>> >> > -    prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
>> >> > -  else
>> >> > -    prevailing->resolution = LDPR_PREVAILING_DEF;
>> >> > +  /* If current lto files represent the whole program,
>> >> > +    it is correct to use LDPR_PREVALING_DEF_IRONLY.
>> >> > +    If current lto files are part of whole program, internal
>> >> > +    resolver doesn't know if it is LDPR_PREVAILING_DEF
>> >> > +    or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to
>> >> > +    using -fwhole-program. Otherwise, it doesn't
>> >> > +    matter using either LDPR_PREVAILING_DEF or
>> >> > +    LDPR_PREVAILING_DEF_IRONLY */
>> >> > +  prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
>> >> >  }
>> >> >
>> >> >  /* Merge all decls in the symbol table chain to the prevailing
>> decl
>> >> and
>> >> > @@ -698,6 +702,18 @@
>> >> >       && TREE_CODE (prevailing->decl) != VAR_DECL)
>> >> >     prevailing->next = NULL;
>> >> >
>> >> > +
>> >> > +  /* Add externally_visible attribute for declaration of
>> >> LDPR_PREVAILING_DEF */
>> >> > +  if (prevailing->resolution == LDPR_PREVAILING_DEF &&
>> >> flag_whole_program)
>> >> > +    {
>> >> > +      if (!lookup_attribute ("externally_visible",
>> >> > +                             DECL_ATTRIBUTES (prevailing->decl)))
>> >> > +        {
>> >> > +          DECL_ATTRIBUTES (prevailing->decl)
>> >> > +            = tree_cons (get_identifier ("externally_visible"),
>> >> NULL_TREE,
>> >> > +                         DECL_ATTRIBUTES (prevailing->decl));
>> >> > +        }
>> >> > +    }
>> >> >   return 1;
>> >> >  }
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> >> >> Sent: 09 June 2010 16:26
>> >> >> To: Bingfeng Mei
>> >> >> Cc: gcc-patches@gcc.gnu.org
>> >> >> Subject: Re: [PATCH, LTO] add externally_visible attribute when
>> >> >> necessary with -fwhole-program and resolution file.
>> >> >>
>> >> >> On Wed, Jun 9, 2010 at 5:20 PM, Bingfeng Mei <bmei@broadcom.com>
>> >> wrote:
>> >> >> > I added an attribute because -fwhole-program/externally_visible
>> is
>> >> >> handled in ipa.c
>> >> >> >
>> >> >> > ...
>> >> >> >  if (lookup_attribute ("externally_visible", DECL_ATTRIBUTES
>> >> (node-
>> >> >> >decl)))
>> >> >> >    return true;
>> >> >> > ...
>> >> >> >
>> >> >> > Adding attribute seems cleaner than changing flags, otherwise I
>> >> need
>> >> >> to change
>> >> >> > handling in ipa.c as well.
>> >> >>
>> >> >> True, but there is an externally_visible flag in cgraph_node,
>> >> >> so I guess that attribute lookup is bogus.
>> >> >>
>> >> >> Richard.
>> >> >>
>> >> >> > Bingfeng
>> >> >> >
>> >> >> >> -----Original Message-----
>> >> >> >> From: Richard Guenther [mailto:richard.guenther@gmail.com]
>> >> >> >> Sent: 09 June 2010 16:02
>> >> >> >> To: Bingfeng Mei
>> >> >> >> Cc: gcc-patches@gcc.gnu.org
>> >> >> >> Subject: Re: [PATCH, LTO] add externally_visible attribute
>> when
>> >> >> >> necessary with -fwhole-program and resolution file.
>> >> >> >>
>> >> >> >> On Wed, Jun 9, 2010 at 4:46 PM, Bingfeng Mei
>> <bmei@broadcom.com>
>> >> >> wrote:
>> >> >> >> > Hi,
>> >> >> >> > This patch addresses issue discussed in
>> >> >> >> > http://gcc.gnu.org/ml/gcc/2010-05/msg00560.html
>> >> >> >> > http://gcc.gnu.org/ml/gcc/2010-06/msg00317.html
>> >> >> >> >
>> >> >> >> > With the patch, any declaration which is resolved as
>> >> >> >> LDPR_PREVAILING_DEF
>> >> >> >> > and compiled with -fwhole-program is annotated with
>> >> >> >> > attribute "externally_visible" if it doesn't exist already.
>> >> >> >> > This eliminates the error-prone process of manual annotation
>> >> >> >> > of the attribute when compiling mixed LTO/non-LTO
>> applications.
>> >> >> >> >
>> >> >> >> > For the following test files:
>> >> >> >> > a.c
>> >> >> >> >
>> >> >> >> > #include <string.h>
>> >> >> >> > #include <stdio.h>
>> >> >> >> > extern int foo(int);
>> >> >> >> > /* Called by b.c, should not be optimized by -fwhole-program
>> */
>> >> >> >> > void bar()
>> >> >> >> > {
>> >> >> >> >  printf("bar\n");
>> >> >> >> > }
>> >> >> >> > /* Not used by others, should be optimized out by -fwhole-
>> >> >> program*/
>> >> >> >> > void bar2()
>> >> >> >> > {
>> >> >> >> >  printf("bar2\n");
>> >> >> >> > }
>> >> >> >> > extern int src[], dst[];
>> >> >> >> > int vvvvvv;
>> >> >> >> > int main()
>> >> >> >> > {
>> >> >> >> >  int ret;
>> >> >> >> >  vvvvvv = 12;
>> >> >> >> >  ret = foo(20);
>> >> >> >> >  bar2();
>> >> >> >> >  memcpy(dst, src, 100);
>> >> >> >> >  return ret + 3;
>> >> >> >> > }
>> >> >> >> >
>> >> >> >> > b.c
>> >> >> >> >
>> >> >> >> > #include <stdio.h>
>> >> >> >> > int src[100];
>> >> >> >> > int dst[100];
>> >> >> >> > extern int vvvvvv;
>> >> >> >> > extern void bar();
>> >> >> >> > int foo(int c)
>> >> >> >> > {
>> >> >> >> >  printf("Hello world: %d\n", c);
>> >> >> >> >  bar();
>> >> >> >> >  return 1000 + vvvvvv;
>> >> >> >> > }
>> >> >> >> >
>> >> >> >> > ~/work/install-x86/bin/gcc  a.c -O2 -c  -flto
>> >> >> >> > ~/work/install-x86/bin/gcc  b.c -O2 -c
>> >> >> >> > ar cru libb.a b.o
>> >> >> >> > ~/work/install-x86/bin/gcc -flto a.o -L. -lb -O2 -fuse-
>> linker-
>> >> >> plugin
>> >> >> >> -o f -fwhole-program
>> >> >> >> >
>> >> >> >> > The code is compiled and linked correctly. bar & vvvvvv
>> don't
>> >> >> become
>> >> >> >> static function
>> >> >> >> > and cause link errors, whereas bar2 is inlined as expected.
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
>> >> >> >> >
>> >> >> >> > Cheers,
>> >> >> >> > Bingfeng Mei
>> >> >> >> >
>> >> >> >> > 2010-06-09  Bingfeng Mei <bmei@broadcom.com>
>> >> >> >> >
>> >> >> >> >        * lto-symbtab.c (lto_symtab_resolve_symbols): Add
>> >> >> >> externally_visible
>> >> >> >> >        attribute for declaration of LDPR_PREVAILING_DEF when
>> >> >> >> compiling with
>> >> >> >> >        -fwhole-program
>> >> >> >> >
>> >> >> >> >
>> >> >> >> > Index: lto-symtab.c
>> >> >> >> >
>> >> >>
>> ===================================================================
>> >> >> >> > --- lto-symtab.c        (revision 160463)
>> >> >> >> > +++ lto-symtab.c        (working copy)
>> >> >> >> > @@ -476,7 +476,19 @@
>> >> >> >> >
>> >> >> >> >   /* If the chain is already resolved there is nothing else
>> to
>> >> >> >> do.  */
>> >> >> >> >   if (e->resolution != LDPR_UNKNOWN)
>> >> >> >> > -    return;
>> >> >> >> > +    {
>> >> >> >> > +      /* Add externally_visible attribute for declaration
>> of
>> >> >> >> LDPR_PREVAILING_DEF */
>> >> >> >> > +      if (e->resolution == LDPR_PREVAILING_DEF &&
>> >> >> flag_whole_program)
>> >> >> >> > +        {
>> >> >> >> > +          if (!lookup_attribute ("externally_visible",
>> >> >> >> DECL_ATTRIBUTES (e->decl)))
>> >> >> >> > +            {
>> >> >> >> > +              DECL_ATTRIBUTES (e->decl)
>> >> >> >> > +                = tree_cons (get_identifier
>> >> >> ("externally_visible"),
>> >> >> >> NULL_TREE,
>> >> >> >> > +                             DECL_ATTRIBUTES (e->decl));
>> >> >> >>
>> >> >> >> I don't think we need to add an attribute here but we can play
>> >> >> >> with some cgraph flags which is cheaper.
>> >> >> >>
>> >> >> >> Also I think this isn't really correct - not everything that
>> >> >> prevails
>> >> >> >> needs to be externally visible (in fact, you seem to simply
>> >> >> >> remove the effect of -fwhole-program completely).
>> >> >> >>
>> >> >> >> A testcase that should still work is
>> >> >> >>
>> >> >> >> t1.c:
>> >> >> >> void foo(void) { bar (); }
>> >> >> >> t2.c
>> >> >> >> extern void foo (void);
>> >> >> >> void bar (void) {}
>> >> >> >> void eliminate_me (void) {}
>> >> >> >> int main()
>> >> >> >> {
>> >> >> >>    foo();
>> >> >> >> }
>> >> >> >>
>> >> >> >> and eliminate_me should still be eliminated with -fwhole-
>> program
>> >> >> >> if you do
>> >> >> >>
>> >> >> >> gcc -c t1.c
>> >> >> >> gcc -O2 t2.c t1.o -flto -fwhole-program -fuse-linker-plugin
>> >> >> >>
>> >> >> >> Thus, the resolution file probably does not have the
>> information
>> >> >> >> you need (a list of references from outside of the LTO file
>> set).
>> >> >> >>
>> >> >> >> Richard.
>> >> >> >
>> >> >> >
>> >> >> >
>> >> >
>> >> >
>> >> >
>> >
>> >
>> >
>
>
>
Cary Coutant - June 14, 2010, 6:43 p.m.
> Yes, the above was a bug that was actually fixed.  The issue that
> it chooses a random fortran common block and not the largest
> one still prevails.
>
> We might consider reverting the change on the trunk (it's certainly
> safe on the branch and makes more things work there).  But it
> would also be nice for either gold or the linker-plugin being fixed
> for the size issue.  (I think you run into the issue when building
> SPEC 2k6, you might also find a closed bugzilla that reports it)

Yes, gold merges common symbols by keeping the first and simply
increasing its size to the maximum of all subsequent ones. By the time
the plugin asks for symbol resolution information, there's no record
of which one was actually the largest. I'll work on a fix for this.

Is this in fact what PR 44149 is about?

-cary
Richard Guenther - June 14, 2010, 6:56 p.m.
On Mon, Jun 14, 2010 at 8:43 PM, Cary Coutant <ccoutant@google.com> wrote:
>> Yes, the above was a bug that was actually fixed.  The issue that
>> it chooses a random fortran common block and not the largest
>> one still prevails.
>>
>> We might consider reverting the change on the trunk (it's certainly
>> safe on the branch and makes more things work there).  But it
>> would also be nice for either gold or the linker-plugin being fixed
>> for the size issue.  (I think you run into the issue when building
>> SPEC 2k6, you might also find a closed bugzilla that reports it)
>
> Yes, gold merges common symbols by keeping the first and simply
> increasing its size to the maximum of all subsequent ones. By the time
> the plugin asks for symbol resolution information, there's no record
> of which one was actually the largest. I'll work on a fix for this.
>
> Is this in fact what PR 44149 is about?

Yes.  In that PR the linker-plugin reports a file with only a DECL_EXTERNAL
symbol as providing the prevailing one even.

Richard.

> -cary
>

Patch

Index: lto-symtab.c
===================================================================
--- lto-symtab.c        (revision 160463)
+++ lto-symtab.c        (working copy)
@@ -530,11 +530,15 @@ 
     return;

 found:
-  if (TREE_CODE (prevailing->decl) == VAR_DECL
-      && TREE_READONLY (prevailing->decl))
-    prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
-  else
-    prevailing->resolution = LDPR_PREVAILING_DEF;
+  /* If current lto files represent the whole program,
+    it is correct to use LDPR_PREVALING_DEF_IRONLY.
+    If current lto files are part of whole program, internal
+    resolver doesn't know if it is LDPR_PREVAILING_DEF
+    or LDPR_PREVAILING_DEF_IRONLY. Use IRONLY conforms to
+    using -fwhole-program. Otherwise, it doesn't
+    matter using either LDPR_PREVAILING_DEF or
+    LDPR_PREVAILING_DEF_IRONLY */
+  prevailing->resolution = LDPR_PREVAILING_DEF_IRONLY;
 }

 /* Merge all decls in the symbol table chain to the prevailing decl and
@@ -698,6 +702,18 @@ 
       && TREE_CODE (prevailing->decl) != VAR_DECL)
     prevailing->next = NULL;

+
+  /* Add externally_visible attribute for declaration of LDPR_PREVAILING_DEF */
+  if (prevailing->resolution == LDPR_PREVAILING_DEF && flag_whole_program)
+    {
+      if (!lookup_attribute ("externally_visible",
+                             DECL_ATTRIBUTES (prevailing->decl)))
+        {
+          DECL_ATTRIBUTES (prevailing->decl)
+            = tree_cons (get_identifier ("externally_visible"), NULL_TREE,
+                         DECL_ATTRIBUTES (prevailing->decl));
+        }
+    }
   return 1;
 }