c++: Modularise start_cleanup_fn [PR98893]

Message ID 679df798.050a0220.24dd54.eec9@mx.google.com
State New
Headers
Series c++: Modularise start_cleanup_fn [PR98893] |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Test passed

Commit Message

Nathaniel Shead Feb. 1, 2025, 10:29 a.m. UTC
  Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

-- >8 --

'start_cleanup_fn' is not currently viable in modules, due to generating
functions relying on the 'start_cleanup_cnt' counter which is reset to 0
with each new TU.  This means that cleanup functions declared in a TU
will conflict with any imported cleanup functions.

This patch mitigates the problem by using the mangled name of the decl
we're destroying as part of the name of the function.  This should avoid
clashes unless the decls would have clashed anyway.

	PR c++/98893

gcc/cp/ChangeLog:

	* decl.cc (start_cleanup_fn): Make name from the mangled name of
	the passed-in decl.
	(register_dtor_fn): Pass decl to start_cleanup_fn.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/pr98893_a.H: New test.
	* g++.dg/modules/pr98893_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/decl.cc                           | 18 +++++++++---------
 gcc/testsuite/g++.dg/modules/pr98893_a.H |  9 +++++++++
 gcc/testsuite/g++.dg/modules/pr98893_b.C | 10 ++++++++++
 3 files changed, 28 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/pr98893_a.H
 create mode 100644 gcc/testsuite/g++.dg/modules/pr98893_b.C
  

Comments

Jason Merrill Feb. 4, 2025, midnight UTC | #1
On 2/1/25 5:29 AM, Nathaniel Shead wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, OK for trunk?

OK.

