diff mbox

Another AIX Bootstrap failure

Message ID 20140616043557.GA6530@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka June 16, 2014, 4:35 a.m. UTC 
> Honza,
> 
> The cgraph patch in r211600 broke AIX bootstrap again.  I cannot find
> the corresponding patch in the GCC Patches mailing list, so I do not
> see where this was discussed or approved.

Sorry, I remember writting mail about this patch but also can't find it.  The
patch introduces reset_section that is used when function or variable is
brought local by the visibility code. In this case we previously incorrectly
cleared user sections and we forgot to introduce implicit sections with
-fdata-section and -ffunction-section.

I will revert this change and lets debug it.

> 
> With the patch in r211600, the "gen*" programs in stage2 go into an
> endless loop.
> 
> Please revert these comdat patches.  These were not tested appropriately.
> 
> Please create a branch and we can debug the problems on AIX.

What patches you refer to? I already disabled the localization within
initializers on AIX (well all targets w/o MAKE_ONE_ONLY support) and
I am testing patch reverting this change (I want to keep reset section
just remove the call to resolve unique section) and will commit once
it converges.

I will send you patch to enable those two changes and lets look into why 
they break. What is common to both patches is that they deal with static
symbol in named sections, perhaps that is not correctly supported by AIX
toolchain...

My apologizes for the breakage. I am attaching the patch for reference.
Honza


	* symtab.c (symtab_node::reset_section): New method.
	* cgraph.c (cgraph_node_cannot_be_local_p_1): Accept non-local
	for localization.
	* cgraph.h (reset_section): Declare.
	* ipa-inline-analysis.c (do_estimate_growth): Check for comdat groups;
	do not consider comdat locals.
	* cgraphclones.c (set_new_clone_decl_and_node_flags): Get section
	for new symbol.
	* ipa-visiblity.c (cgraph_externally_visible_p): Cleanup.
	(update_visibility_by_resolution_info): Consider UNDEF; fix checking;
	reset sections of symbols dragged out of the comdats.
	(function_and_variable_visibility): Reset sections of localized symbols.

Comments

David Edelsohn June 16, 2014, 11:53 a.m. UTC | #1 
Honza,

Thanks for reverting the patch. I will check if this resolves the
current bootstrap problem.

I was suggesting that you create a branch for all of the visibility
changes to make it easier to track the various original patches and
later correction patches from you.

I don't know if the gen* programs hang because of the visibility
changes or because of the change in sections. The change in sections
could conflict with the GCC code to handle AIX XCOFF CSECTs for
functions.

AIX recently added support for ELF-like visibility. AIX previously
supported the equivalent of visibility through "export" files. The
recent problems could be due to issues with assembly file ordering,
but they also could be related to the visibility changes affecting the
way that GCC emits code to branch to global functions.

Also, your recent ChangeLog entry included a Subversion conflict marker:


2014-06-15  Jan Hubicka  <hubicka@ucw.cz>

        * c-family/c-common.c (handle_tls_model_attribute): Use set_decl_tls_mod
el.
        * c-family/c-common.c (handle_tls_model_attribute): Use
        set_decl_tls_model.
>>>>>>> .r211699
        * cgraph.h (struct varpool_node): Add tls_model.
        * tree.c (decl_tls_model, set_decl_tls_model): New functions.
        * tree.h (DECL_TLS_MODEL): Update.


Thanks, David



