diff mbox series

c++: Improve contracts support in modules [PR108205]

Message ID 679e0dac.170a0220.2b89fa.13ce@mx.google.com
State New
Headers
Series c++: Improve contracts support in modules [PR108205] |

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, 12:03 p.m. UTC 
  Regtested on x86_64-pc-linux-gnu (so far just "dg.exp=contract*
modules.exp=contract*"), OK for trunk if full bootstrap+regtest passes?

-- >8 --

Modules makes some assumptions about types that currently aren't
fulfilled by the types created in contracts logic.  This patch ensures
that exporting inline functions using contracts works again with
modules.

	PR c++/108205

gcc/cp/ChangeLog:

	* contracts.cc (get_pseudo_contract_violation_type): Give names
	to generated FIELD_DECLs.
	(declare_handle_contract_violation): Mark contract_violation
	type as external linkage.
	(build_contract_handler_call): Ensure any builtin declarations
	created here aren't treated as attached to the current module.

gcc/testsuite/ChangeLog:

	* g++.dg/modules/contracts-5_a.C: New test.
	* g++.dg/modules/contracts-5_b.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 gcc/cp/contracts.cc                          | 27 +++++++++++++-------
 gcc/testsuite/g++.dg/modules/contracts-5_a.C |  8 ++++++
 gcc/testsuite/g++.dg/modules/contracts-5_b.C | 20 +++++++++++++++
 3 files changed, 46 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/contracts-5_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/contracts-5_b.C

Comments

Jason Merrill Feb. 3, 2025, 11:57 p.m. UTC | #1 
On 2/1/25 7:03 AM, Nathaniel Shead wrote:
> Regtested on x86_64-pc-linux-gnu (so far just "dg.exp=contract*
> modules.exp=contract*"), OK for trunk if full bootstrap+regtest passes?
> 
> -- >8 --
> 
> Modules makes some assumptions about types that currently aren't
> fulfilled by the types created in contracts logic.  This patch ensures
> that exporting inline functions using contracts works again with
> modules.
> 
> 	PR c++/108205
> 
> gcc/cp/ChangeLog:
> 
> 	* contracts.cc (get_pseudo_contract_violation_type): Give names
> 	to generated FIELD_DECLs.
> 	(declare_handle_contract_violation): Mark contract_violation
> 	type as external linkage.
> 	(build_contract_handler_call): Ensure any builtin declarations
> 	created here aren't treated as attached to the current module.

OK, but now I'm curious why we don't need this sort of thing in rtti.cc?

> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/modules/contracts-5_a.C: New test.
> 	* g++.dg/modules/contracts-5_b.C: New test.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/contracts.cc                          | 27 +++++++++++++-------
>   gcc/testsuite/g++.dg/modules/contracts-5_a.C |  8 ++++++
>   gcc/testsuite/g++.dg/modules/contracts-5_b.C | 20 +++++++++++++++
>   3 files changed, 46 insertions(+), 9 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/modules/contracts-5_a.C
>   create mode 100644 gcc/testsuite/g++.dg/modules/contracts-5_b.C
> 
> diff --git a/gcc/cp/contracts.cc b/gcc/cp/contracts.cc
> index 5782ec8bf29..f2b126c8d6b 100644
> --- a/gcc/cp/contracts.cc
> +++ b/gcc/cp/contracts.cc
> @@ -1633,19 +1633,22 @@ get_pseudo_contract_violation_type ()
>   	   signed char _M_continue;
>   	 If this changes, also update the initializer in
>   	 build_contract_violation.  */
> -      const tree types[] = { const_string_type_node,
> -			     const_string_type_node,
> -			     const_string_type_node,
> -			     const_string_type_node,
> -			     const_string_type_node,
> -			     uint_least32_type_node,
> -			     signed_char_type_node };
> +      struct field_info { tree type; const char* name; };
> +      const field_info info[] = {
> +	{ const_string_type_node, "_M_file" },
> +	{ const_string_type_node, "_M_function" },
> +	{ const_string_type_node, "_M_comment" },
> +	{ const_string_type_node, "_M_level" },
> +	{ const_string_type_node, "_M_role" },
> +	{ uint_least32_type_node, "_M_line" },
> +	{ signed_char_type_node, "_M_continue" }
> +      };
>         tree fields = NULL_TREE;
> -      for (tree type : types)
> +      for (const field_info& i : info)
>   	{
>   	  /* finish_builtin_struct wants fieldss chained in reverse.  */
>   	  tree next = build_decl (BUILTINS_LOCATION, FIELD_DECL,
> -				  NULL_TREE, type);
> +				  get_identifier (i.name), i.type);
>   	  DECL_CHAIN (next) = fields;
>   	  fields = next;
>   	}
> @@ -1737,6 +1740,7 @@ declare_handle_contract_violation ()
>         create_implicit_typedef (viol_name, violation);
>         DECL_SOURCE_LOCATION (TYPE_NAME (violation)) = BUILTINS_LOCATION;
>         DECL_CONTEXT (TYPE_NAME (violation)) = current_namespace;
> +      TREE_PUBLIC (TYPE_NAME (violation)) = true;
>         pushdecl_namespace_level (TYPE_NAME (violation), /*hidden*/true);
>         pop_namespace ();
>         pop_nested_namespace (std_node);
> @@ -1761,6 +1765,11 @@ static void
>   build_contract_handler_call (tree contract,
>   			     contract_continuation cmode)
>   {
> +  /* We may need to declare new types, ensure they are not considered
> +     attached to a named module.  */
> +  auto module_kind_override = make_temp_override
> +    (module_kind, module_kind & ~(MK_PURVIEW | MK_ATTACH | MK_EXPORTING));
> +
>     tree violation = build_contract_violation (contract, cmode);
>     tree violation_fn = declare_handle_contract_violation ();
>     tree call = build_call_n (violation_fn, 1, build_address (violation));
> diff --git a/gcc/testsuite/g++.dg/modules/contracts-5_a.C b/gcc/testsuite/g++.dg/modules/contracts-5_a.C
> new file mode 100644
> index 00000000000..2ff6701ff3f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/contracts-5_a.C
> @@ -0,0 +1,8 @@
> +// PR c++/108205
> +// Test that the implicitly declared handle_contract_violation function is
> +// properly matched with a later declaration in an importing TU.
> +// { dg-additional-options "-fmodules -fcontracts -fcontract-continuation-mode=on" }
> +// { dg-module-cmi test }
> +
> +export module test;
> +export inline void foo(int x) noexcept [[ pre: x != 0 ]] {}
> diff --git a/gcc/testsuite/g++.dg/modules/contracts-5_b.C b/gcc/testsuite/g++.dg/modules/contracts-5_b.C
> new file mode 100644
> index 00000000000..0e794b8ae45
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/modules/contracts-5_b.C
> @@ -0,0 +1,20 @@
> +// PR c++/108205
> +// { dg-module-do run }
> +// { dg-additional-options "-fmodules -fcontracts -fcontract-continuation-mode=on" }
> +
> +#include <experimental/contract>
> +import test;
> +
> +bool saw_violation = false;
> +void handle_contract_violation(const std::experimental::contract_violation& v) {
> +  saw_violation = true;
> +}
> +
> +int main() {
> +  foo(10);
> +  if (saw_violation)
> +    __builtin_abort();
> +  foo(0);
> +  if (!saw_violation)
> +    __builtin_abort();
> +}
Nathaniel Shead Feb. 4, 2025, 12:40 a.m. UTC | #2 
On Mon, Feb 03, 2025 at 06:57:14PM -0500, Jason Merrill wrote:
> On 2/1/25 7:03 AM, Nathaniel Shead wrote:
> > Regtested on x86_64-pc-linux-gnu (so far just "dg.exp=contract*
> > modules.exp=contract*"), OK for trunk if full bootstrap+regtest passes?
> > 
> > -- >8 --
> > 
> > Modules makes some assumptions about types that currently aren't
> > fulfilled by the types created in contracts logic.  This patch ensures
> > that exporting inline functions using contracts works again with
> > modules.
> > 
> > 	PR c++/108205
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* contracts.cc (get_pseudo_contract_violation_type): Give names
> > 	to generated FIELD_DECLs.
> > 	(declare_handle_contract_violation): Mark contract_violation
> > 	type as external linkage.
> > 	(build_contract_handler_call): Ensure any builtin declarations
> > 	created here aren't treated as attached to the current module.
> 
> OK, but now I'm curious why we don't need this sort of thing in rtti.cc?
> 

Modules streaming ignores the types built for RTTI because DECL_TINFO_P
is handled specially in trees_out::decl_node (it just writes enough
information for the importer to rebuild the type itself).  But it might
be worth at least forcing global attachment just in case the types
having module attachment causes something else to go wrong; thoughts?

That said, we will definitely need something like this for the types
built for ubsan (PR98735), which I have some ideas on how to fix but
probably won't get to for GCC15 since there's some other complications
there.

Nathaniel

> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/modules/contracts-5_a.C: New test.
> > 	* g++.dg/modules/contracts-5_b.C: New test.
> > 
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >   gcc/cp/contracts.cc                          | 27 +++++++++++++-------
> >   gcc/testsuite/g++.dg/modules/contracts-5_a.C |  8 ++++++
> >   gcc/testsuite/g++.dg/modules/contracts-5_b.C | 20 +++++++++++++++
> >   3 files changed, 46 insertions(+), 9 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/modules/contracts-5_a.C
> >   create mode 100644 gcc/testsuite/g++.dg/modules/contracts-5_b.C
> > 
> > diff --git a/gcc/cp/contracts.cc b/gcc/cp/contracts.cc
> > index 5782ec8bf29..f2b126c8d6b 100644
> > --- a/gcc/cp/contracts.cc
> > +++ b/gcc/cp/contracts.cc
> > @@ -1633,19 +1633,22 @@ get_pseudo_contract_violation_type ()
> >   	   signed char _M_continue;
> >   	 If this changes, also update the initializer in
> >   	 build_contract_violation.  */
> > -      const tree types[] = { const_string_type_node,
> > -			     const_string_type_node,
> > -			     const_string_type_node,
> > -			     const_string_type_node,
> > -			     const_string_type_node,
> > -			     uint_least32_type_node,
> > -			     signed_char_type_node };
> > +      struct field_info { tree type; const char* name; };
> > +      const field_info info[] = {
> > +	{ const_string_type_node, "_M_file" },
> > +	{ const_string_type_node, "_M_function" },
> > +	{ const_string_type_node, "_M_comment" },
> > +	{ const_string_type_node, "_M_level" },
> > +	{ const_string_type_node, "_M_role" },
> > +	{ uint_least32_type_node, "_M_line" },
> > +	{ signed_char_type_node, "_M_continue" }
> > +      };
> >         tree fields = NULL_TREE;
> > -      for (tree type : types)
> > +      for (const field_info& i : info)
> >   	{
> >   	  /* finish_builtin_struct wants fieldss chained in reverse.  */
> >   	  tree next = build_decl (BUILTINS_LOCATION, FIELD_DECL,
> > -				  NULL_TREE, type);
> > +				  get_identifier (i.name), i.type);
> >   	  DECL_CHAIN (next) = fields;
> >   	  fields = next;
> >   	}
> > @@ -1737,6 +1740,7 @@ declare_handle_contract_violation ()
> >         create_implicit_typedef (viol_name, violation);
> >         DECL_SOURCE_LOCATION (TYPE_NAME (violation)) = BUILTINS_LOCATION;
> >         DECL_CONTEXT (TYPE_NAME (violation)) = current_namespace;
> > +      TREE_PUBLIC (TYPE_NAME (violation)) = true;
> >         pushdecl_namespace_level (TYPE_NAME (violation), /*hidden*/true);
> >         pop_namespace ();
> >         pop_nested_namespace (std_node);
> > @@ -1761,6 +1765,11 @@ static void
> >   build_contract_handler_call (tree contract,
> >   			     contract_continuation cmode)
> >   {
> > +  /* We may need to declare new types, ensure they are not considered
> > +     attached to a named module.  */
> > +  auto module_kind_override = make_temp_override
> > +    (module_kind, module_kind & ~(MK_PURVIEW | MK_ATTACH | MK_EXPORTING));
> > +
> >     tree violation = build_contract_violation (contract, cmode);
> >     tree violation_fn = declare_handle_contract_violation ();
> >     tree call = build_call_n (violation_fn, 1, build_address (violation));
> > diff --git a/gcc/testsuite/g++.dg/modules/contracts-5_a.C b/gcc/testsuite/g++.dg/modules/contracts-5_a.C
> > new file mode 100644
> > index 00000000000..2ff6701ff3f
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/contracts-5_a.C
> > @@ -0,0 +1,8 @@
> > +// PR c++/108205
> > +// Test that the implicitly declared handle_contract_violation function is
> > +// properly matched with a later declaration in an importing TU.
> > +// { dg-additional-options "-fmodules -fcontracts -fcontract-continuation-mode=on" }
> > +// { dg-module-cmi test }
> > +
> > +export module test;
> > +export inline void foo(int x) noexcept [[ pre: x != 0 ]] {}
> > diff --git a/gcc/testsuite/g++.dg/modules/contracts-5_b.C b/gcc/testsuite/g++.dg/modules/contracts-5_b.C
> > new file mode 100644
> > index 00000000000..0e794b8ae45
> > --- /dev/null
> > +++ b/gcc/testsuite/g++.dg/modules/contracts-5_b.C
> > @@ -0,0 +1,20 @@
> > +// PR c++/108205
> > +// { dg-module-do run }
> > +// { dg-additional-options "-fmodules -fcontracts -fcontract-continuation-mode=on" }
> > +
> > +#include <experimental/contract>
> > +import test;
> > +
> > +bool saw_violation = false;
> > +void handle_contract_violation(const std::experimental::contract_violation& v) {
> > +  saw_violation = true;
> > +}
> > +
> > +int main() {
> > +  foo(10);
> > +  if (saw_violation)
> > +    __builtin_abort();
> > +  foo(0);
> > +  if (!saw_violation)
> > +    __builtin_abort();
> > +}
>
diff mbox series

Patch

diff --git a/gcc/cp/contracts.cc b/gcc/cp/contracts.cc
index 5782ec8bf29..f2b126c8d6b 100644
--- a/gcc/cp/contracts.cc
+++ b/gcc/cp/contracts.cc
@@ -1633,19 +1633,22 @@  get_pseudo_contract_violation_type ()
 	   signed char _M_continue;
 	 If this changes, also update the initializer in
 	 build_contract_violation.  */
-      const tree types[] = { const_string_type_node,
-			     const_string_type_node,
-			     const_string_type_node,
-			     const_string_type_node,
-			     const_string_type_node,
-			     uint_least32_type_node,
-			     signed_char_type_node };
+      struct field_info { tree type; const char* name; };
+      const field_info info[] = {
+	{ const_string_type_node, "_M_file" },
+	{ const_string_type_node, "_M_function" },
+	{ const_string_type_node, "_M_comment" },
+	{ const_string_type_node, "_M_level" },
+	{ const_string_type_node, "_M_role" },
+	{ uint_least32_type_node, "_M_line" },
+	{ signed_char_type_node, "_M_continue" }
+      };
       tree fields = NULL_TREE;
-      for (tree type : types)
+      for (const field_info& i : info)
 	{
 	  /* finish_builtin_struct wants fieldss chained in reverse.  */
 	  tree next = build_decl (BUILTINS_LOCATION, FIELD_DECL,
-				  NULL_TREE, type);
+				  get_identifier (i.name), i.type);
 	  DECL_CHAIN (next) = fields;
 	  fields = next;
 	}
@@ -1737,6 +1740,7 @@  declare_handle_contract_violation ()
       create_implicit_typedef (viol_name, violation);
       DECL_SOURCE_LOCATION (TYPE_NAME (violation)) = BUILTINS_LOCATION;
       DECL_CONTEXT (TYPE_NAME (violation)) = current_namespace;
+      TREE_PUBLIC (TYPE_NAME (violation)) = true;
       pushdecl_namespace_level (TYPE_NAME (violation), /*hidden*/true);
       pop_namespace ();
       pop_nested_namespace (std_node);
@@ -1761,6 +1765,11 @@  static void
 build_contract_handler_call (tree contract,
 			     contract_continuation cmode)
 {
+  /* We may need to declare new types, ensure they are not considered
+     attached to a named module.  */
+  auto module_kind_override = make_temp_override
+    (module_kind, module_kind & ~(MK_PURVIEW | MK_ATTACH | MK_EXPORTING));
+
   tree violation = build_contract_violation (contract, cmode);
   tree violation_fn = declare_handle_contract_violation ();
   tree call = build_call_n (violation_fn, 1, build_address (violation));
diff --git a/gcc/testsuite/g++.dg/modules/contracts-5_a.C b/gcc/testsuite/g++.dg/modules/contracts-5_a.C
new file mode 100644
index 00000000000..2ff6701ff3f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/contracts-5_a.C
@@ -0,0 +1,8 @@ 
+// PR c++/108205
+// Test that the implicitly declared handle_contract_violation function is
+// properly matched with a later declaration in an importing TU.
+// { dg-additional-options "-fmodules -fcontracts -fcontract-continuation-mode=on" }
+// { dg-module-cmi test }
+
+export module test;
+export inline void foo(int x) noexcept [[ pre: x != 0 ]] {}
diff --git a/gcc/testsuite/g++.dg/modules/contracts-5_b.C b/gcc/testsuite/g++.dg/modules/contracts-5_b.C
new file mode 100644
index 00000000000..0e794b8ae45
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/contracts-5_b.C
@@ -0,0 +1,20 @@ 
+// PR c++/108205
+// { dg-module-do run }
+// { dg-additional-options "-fmodules -fcontracts -fcontract-continuation-mode=on" }
+
+#include <experimental/contract>
+import test;
+
+bool saw_violation = false;
+void handle_contract_violation(const std::experimental::contract_violation& v) {
+  saw_violation = true;
+}
+
+int main() {
+  foo(10);
+  if (saw_violation)
+    __builtin_abort();
+  foo(0);
+  if (!saw_violation)
+    __builtin_abort();
+}