[PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Arsen Arsenović]

If the mangler is relied on, functions with extern "C" on them emit multiple
definitions of the same name.

gcc/cp/ChangeLog:

	* contracts.cc (build_contract_condition_function): Add pre/post
	suffixes to pre- and postcondition clones.
	* mangle.cc (write_encoding): Don't mangle pre- and postconditions.

gcc/testsuite/ChangeLog:

	* g++.dg/contracts/contracts-externC.C: New test.
---
Afternoon,

This change prevents contracts from emitting wrapper functions that have the
same name as the original function, by moving the logic that disambiguates them
from the mangler into the build_contract_condition_function helper.

Thanks, have nice day.

 gcc/cp/contracts.cc                           |  3 +++
 gcc/cp/mangle.cc                              |  7 -------
 .../g++.dg/contracts/contracts-externC.C      | 19 +++++++++++++++++++
 3 files changed, 22 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/contracts/contracts-externC.C

diff --git a/gcc/cp/contracts.cc b/gcc/cp/contracts.cc
index 26316372389..f09eb87e283 100644
--- a/gcc/cp/contracts.cc
+++ b/gcc/cp/contracts.cc
@@ -161,6 +161,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "tree-iterator.h"
 #include "print-tree.h"
 #include "stor-layout.h"
+#include "cgraph.h"
 
 const int max_custom_roles = 32;
 static contract_role contract_build_roles[max_custom_roles] = {
@@ -1451,6 +1452,8 @@ build_contract_condition_function (tree fndecl, bool pre)
     TREE_TYPE (fn) = build_method_type (class_type, TREE_TYPE (fn));
 
   DECL_NAME (fn) = copy_node (DECL_NAME (fn));
+  auto suffix = pre ? "pre" : "post";
+  SET_DECL_ASSEMBLER_NAME (fn, clone_function_name (fn, suffix));
   DECL_INITIAL (fn) = error_mark_node;
   DECL_ABSTRACT_ORIGIN (fn) = fndecl;
 
diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
index e363ef35b9f..e97428e8f30 100644
--- a/gcc/cp/mangle.cc
+++ b/gcc/cp/mangle.cc
@@ -856,13 +856,6 @@ write_encoding (const tree decl)
 				mangle_return_type_p (decl),
 				d);
 
-      /* If this is the pre/post function for a guarded function, append
-	 .pre/post, like something from create_virtual_clone.  */
-      if (DECL_IS_PRE_FN_P (decl))
-	write_string (".pre");
-      else if (DECL_IS_POST_FN_P (decl))
-	write_string (".post");
-
       /* If this is a coroutine helper, then append an appropriate string to
 	 identify which.  */
       if (tree ramp = DECL_RAMP_FN (decl))
diff --git a/gcc/testsuite/g++.dg/contracts/contracts-externC.C b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
new file mode 100644
index 00000000000..873056b742b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
@@ -0,0 +1,19 @@
+// simple check to ensure we don't emit a function with the same name twice,
+// when wrapping functions in pre- and postconditions.
+// { dg-do link }
+// { dg-options "-std=c++2a -fcontracts -fcontract-continuation-mode=on" }
+
+volatile int x = 10;
+
+extern "C" void
+f ()
+  [[ pre: x < 10 ]]
+{
+}
+
+int
+main ()
+  [[ post: x > 10 ]]
+{
+  f();
+}
-- 
2.38.1

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 424 bytes --]


Arsen Arsenović <arsen@aarsen.me> writes:

> Afternoon,
>
> This change prevents contracts from emitting wrapper functions that have the
> same name as the original function, by moving the logic that disambiguates them
> from the mangler into the build_contract_condition_function helper.
>
> Thanks, have nice day.

Oh, forgot to mention; tested on x86_64-pc-linux-gnu via check-g++.

-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Bernhard Reutner-Fischer]

Hi Arsen,

On 10 December 2022 14:14:59 CET, "Arsen Arsenović via Gcc-patches" <gcc-patches@gcc.gnu.org> wrote:
>
>Arsen Arsenović <arsen@aarsen.me> writes:
>
>> Afternoon,
>>
>> This change prevents contracts from emitting wrapper functions that have the
>> same name as the original function, by moving the logic that disambiguates them
>> from the mangler into the build_contract_condition_function helper.
>>
>> Thanks, have nice day.
>
>Oh, forgot to mention; tested on x86_64-pc-linux-gnu via check-g++.
>