On Mon, Jun 16, 2014 at 12:35 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Honza,
>>
>> The cgraph patch in r211600 broke AIX bootstrap again.  I cannot find
>> the corresponding patch in the GCC Patches mailing list, so I do not
>> see where this was discussed or approved.
>
> Sorry, I remember writting mail about this patch but also can't find it.  The
> patch introduces reset_section that is used when function or variable is
> brought local by the visibility code. In this case we previously incorrectly
> cleared user sections and we forgot to introduce implicit sections with
> -fdata-section and -ffunction-section.
>
> I will revert this change and lets debug it.
>
>>
>> With the patch in r211600, the "gen*" programs in stage2 go into an
>> endless loop.
>>
>> Please revert these comdat patches.  These were not tested appropriately.
>>
>> Please create a branch and we can debug the problems on AIX.
>
> What patches you refer to? I already disabled the localization within
> initializers on AIX (well all targets w/o MAKE_ONE_ONLY support) and
> I am testing patch reverting this change (I want to keep reset section
> just remove the call to resolve unique section) and will commit once
> it converges.
>
> I will send you patch to enable those two changes and lets look into why
> they break. What is common to both patches is that they deal with static
> symbol in named sections, perhaps that is not correctly supported by AIX
> toolchain...
>
> My apologizes for the breakage. I am attaching the patch for reference.
> Honza
>
>
>         * symtab.c (symtab_node::reset_section): New method.
>         * cgraph.c (cgraph_node_cannot_be_local_p_1): Accept non-local
>         for localization.
>         * cgraph.h (reset_section): Declare.
>         * ipa-inline-analysis.c (do_estimate_growth): Check for comdat groups;
>         do not consider comdat locals.
>         * cgraphclones.c (set_new_clone_decl_and_node_flags): Get section
>         for new symbol.
>         * ipa-visiblity.c (cgraph_externally_visible_p): Cleanup.
>         (update_visibility_by_resolution_info): Consider UNDEF; fix checking;
>         reset sections of symbols dragged out of the comdats.
>         (function_and_variable_visibility): Reset sections of localized symbols.
> Index: symtab.c
> ===================================================================
> --- symtab.c    (revision 211489)
> +++ symtab.c    (working copy)
> @@ -1176,6 +1176,21 @@ symtab_node::set_section (const char *se
>    symtab_for_node_and_aliases (this, set_section_1, const_cast<char *>(section), true);
>  }
>
> +/* Reset section of NODE.  That is when NODE is being brought local
> +   we may want to clear section produced for comdat group and depending
> +   on function-sections produce now, local, unique section for it.  */
> +
> +void
> +symtab_node::reset_section ()
> +{
> +  if (!this->implicit_section)
> +    return;
> +  this->set_section (NULL);
> +  resolve_unique_section (this->decl, 0,
> +                         is_a <cgraph_node *> (this)
> +                         ? flag_function_sections : flag_data_sections);
> +}
> +
>  /* Worker for symtab_resolve_alias.  */
>
>  static bool
> Index: cgraph.c
> ===================================================================
> --- cgraph.c    (revision 211488)
> +++ cgraph.c    (working copy)
> @@ -2169,6 +2169,7 @@ cgraph_node_cannot_be_local_p_1 (struct
>                 && !node->forced_by_abi
>                 && !symtab_used_from_object_file_p (node)
>                 && !node->same_comdat_group)
> +              || DECL_EXTERNAL (node->decl)
>                || !node->externally_visible));
>  }
>
> @@ -2259,14 +2260,14 @@ cgraph_make_node_local_1 (struct cgraph_
>      {
>        symtab_make_decl_local (node->decl);
>
> -      node->set_section (NULL);
>        node->set_comdat_group (NULL);
>        node->externally_visible = false;
>        node->forced_by_abi = false;
>        node->local.local = true;
> -      node->set_section (NULL);
> +      node->reset_section ();
>        node->unique_name = (node->resolution == LDPR_PREVAILING_DEF_IRONLY
> -                                 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP);
> +                          || node->unique_name
> +                          || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP);
>        node->resolution = LDPR_PREVAILING_DEF_IRONLY;
>        gcc_assert (cgraph_function_body_availability (node) == AVAIL_LOCAL);
>      }
> Index: cgraph.h
> ===================================================================
> --- cgraph.h    (revision 211489)
> +++ cgraph.h    (working copy)
> @@ -208,6 +208,7 @@ public:
>    /* Set section for symbol and its aliases.  */
>    void set_section (const char *section);
>    void set_section_for_node (const char *section);
> +  void reset_section ();
>  };
>
>  enum availability
> Index: ipa-inline-analysis.c
> ===================================================================
> --- ipa-inline-analysis.c       (revision 211488)
> +++ ipa-inline-analysis.c       (working copy)
> @@ -3877,7 +3877,7 @@ do_estimate_growth (struct cgraph_node *
>        /* COMDAT functions are very often not shared across multiple units
>           since they come from various template instantiations.
>           Take this into account.  */
> -      else if (DECL_COMDAT (node->decl)
> +      else if (node->externally_visible && node->get_comdat_group ()
>                && cgraph_can_remove_if_no_direct_calls_p (node))
>         d.growth -= (info->size
>                      * (100 - PARAM_VALUE (PARAM_COMDAT_SHARING_PROBABILITY))
> @@ -3928,7 +3928,7 @@ growth_likely_positive (struct cgraph_no
>        && (ret = node_growth_cache[node->uid]))
>      return ret > 0;
>    if (!cgraph_will_be_removed_from_program_if_no_direct_calls (node)
> -      && (!DECL_COMDAT (node->decl)
> +      && (!node->externally_visible || !node->get_comdat_group ()
>           || !cgraph_can_remove_if_no_direct_calls_p (node)))
>      return true;
>    max_callers = inline_summary (node)->size * 4 / edge_growth + 2;
> Index: cgraphclones.c
> ===================================================================
> --- cgraphclones.c      (revision 211488)
> +++ cgraphclones.c      (working copy)
> @@ -293,6 +293,7 @@ set_new_clone_decl_and_node_flags (cgrap
>    new_node->externally_visible = 0;
>    new_node->local.local = 1;
>    new_node->lowered = true;
> +  new_node->reset_section ();
>  }
>
>  /* Duplicate thunk THUNK if necessary but make it to refer to NODE.
> Index: ipa-visibility.c
> ===================================================================
> --- ipa-visibility.c    (revision 211488)
> +++ ipa-visibility.c    (working copy)
> @@ -236,7 +236,7 @@ cgraph_externally_visible_p (struct cgra
>      return true;
>    if (node->resolution == LDPR_PREVAILING_DEF_IRONLY)
>      return false;
> -  /* When doing LTO or whole program, we can bring COMDAT functoins static.
> +  /* When doing LTO or whole program, we can bring COMDAT functions static.
>       This improves code quality and we know we will duplicate them at most twice
>       (in the case that we are not using plugin and link with object file
>        implementing same COMDAT)  */
> @@ -295,8 +295,6 @@ varpool_externally_visible_p (varpool_no
>       Even if the linker clams the symbol is unused, never bring internal
>       symbols that are declared by user as used or externally visible.
>       This is needed for i.e. references from asm statements.   */
> -  if (symtab_used_from_object_file_p (vnode))
> -    return true;
>    if (vnode->resolution == LDPR_PREVAILING_DEF_IRONLY)
>      return false;
>
> @@ -386,7 +384,8 @@ update_visibility_by_resolution_info (sy
>
>    if (!node->externally_visible
>        || (!DECL_WEAK (node->decl) && !DECL_ONE_ONLY (node->decl))
> -      || node->resolution == LDPR_UNKNOWN)
> +      || node->resolution == LDPR_UNKNOWN
> +      || node->resolution == LDPR_UNDEF)
>      return;
>
>    define = (node->resolution == LDPR_PREVAILING_DEF_IRONLY
> @@ -397,7 +396,7 @@ update_visibility_by_resolution_info (sy
>    if (node->same_comdat_group)
>      for (symtab_node *next = node->same_comdat_group;
>          next != node; next = next->same_comdat_group)
> -      gcc_assert (!node->externally_visible
> +      gcc_assert (!next->externally_visible
>                   || define == (next->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                 || next->resolution == LDPR_PREVAILING_DEF
>                                 || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP));
> @@ -411,11 +410,15 @@ update_visibility_by_resolution_info (sy
>         if (next->externally_visible
>             && !define)
>           DECL_EXTERNAL (next->decl) = true;
> +       if (!next->alias)
> +         next->reset_section ();
>        }
>    node->set_comdat_group (NULL);
>    DECL_WEAK (node->decl) = false;
>    if (!define)
>      DECL_EXTERNAL (node->decl) = true;
> +  if (!node->alias)
> +    node->reset_section ();
>    symtab_dissolve_same_comdat_group_list (node);
>  }
>
> @@ -476,7 +479,7 @@ function_and_variable_visibility (bool w
>           symtab_dissolve_same_comdat_group_list (node);
>         }
>        gcc_assert ((!DECL_WEAK (node->decl)
> -                 && !DECL_COMDAT (node->decl))
> +                  && !DECL_COMDAT (node->decl))
>                   || TREE_PUBLIC (node->decl)
>                   || node->weakref
>                   || DECL_EXTERNAL (node->decl));
> @@ -494,6 +497,7 @@ function_and_variable_visibility (bool w
>           && node->definition && !node->weakref
>           && !DECL_EXTERNAL (node->decl))
>         {
> +         bool reset = TREE_PUBLIC (node->decl);
>           gcc_assert (whole_program || in_lto_p
>                       || !TREE_PUBLIC (node->decl));
>           node->unique_name = ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
> @@ -512,9 +516,9 @@ function_and_variable_visibility (bool w
>                      next = next->same_comdat_group)
>                 {
>                   next->set_comdat_group (NULL);
> -                 if (!next->alias)
> -                   next->set_section (NULL);
>                   symtab_make_decl_local (next->decl);
> +                 if (!node->alias)
> +                   node->reset_section ();
>                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                         || next->unique_name
>                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> @@ -528,9 +532,9 @@ function_and_variable_visibility (bool w
>             }
>           if (TREE_PUBLIC (node->decl))
>             node->set_comdat_group (NULL);
> -         if (DECL_COMDAT (node->decl) && !node->alias)
> -           node->set_section (NULL);
>           symtab_make_decl_local (node->decl);
> +         if (reset && !node->alias)
> +           node->reset_section ();
>         }
>
>        if (node->thunk.thunk_p
> @@ -632,6 +636,7 @@ function_and_variable_visibility (bool w
>        if (!vnode->externally_visible
>           && !vnode->weakref)
>         {
> +         bool reset = TREE_PUBLIC (vnode->decl);
>           gcc_assert (in_lto_p || whole_program || !TREE_PUBLIC (vnode->decl));
>           vnode->unique_name = ((vnode->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                        || vnode->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> @@ -647,9 +652,9 @@ function_and_variable_visibility (bool w
>                      next = next->same_comdat_group)
>                 {
>                   next->set_comdat_group (NULL);
> -                 if (!next->alias)
> -                   next->set_section (NULL);
>                   symtab_make_decl_local (next->decl);
> +                 if (!next->alias)
> +                   next->reset_section ();
>                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                         || next->unique_name
>                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> @@ -659,9 +664,9 @@ function_and_variable_visibility (bool w
>             }
>           if (TREE_PUBLIC (vnode->decl))
>             vnode->set_comdat_group (NULL);
> -         if (DECL_COMDAT (vnode->decl) && !vnode->alias)
> -           vnode->set_section (NULL);
>           symtab_make_decl_local (vnode->decl);
> +         if (reset && !vnode->alias)
> +           vnode->reset_section ();
>           vnode->resolution = LDPR_PREVAILING_DEF_IRONLY;
>         }
>        update_visibility_by_resolution_info (vnode);
Ramana Radhakrishnan June 16, 2014, 12:01 p.m. UTC | #2 
On Mon, Jun 16, 2014 at 5:35 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Honza,
>>
>> The cgraph patch in r211600 broke AIX bootstrap again.  I cannot find
>> the corresponding patch in the GCC Patches mailing list, so I do not
>> see where this was discussed or approved.
>
> Sorry, I remember writting mail about this patch but also can't find it.  The
> patch introduces reset_section that is used when function or variable is
> brought local by the visibility code. In this case we previously incorrectly
> cleared user sections and we forgot to introduce implicit sections with
> -fdata-section and -ffunction-section.
>
> I will revert this change and lets debug it.
>

It looks like this broke arm-none-linux-gnueabihf bootstraps with
section conflicts in strstream.c. Is that the kind of failures you are
seeing on AIX ?

/work/trunk-nightlies/builds/build-210919/armv7l-unknown-linux-gnueabihf/libstdc++-v3/include/backward/strstream:126:9:
error: std::istrstream::_ZTVSt10istrstream.localalias.0 causes a
section type conflict with std::istrstream::_ZTVSt10istrstream

 Can you please test it on this platform before reapplying / or let me
know when you have the smaller patches. I'll see what I can do.

regards
Ramana

>>
>> With the patch in r211600, the "gen*" programs in stage2 go into an
>> endless loop.
>>
>> Please revert these comdat patches.  These were not tested appropriately.
>>
>> Please create a branch and we can debug the problems on AIX.
>
> What patches you refer to? I already disabled the localization within
> initializers on AIX (well all targets w/o MAKE_ONE_ONLY support) and
> I am testing patch reverting this change (I want to keep reset section
> just remove the call to resolve unique section) and will commit once
> it converges.
>
> I will send you patch to enable those two changes and lets look into why
> they break. What is common to both patches is that they deal with static
> symbol in named sections, perhaps that is not correctly supported by AIX
> toolchain...
>
> My apologizes for the breakage. I am attaching the patch for reference.
> Honza
>
>
>         * symtab.c (symtab_node::reset_section): New method.
>         * cgraph.c (cgraph_node_cannot_be_local_p_1): Accept non-local
>         for localization.
>         * cgraph.h (reset_section): Declare.
>         * ipa-inline-analysis.c (do_estimate_growth): Check for comdat groups;
>         do not consider comdat locals.
>         * cgraphclones.c (set_new_clone_decl_and_node_flags): Get section
>         for new symbol.
>         * ipa-visiblity.c (cgraph_externally_visible_p): Cleanup.
>         (update_visibility_by_resolution_info): Consider UNDEF; fix checking;
>         reset sections of symbols dragged out of the comdats.
>         (function_and_variable_visibility): Reset sections of localized symbols.
> Index: symtab.c
> ===================================================================
> --- symtab.c    (revision 211489)
> +++ symtab.c    (working copy)
> @@ -1176,6 +1176,21 @@ symtab_node::set_section (const char *se
>    symtab_for_node_and_aliases (this, set_section_1, const_cast<char *>(section), true);
>  }
>
> +/* Reset section of NODE.  That is when NODE is being brought local
> +   we may want to clear section produced for comdat group and depending
> +   on function-sections produce now, local, unique section for it.  */
> +
> +void
> +symtab_node::reset_section ()
> +{
> +  if (!this->implicit_section)
> +    return;
> +  this->set_section (NULL);
> +  resolve_unique_section (this->decl, 0,
> +                         is_a <cgraph_node *> (this)
> +                         ? flag_function_sections : flag_data_sections);
> +}
> +
>  /* Worker for symtab_resolve_alias.  */
>
>  static bool
> Index: cgraph.c
> ===================================================================
> --- cgraph.c    (revision 211488)
> +++ cgraph.c    (working copy)
> @@ -2169,6 +2169,7 @@ cgraph_node_cannot_be_local_p_1 (struct
>                 && !node->forced_by_abi
>                 && !symtab_used_from_object_file_p (node)
>                 && !node->same_comdat_group)
> +              || DECL_EXTERNAL (node->decl)
>                || !node->externally_visible));
>  }
>
> @@ -2259,14 +2260,14 @@ cgraph_make_node_local_1 (struct cgraph_
>      {
>        symtab_make_decl_local (node->decl);
>
> -      node->set_section (NULL);
>        node->set_comdat_group (NULL);
>        node->externally_visible = false;
>        node->forced_by_abi = false;
>        node->local.local = true;
> -      node->set_section (NULL);
> +      node->reset_section ();
>        node->unique_name = (node->resolution == LDPR_PREVAILING_DEF_IRONLY
> -                                 || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP);
> +                          || node->unique_name
> +                          || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP);
>        node->resolution = LDPR_PREVAILING_DEF_IRONLY;
>        gcc_assert (cgraph_function_body_availability (node) == AVAIL_LOCAL);
>      }
> Index: cgraph.h
> ===================================================================
> --- cgraph.h    (revision 211489)
> +++ cgraph.h    (working copy)
> @@ -208,6 +208,7 @@ public:
>    /* Set section for symbol and its aliases.  */
>    void set_section (const char *section);
>    void set_section_for_node (const char *section);
> +  void reset_section ();
>  };
>
>  enum availability
> Index: ipa-inline-analysis.c
> ===================================================================
> --- ipa-inline-analysis.c       (revision 211488)
> +++ ipa-inline-analysis.c       (working copy)
> @@ -3877,7 +3877,7 @@ do_estimate_growth (struct cgraph_node *
>        /* COMDAT functions are very often not shared across multiple units
>           since they come from various template instantiations.
>           Take this into account.  */
> -      else if (DECL_COMDAT (node->decl)
> +      else if (node->externally_visible && node->get_comdat_group ()
>                && cgraph_can_remove_if_no_direct_calls_p (node))
>         d.growth -= (info->size
>                      * (100 - PARAM_VALUE (PARAM_COMDAT_SHARING_PROBABILITY))
> @@ -3928,7 +3928,7 @@ growth_likely_positive (struct cgraph_no
>        && (ret = node_growth_cache[node->uid]))
>      return ret > 0;
>    if (!cgraph_will_be_removed_from_program_if_no_direct_calls (node)
> -      && (!DECL_COMDAT (node->decl)
> +      && (!node->externally_visible || !node->get_comdat_group ()
>           || !cgraph_can_remove_if_no_direct_calls_p (node)))
>      return true;
>    max_callers = inline_summary (node)->size * 4 / edge_growth + 2;
> Index: cgraphclones.c
> ===================================================================
> --- cgraphclones.c      (revision 211488)
> +++ cgraphclones.c      (working copy)
> @@ -293,6 +293,7 @@ set_new_clone_decl_and_node_flags (cgrap
>    new_node->externally_visible = 0;
>    new_node->local.local = 1;
>    new_node->lowered = true;
> +  new_node->reset_section ();
>  }
>
>  /* Duplicate thunk THUNK if necessary but make it to refer to NODE.
> Index: ipa-visibility.c
> ===================================================================
> --- ipa-visibility.c    (revision 211488)
> +++ ipa-visibility.c    (working copy)
> @@ -236,7 +236,7 @@ cgraph_externally_visible_p (struct cgra
>      return true;
>    if (node->resolution == LDPR_PREVAILING_DEF_IRONLY)
>      return false;
> -  /* When doing LTO or whole program, we can bring COMDAT functoins static.
> +  /* When doing LTO or whole program, we can bring COMDAT functions static.
>       This improves code quality and we know we will duplicate them at most twice
>       (in the case that we are not using plugin and link with object file
>        implementing same COMDAT)  */
> @@ -295,8 +295,6 @@ varpool_externally_visible_p (varpool_no
>       Even if the linker clams the symbol is unused, never bring internal
>       symbols that are declared by user as used or externally visible.
>       This is needed for i.e. references from asm statements.   */
> -  if (symtab_used_from_object_file_p (vnode))
> -    return true;
>    if (vnode->resolution == LDPR_PREVAILING_DEF_IRONLY)
>      return false;
>
> @@ -386,7 +384,8 @@ update_visibility_by_resolution_info (sy
>
>    if (!node->externally_visible
>        || (!DECL_WEAK (node->decl) && !DECL_ONE_ONLY (node->decl))
> -      || node->resolution == LDPR_UNKNOWN)
> +      || node->resolution == LDPR_UNKNOWN
> +      || node->resolution == LDPR_UNDEF)
>      return;
>
>    define = (node->resolution == LDPR_PREVAILING_DEF_IRONLY
> @@ -397,7 +396,7 @@ update_visibility_by_resolution_info (sy
>    if (node->same_comdat_group)
>      for (symtab_node *next = node->same_comdat_group;
>          next != node; next = next->same_comdat_group)
> -      gcc_assert (!node->externally_visible
> +      gcc_assert (!next->externally_visible
>                   || define == (next->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                 || next->resolution == LDPR_PREVAILING_DEF
>                                 || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP));
> @@ -411,11 +410,15 @@ update_visibility_by_resolution_info (sy
>         if (next->externally_visible
>             && !define)
>           DECL_EXTERNAL (next->decl) = true;
> +       if (!next->alias)
> +         next->reset_section ();
>        }
>    node->set_comdat_group (NULL);
>    DECL_WEAK (node->decl) = false;
>    if (!define)
>      DECL_EXTERNAL (node->decl) = true;
> +  if (!node->alias)
> +    node->reset_section ();
>    symtab_dissolve_same_comdat_group_list (node);
>  }
>
> @@ -476,7 +479,7 @@ function_and_variable_visibility (bool w
>           symtab_dissolve_same_comdat_group_list (node);
>         }
>        gcc_assert ((!DECL_WEAK (node->decl)
> -                 && !DECL_COMDAT (node->decl))
> +                  && !DECL_COMDAT (node->decl))
>                   || TREE_PUBLIC (node->decl)
>                   || node->weakref
>                   || DECL_EXTERNAL (node->decl));
> @@ -494,6 +497,7 @@ function_and_variable_visibility (bool w
>           && node->definition && !node->weakref
>           && !DECL_EXTERNAL (node->decl))
>         {
> +         bool reset = TREE_PUBLIC (node->decl);
>           gcc_assert (whole_program || in_lto_p
>                       || !TREE_PUBLIC (node->decl));
>           node->unique_name = ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
> @@ -512,9 +516,9 @@ function_and_variable_visibility (bool w
>                      next = next->same_comdat_group)
>                 {
>                   next->set_comdat_group (NULL);
> -                 if (!next->alias)
> -                   next->set_section (NULL);
>                   symtab_make_decl_local (next->decl);
> +                 if (!node->alias)
> +                   node->reset_section ();
>                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                         || next->unique_name
>                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> @@ -528,9 +532,9 @@ function_and_variable_visibility (bool w
>             }
>           if (TREE_PUBLIC (node->decl))
>             node->set_comdat_group (NULL);
> -         if (DECL_COMDAT (node->decl) && !node->alias)
> -           node->set_section (NULL);
>           symtab_make_decl_local (node->decl);
> +         if (reset && !node->alias)
> +           node->reset_section ();
>         }
>
>        if (node->thunk.thunk_p
> @@ -632,6 +636,7 @@ function_and_variable_visibility (bool w
>        if (!vnode->externally_visible
>           && !vnode->weakref)
>         {
> +         bool reset = TREE_PUBLIC (vnode->decl);
>           gcc_assert (in_lto_p || whole_program || !TREE_PUBLIC (vnode->decl));
>           vnode->unique_name = ((vnode->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                        || vnode->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> @@ -647,9 +652,9 @@ function_and_variable_visibility (bool w
>                      next = next->same_comdat_group)
>                 {
>                   next->set_comdat_group (NULL);
> -                 if (!next->alias)
> -                   next->set_section (NULL);
>                   symtab_make_decl_local (next->decl);
> +                 if (!next->alias)
> +                   next->reset_section ();
>                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                         || next->unique_name
>                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> @@ -659,9 +664,9 @@ function_and_variable_visibility (bool w
>             }
>           if (TREE_PUBLIC (vnode->decl))
>             vnode->set_comdat_group (NULL);
> -         if (DECL_COMDAT (vnode->decl) && !vnode->alias)
> -           vnode->set_section (NULL);
>           symtab_make_decl_local (vnode->decl);
> +         if (reset && !vnode->alias)
> +           vnode->reset_section ();
>           vnode->resolution = LDPR_PREVAILING_DEF_IRONLY;
>         }
>        update_visibility_by_resolution_info (vnode);
Jan Hubicka June 16, 2014, 3:08 p.m. UTC | #3 
> Honza,
> 
> Thanks for reverting the patch. I will check if this resolves the
> current bootstrap problem.
> 
> I was suggesting that you create a branch for all of the visibility
> changes to make it easier to track the various original patches and
> later correction patches from you.
> 
> I don't know if the gen* programs hang because of the visibility
> changes or because of the change in sections. The change in sections
> could conflict with the GCC code to handle AIX XCOFF CSECTs for
> functions.
> 
> AIX recently added support for ELF-like visibility. AIX previously
> supported the equivalent of visibility through "export" files. The
> recent problems could be due to issues with assembly file ordering,
> but they also could be related to the visibility changes affecting the
> way that GCC emits code to branch to global functions.

I comitted the revert now (my original testing got struct on ICE in
auto-inc-dec pass that is unrelated).  I probably won't have time to analye
what went wrong until Wednesday. The patch did not really play with
ELF visibilities it was again related to bringing symbols local.
I tried a case disabling the new conditional on clearning user section
but that did not help. The patch basically collected few cleanups
and fixes of corner case.  Last change is fix in the inline heuristics
to not try to enale DECL_ONE_ONLY section sharing on targets not supporting
it.  Obviously it should not lead to wrong code, since any inlining decision
change should not, but I am testing it independnely now.

Honza
Ramana Radhakrishnan June 16, 2014, 4:16 p.m. UTC | #4 
On Mon, Jun 16, 2014 at 4:08 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Honza,
>>
>> Thanks for reverting the patch. I will check if this resolves the
>> current bootstrap problem.
>>
>> I was suggesting that you create a branch for all of the visibility
>> changes to make it easier to track the various original patches and
>> later correction patches from you.
>>
>> I don't know if the gen* programs hang because of the visibility
>> changes or because of the change in sections. The change in sections
>> could conflict with the GCC code to handle AIX XCOFF CSECTs for
>> functions.
>>
>> AIX recently added support for ELF-like visibility. AIX previously
>> supported the equivalent of visibility through "export" files. The
>> recent problems could be due to issues with assembly file ordering,
>> but they also could be related to the visibility changes affecting the
>> way that GCC emits code to branch to global functions.
>
> I comitted the revert now (my original testing got struct on ICE in
> auto-inc-dec pass that is unrelated).  I probably won't have time to analye
> what went wrong until Wednesday. The patch did not really play with
> ELF visibilities it was again related to bringing symbols local.
> I tried a case disabling the new conditional on clearning user section
> but that did not help. The patch basically collected few cleanups
> and fixes of corner case.  Last change is fix in the inline heuristics
> to not try to enale DECL_ONE_ONLY section sharing on targets not supporting
> it.  Obviously it should not lead to wrong code, since any inlining decision
> change should not, but I am testing it independnely now.

Can you please verify the testcase in PR61523 doesn't fail with your
reworked patch for arm-none-linux-gnueabihf ?

Thanks,
Ramana

>
> Honza
David Edelsohn June 16, 2014, 5:13 p.m. UTC | #5 
Honza,

FYI, your bootstrap on gcc111 is hung in the exact same place as I
have observed: all of the stage2 gen* programs spinning.

- David


On Mon, Jun 16, 2014 at 11:08 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> Honza,
>>
>> Thanks for reverting the patch. I will check if this resolves the
>> current bootstrap problem.
>>
>> I was suggesting that you create a branch for all of the visibility
>> changes to make it easier to track the various original patches and
>> later correction patches from you.
>>
>> I don't know if the gen* programs hang because of the visibility
>> changes or because of the change in sections. The change in sections
>> could conflict with the GCC code to handle AIX XCOFF CSECTs for
>> functions.
>>
>> AIX recently added support for ELF-like visibility. AIX previously
>> supported the equivalent of visibility through "export" files. The
>> recent problems could be due to issues with assembly file ordering,
>> but they also could be related to the visibility changes affecting the
>> way that GCC emits code to branch to global functions.
>
> I comitted the revert now (my original testing got struct on ICE in
> auto-inc-dec pass that is unrelated).  I probably won't have time to analye
> what went wrong until Wednesday. The patch did not really play with
> ELF visibilities it was again related to bringing symbols local.
> I tried a case disabling the new conditional on clearning user section
> but that did not help. The patch basically collected few cleanups
> and fixes of corner case.  Last change is fix in the inline heuristics
> to not try to enale DECL_ONE_ONLY section sharing on targets not supporting
> it.  Obviously it should not lead to wrong code, since any inlining decision
> change should not, but I am testing it independnely now.
>
> Honza
David Edelsohn June 16, 2014, 6:55 p.m. UTC | #6 
On Mon, Jun 16, 2014 at 12:35 AM, Jan Hubicka <hubicka@ucw.cz> wrote:

> @@ -512,9 +516,9 @@ function_and_variable_visibility (bool w
>                      next = next->same_comdat_group)
>                 {
>                   next->set_comdat_group (NULL);
> -                 if (!next->alias)
> -                   next->set_section (NULL);
>                   symtab_make_decl_local (next->decl);
> +                 if (!node->alias)
> +                   node->reset_section ();
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>                                         || next->unique_name
>                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)

Honza, did you really intend to change the above from next->alias and
next->set_section () to node->alias and node->reset_section () ?  That
doesn't look right.

- David
Jan Hubicka June 16, 2014, 10:06 p.m. UTC | #7 
> On Mon, Jun 16, 2014 at 12:35 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
> 
> > @@ -512,9 +516,9 @@ function_and_variable_visibility (bool w
> >                      next = next->same_comdat_group)
> >                 {
> >                   next->set_comdat_group (NULL);
> > -                 if (!next->alias)
> > -                   next->set_section (NULL);
> >                   symtab_make_decl_local (next->decl);
> > +                 if (!node->alias)
> > +                   node->reset_section ();
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
> >                                         || next->unique_name
> >                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
> 
> Honza, did you really intend to change the above from next->alias and
> next->set_section () to node->alias and node->reset_section () ?  That
> doesn't look right.

You are right, this is a pasto in anoying code duplication here.  I fixed that
in my local copy of the patch.  I have a followup patch in the ipa-visibility
TLC to merge the code duplication here, I am just holding it until we debug the
issues. 

This bug will not affect AIX, becuase there are no comdat groups.  It only
wastes a bit of code size on ELF systems because of extra alignment coming
from the named section.

The hang happens in 
(gdb) bt
#0  0x100be244 in __gnu_parallel::_Settings::_Settings() ()
#1  0x10008d54 in _GLOBAL__FI_libstdc___so ()
#2  0x10008e88 in _GLOBAL__AIXI_libstdc___so ()
#3  0x100be954 in _GLOBAL__FI_genconstants ()
#4  0xd017fa54 in mod_init1 () from /usr/lib/libc.a(shr.o)
#5  0xd017f774 in __modinit () from /usr/lib/libc.a(shr.o)
#6  0x100001a0 in __start ()

I suppose it may be crtbegin/crtend miscompile.  Any insight would be welcome,
otherwise I will try to progress on debugging tonight or tomorrow.

Honza
David Edelsohn June 17, 2014, 1:51 a.m. UTC | #8 
It looks like _Settings is miscompiled, or more specifically a
function descriptor has a bogus value

   0x100bcbac <_ZN14__gnu_parallel9_SettingsC1Ev>:      addi    r12,r2,-168
   0x100bcbb0 <_ZN14__gnu_parallel9_SettingsC1Ev+4>:    stw     r2,20(r1)
   0x100bcbb4 <_ZN14__gnu_parallel9_SettingsC1Ev+8>:    lwz     r0,0(r12)
   0x100bcbb8 <_ZN14__gnu_parallel9_SettingsC1Ev+12>:   lwz     r2,4(r12)
   0x100bcbbc <_ZN14__gnu_parallel9_SettingsC1Ev+16>:   mtctr   r0
=> 0x100bcbc0 <_ZN14__gnu_parallel9_SettingsC1Ev+20>:   bctr

(gdb) x/x $r2-168
0x3002f90c <__dbargs+11440>:    0x100bcbac
(gdb) x/x $r12
0x3002f90c <__dbargs+11440>:    0x100bcbac
(gdb) p/x $ctr
$5 = 0x100bcbac

For some reason, the additional inlining is creating the wrong address
for the target.

- David



On Mon, Jun 16, 2014 at 6:06 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Mon, Jun 16, 2014 at 12:35 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> > @@ -512,9 +516,9 @@ function_and_variable_visibility (bool w
>> >                      next = next->same_comdat_group)
>> >                 {
>> >                   next->set_comdat_group (NULL);
>> > -                 if (!next->alias)
>> > -                   next->set_section (NULL);
>> >                   symtab_make_decl_local (next->decl);
>> > +                 if (!node->alias)
>> > +                   node->reset_section ();
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>> >                                         || next->unique_name
>> >                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
>>
>> Honza, did you really intend to change the above from next->alias and
>> next->set_section () to node->alias and node->reset_section () ?  That
>> doesn't look right.
>
> You are right, this is a pasto in anoying code duplication here.  I fixed that
> in my local copy of the patch.  I have a followup patch in the ipa-visibility
> TLC to merge the code duplication here, I am just holding it until we debug the
> issues.
>
> This bug will not affect AIX, becuase there are no comdat groups.  It only
> wastes a bit of code size on ELF systems because of extra alignment coming
> from the named section.
>
> The hang happens in
> (gdb) bt
> #0  0x100be244 in __gnu_parallel::_Settings::_Settings() ()
> #1  0x10008d54 in _GLOBAL__FI_libstdc___so ()
> #2  0x10008e88 in _GLOBAL__AIXI_libstdc___so ()
> #3  0x100be954 in _GLOBAL__FI_genconstants ()
> #4  0xd017fa54 in mod_init1 () from /usr/lib/libc.a(shr.o)
> #5  0xd017f774 in __modinit () from /usr/lib/libc.a(shr.o)
> #6  0x100001a0 in __start ()
>
> I suppose it may be crtbegin/crtend miscompile.  Any insight would be welcome,
> otherwise I will try to progress on debugging tonight or tomorrow.
>
> Honza
David Edelsohn June 17, 2014, 2:55 a.m. UTC | #9 
Without the ipa-inline-analysis.c change, g++ creates a static
constructor with global visibility

        .globl ._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0

._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0:

With the patch, g++ creates weak method

        .weak   ._ZN14__gnu_parallel9_SettingsC1Ev
._ZN14__gnu_parallel9_SettingsC1Ev:

with non-global alias

        .lglobl ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
        .set
._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0,._ZN14__gnu_parallel9_SettingsC1Ev

and the static constructor branches to the alias

        .globl ._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0
._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0:
        lwz 3,LC..8(2)
        b ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0

The code where it's hanging is the AIX "glink" code, essentially a PLT
stub, trying to call the method ._ZN14__gnu_parallel9_SettingsC1Ev

The linker is not seeing the local definition of
._ZN14__gnu_parallel9_SettingsC1Ev.  libstdc++ is built with
Linux-like semantics, so it allows symbols to be overridden. AIX calls
everything through the PLT. But the real definition of the function is
not being seen.

I'm not exactly sure why inlining changing this and what these extra
levels of indirections are trying to accomplish. The visibility of the
symbols as declared in the XCOFF assembly files appears to be
preventing the AIX linker and loader from resolving the static
constructor functions.

Thanks David


On Mon, Jun 16, 2014 at 6:06 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> On Mon, Jun 16, 2014 at 12:35 AM, Jan Hubicka <hubicka@ucw.cz> wrote:
>>
>> > @@ -512,9 +516,9 @@ function_and_variable_visibility (bool w
>> >                      next = next->same_comdat_group)
>> >                 {
>> >                   next->set_comdat_group (NULL);
>> > -                 if (!next->alias)
>> > -                   next->set_section (NULL);
>> >                   symtab_make_decl_local (next->decl);
>> > +                 if (!node->alias)
>> > +                   node->reset_section ();
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> >                   next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
>> >                                         || next->unique_name
>> >                                         || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
>>
>> Honza, did you really intend to change the above from next->alias and
>> next->set_section () to node->alias and node->reset_section () ?  That
>> doesn't look right.
>
> You are right, this is a pasto in anoying code duplication here.  I fixed that
> in my local copy of the patch.  I have a followup patch in the ipa-visibility
> TLC to merge the code duplication here, I am just holding it until we debug the
> issues.
>
> This bug will not affect AIX, becuase there are no comdat groups.  It only
> wastes a bit of code size on ELF systems because of extra alignment coming
> from the named section.
>
> The hang happens in
> (gdb) bt
> #0  0x100be244 in __gnu_parallel::_Settings::_Settings() ()
> #1  0x10008d54 in _GLOBAL__FI_libstdc___so ()
> #2  0x10008e88 in _GLOBAL__AIXI_libstdc___so ()
> #3  0x100be954 in _GLOBAL__FI_genconstants ()
> #4  0xd017fa54 in mod_init1 () from /usr/lib/libc.a(shr.o)
> #5  0xd017f774 in __modinit () from /usr/lib/libc.a(shr.o)
> #6  0x100001a0 in __start ()
>
> I suppose it may be crtbegin/crtend miscompile.  Any insight would be welcome,
> otherwise I will try to progress on debugging tonight or tomorrow.
>
> Honza
Jan Hubicka June 17, 2014, 3:44 a.m. UTC | #10 
David,
thanks for the analysis!
> Without the ipa-inline-analysis.c change, g++ creates a static
> constructor with global visibility
> 
>         .globl ._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0
> 
> ._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0:
> 
> With the patch, g++ creates weak method
> 
>         .weak   ._ZN14__gnu_parallel9_SettingsC1Ev
> ._ZN14__gnu_parallel9_SettingsC1Ev:
> 
> with non-global alias
> 
>         .lglobl ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
>         .set
> ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0,._ZN14__gnu_parallel9_SettingsC1Ev
> 
> and the static constructor branches to the alias
> 
>         .globl ._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0
> ._GLOBAL__I_65535_0__home_dje_src_src_libstdc___v3_src_c__98_parallel_settings.cc_2984A295_0:
>         lwz 3,LC..8(2)
>         b ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
> 
> The code where it's hanging is the AIX "glink" code, essentially a PLT
> stub, trying to call the method ._ZN14__gnu_parallel9_SettingsC1Ev

I see, one can get this with -fno-inline more easily and probably it affects
4.9, too.
> 
> The linker is not seeing the local definition of
> ._ZN14__gnu_parallel9_SettingsC1Ev.  libstdc++ is built with
> Linux-like semantics, so it allows symbols to be overridden. AIX calls
> everything through the PLT. But the real definition of the function is

Even static functions?

> not being seen.
> 
> I'm not exactly sure why inlining changing this and what these extra
> levels of indirections are trying to accomplish. The visibility of the

To avoid using PLT and GOT when the unit refers to the symbol and we know
that interposition does not matter.

Why branch to a non-global (static) symbol
  b ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
leads to PLT stub here and why branching to such symbols seems to work otherwise? 

> symbols as declared in the XCOFF assembly files appears to be
> preventing the AIX linker and loader from resolving the static
> constructor functions.

The failing branch is
>         b ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
so the call to static construction seems to have happened correctly but we can
not get right the call from the constructor to static function (that is an alias
of a global symbol)

I can get simliar code with -fno-inline.  Here we apparently handle first call
correctly but fail in the second call. First is to static function other
is to static alias of global function. Both should result in same code but doesn't

One is:
        .lglobl ._GLOBAL__sub_I_parallel_settings.cc
        .csect _GLOBAL__sub_I_parallel_settings.cc[DS]
_GLOBAL__sub_I_parallel_settings.cc:
        .long ._GLOBAL__sub_I_parallel_settings.cc, TOC[tc0], 0
        .csect ..text.startup[PR],2
._GLOBAL__sub_I_parallel_settings.cc:

Other is
        .csect ._ZN14__gnu_parallel9_SettingsC1Ev[PR],2
        .align 2
        .align 4
        .weak   _ZN14__gnu_parallel9_SettingsC1Ev[DS]
        .weak   ._ZN14__gnu_parallel9_SettingsC1Ev
        .csect _ZN14__gnu_parallel9_SettingsC1Ev[DS]
_ZN14__gnu_parallel9_SettingsC1Ev:
        .long ._ZN14__gnu_parallel9_SettingsC1Ev, TOC[tc0], 0
        .csect ._ZN14__gnu_parallel9_SettingsC1Ev[PR],2
._ZN14__gnu_parallel9_SettingsC1Ev:
        .bi     "/home/jh/trunk/build/powerpc-ibm-aix7.1.0.0/libstdc++-v3/include/parallel/settings.h"
        .stabx  "_ZN14__gnu_parallel9_SettingsC1Ev:F-11",._ZN14__gnu_parallel9_SettingsC1Ev,142,0
        .function ._ZN14__gnu_parallel9_SettingsC1Ev,._ZN14__gnu_parallel9_SettingsC1Ev,16,044,FE.._ZN14__gnu_parallel9_SettingsC1Ev-._ZN14__gnu_parallel9_SettingsC1Ev
....
FE.._ZN14__gnu_parallel9_SettingsC1Ev:
LFE..146:
        .lglobl ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
        .lglobl _ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
        .set    ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0,._ZN14__gnu_parallel9_SettingsC1Ev
        .set _ZN14__gnu_parallel9_SettingsC1Ev.localalias.0,_ZN14__gnu_parallel9_SettingsC1Ev

Let me see if I can derive something more self contained.

Honza
> 
> Thanks David
David Edelsohn June 17, 2014, 2:06 p.m. UTC | #11 
On Mon, Jun 16, 2014 at 11:44 PM, Jan Hubicka <hubicka@ucw.cz> wrote:

>> The linker is not seeing the local definition of
>> ._ZN14__gnu_parallel9_SettingsC1Ev.  libstdc++ is built with
>> Linux-like semantics, so it allows symbols to be overridden. AIX calls
>> everything through the PLT. But the real definition of the function is
>
> Even static functions?
>
>> not being seen.
>>
>> I'm not exactly sure why inlining changing this and what these extra
>> levels of indirections are trying to accomplish. The visibility of the
>
> To avoid using PLT and GOT when the unit refers to the symbol and we know
> that interposition does not matter.

I am not certain if the linker is creating the PLT stub code because
it wants to allow interpolation or because it cannot see a definition
of the function and wants to allow for some other shared library to
provide the definition at runtime.

> Why branch to a non-global (static) symbol
>   b ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
> leads to PLT stub here and why branching to such symbols seems to work otherwise?

Branching to non-global (static) symbol, even an alias, is working
here. The weak function seems to be the problem.

> The failing branch is
>>         b ._ZN14__gnu_parallel9_SettingsC1Ev.localalias.0
> so the call to static construction seems to have happened correctly but we can
> not get right the call from the constructor to static function (that is an alias
> of a global symbol)

The linker appears to not want to resolve the weak function. If I
change ._ZN14__gnu_parallel9_SettingsC1Ev to lglobl, it works. If I
change the static constructor to call the weak function directly,
avoiding the alias, it shows the same failure mode.

I don't know what code generation looked like before.  Was GCC
generating calls to weak functions within the same file?

Thanks, David
diff mbox

Patch

Index: symtab.c
===================================================================
--- symtab.c	(revision 211489)
+++ symtab.c	(working copy)
@@ -1176,6 +1176,21 @@  symtab_node::set_section (const char *se
   symtab_for_node_and_aliases (this, set_section_1, const_cast<char *>(section), true);
 }
 
+/* Reset section of NODE.  That is when NODE is being brought local
+   we may want to clear section produced for comdat group and depending
+   on function-sections produce now, local, unique section for it.  */
+
+void
+symtab_node::reset_section ()
+{
+  if (!this->implicit_section)
+    return;
+  this->set_section (NULL);
+  resolve_unique_section (this->decl, 0,
+			  is_a <cgraph_node *> (this)
+			  ? flag_function_sections : flag_data_sections);
+}
+
 /* Worker for symtab_resolve_alias.  */
 
 static bool
Index: cgraph.c
===================================================================
--- cgraph.c	(revision 211488)
+++ cgraph.c	(working copy)
@@ -2169,6 +2169,7 @@  cgraph_node_cannot_be_local_p_1 (struct
 		&& !node->forced_by_abi
 	        && !symtab_used_from_object_file_p (node)
 		&& !node->same_comdat_group)
+	       || DECL_EXTERNAL (node->decl)
 	       || !node->externally_visible));
 }
 
@@ -2259,14 +2260,14 @@  cgraph_make_node_local_1 (struct cgraph_
     {
       symtab_make_decl_local (node->decl);
 
-      node->set_section (NULL);
       node->set_comdat_group (NULL);
       node->externally_visible = false;
       node->forced_by_abi = false;
       node->local.local = true;
-      node->set_section (NULL);
+      node->reset_section ();
       node->unique_name = (node->resolution == LDPR_PREVAILING_DEF_IRONLY
-				  || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP);
+			   || node->unique_name
+			   || node->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP);
       node->resolution = LDPR_PREVAILING_DEF_IRONLY;
       gcc_assert (cgraph_function_body_availability (node) == AVAIL_LOCAL);
     }
Index: cgraph.h
===================================================================
--- cgraph.h	(revision 211489)
+++ cgraph.h	(working copy)
@@ -208,6 +208,7 @@  public:
   /* Set section for symbol and its aliases.  */
   void set_section (const char *section);
   void set_section_for_node (const char *section);
+  void reset_section ();
 };
 
 enum availability
Index: ipa-inline-analysis.c
===================================================================
--- ipa-inline-analysis.c	(revision 211488)
+++ ipa-inline-analysis.c	(working copy)
@@ -3877,7 +3877,7 @@  do_estimate_growth (struct cgraph_node *
       /* COMDAT functions are very often not shared across multiple units
          since they come from various template instantiations.
          Take this into account.  */
-      else if (DECL_COMDAT (node->decl)
+      else if (node->externally_visible && node->get_comdat_group ()
 	       && cgraph_can_remove_if_no_direct_calls_p (node))
 	d.growth -= (info->size
 		     * (100 - PARAM_VALUE (PARAM_COMDAT_SHARING_PROBABILITY))
@@ -3928,7 +3928,7 @@  growth_likely_positive (struct cgraph_no
       && (ret = node_growth_cache[node->uid]))
     return ret > 0;
   if (!cgraph_will_be_removed_from_program_if_no_direct_calls (node)
-      && (!DECL_COMDAT (node->decl)
+      && (!node->externally_visible || !node->get_comdat_group ()
 	  || !cgraph_can_remove_if_no_direct_calls_p (node)))
     return true;
   max_callers = inline_summary (node)->size * 4 / edge_growth + 2;
Index: cgraphclones.c
===================================================================
--- cgraphclones.c	(revision 211488)
+++ cgraphclones.c	(working copy)
@@ -293,6 +293,7 @@  set_new_clone_decl_and_node_flags (cgrap
   new_node->externally_visible = 0;
   new_node->local.local = 1;
   new_node->lowered = true;
+  new_node->reset_section ();
 }
 
 /* Duplicate thunk THUNK if necessary but make it to refer to NODE.
Index: ipa-visibility.c
===================================================================
--- ipa-visibility.c	(revision 211488)
+++ ipa-visibility.c	(working copy)
@@ -236,7 +236,7 @@  cgraph_externally_visible_p (struct cgra
     return true;
   if (node->resolution == LDPR_PREVAILING_DEF_IRONLY)
     return false;
-  /* When doing LTO or whole program, we can bring COMDAT functoins static.
+  /* When doing LTO or whole program, we can bring COMDAT functions static.
      This improves code quality and we know we will duplicate them at most twice
      (in the case that we are not using plugin and link with object file
       implementing same COMDAT)  */
@@ -295,8 +295,6 @@  varpool_externally_visible_p (varpool_no
      Even if the linker clams the symbol is unused, never bring internal
      symbols that are declared by user as used or externally visible.
      This is needed for i.e. references from asm statements.   */
-  if (symtab_used_from_object_file_p (vnode))
-    return true;
   if (vnode->resolution == LDPR_PREVAILING_DEF_IRONLY)
     return false;
 
@@ -386,7 +384,8 @@  update_visibility_by_resolution_info (sy
 
   if (!node->externally_visible
       || (!DECL_WEAK (node->decl) && !DECL_ONE_ONLY (node->decl))
-      || node->resolution == LDPR_UNKNOWN)
+      || node->resolution == LDPR_UNKNOWN
+      || node->resolution == LDPR_UNDEF)
     return;
 
   define = (node->resolution == LDPR_PREVAILING_DEF_IRONLY
@@ -397,7 +396,7 @@  update_visibility_by_resolution_info (sy
   if (node->same_comdat_group)
     for (symtab_node *next = node->same_comdat_group;
 	 next != node; next = next->same_comdat_group)
-      gcc_assert (!node->externally_visible
+      gcc_assert (!next->externally_visible
 		  || define == (next->resolution == LDPR_PREVAILING_DEF_IRONLY
 			        || next->resolution == LDPR_PREVAILING_DEF
 			        || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP));
@@ -411,11 +410,15 @@  update_visibility_by_resolution_info (sy
 	if (next->externally_visible
 	    && !define)
 	  DECL_EXTERNAL (next->decl) = true;
+	if (!next->alias)
+	  next->reset_section ();
       }
   node->set_comdat_group (NULL);
   DECL_WEAK (node->decl) = false;
   if (!define)
     DECL_EXTERNAL (node->decl) = true;
+  if (!node->alias)
+    node->reset_section ();
   symtab_dissolve_same_comdat_group_list (node);
 }
 
@@ -476,7 +479,7 @@  function_and_variable_visibility (bool w
 	  symtab_dissolve_same_comdat_group_list (node);
 	}
       gcc_assert ((!DECL_WEAK (node->decl)
-		  && !DECL_COMDAT (node->decl))
+		   && !DECL_COMDAT (node->decl))
       	          || TREE_PUBLIC (node->decl)
 		  || node->weakref
 		  || DECL_EXTERNAL (node->decl));
@@ -494,6 +497,7 @@  function_and_variable_visibility (bool w
 	  && node->definition && !node->weakref
 	  && !DECL_EXTERNAL (node->decl))
 	{
+	  bool reset = TREE_PUBLIC (node->decl);
 	  gcc_assert (whole_program || in_lto_p
 		      || !TREE_PUBLIC (node->decl));
 	  node->unique_name = ((node->resolution == LDPR_PREVAILING_DEF_IRONLY
@@ -512,9 +516,9 @@  function_and_variable_visibility (bool w
 		     next = next->same_comdat_group)
 		{
 		  next->set_comdat_group (NULL);
-		  if (!next->alias)
-		    next->set_section (NULL);
 		  symtab_make_decl_local (next->decl);
+		  if (!node->alias)
+		    node->reset_section ();
 		  next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
 					|| next->unique_name
 					|| next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
@@ -528,9 +532,9 @@  function_and_variable_visibility (bool w
 	    }
 	  if (TREE_PUBLIC (node->decl))
 	    node->set_comdat_group (NULL);
-	  if (DECL_COMDAT (node->decl) && !node->alias)
-	    node->set_section (NULL);
 	  symtab_make_decl_local (node->decl);
+	  if (reset && !node->alias)
+	    node->reset_section ();
 	}
 
       if (node->thunk.thunk_p
@@ -632,6 +636,7 @@  function_and_variable_visibility (bool w
       if (!vnode->externally_visible
 	  && !vnode->weakref)
 	{
+	  bool reset = TREE_PUBLIC (vnode->decl);
 	  gcc_assert (in_lto_p || whole_program || !TREE_PUBLIC (vnode->decl));
 	  vnode->unique_name = ((vnode->resolution == LDPR_PREVAILING_DEF_IRONLY
 				       || vnode->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
@@ -647,9 +652,9 @@  function_and_variable_visibility (bool w
 		     next = next->same_comdat_group)
 		{
 		  next->set_comdat_group (NULL);
-		  if (!next->alias)
-		    next->set_section (NULL);
 		  symtab_make_decl_local (next->decl);
+		  if (!next->alias)
+		    next->reset_section ();
 		  next->unique_name = ((next->resolution == LDPR_PREVAILING_DEF_IRONLY
 					|| next->unique_name
 					|| next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP)
@@ -659,9 +664,9 @@  function_and_variable_visibility (bool w
 	    }
 	  if (TREE_PUBLIC (vnode->decl))
 	    vnode->set_comdat_group (NULL);
-	  if (DECL_COMDAT (vnode->decl) && !vnode->alias)
-	    vnode->set_section (NULL);
 	  symtab_make_decl_local (vnode->decl);
+	  if (reset && !vnode->alias)
+	    vnode->reset_section ();
 	  vnode->resolution = LDPR_PREVAILING_DEF_IRONLY;
 	}
       update_visibility_by_resolution_info (vnode);