c++/modules: Stream additional fields for DECL_STRUCT_FUNCTION [PR113580]

This patch just adds enough of the fields from 'function' to fix the ICE
in the linked PR. I suppose there might be more fields from this type
that should be propagated, but I don't know enough to find out which
they might be yet, since a lot of them seem to be only set after
gimplification.

Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

Currently the DECL_STRUCT_FUNCTION for a declaration is always
reconstructed from scratch. This causes issues though, as some fields
used by other parts of the compiler (in this case, specifically
'function_{start,end}_locus') are then not correctly initialised. This
patch makes sure that these fields are also read and written.

	PR c++/113580

gcc/cp/ChangeLog:

	* module.cc (struct post_process_data): Create.
	(trees_in::post_decls): Use.
	(trees_in::post_process): Return entire vector at once.
	Change overload to take post_process_data instead of tree.
	(trees_out::write_function_def): Write needed flags from
	DECL_STRUCT_FUNCTION.
	(trees_in::read_function_def): Read them and pass to
	post_process.
	(module_state::read_cluster): Write flags into cfun.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr113580_a.C: New test.
	* g++.dg/modules/pr113580_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/module.cc                          | 47 ++++++++++++++++++-----
 gcc/testsuite/g++.dg/modules/pr113580_a.C | 10 +++++
 gcc/testsuite/g++.dg/modules/pr113580_b.C | 10 +++++
 3 files changed, 58 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr113580_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/pr113580_b.C

On Fri, 26 Jan 2024, Nathaniel Shead wrote:

> This patch just adds enough of the fields from 'function' to fix the ICE
> in the linked PR. I suppose there might be more fields from this type
> that should be propagated, but I don't know enough to find out which
> they might be yet, since a lot of them seem to be only set after
> gimplification.
> 
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
> 
> -- >8 --
> 
> Currently the DECL_STRUCT_FUNCTION for a declaration is always
> reconstructed from scratch. This causes issues though, as some fields
> used by other parts of the compiler (in this case, specifically
> 'function_{start,end}_locus') are then not correctly initialised. This
> patch makes sure that these fields are also read and written.

LGTM

