Re: [PATCH] c++: Optimize in maybe_clone_body aliases even when not at_eof [PR113208]

[Jason Merrill <jason@redhat.com>]

On 5/13/24 06:19, Jakub Jelinek wrote:
> On Fri, May 10, 2024 at 03:59:25PM -0400, Jason Merrill wrote:
>>> 2024-05-09  Jakub Jelinek  <jakub@redhat.com>
>>> 	    Jason Merrill  <jason@redhat.com>
>>>
>>> 	PR lto/113208
>>> 	* cp-tree.h (maybe_optimize_cdtor): Remove.
>>> 	* decl2.cc (tentative_decl_linkage): Call maybe_make_one_only
>>> 	for implicit instantiations of maybe in charge ctors/dtors
>>> 	declared inline.
>>> 	(import_export_decl): Don't call maybe_optimize_cdtor.
>>> 	(c_parse_final_cleanups): Formatting fixes.
>>> 	* optimize.cc (can_alias_cdtor): Adjust condition, for
>>> 	HAVE_COMDAT_GROUP && DECL_ONE_ONLY && DECL_WEAK return true even
>>> 	if not DECL_INTERFACE_KNOWN.
>>
>>> --- gcc/cp/optimize.cc.jj	2024-04-25 20:33:30.771858912 +0200
>>> +++ gcc/cp/optimize.cc	2024-05-09 17:10:23.920478922 +0200
>>> @@ -220,10 +220,8 @@ can_alias_cdtor (tree fn)
>>>      gcc_assert (DECL_MAYBE_IN_CHARGE_CDTOR_P (fn));
>>>      /* Don't use aliases for weak/linkonce definitions unless we can put both
>>>         symbols in the same COMDAT group.  */
>>> -  return (DECL_INTERFACE_KNOWN (fn)
>>> -	  && (SUPPORTS_ONE_ONLY || !DECL_WEAK (fn))
>>> -	  && (!DECL_ONE_ONLY (fn)
>>> -	      || (HAVE_COMDAT_GROUP && DECL_WEAK (fn))));
>>> +  return (DECL_WEAK (fn) ? (HAVE_COMDAT_GROUP && DECL_ONE_ONLY (fn))
>>> +			 : (DECL_INTERFACE_KNOWN (fn) && !DECL_ONE_ONLY (fn)));
>>
>> Hmm, would
>>
>> (DECL_ONE_ONLY (fn) ? HAVE_COMDAT_GROUP
>>   : (DECL_INTERFACE_KNOWN (fn) && !DECL_WEAK (fn)))
>>
>> make sense instead?  I don't think DECL_WEAK is necessary for COMDAT.
> 
> I think it isn't indeed necessary for COMDAT, although e.g. comdat_linkage
> will not call make_decl_one_only if !flag_weak.
> 
> But I think it is absolutely required for the alias cdtor optimization
> in question, because otherwise it would be an ABI change.
> Consider older version of GCC or some other compiler emitting
> _ZN6vectorI12QualityValueEC1ERKS1_
> and
> _ZN6vectorI12QualityValueEC2ERKS1_
> symbols not as aliases, each in their own comdat groups, so
> .text._ZN6vectorI12QualityValueEC1ERKS1_ in _ZN6vectorI12QualityValueEC1ERKS1_
> comdat group and
> .text._ZN6vectorI12QualityValueEC2ERKS1_ in _ZN6vectorI12QualityValueEC2ERKS1_
> comdat group.  And then comes GCC with the above patch without the DECL_WEAK
> check in there, and decides to use alias, so
> _ZN6vectorI12QualityValueEC1ERKS1_ is an alias to
> _ZN6vectorI12QualityValueEC2ERKS1_ and both live in
> .text._ZN6vectorI12QualityValueEC2ERKS1_ section in
> _ZN6vectorI12QualityValueEC5ERKS1_ comdat group.  If you mix TUs with this,
> the linker can keep one of the section sets from the _ZN6vectorI12QualityValueEC1ERKS1_
> and _ZN6vectorI12QualityValueEC2ERKS1_ and _ZN6vectorI12QualityValueEC5ERKS1_
> comdat groups.  If there is no .weak for the symbols, this will fail to
> link, one can emit it either the old way or the new way but never both, it
> is part of an ABI.
> While with .weak, mixing it is possible, worst case one gets some unused
> code in the linked binary or shared library.  Of course the desirable case
> is that there is no mixing and there is no unused code, but if it happens,
> no big deal.  Without .weak it is a big deal.

Makes sense, the patch is OK.

Jason