Re: [PATCH] c++, v3: Retry the aliasing of base/complete cdtor optimization at import_export_decl time [PR113208]

[Jason Merrill <jason@redhat.com>]

On 4/24/24 09:16, Jakub Jelinek wrote:
> On Wed, Apr 24, 2024 at 10:16:05AM +0100, Jonathan Wakely wrote:
>>> That fixes the testcases too, but seems to regress
>>> +FAIL: libstdc++-abi/abi_check
> 
>> There are explicit instantiation definitions that should instantiate
>> those types:
>>
>> src/c++17/fs_dir.cc:template class std::__shared_ptr<fs::_Dir>;
>> src/c++17/fs_dir.cc:template class
>> std::__shared_ptr<fs::recursive_directory_iterator::_Dir_stack>;
>> src/c++17/fs_path.cc:template class std::__shared_ptr<const
>> fs::filesystem_error::_Impl>;
>>
>> So the missing symbols should be present in cow-fs_dir.o and cow-fs_path.o
> 
> So this boils down to (cvise reduced):
> namespace __gnu_cxx { enum _Lock_policy { _S_single, _S_mutex, _S_atomic } const __default_lock_policy = _S_atomic; }
> namespace std {
> using __gnu_cxx::__default_lock_policy;
> using __gnu_cxx::_Lock_policy;
> template <typename, _Lock_policy = __default_lock_policy> struct __shared_ptr { constexpr __shared_ptr() {} };
> namespace filesystem {
> struct _Dir;
> struct directory_iterator { __shared_ptr<_Dir> _M_dir; };
> void end() { directory_iterator(); } }
> extern template class __shared_ptr<filesystem::_Dir>;
> }
> namespace fs = std::filesystem;
> template class std::__shared_ptr<fs::_Dir>;
> at -O2, previously it used to emit
> _ZNSt12__shared_ptrINSt10filesystem4_DirELN9__gnu_cxx12_Lock_policyE2EEC2Ev
> _ZNSt12__shared_ptrINSt10filesystem4_DirELN9__gnu_cxx12_Lock_policyE1EEC2Ev
> but now no longer does with the yesterday posted PR113208 patch.
> 
> The following updated patch fixes that by calling note_vague_linkage_fn for
> the cdtor clones from maybe_clone_body if the flags suggest that the
> maybe-in-charge cdtor had tentative_decl_linkage -> note_vague_linkage_fn
> called too.  And then I've noticed that in some cases the updated comdat
> group set by maybe_clone_body (*C5* or *D5*) was then overwritten again by
> maybe_make_one_only.  So the patch tweaks cxx_comdat_group, so that when
> some comdat group has been chosen already it doesn't try to use some
> different one.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, this one doesn't
> regress anything unlike the earlier patch.
> 
> 2024-04-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR lto/113208
> 	* decl2.cc (tentative_decl_linkage): Use comdat_linkage also
> 	for implicit instantiations of maybe in charge ctors/dtors
> 	if -fimplicit-templates or -fimplicit-inline-templates and
> 	-fweak and target supports aliases.
> 	* optimize.cc (maybe_clone_body): Call note_vague_linkage_fn
> 	on clone if fn has DECL_INTERFACE_KNOWN, DECL_NOT_REALLY_EXTERN
> 	and DECL_DEFER_OUTPUT flags set.
> 	* decl.cc (cxx_comdat_group): For DECL_CLONED_FUNCTION_P
> 	functions if SUPPORTS_ONE_ONLY return DECL_COMDAT_GROUP if already
> 	set.
> 
> 	* g++.dg/abi/comdat2.C: New test.
> 	* g++.dg/abi/comdat3.C: New test.
> 	* g++.dg/lto/pr113208_0.C: New test.
> 	* g++.dg/lto/pr113208_1.C: New file.
> 	* g++.dg/lto/pr113208.h: New file.
> 
> --- gcc/cp/decl2.cc.jj	2024-04-23 14:49:41.933186265 +0200
> +++ gcc/cp/decl2.cc	2024-04-24 15:17:09.043625729 +0200
> @@ -3314,7 +3314,16 @@ tentative_decl_linkage (tree decl)
>   	     to mark the functions at this point.  */
>   	  if (DECL_DECLARED_INLINE_P (decl)
>   	      && (!DECL_IMPLICIT_INSTANTIATION (decl)
> -		  || DECL_DEFAULTED_FN (decl)))
> +		  || DECL_DEFAULTED_FN (decl)
> +		  /* For implicit instantiations of cdtors,
> +		     if import_export_decl would use comdat linkage,
> +		     make sure to use it right away, so that maybe_clone_body
> +		     can use aliases.  See PR113208.  */
> +		  || (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl)
> +		      && (flag_implicit_templates
> +			  || flag_implicit_inline_templates)
> +		      && flag_weak
> +		      && TARGET_SUPPORTS_ALIASES)))

It seems wrong to set DECL_INTERFACE_KNOWN for cdtors that might get an 
explicit instantiation later, and likewise for comdat_linkage when 
!flag_weak; instead of adding this condition to the if, how about adding 
an else like

>           else if (DECL_MAYBE_IN_CHARGE_CDTOR_P (decl))
>             /* For implicit instantiations of cdtors,                                                                    
>                if import_export_decl would use comdat linkage,                                                           
>                make sure to use it right away, so that maybe_clone_body                                                  
>                can use aliases.  See PR113208.  */
>             maybe_make_one_only (decl);

?

Jason