Re: [PATCH] c++/modules: local class merging [PR99426]

[Jason Merrill <jason@redhat.com>]

On 4/12/24 10:35, Patrick Palka wrote:
> On Wed, 10 Apr 2024, Jason Merrill wrote:
> 
>> On 4/10/24 14:48, Patrick Palka wrote:
>>> On Tue, 9 Apr 2024, Jason Merrill wrote:
>>>
>>>> On 3/5/24 10:31, Patrick Palka wrote:
>>>>> On Tue, 27 Feb 2024, Patrick Palka wrote:
>>>>>
>>>>> Subject: [PATCH] c++/modules: local type merging [PR99426]
>>>>>
>>>>> One known missing piece in the modules implementation is merging of a
>>>>> streamed-in local type (class or enum) with the corresponding in-TU
>>>>> version of the local type.  This missing piece turns out to cause a
>>>>> hard-to-reduce use-after-free GC issue due to the entity_ary not being
>>>>> marked as a GC root (deliberately), and manifests as a serialization
>>>>> error on stream-in as in PR99426 (see comment #6 for a reduction).  It's
>>>>> also reproducible on trunk when running the xtreme-header tests without
>>>>> -fno-module-lazy.
>>>>>
>>>>> This patch makes us merge such local types according to their position
>>>>> within the containing function's definition, analogous to how we merge
>>>>> FIELD_DECLs of a class according to their index in the TYPE_FIELDS
>>>>> list.
>>>>>
>>>>> 	PR c++/99426
>>>>>
>>>>> gcc/cp/ChangeLog:
>>>>>
>>>>> 	* module.cc (merge_kind::MK_local_type): New enumerator.
>>>>> 	(merge_kind_name): Update.
>>>>> 	(trees_out::chained_decls): Move BLOCK-specific handling
>>>>> 	of DECL_LOCAL_DECL_P decls to ...
>>>>> 	(trees_out::core_vals) <case BLOCK>: ... here.  Stream
>>>>> 	BLOCK_VARS manually.
>>>>> 	(trees_in::core_vals) <case BLOCK>: Stream BLOCK_VARS
>>>>> 	manually.  Handle deduplicated local types..
>>>>> 	(trees_out::key_local_type): Define.
>>>>> 	(trees_in::key_local_type): Define.
>>>>> 	(trees_out::get_merge_kind) <case FUNCTION_DECL>: Return
>>>>> 	MK_local_type for a local type.
>>>>> 	(trees_out::key_mergeable) <case FUNCTION_DECL>: Use
>>>>> 	key_local_type.
>>>>> 	(trees_in::key_mergeable) <case FUNCTION_DECL>: Likewise.
>>>>> 	(trees_in::is_matching_decl): Be flexible with type mismatches
>>>>> 	for local entities.
>>>>>
>>>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
>>>>> index 80b63a70a62..d9e34e9a4b9 100644
>>>>> --- a/gcc/cp/module.cc
>>>>> +++ b/gcc/cp/module.cc
>>>>> @@ -6714,7 +6720,37 @@ trees_in::core_vals (tree t)
>>>>>         case BLOCK:
>>>>>           t->block.locus = state->read_location (*this);
>>>>>           t->block.end_locus = state->read_location (*this);
>>>>> -      t->block.vars = chained_decls ();
>>>>> +
>>>>> +      for (tree *chain = &t->block.vars;;)
>>>>> +	if (tree decl = tree_node ())
>>>>> +	  {
>>>>> +	    /* For a deduplicated local type or enumerator, chain the
>>>>> +	       duplicate decl instead of the canonical in-TU decl.  Seeing
>>>>> +	       a duplicate here means the containing function whose body
>>>>> +	       we're streaming in is a duplicate too, so we'll end up
>>>>> +	       discarding this BLOCK (and the rest of the duplicate function
>>>>> +	       body) anyway.  */
>>>>> +	    if (is_duplicate (decl))
>>>>> +	      decl = maybe_duplicate (decl);
>>>>> +	    else if (DECL_IMPLICIT_TYPEDEF_P (decl)
>>>>> +		     && TYPE_TEMPLATE_INFO (TREE_TYPE (decl)))
>>>>> +	      {
>>>>> +		tree tmpl = TYPE_TI_TEMPLATE (TREE_TYPE (decl));
>>>>> +		if (DECL_TEMPLATE_RESULT (tmpl) == decl && is_duplicate
>>>>> (tmpl))
>>>>> +		  decl = DECL_TEMPLATE_RESULT (maybe_duplicate (tmpl));
>>>>> +	      }
>>>>
>>>> This seems like a lot of generally-applicable code for finding the
>>>> duplicate,
>>>> which other calls to maybe_duplicate/odr_duplicate don't use.  If the
>>>> template
>>>> is a duplicate, why isn't its result?  If there's a good reason for that,
>>>> should this template handling go into maybe_duplicate?
>>>
>>> Ah yeah, that makes sense.
>>>
>>> Some context: IIUC modules treats the TEMPLATE_DECL instead of the
>>> DECL_TEMPLATE_RESULT as the canonical decl, which in turn means we'll
>>> register_duplicate only the TEMPLATE_DECL.  But BLOCK_VARS never contains
>>> a TEMPLATE_DECL, always the DECL_TEMPLATE_RESULT (i.e. a TYPE_DECL),
>>> hence the extra handling.
>>>
>>> Given that it's relatively more difficult to get at the TEMPLATE_DECL
>>> from the DECL_TEMPLATE_RESULT rather than vice versa, maybe we should
>>> just register both as duplicates from register_duplicate?  That way
>>> callers can just simply pass the DECL_TEMPLATE_RESULT to maybe_duplicate
>>> and it'll do the right thing.
>>
>> Sounds good.
>>
>>>>> @@ -10337,6 +10373,83 @@ trees_in::fn_parms_fini (int tag, tree fn, tree
>>>>> existing, bool is_defn)
>>>>>         }
>>>>>     }
>>>>>     +/* Encode into KEY the position of the local type (class or enum)
>>>>> +   declaration DECL within FN.  The position is encoded as the
>>>>> +   index of the innermost BLOCK (numbered in BFS order) along with
>>>>> +   the index within its BLOCK_VARS list.  */
>>>>
>>>> Since we already set DECL_DISCRIMINATOR for mangling, could we use it+name
>>>> for
>>>> the key as well?
>>>
>>> We could (and IIUc that'd be more robust to ODR violations), but
>>> wouldn't it mean we'd have to do a linear walk over all BLOCK_VARs of
>>> all BLOCKS in order to find the one with the matching
>>> name+discriminator?  That'd be slower than the current approach which
>>> lets us skip to the correct BLOCK and walk only its BLOCK_VARS.
>>
>> Ah, good point.  How about block number + name instead of the index?
> 
> It seems DECL_DISCRIMINATOR is only set at instantiation time and so for
> local types from a function template pattern the field is empty, which
> means we can't use it as the key in general :/

I meant just block number and name, without DECL_DISCRIMINATOR.  Just 
using the name instead of an index in BLOCK_VARS.

Jason