Message ID | 3b5615c7-ec54-7b27-f353-1e898b798677@acm.org |
---|---|
State | New |
Headers | show |
> honza, > this is the fix for the partitioned WPA bug I was tracking down. > > We have base and complete dtors sharing a comdat group (one's an alias for > the other). The linker tells us the complete dtor is PREVAILING_DEF, as > it's referenced from some other library. The base dtor is UNKNOWN. > > We therefore internalize the base dtor, making it PREVAILING_DEF_IRONLY and > split the comdat group. But the comdat group splitting also internalizes > the complete dtor /and/ it does so inconsistently. > > The bug manifested at runtime w/o link error as the complete dtor resolved > to zero in the final link from wpa partitions that didn't contain its > definition. (For extra fun, that was via a call to __cxa_at_exit registering > a null function pointer, and getting the subsequent seg fault at program > exit.) When we created WPA partitions node->externally_visible was still > set, so we thought the now-internalized complete dtor was still externally > visible -- but varasm looks at the DECL itself and emits an internal one. > Plus the references to it were weak (& now hidden), so resolved to zero, > rather than link error. And the external library either had its own > definition which then prevailed for it. All rather 'ew'. > > Anyway, this patch does 3 things > 1) Moves the next->unique_name adjustment to before make_decl_local for > members of the comdat group -- that matches the behaviour of the decl of > interest itself. That part is OK. > > 2) For LDPR_PREVAILING_DEF members we don't make_decl_local, but instead > clear DECL_COMDAT and DECL_WEAK. Thus forcing this decl to be the > prevailing decl in the final link > > 3) For decls we localize, we also clear node->externally_visible and > node->force_by_abi. That matches the behavior for the decl of interest too > and will clue the wpa partitioning logic into knowing it needs to > hidden-externalize the decl. So at the moment it works in a way 1) we walk first symbol of the comdat and it is LDPR_PREVAILING_DEF and thus we set externall visible flag 2) we walk second symbol of the comdat and it is LDPR_PREVAILING_DEF_IRONLY and thus we decide to privatize the whole comdat group, during this process we force the first symbol local and clear the externally_visible flag? I think at a time we decide on external visibility of a symbol in a comdat group, we need to check that the comdat group as a whole is not exporte (i.e. no LDPR_PREVAILING_DEF_EXP or incremental linking). Then we know we can dissolve the comdat group (without actually affecting visibility) and then we can handle each symbol independently. Honza > > nathan > > -- > Nathan Sidwell > 2017-01-18 Nathan Sidwell <nathan@acm.org> > > * ipa-visibility.c (localize_node): Set comdat's unique name > before adjusting resolution. Make PREVAILING_DEF members strongly > public. Set visibility to false for localized decls. > > Index: ipa-visibility.c > =================================================================== > --- ipa-visibility.c (revision 244546) > +++ ipa-visibility.c (working copy) > @@ -542,16 +542,32 @@ localize_node (bool whole_program, symta > for (symtab_node *next = node->same_comdat_group; > next != node; next = next->same_comdat_group) > { > - next->set_comdat_group (NULL); > - if (!next->alias) > - next->set_section (NULL); > - if (!next->transparent_alias) > - next->make_decl_local (); > next->unique_name > |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY > || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP) > && TREE_PUBLIC (next->decl) > && !flag_incremental_link); > + > + next->set_comdat_group (NULL); > + if (!next->alias) > + next->set_section (NULL); > + if (next->transparent_alias) > + /* Do nothing. */; > + else if (next->resolution == LDPR_PREVAILING_DEF) > + { > + /* Make this a strong defn, so the external > + users don't mistakenly choose some other > + instance. */ > + DECL_COMDAT (next->decl) = false; > + DECL_WEAK (next->decl) = false; > + } > + else > + { > + next->externally_visible = false; > + next->forced_by_abi = false; > + next->resolution = LDPR_PREVAILING_DEF_IRONLY; > + next->make_decl_local (); > + } > } > > /* Now everything's localized, the grouping has no meaning, and
On 01/19/2017 10:26 AM, Jan Hubicka wrote: >> 2) For LDPR_PREVAILING_DEF members we don't make_decl_local, but instead >> clear DECL_COMDAT and DECL_WEAK. Thus forcing this decl to be the >> prevailing decl in the final link >> >> 3) For decls we localize, we also clear node->externally_visible and >> node->force_by_abi. That matches the behavior for the decl of interest too >> and will clue the wpa partitioning logic into knowing it needs to >> hidden-externalize the decl. > > So at the moment it works in a way > 1) we walk first symbol of the comdat and it is LDPR_PREVAILING_DEF and thus > we set externall visible flag > 2) we walk second symbol of the comdat and it is LDPR_PREVAILING_DEF_IRONLY > and thus we decide to privatize the whole comdat group, during this > process we force the first symbol local and clear the externally_visible > flag? Nearly correct. We attempt to do #2 but we fail to clear node->externally_visible (but we do update DECL_PUBLIC). Thus the PREVAILING_DEF symbol is in an inconsistent state (which is what confused the partitioning logic). > I think at a time we decide on external visibility of a symbol in a comdat > group, we need to check that the comdat group as a whole is not exporte (i.e. > no LDPR_PREVAILING_DEF_EXP or incremental linking). Then we know we can dissolve > the comdat group (without actually affecting visibility) and then we can > handle each symbol independently. Could be. I did consider splitting this loop into two (which I think is an outcome of what you suggest) -- one that set each nodes external_visiblilty as desired and the a second loop that did the processing. It also occurs to me that if we did that we'd need to process the vardecls in a similar order. I.e. go from the current: foreach FN determine visibility adjust decl/break comdat for each VAR determine visibility adjust decl/break comdat to foreach FN determine visibility foreach VAR determine visibility foreach FN adjust decl/maybe break comdat foreach VAR adjust decl/maybe break comdat nathan
2017-01-18 Nathan Sidwell <nathan@acm.org> * ipa-visibility.c (localize_node): Set comdat's unique name before adjusting resolution. Make PREVAILING_DEF members strongly public. Set visibility to false for localized decls. Index: ipa-visibility.c =================================================================== --- ipa-visibility.c (revision 244546) +++ ipa-visibility.c (working copy) @@ -542,16 +542,32 @@ localize_node (bool whole_program, symta for (symtab_node *next = node->same_comdat_group; next != node; next = next->same_comdat_group) { - next->set_comdat_group (NULL); - if (!next->alias) - next->set_section (NULL); - if (!next->transparent_alias) - next->make_decl_local (); next->unique_name |= ((next->resolution == LDPR_PREVAILING_DEF_IRONLY || next->resolution == LDPR_PREVAILING_DEF_IRONLY_EXP) && TREE_PUBLIC (next->decl) && !flag_incremental_link); + + next->set_comdat_group (NULL); + if (!next->alias) + next->set_section (NULL); + if (next->transparent_alias) + /* Do nothing. */; + else if (next->resolution == LDPR_PREVAILING_DEF) + { + /* Make this a strong defn, so the external + users don't mistakenly choose some other + instance. */ + DECL_COMDAT (next->decl) = false; + DECL_WEAK (next->decl) = false; + } + else + { + next->externally_visible = false; + next->forced_by_abi = false; + next->resolution = LDPR_PREVAILING_DEF_IRONLY; + next->make_decl_local (); + } } /* Now everything's localized, the grouping has no meaning, and