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

[Jason Merrill <jason@redhat.com>]

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?

> @@ -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?

Jason