Re: [PATCH] c++: Keep DECL_SAVED_TREE of destructor instantiations in modules [PR104040]

[Jason Merrill <jason@redhat.com>]

On 4/4/24 07:27, Nathaniel Shead wrote:
> On Wed, Apr 03, 2024 at 11:18:01AM -0400, Jason Merrill wrote:
>> On 4/2/24 20:57, Nathaniel Shead wrote:
>>> On Tue, Apr 02, 2024 at 01:18:17PM -0400, Jason Merrill wrote:
>>>> On 3/28/24 23:21, Nathaniel Shead wrote:
>>>>> -	  && !(modules_p () && DECL_DECLARED_INLINE_P (fn)))
>>>>> +	  && !(modules_p ()
>>>>> +	       && (DECL_DECLARED_INLINE_P (fn)
>>>>> +		   || DECL_TEMPLATE_INSTANTIATION (fn))))
>>>>
>>>> How about using vague_linkage_p?
>>>>
>>>
>>> Right, of course.  How about this?
>>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>>
>>> -- >8 --
>>>
>>> A template instantiation still needs to have its DECL_SAVED_TREE so that
>>> its definition is emitted into the CMI. This way it can be emitted in
>>> the object file of any importers that use it, in case it doesn't end up
>>> getting emitted in this TU.
>>>
>>> 	PR c++/104040
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* semantics.cc (expand_or_defer_fn_1): Keep DECL_SAVED_TREE for
>>> 	all vague linkage functions.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/modules/pr104040_a.C: New test.
>>> 	* g++.dg/modules/pr104040_b.C: New test.
>>>
>>> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
>>> Reviewed-by: Jason Merrill <jason@redhat.com>
>>> ---
>>>    gcc/cp/semantics.cc                       |  5 +++--
>>>    gcc/testsuite/g++.dg/modules/pr104040_a.C | 14 ++++++++++++++
>>>    gcc/testsuite/g++.dg/modules/pr104040_b.C |  8 ++++++++
>>>    3 files changed, 25 insertions(+), 2 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/modules/pr104040_a.C
>>>    create mode 100644 gcc/testsuite/g++.dg/modules/pr104040_b.C
>>>
>>> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
>>> index adb1ba48d29..03800a20b26 100644
>>> --- a/gcc/cp/semantics.cc
>>> +++ b/gcc/cp/semantics.cc
>>> @@ -5033,9 +5033,10 @@ expand_or_defer_fn_1 (tree fn)
>>>          /* We don't want to process FN again, so pretend we've written
>>>    	 it out, even though we haven't.  */
>>>          TREE_ASM_WRITTEN (fn) = 1;
>>> -      /* If this is a constexpr function, keep DECL_SAVED_TREE.  */
>>> +      /* If this is a constexpr function, or the body might need to be
>>> +	 exported from a module CMI, keep DECL_SAVED_TREE.  */
>>>          if (!DECL_DECLARED_CONSTEXPR_P (fn)
>>> -	  && !(modules_p () && DECL_DECLARED_INLINE_P (fn)))
>>> +	  && !(modules_p () && vague_linkage_p (fn)))
>>
>> Also, how about module_maybe_has_cmi_p?  OK with that change.
> 
> Using 'module_maybe_has_cmi_p' doesn't seem to work.  This is for two
> reasons, one of them fixable and one of them not (easily):
> 
> - It seems that header modules don't count for 'module_maybe_has_cmi_p';
>    I didn't notice this initially, and maybe they should for the
>    no-linkage decls too?

I think so; they could similarly be referred to by an importer.

>  But even accounting for this,
> 
> - For some reason only clearing it if the module might have a CMI causes
>    crashes in importers for some testcases.  I'm not 100% sure why yet,
>    but I suspect it might be some duplicate-decls thing where the type
>    inconsistently has DECL_SAVED_TREE applied, since this is also called
>    on streamed-in declarations.

Clearing if the module might have a CMI sounds backwards, I'd expect 
that to be the case where we want to leave it alone.  Is that the 
problem, or just a typo?

> Out of interest, what was the reason that it was cleared at all in the
> first place?  I wasn't able to find anything with git blame; is it just
> for performance reasons in avoiding excess lowering later?

That change goes back to the LTO merge, I believe it was to reduce 
unnecessary LTO streaming.

But now that I think about it some more, I don't see why handling 
modules specially here is necessary at all; the point of this code is 
that after we build the destructor clones, the DECL_SAVED_TREE of the 
cloned function is no longer useful.  Why would modules care about the 
maybe-in-charge function?

Jason