From a very distant POV this auto sounds really like an ugly bandaid and I, personally would have used a conditional in place argument. auto is really like c++ didn't have the guts to go straight to implicit typed LHS like python et al nor to learn from Fortran's IMPLICIT defaults from some 60 years ago by now (which might have made sense on punchcards, but IMHO not so much nowadays, at least in the light of multi lingual people).

The real change does sound plausible, but I didn't really look closely. All in all, not even a comment ;-)

Cheers,

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 1638 bytes --]

Hi Bernhard,

Bernhard Reutner-Fischer <rep.dot.nop@gmail.com> writes:

> From a very distant POV this auto sounds really like an ugly bandaid and I,
> personally would have used a conditional in place argument. auto is really like
> c++ didn't have the guts to go straight to implicit typed LHS like python et al
> nor to learn from Fortran's IMPLICIT defaults from some 60 years ago by now
> (which might have made sense on punchcards, but IMHO not so much nowadays, at
> least in the light of multi lingual people).
>
> The real change does sound plausible, but I didn't really look closely. All in all, not even a comment ;-)

This actually preceded significantly more complex code in my original
revision, which got simplified by a good bit by the time I sent it out.
I didn't really feel the need to make the parameter inline since it
would only prolong the line here, I think.  I'm fine with doing that
too, though.

I don't think there's any ban on auto across the codebase either, which
is why I initially used it.

I think C++ is significantly helped by auto, since there's no need to be
specific with types in most places, as long as they can still be deduced
from context, and I think that property holds here.  An alternative
universe with fancier implicit typing does sound nice, though :D

As for the change, AFAICT, the code before and after the change still
produces the same symbols, so I think it should be generally fine across
the board, just that this time around we don't need the mangler when it
isn't used.

Thanks for the review, and have a wonderful day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Jason Merrill]

On 12/10/22 08:13, Arsen Arsenović wrote:
> If the mangler is relied on, functions with extern "C" on them emit multiple
> definitions of the same name.

But doing it here interferes with lazy mangling.  How about appending 
the suffix into write_mangled_name instead of write_encoding?  The 
demangler already expects "clone" suffixes at the end of the mangled name.

> gcc/cp/ChangeLog:
> 
> 	* contracts.cc (build_contract_condition_function): Add pre/post
> 	suffixes to pre- and postcondition clones.
> 	* mangle.cc (write_encoding): Don't mangle pre- and postconditions.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/contracts/contracts-externC.C: New test.
> ---
> Afternoon,
> 
> This change prevents contracts from emitting wrapper functions that have the
> same name as the original function, by moving the logic that disambiguates them
> from the mangler into the build_contract_condition_function helper.
> 
> Thanks, have nice day.
> 
>   gcc/cp/contracts.cc                           |  3 +++
>   gcc/cp/mangle.cc                              |  7 -------
>   .../g++.dg/contracts/contracts-externC.C      | 19 +++++++++++++++++++
>   3 files changed, 22 insertions(+), 7 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/contracts/contracts-externC.C
> 
> diff --git a/gcc/cp/contracts.cc b/gcc/cp/contracts.cc
> index 26316372389..f09eb87e283 100644
> --- a/gcc/cp/contracts.cc
> +++ b/gcc/cp/contracts.cc
> @@ -161,6 +161,7 @@ along with GCC; see the file COPYING3.  If not see
>   #include "tree-iterator.h"
>   #include "print-tree.h"
>   #include "stor-layout.h"
> +#include "cgraph.h"
>   
>   const int max_custom_roles = 32;
>   static contract_role contract_build_roles[max_custom_roles] = {
> @@ -1451,6 +1452,8 @@ build_contract_condition_function (tree fndecl, bool pre)
>       TREE_TYPE (fn) = build_method_type (class_type, TREE_TYPE (fn));
>   
>     DECL_NAME (fn) = copy_node (DECL_NAME (fn));
> +  auto suffix = pre ? "pre" : "post";
> +  SET_DECL_ASSEMBLER_NAME (fn, clone_function_name (fn, suffix));
>     DECL_INITIAL (fn) = error_mark_node;
>     DECL_ABSTRACT_ORIGIN (fn) = fndecl;
>   
> diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
> index e363ef35b9f..e97428e8f30 100644
> --- a/gcc/cp/mangle.cc
> +++ b/gcc/cp/mangle.cc
> @@ -856,13 +856,6 @@ write_encoding (const tree decl)
>   				mangle_return_type_p (decl),
>   				d);
>   
> -      /* If this is the pre/post function for a guarded function, append
> -	 .pre/post, like something from create_virtual_clone.  */
> -      if (DECL_IS_PRE_FN_P (decl))
> -	write_string (".pre");
> -      else if (DECL_IS_POST_FN_P (decl))
> -	write_string (".post");
> -
>         /* If this is a coroutine helper, then append an appropriate string to
>   	 identify which.  */
>         if (tree ramp = DECL_RAMP_FN (decl))
> diff --git a/gcc/testsuite/g++.dg/contracts/contracts-externC.C b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
> new file mode 100644
> index 00000000000..873056b742b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
> @@ -0,0 +1,19 @@
> +// simple check to ensure we don't emit a function with the same name twice,
> +// when wrapping functions in pre- and postconditions.
> +// { dg-do link }
> +// { dg-options "-std=c++2a -fcontracts -fcontract-continuation-mode=on" }
> +
> +volatile int x = 10;
> +
> +extern "C" void
> +f ()
> +  [[ pre: x < 10 ]]
> +{
> +}
> +
> +int
> +main ()
> +  [[ post: x > 10 ]]
> +{
> +  f();
> +}

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Arsen Arsenović]