> 
> 	PR c++/113580
> 
> gcc/cp/ChangeLog:
> 
> 	* module.cc (struct post_process_data): Create.
> 	(trees_in::post_decls): Use.
> 	(trees_in::post_process): Return entire vector at once.
> 	Change overload to take post_process_data instead of tree.
> 	(trees_out::write_function_def): Write needed flags from
> 	DECL_STRUCT_FUNCTION.
> 	(trees_in::read_function_def): Read them and pass to
> 	post_process.
> 	(module_state::read_cluster): Write flags into cfun.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/pr113580_a.C: New test.
> 	* g++.dg/modules/pr113580_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  gcc/cp/module.cc                          | 47 ++++++++++++++++++-----
>  gcc/testsuite/g++.dg/modules/pr113580_a.C | 10 +++++
>  gcc/testsuite/g++.dg/modules/pr113580_b.C | 10 +++++
>  3 files changed, 58 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr113580_a.C
>  create mode 100644 gcc/testsuite/g++.dg/modules/pr113580_b.C
> 
> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc
> index 6176801b7a7..840c7ef6dab 100644
> --- a/gcc/cp/module.cc
> +++ b/gcc/cp/module.cc
> @@ -2837,6 +2837,13 @@ typedef hash_map<tree,uintptr_t,
>  		 simple_hashmap_traits<nodel_ptr_hash<tree_node>,uintptr_t> >
>  duplicate_hash_map;
>  
> +/* Data needed for post-processing.  */
> +struct post_process_data {
> +  tree decl;
> +  location_t start_locus;
> +  location_t end_locus;
> +};
> +
>  /* Tree stream reader.  Note that reading a stream doesn't mark the
>     read trees with TREE_VISITED.  Thus it's quite safe to have
>     multiple concurrent readers.  Which is good, because lazy
> @@ -2848,7 +2855,7 @@ private:
>    module_state *state;		/* Module being imported.  */
>    vec<tree> back_refs;		/* Back references.  */
>    duplicate_hash_map *duplicates;	/* Map from existings to duplicate.  */
> -  vec<tree> post_decls;		/* Decls to post process.  */
> +  vec<post_process_data> post_decls;	/* Decls to post process.  */
>    unsigned unused;		/* Inhibit any interior TREE_USED
>  				   marking.  */
>  
> @@ -2945,16 +2952,16 @@ public:
>    tree odr_duplicate (tree decl, bool has_defn);
>  
>  public:
> -  /* Return the next decl to postprocess, or NULL.  */
> -  tree post_process ()
> +  /* Return the decls to postprocess.  */
> +  const vec<post_process_data>& post_process ()
>    {
> -    return post_decls.length () ? post_decls.pop () : NULL_TREE;
> +    return post_decls;
>    }
>  private:
> -  /* Register DECL for postprocessing.  */
> -  void post_process (tree decl)
> +  /* Register DATA for postprocessing.  */
> +  void post_process (post_process_data data)
>    {
> -    post_decls.safe_push (decl);
> +    post_decls.safe_push (data);
>    }
>  
>  private:
> @@ -11667,15 +11674,25 @@ trees_out::write_function_def (tree decl)
>        tree_node (cexpr->body);
>      }
>  
> +  function* f = DECL_STRUCT_FUNCTION (decl);
> +
>    if (streaming_p ())
>      {
>        unsigned flags = 0;
>  
> +      if (f)
> +	flags |= 2;
>        if (DECL_NOT_REALLY_EXTERN (decl))
>  	flags |= 1;
>  
>        u (flags);
>      }
> +
> +  if (state && f)
> +    {
> +      state->write_location (*this, f->function_start_locus);
> +      state->write_location (*this, f->function_end_locus);
> +    }
>  }
>  
>  void
> @@ -11692,6 +11709,8 @@ trees_in::read_function_def (tree decl, tree maybe_template)
>    tree saved = tree_node ();
>    tree context = tree_node ();
>    constexpr_fundef cexpr;
> +  post_process_data pdata {};
> +  pdata.decl = maybe_template;
>  
>    tree maybe_dup = odr_duplicate (maybe_template, DECL_SAVED_TREE (decl));
>    bool installing = maybe_dup && !DECL_SAVED_TREE (decl);
> @@ -11708,6 +11727,12 @@ trees_in::read_function_def (tree decl, tree maybe_template)
>  
>    unsigned flags = u ();
>  
> +  if (flags & 2)
> +    {
> +      pdata.start_locus = state->read_location (*this);
> +      pdata.end_locus = state->read_location (*this);
> +    }
> +
>    if (get_overrun ())
>      return NULL_TREE;
>  
> @@ -11722,7 +11747,7 @@ trees_in::read_function_def (tree decl, tree maybe_template)
>  	SET_DECL_FRIEND_CONTEXT (decl, context);
>        if (cexpr.decl)
>  	register_constexpr_fundef (cexpr);
> -      post_process (maybe_template);
> +      post_process (pdata);
>      }
>    else if (maybe_dup)
>      {
> @@ -15100,8 +15125,10 @@ module_state::read_cluster (unsigned snum)
>       push_function_context does too much work.   */
>    tree old_cfd = current_function_decl;
>    struct function *old_cfun = cfun;
> -  while (tree decl = sec.post_process ())
> +  for (const post_process_data& pdata : sec.post_process ())
>      {
> +      tree decl = pdata.decl;
> +
>        bool abstract = false;
>        if (TREE_CODE (decl) == TEMPLATE_DECL)
>  	{
> @@ -15113,6 +15140,8 @@ module_state::read_cluster (unsigned snum)
>        allocate_struct_function (decl, abstract);
>        cfun->language = ggc_cleared_alloc<language_function> ();
>        cfun->language->base.x_stmt_tree.stmts_are_full_exprs_p = 1;
> +      cfun->function_start_locus = pdata.start_locus;
> +      cfun->function_end_locus = pdata.end_locus;
>  
>        if (abstract)
>  	;
> diff --git a/gcc/testsuite/g++.dg/modules/pr113580_a.C b/gcc/testsuite/g++.dg/modules/pr113580_a.C
> new file mode 100644
> index 00000000000..954333f5038
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr113580_a.C
> @@ -0,0 +1,10 @@
> +// PR c++/113580
> +// { dg-additional-options "-fmodules-ts" }
> +// { dg-module-cmi A }
> +
> +export module A;
> +
> +export {
> +  template <typename T>
> +  void fun(T x) {}
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/pr113580_b.C b/gcc/testsuite/g++.dg/modules/pr113580_b.C
> new file mode 100644
> index 00000000000..a72cd750c72
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr113580_b.C
> @@ -0,0 +1,10 @@
> +// PR c++/113580
> +// { dg-additional-options "-fmodules-ts -Wunused-parameter" }
> +
> +import A;
> +
> +int main() {
> +  fun(42);  // { dg-message "required from here" }
> +}
> +
> +// { dg-warning "unused parameter" "" { target *-*-* } 0 }
> -- 
> 2.43.0
> 
>

On 1/26/24 10:49, Patrick Palka wrote:
> On Fri, 26 Jan 2024, Nathaniel Shead wrote:
> 
>> This patch just adds enough of the fields from 'function' to fix the ICE
>> in the linked PR. I suppose there might be more fields from this type
>> that should be propagated, but I don't know enough to find out which
>> they might be yet, since a lot of them seem to be only set after
>> gimplification.
>>
>> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?
>>
>> -- >8 --
>>
>> Currently the DECL_STRUCT_FUNCTION for a declaration is always
>> reconstructed from scratch. This causes issues though, as some fields
>> used by other parts of the compiler (in this case, specifically
>> 'function_{start,end}_locus') are then not correctly initialised. This
>> patch makes sure that these fields are also read and written.
> 
> LGTM

Yep, OK.

Jason