> -- >8 --
> 
> 'start_cleanup_fn' is not currently viable in modules, due to generating
> functions relying on the 'start_cleanup_cnt' counter which is reset to 0
> with each new TU.  This means that cleanup functions declared in a TU
> will conflict with any imported cleanup functions.
> 
> This patch mitigates the problem by using the mangled name of the decl
> we're destroying as part of the name of the function.  This should avoid
> clashes unless the decls would have clashed anyway.
> 
> 	PR c++/98893
> 
> gcc/cp/ChangeLog:
> 
> 	* decl.cc (start_cleanup_fn): Make name from the mangled name of
> 	the passed-in decl.
> 	(register_dtor_fn): Pass decl to start_cleanup_fn.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/pr98893_a.H: New test.
> 	* g++.dg/modules/pr98893_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/decl.cc                           | 18 +++++++++---------
>   gcc/testsuite/g++.dg/modules/pr98893_a.H |  9 +++++++++
>   gcc/testsuite/g++.dg/modules/pr98893_b.C | 10 ++++++++++
>   3 files changed, 28 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/pr98893_a.H
>   create mode 100644 gcc/testsuite/g++.dg/modules/pr98893_b.C
> 
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index cf5e055e146..7219543823b 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -96,7 +96,7 @@ static void record_key_method_defined (tree);
>   static tree create_array_type_for_decl (tree, tree, tree, location_t);
>   static tree get_atexit_node (void);
>   static tree get_dso_handle_node (void);
> -static tree start_cleanup_fn (bool);
> +static tree start_cleanup_fn (tree, bool);
>   static void end_cleanup_fn (void);
>   static tree cp_make_fname_decl (location_t, tree, int);
>   static void initialize_predefined_identifiers (void);
> @@ -10373,23 +10373,23 @@ get_dso_handle_node (void)
>   }
>   
>   /* Begin a new function with internal linkage whose job will be simply
> -   to destroy some particular variable.  OB_PARM is true if object pointer
> +   to destroy some particular DECL.  OB_PARM is true if object pointer
>      is passed to the cleanup function, otherwise no argument is passed.  */
>   
> -static GTY(()) int start_cleanup_cnt;
> -
>   static tree
> -start_cleanup_fn (bool ob_parm)
> +start_cleanup_fn (tree decl, bool ob_parm)
>   {
> -  char name[32];
> -
>     push_to_top_level ();
>   
>     /* No need to mangle this.  */
>     push_lang_context (lang_name_c);
>   
>     /* Build the name of the function.  */
> -  sprintf (name, "__tcf_%d", start_cleanup_cnt++);
> +  gcc_checking_assert (HAS_DECL_ASSEMBLER_NAME_P (decl));
> +  const char *dname = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
> +  dname = targetm.strip_name_encoding (dname);
> +  char *name = ACONCAT (("__tcf", dname, NULL));
> +
>     tree fntype = TREE_TYPE (ob_parm ? get_cxa_atexit_fn_ptr_type ()
>   				   : get_atexit_fn_ptr_type ());
>     /* Build the function declaration.  */
> @@ -10482,7 +10482,7 @@ register_dtor_fn (tree decl)
>         build_cleanup (decl);
>   
>         /* Now start the function.  */
> -      cleanup = start_cleanup_fn (ob_parm);
> +      cleanup = start_cleanup_fn (decl, ob_parm);
>   
>         /* Now, recompute the cleanup.  It may contain SAVE_EXPRs that refer
>   	 to the original function, rather than the anonymous one.  That
> diff --git a/gcc/testsuite/g++.dg/modules/pr98893_a.H b/gcc/testsuite/g++.dg/modules/pr98893_a.H
> new file mode 100644
> index 00000000000..062ab6d9ccc
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr98893_a.H
> @@ -0,0 +1,9 @@
> +// { dg-additional-options "-fmodule-header" }
> +// { dg-module-cmi {} }
> +
> +struct S {
> +  ~S() {}
> +};
> +inline void foo() {
> +  static S a[1];
> +}
> diff --git a/gcc/testsuite/g++.dg/modules/pr98893_b.C b/gcc/testsuite/g++.dg/modules/pr98893_b.C
> new file mode 100644
> index 00000000000..9065589bdfb
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/pr98893_b.C
> @@ -0,0 +1,10 @@
> +// { dg-additional-options "-fmodules" }
> +
> +import "pr98893_a.H";
> +static S b[1];
> +int main() {
> +  foo();
> +}
> +
> +// { dg-final { scan-assembler {__tcf_ZZ3foovE1a:} } }
> +// { dg-final { scan-assembler {__tcf_ZL1b:} } }
  

Patch

diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
index cf5e055e146..7219543823b 100644
--- a/gcc/cp/decl.cc
+++ b/gcc/cp/decl.cc
@@ -96,7 +96,7 @@  static void record_key_method_defined (tree);
 static tree create_array_type_for_decl (tree, tree, tree, location_t);
 static tree get_atexit_node (void);
 static tree get_dso_handle_node (void);
-static tree start_cleanup_fn (bool);
+static tree start_cleanup_fn (tree, bool);
 static void end_cleanup_fn (void);
 static tree cp_make_fname_decl (location_t, tree, int);
 static void initialize_predefined_identifiers (void);
@@ -10373,23 +10373,23 @@  get_dso_handle_node (void)
 }
 
 /* Begin a new function with internal linkage whose job will be simply
-   to destroy some particular variable.  OB_PARM is true if object pointer
+   to destroy some particular DECL.  OB_PARM is true if object pointer
    is passed to the cleanup function, otherwise no argument is passed.  */
 
-static GTY(()) int start_cleanup_cnt;
-
 static tree
-start_cleanup_fn (bool ob_parm)
+start_cleanup_fn (tree decl, bool ob_parm)
 {
-  char name[32];
-
   push_to_top_level ();
 
   /* No need to mangle this.  */
   push_lang_context (lang_name_c);
 
   /* Build the name of the function.  */
-  sprintf (name, "__tcf_%d", start_cleanup_cnt++);
+  gcc_checking_assert (HAS_DECL_ASSEMBLER_NAME_P (decl));
+  const char *dname = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
+  dname = targetm.strip_name_encoding (dname);
+  char *name = ACONCAT (("__tcf", dname, NULL));
+
   tree fntype = TREE_TYPE (ob_parm ? get_cxa_atexit_fn_ptr_type ()
 				   : get_atexit_fn_ptr_type ());
   /* Build the function declaration.  */
@@ -10482,7 +10482,7 @@  register_dtor_fn (tree decl)
       build_cleanup (decl);
 
       /* Now start the function.  */
-      cleanup = start_cleanup_fn (ob_parm);
+      cleanup = start_cleanup_fn (decl, ob_parm);
 
       /* Now, recompute the cleanup.  It may contain SAVE_EXPRs that refer
 	 to the original function, rather than the anonymous one.  That
diff --git a/gcc/testsuite/g++.dg/modules/pr98893_a.H b/gcc/testsuite/g++.dg/modules/pr98893_a.H
new file mode 100644
index 00000000000..062ab6d9ccc
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr98893_a.H
@@ -0,0 +1,9 @@ 
+// { dg-additional-options "-fmodule-header" }
+// { dg-module-cmi {} }
+
+struct S {
+  ~S() {}
+};
+inline void foo() {
+  static S a[1];
+}
diff --git a/gcc/testsuite/g++.dg/modules/pr98893_b.C b/gcc/testsuite/g++.dg/modules/pr98893_b.C
new file mode 100644
index 00000000000..9065589bdfb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/pr98893_b.C
@@ -0,0 +1,10 @@ 
+// { dg-additional-options "-fmodules" }
+
+import "pr98893_a.H";
+static S b[1];
+int main() {
+  foo();
+}
+
+// { dg-final { scan-assembler {__tcf_ZZ3foovE1a:} } }
+// { dg-final { scan-assembler {__tcf_ZL1b:} } }