[-- Attachment #1.1: Type: text/plain, Size: 717 bytes --]

Hi Jason,

Jason Merrill <jason@redhat.com> writes:

> On 12/10/22 08:13, Arsen Arsenović wrote:
>> If the mangler is relied on, functions with extern "C" on them emit multiple
>> definitions of the same name.
>
> But doing it here interferes with lazy mangling.  How about appending the
> suffix into write_mangled_name instead of write_encoding?  The demangler
> already expects "clone" suffixes at the end of the mangled name.

Ah, sorry.  I'm not well versed in the mangler code, so I didn't realize
(frankly, I was initially surprised when I saw that DECL_ASSEMBLER_NAME
was set that early, but went with it).  That makes sense.

How about this?  Tested on x86_64-pc-linux-gnu via check-g++.


[-- Attachment #1.2: c++: Mangle contracts in write_mangled_name unconditionally --]
[-- Type: text/plain, Size: 2644 bytes --]

From 2a2d98e94bdd7a8d7f862b2accda849927e4509e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
Date: Thu, 15 Dec 2022 18:56:59 +0100
Subject: [PATCH v2] c++: Mangle contracts in write_mangled_name
 unconditionally

This fixes contract-checked extern "C" functions.

gcc/cp/ChangeLog:

	* mangle.cc (write_encoding): Move contract pre/post function mangling
	from here...
	(write_mangled_name): ... to here, and make it happen always.

gcc/testsuite/ChangeLog:

	* g++.dg/contracts/contracts-externC.C: New test.
---
 gcc/cp/mangle.cc                              | 14 +++++++-------
 .../g++.dg/contracts/contracts-externC.C      | 19 +++++++++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/contracts/contracts-externC.C

diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
index e363ef35b9f..074cf27ec7a 100644
--- a/gcc/cp/mangle.cc
+++ b/gcc/cp/mangle.cc
@@ -798,6 +798,13 @@ write_mangled_name (const tree decl, bool top_level)
       write_string ("_Z");
       write_encoding (decl);
     }
+
+  /* If this is the pre/post function for a guarded function, append
+     .pre/post, like something from create_virtual_clone.  */
+  if (DECL_IS_PRE_FN_P (decl))
+    write_string (".pre");
+  else if (DECL_IS_POST_FN_P (decl))
+    write_string (".post");
 }
 
 /* Returns true if the return type of DECL is part of its signature, and
@@ -856,13 +863,6 @@ write_encoding (const tree decl)
 				mangle_return_type_p (decl),
 				d);
 
-      /* If this is the pre/post function for a guarded function, append
-	 .pre/post, like something from create_virtual_clone.  */
-      if (DECL_IS_PRE_FN_P (decl))
-	write_string (".pre");
-      else if (DECL_IS_POST_FN_P (decl))
-	write_string (".post");
-
       /* If this is a coroutine helper, then append an appropriate string to
 	 identify which.  */
       if (tree ramp = DECL_RAMP_FN (decl))
diff --git a/gcc/testsuite/g++.dg/contracts/contracts-externC.C b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
new file mode 100644
index 00000000000..873056b742b
--- /dev/null
+++ b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
@@ -0,0 +1,19 @@
+// simple check to ensure we don't emit a function with the same name twice,
+// when wrapping functions in pre- and postconditions.
+// { dg-do link }
+// { dg-options "-std=c++2a -fcontracts -fcontract-continuation-mode=on" }
+
+volatile int x = 10;
+
+extern "C" void
+f ()
+  [[ pre: x < 10 ]]
+{
+}
+
+int
+main ()
+  [[ post: x > 10 ]]
+{
+  f();
+}
-- 
2.39.0


[-- Attachment #1.3: Type: text/plain, Size: 554 bytes --]


I did run c++filt (afaik, it uses the libiberty demangler) on this
revision, and I got:

          .type	f(int) [clone .pre], @function
  f(int) [clone .pre]:

out of ``void f(int x) [[ pre: x > 10 ]] {}'', which seems to match your
description.

If I understand this right, write_xxx corresponds to xxx in the Itanium
ABI mangling BNF, in which case, I believe I have the correct spot here.
In that case, a similar change should happen for coroutines; I think
Iain was working on that.

Thanks, have a great day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Iain Sandoe]

Hi Arsen,

> On 15 Dec 2022, at 18:00, Arsen Arsenović via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> Jason Merrill <jason@redhat.com> writes:
> 
>> On 12/10/22 08:13, Arsen Arsenović wrote:
>>> If the mangler is relied on, functions with extern "C" on them emit multiple
>>> definitions of the same name.
>> 
>> But doing it here interferes with lazy mangling.  How about appending the
>> suffix into write_mangled_name instead of write_encoding?  The demangler
>> already expects "clone" suffixes at the end of the mangled name.
> 
> Ah, sorry.  I'm not well versed in the mangler code, so I didn't realize
> (frankly, I was initially surprised when I saw that DECL_ASSEMBLER_NAME
> was set that early, but went with it).  That makes sense.
> 
> How about this?  Tested on x86_64-pc-linux-gnu via check-g++.
> 
> From 2a2d98e94bdd7a8d7f862b2accda849927e4509e Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Arsen=20Arsenovi=C4=87?= <arsen@aarsen.me>
> Date: Thu, 15 Dec 2022 18:56:59 +0100
> Subject: [PATCH v2] c++: Mangle contracts in write_mangled_name
> unconditionally
> 
> This fixes contract-checked extern "C" functions.
> 
> gcc/cp/ChangeLog:
> 
> 	* mangle.cc (write_encoding): Move contract pre/post function mangling
> 	from here...
> 	(write_mangled_name): ... to here, and make it happen always.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/contracts/contracts-externC.C: New test.
> ---
> gcc/cp/mangle.cc                              | 14 +++++++-------
> .../g++.dg/contracts/contracts-externC.C      | 19 +++++++++++++++++++
> 2 files changed, 26 insertions(+), 7 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/contracts/contracts-externC.C
> 
> diff --git a/gcc/cp/mangle.cc b/gcc/cp/mangle.cc
> index e363ef35b9f..074cf27ec7a 100644
> --- a/gcc/cp/mangle.cc
> +++ b/gcc/cp/mangle.cc
> @@ -798,6 +798,13 @@ write_mangled_name (const tree decl, bool top_level)
>       write_string ("_Z");
>       write_encoding (decl);
>     }
> +
> +  /* If this is the pre/post function for a guarded function, append
> +     .pre/post, like something from create_virtual_clone.  */
> +  if (DECL_IS_PRE_FN_P (decl))
> +    write_string (".pre");
> +  else if (DECL_IS_POST_FN_P (decl))
> +    write_string (".post");
> }

I think you want to use 'write_string (JOIN_STR “pre”);’ etc. since that handles targets
that cannot use a period in symbol names.

> 
> /* Returns true if the return type of DECL is part of its signature, and
> @@ -856,13 +863,6 @@ write_encoding (const tree decl)
> 				mangle_return_type_p (decl),
> 				d);
> 
> -      /* If this is the pre/post function for a guarded function, append
> -	 .pre/post, like something from create_virtual_clone.  */
> -      if (DECL_IS_PRE_FN_P (decl))
> -	write_string (".pre");
> -      else if (DECL_IS_POST_FN_P (decl))
> -	write_string (".post");
> -
>       /* If this is a coroutine helper, then append an appropriate string to
> 	 identify which.  */
>       if (tree ramp = DECL_RAMP_FN (decl))
> diff --git a/gcc/testsuite/g++.dg/contracts/contracts-externC.C b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
> new file mode 100644
> index 00000000000..873056b742b
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/contracts/contracts-externC.C
> @@ -0,0 +1,19 @@
> +// simple check to ensure we don't emit a function with the same name twice,
> +// when wrapping functions in pre- and postconditions.
> +// { dg-do link }
> +// { dg-options "-std=c++2a -fcontracts -fcontract-continuation-mode=on" }
> +
> +volatile int x = 10;
> +
> +extern "C" void
> +f ()
> +  [[ pre: x < 10 ]]
> +{
> +}
> +
> +int
> +main ()
> +  [[ post: x > 10 ]]
> +{
> +  f();
> +}
> -- 
> 2.39.0
> 
> 
> I did run c++filt (afaik, it uses the libiberty demangler) on this
> revision, and I got:
> 
>          .type	f(int) [clone .pre], @function
>  f(int) [clone .pre]:
> 
> out of ``void f(int x) [[ pre: x > 10 ]] {}'', which seems to match your
> description.
> 
> If I understand this right, write_xxx corresponds to xxx in the Itanium
> ABI mangling BNF, in which case, I believe I have the correct spot here.
> In that case, a similar change should happen for coroutines; I think
> Iain was working on that.

If this is the right place, then I can update my patch for coroutines - both
Gor and Lewis replied that ‘extern “C”’ coroutines seemed reasonable to
them.

Iain

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Jason Merrill]

On 12/15/22 13:00, Arsen Arsenović wrote:
> Hi Jason,
> 
> Jason Merrill <jason@redhat.com> writes:
> 
>> On 12/10/22 08:13, Arsen Arsenović wrote:
>>> If the mangler is relied on, functions with extern "C" on them emit multiple
>>> definitions of the same name.
>>
>> But doing it here interferes with lazy mangling.  How about appending the
>> suffix into write_mangled_name instead of write_encoding?  The demangler
>> already expects "clone" suffixes at the end of the mangled name.
> 
> Ah, sorry.  I'm not well versed in the mangler code, so I didn't realize
> (frankly, I was initially surprised when I saw that DECL_ASSEMBLER_NAME
> was set that early, but went with it).  That makes sense.
> 
> How about this?  Tested on x86_64-pc-linux-gnu via check-g++.
> 
> 
> 
> I did run c++filt (afaik, it uses the libiberty demangler) on this
> revision, and I got:
> 
>            .type	f(int) [clone .pre], @function
>    f(int) [clone .pre]:
> 
> out of ``void f(int x) [[ pre: x > 10 ]] {}'', which seems to match your
> description.
> 
> If I understand this right, write_xxx corresponds to xxx in the Itanium
> ABI mangling BNF, in which case, I believe I have the correct spot here.
> In that case, a similar change should happen for coroutines; I think
> Iain was working on that.

> 	* mangle.cc (write_encoding): Move contract pre/post function mangling

It's best to wrap the ChangeLog entry at column 75 so that the extra 
indentation added by git log doesn't push past column 80.

Applied with that change.

Jason

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 334 bytes --]


Jason Merrill <jason@redhat.com> writes:
>
> It's best to wrap the ChangeLog entry at column 75 so that the extra
> indentation added by git log doesn't push past column 80.
>
> Applied with that change.
>
> Jason

Heh, I thought I fixed my fill-column.  Will check.

Thanks!

Have a great day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: [PATCH] contracts: Stop relying on mangling for naming .pre/.post clones

[Arsen Arsenović]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]


Iain Sandoe <idsandoe@googlemail.com> writes:

> I think you want to use 'write_string (JOIN_STR “pre”);’ etc. since that handles targets
> that cannot use a period in symbol names.

Ah - I think you're right.  I'll have to delay checking/fixing for now.
Thanks for the note.

> If this is the right place, then I can update my patch for coroutines - both
> Gor and Lewis replied that ‘extern “C”’ coroutines seemed reasonable to
> them.

Yes, that seems about right to me.

Have a great day.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]