Re: [PATCH] PING implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Hello again :)  Wanted to shoot a quick status update. Some github issues have
been created for points of feedback, and we've been working on addressing them.
A few changes have been pushed to the contracts-jac-alt branch, while there's
also an active more in depth rewrite branch. Some specific comments inline
below.

On 5/17/21, Jason Merrill  wrote:
> On 5/14/21 4:54 PM, Jason Merrill wrote:
>> On 4/30/21 1:44 PM, Jeff Chapman wrote:
>>> Hello! Looping back around to this. re:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567334.html
>>>
>>> On 3/25/21, Jason Merrill  wrote:
 On 3/1/21 8:12 AM, Jeff Chapman wrote:
> On 1/18/21, Jason Merrill  wrote:
>> On 1/4/21 9:58 AM, Jeff Chapman wrote:
>>> Ping. re:
>>> https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html
>>> 
>>>
>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>> 
>>>
>> Why is some of the code in c-family?  From the modules merge there is
>> now a cp_handle_option function that you could add the option
>> handling
>> to, and I don't see a reason for cxx-contracts.c to be in c-family/
>> rather than cp/.
>
> I've been pushing changes that address the points raised and wanted to
> ping to see if there's more feedback and give a status summary. The
> notable change is making sure the implementation plays nicely with
> modules and a mangling change that did away with a good chunk of code.
>
> The contracts code outside of cp/ has been moved into it, and the
> contract attributes have been altered to work with language
> independent
> handling code. More of the contracts code can probably still be
> moved to
> cxx-contracts which I'll loop back to after other refactoring. The
> naming, spelling, and comments (or lack thereof) have been addressed.

 Sounds good.  I plan to get back to this when GCC 11 branches, which
 should be mid-April.
>>>
>>> Please let me know if you see any more issues when you pick it back up.
>>> Particularly in modules interop, since I don't think you've had a chance
>>> to look at that yet.
>>>
>>> Completed another merge with master earlier this week which didn't bring
>>> to light any new issues or regressions, but I'll keep on that :)
>>>
>>> +  /* If we have contracts, check that they're valid in this
>>> context. */
>>> +  // FIXME: These aren't entirely correct.
>>
>> How not?  Can local extern function decls have contract attributes?
>>
>>> + /* FIXME when we instatiate a template class with
>>> guarded
>>> +  * members, particularly guarded template members,
>>> the resulting
>>> +  * pre/post functions end up being inaccessible
>>> because their
>>> +  * template info's context is the original
>>> uninstantiated class.
>>
>> This sounds like a significant functionality gap.  I'm guessing you
>> want
>> to reconsider the unshare_template approach.
>>
>> One approach would be to only do the pre/post/guarded/unguarded
>> transformation for a fully-instantiated function; a temploid function
>> would leave the contracts as attributes.
>>

There's an in progress rewrite which moves pre/post function generation until
much later which should resolve this. Like you suggested, these are not created
until we're completely out of template processing.

>>> +  /* FIXME do we need magic to perfectly forward this so we
>>> don't clobber
>>> +RVO/NRVO etc?  */
>>
>> Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you
>> want.
>
> These points are still being investigated and addressed; including
> them
> for reference.
>>
>> Any update?
>>

CALL_FROM_THUNK_P is being set now when an actual call is built. Is there a good
way to test that the optimization isn't being broken in general?

>> More soon.
>
> Please let me know what other issues need work.
>>>
>>> If there's anything I can do to make the process smoother please don't
>>> hesitate to ask.
>>
>> Larger-scope comments:
>>
>> Please add an overview of the implementation strategy to the top of
>> cxx-contracts.c.  Particularly to discuss the why and how of
>> pre/post/guarded/unguarded functions.

An initial overview has been added, though it'll need updated with some of the
pending rewrites.

>
>> I'm confused by the approach to late parsing of contracts; it seems like
>> you wait until the end of parsing the function to go back and parse the
>> contracts.  Why not parse them sooner, along with nsdmis, noexcept, and
>> function bodies?

Parsing has been moved forward on the rewrite branch.

>>
>> Smaller-scope comments:
>>
 +   /* If we didn't build a check, insert a NOP so we don't leak
 + 

Re: [PATCH] PING implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Hello! Looping back around to this. re:
https://gcc.gnu.org/pipermail/gcc-patches/2021-March/567334.html

On 3/25/21, Jason Merrill  wrote:
> On 3/1/21 8:12 AM, Jeff Chapman wrote:
>> On 1/18/21, Jason Merrill  wrote:
>>> On 1/4/21 9:58 AM, Jeff Chapman wrote:
 Ping. re:
 https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html
 

 https://github.com/lock3/gcc/tree/contracts-jac-alt
 

>>> Why is some of the code in c-family?  From the modules merge there is
>>> now a cp_handle_option function that you could add the option handling
>>> to, and I don't see a reason for cxx-contracts.c to be in c-family/
>>> rather than cp/.
>>
>> I've been pushing changes that address the points raised and wanted to
>> ping to see if there's more feedback and give a status summary. The
>> notable change is making sure the implementation plays nicely with
>> modules and a mangling change that did away with a good chunk of code.
>>
>> The contracts code outside of cp/ has been moved into it, and the
>> contract attributes have been altered to work with language independent
>> handling code. More of the contracts code can probably still be moved to
>> cxx-contracts which I'll loop back to after other refactoring. The
>> naming, spelling, and comments (or lack thereof) have been addressed.
>
> Sounds good.  I plan to get back to this when GCC 11 branches, which
> should be mid-April.

Please let me know if you see any more issues when you pick it back up.
Particularly in modules interop, since I don't think you've had a chance
to look at that yet.

Completed another merge with master earlier this week which didn't bring
to light any new issues or regressions, but I'll keep on that :)

 +  /* If we have contracts, check that they're valid in this context. */
 +  // FIXME: These aren't entirely correct.
>>>
>>> How not?  Can local extern function decls have contract attributes?
>>>
 + /* FIXME when we instatiate a template class with guarded
 +  * members, particularly guarded template members, the 
 resulting
 +  * pre/post functions end up being inaccessible because their
 +  * template info's context is the original uninstantiated 
 class.
>>>
>>> This sounds like a significant functionality gap.  I'm guessing you want
>>> to reconsider the unshare_template approach.
>>>
 +  /* FIXME do we need magic to perfectly forward this so we don't 
 clobber
 +RVO/NRVO etc?  */
>>>
>>> Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you
>>> want.
>>
>> These points are still being investigated and addressed; including them
>> for reference.
>>
>>> More soon.
>>
>> Please let me know what other issues need work.

If there's anything I can do to make the process smoother please don't
hesitate to ask.

Thank you,
Jeff Chapman

Re: [PATCH] PING implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

On 1/18/21, Jason Merrill  wrote:
> On 1/4/21 9:58 AM, Jeff Chapman wrote:
>> Ping. re:
>> https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html
>> 
>>
>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>> 
>>
> Why is some of the code in c-family?  From the modules merge there is
> now a cp_handle_option function that you could add the option handling
> to, and I don't see a reason for cxx-contracts.c to be in c-family/
> rather than cp/.

I've been pushing changes that address the points raised and wanted to
ping to see if there's more feedback and give a status summary. The
notable change is making sure the implementation plays nicely with
modules and a mangling change that did away with a good chunk of code.

The contracts code outside of cp/ has been moved into it, and the
contract attributes have been altered to work with language independent
handling code. More of the contracts code can probably still be moved to
cxx-contracts which I'll loop back to after other refactoring. The
naming, spelling, and comments (or lack thereof) have been addressed.

>> +set_decl_contracts (tree decl, tree contract_attrs)
> I think you want to use 'chainon' here.

>> +build_arg_list (tree fn)
> You can use is_this_parameter here.

Thanks; I knew of chainon but didn't think of it and is_this_parameter
is new to me. Always fun to delete code :)

>> +  /* If we have contracts, check that they're valid in this context.  */
>> +  // FIXME: These aren't entirely correct.
>
> How not?  Can local extern function decls have contract attributes?
>
>> + /* FIXME when we instatiate a template class with guarded
>> +  * members, particularly guarded template members, the 
>> resulting
>> +  * pre/post functions end up being inaccessible because their
>> +  * template info's context is the original uninstantiated 
>> class.
>
> This sounds like a significant functionality gap.  I'm guessing you want
> to reconsider the unshare_template approach.
>
>> +  /* FIXME do we need magic to perfectly forward this so we don't 
>> clobber
>> +RVO/NRVO etc?  */
>
> Yes.  CALL_FROM_THUNK_P will probably get you some of the magic you want.

These points are still being investigated and addressed; including them
for reference.

> More soon.

Please let me know what other issues need work.


Thank you!
Jeff Chapman

[PATCH] PING implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Ping. re:
https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html

> OK, I'll start with -alt then, thanks.
>
> Andrew is exactly correct, contracts-jac-alt is still the current branch
> we're focusing our upstreaming efforts on.
>
> It's trailing upstream master by a fair bit at this point. I'll get a
> merge pushed shortly.
>

The latest is still on the same branch, which hasn't been updated since
that last merge:
https://github.com/lock3/gcc/tree/contracts-jac-alt

Would you prefer me to keep it from trailing upstream too much through
regular merges, or would it be more beneficial for it to be left alone so
you have a more stable review target?

Please let me know if there's initial feedback I can start addressing, or
anything I can do to help the review process along in general.

Thank you,
Jeff Chapman

Re: [PATCH] implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

> OK, I'll start with -alt then, thanks.

Andrew is exactly correct, contracts-jac-alt is still the current branch
we're focusing our upstreaming efforts on.

It's trailing upstream master by a fair bit at this point. I'll get a merge
pushed shortly.

Please let me know if there's anything I can do to help review along!


On Thu, Dec 3, 2020 at 12:41 PM Jason Merrill  wrote:

> On 12/3/20 12:07 PM, Andrew Sutton wrote:
> >
> >  > Attached is a new squashed revision of the patch sans ChangeLogs.
> The
> >  > current work is now being done on github:
> >  > https://github.com/lock3/gcc/tree/contracts-jac-alt
> > 
> >
> > I'm starting to review this now, sorry for the delay. Is this still
> the
> > branch you want me to consider for GCC 11?  I notice that the
> > -constexpr
> > and -mangled-config branches are newer.
> >
> >
> > I think so. Jeff can answer more authoritatively. I know we had one set
> > of changes to the design (how contracts) work aimed at improving the
> > debugging experience for violated contracts. I'm not sure if that's in
> > the jac-alt branch though.
> >
> > The -constexpr branch checks for trivially satisfied contracts (e.g.,
> > [[assert: true]]) and issues warnings. It also preemptively checks
> > preconditions against constant function arguments. It's probably worth
> > reviewing that separately.
> >
> > I'm not sure the -manged-config branch is worth considering for merging
> > at this point. It's trying to solve a problem that might not be worth
> > solving.
>
> OK, I'll start with -alt then, thanks.
>
> > Out of curiosity, are you concerned that future versions of contracts
> > might have considerably different syntax or configurability? I'd hope it
> > wouldn't, but who knows where SG21 is going :)
>
> Not particularly; I figure that most of the implementation would be
> unaffected.
>
> Jason
>
>

[PATCH 2/2] modules: c++: Fix cross module member redecl/add long distance friendship warning

[Jeff Chapman via Gcc-patches]

Attached is a patch adding a -Wlong-distance-friends flag to diagnose long
distance (cross module) friendship.


2020-08-14  Jeff Chapman II  

gcc/c-family/
* c.opt (Wlong-distance-friends): New.

gcc/cp/
* cp-tree.h (module_friendship_compatible): New.
* friend.c (add_friend): Warn when befriending a function owned
by a different module.
(make_friend_class): Warn when befriending a class owned by a
different module.
* module.cc (module_friendship_compatible): New function.
Returns true if a decl may be befriended by another without
issuing a long distance friend warning.

gcc/testsuite/
* g++.dg/modules/long-distance-friend-1_[ab].C: New test.
* g++.dg/modules/long-distance-friend-2_a.[Ch]: Ditto.
* g++.dg/modules/long-distance-friend-3_[ab].[Ch]: Ditto.
* g++.dg/modules/redecl-3_b.C: Add case for -Wlong-distance-friends.


Thanks,
Jeff Chapman II
From 3d229111dcbe4bc3438207a500452c4420fba527 Mon Sep 17 00:00:00 2001
From: Jeff Chapman II 
Date: Fri, 20 Dec 2019 11:02:20 -0500
Subject: [PATCH 2/2] c++: Add warning flag for cross module friendship

2020-08-14  Jeff Chapman II  

gcc/c-family/
	* c.opt (Wlong-distance-friends): New.

gcc/cp/
	* cp-tree.h (module_friendship_compatible): New.
	* friend.c (add_friend): Warn when befriending a function owned
	by a different module.
	(make_friend_class): Warn when befriending a class owned by a
	different module.
	* module.cc (module_friendship_compatible): New function.
	Returns true if a decl may be befriended by another without
	issuing a long distance friend warning.

gcc/testsuite/
	* g++.dg/modules/long-distance-friend-1_[ab].C: New test.
	* g++.dg/modules/long-distance-friend-2_a.[Ch]: Ditto.
	* g++.dg/modules/long-distance-friend-3_[ab].[Ch]: Ditto.
	* g++.dg/modules/redecl-3_b.C: Add case for -Wlong-distance-friends.
---
 gcc/c-family/c.opt|  4 ++
 gcc/cp/cp-tree.h  |  1 +
 gcc/cp/friend.c   | 24 
 gcc/cp/module.cc  | 38 +++
 .../g++.dg/modules/long-distance-friend-1_a.C | 17 +
 .../g++.dg/modules/long-distance-friend-1_b.C | 24 
 .../g++.dg/modules/long-distance-friend-2_a.C | 18 +
 .../g++.dg/modules/long-distance-friend-2_a.h |  7 
 .../g++.dg/modules/long-distance-friend-3_a.C |  9 +
 .../g++.dg/modules/long-distance-friend-3_b.C |  9 +
 .../g++.dg/modules/long-distance-friend-3_b.h | 15 
 gcc/testsuite/g++.dg/modules/redecl-3_b.C |  4 +-
 12 files changed, 168 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/modules/long-distance-friend-1_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/long-distance-friend-1_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/long-distance-friend-2_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/long-distance-friend-2_a.h
 create mode 100644 gcc/testsuite/g++.dg/modules/long-distance-friend-3_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/long-distance-friend-3_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/long-distance-friend-3_b.h

diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
index 506d091e5f2..2d39ac5f974 100644
--- a/gcc/c-family/c.opt
+++ b/gcc/c-family/c.opt
@@ -739,6 +739,10 @@ Wlogical-not-parentheses
 C ObjC C++ ObjC++ Var(warn_logical_not_paren) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall)
 Warn when logical not is used on the left hand side operand of a comparison.
 
+Wlong-distance-friends
+C++ Var(warn_long_distance_friends) Warning LangEnabledBy(C++,Wall)
+Warn when friends are declared across module boundaries.
+
 Wlong-long
 C ObjC C++ ObjC++ CPP(cpp_warn_long_long) CppReason(CPP_W_LONG_LONG) Var(warn_long_long) Init(-1) Warning LangEnabledBy(C ObjC,Wc90-c99-compat)
 Do not warn about using \"long long\" when -pedantic.
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
index d633b4d5c70..9299625f33b 100644
--- a/gcc/cp/cp-tree.h
+++ b/gcc/cp/cp-tree.h
@@ -7071,6 +7071,7 @@ inline bool module_exporting_p ()
 extern module_state *get_module (tree name, module_state *parent = NULL,
  bool partition = false);
 extern bool module_may_redeclare (tree decl);
+extern bool module_friendship_compatible (tree decl1, tree decl2);
 
 extern int module_initializer_kind ();
 extern void module_add_import_initializers ();
diff --git a/gcc/cp/friend.c b/gcc/cp/friend.c
index 9b27d7d340a..1e6e746a3a5 100644
--- a/gcc/cp/friend.c
+++ b/gcc/cp/friend.c
@@ -173,6 +173,17 @@ add_friend (tree type, tree decl, bool complain)
   if (decl == error_mark_node)
 return;
 
+  if (modules_p ()
+  && complain
+  && warn_long_distance_friends
+  && !module_friendship_compatible (TYPE_NAME (type), decl))
+{
+  warning (OPT_Wlong_distance_friends,
+	   "%q#T is not visible to befriended declaration %q#D",
+	   type, decl);
+  inform 

[PATCH 1/2] modules: c++: Fix cross module member redecl/add long distance friendship warning

[Jeff Chapman via Gcc-patches]

Hello again,

This is part one of a patchset to add an optional warning for long distance
(cross module) friendship when the friendship has no useful/sensical meaning.

Attached is a patch to error when trying to define a member function of a type
owned by a different module. This also fixes an issue where a friend decl lets
a user define a function owned by a different module.


2020-08-14  Jeff Chapman II  

gcc/cp/
* decl.c (duplicate_decls): Return original decl when
attempting to redeclare a function owned by another module
instead of clobbering the original decl.
(grokfndecl): Error on member declarations of types owned by
another module.

gcc/testsuite/
* g++.dg/modules/redecl-1_[ab].C: New test.
* g++.dg/modules/redecl-2_[ab].C: Ditto.
* g++.dg/modules/redecl-3_[ab].C: Ditto.


Please let me know if there are any questions,
Jeff Chapman II
From fc95c562ec87520f66388a35009649c4b020a843 Mon Sep 17 00:00:00 2001
From: Jeff Chapman II 
Date: Thu, 19 Dec 2019 09:43:16 -0500
Subject: [PATCH 1/2] c++: Fix cross module member redecl

2020-08-14  Jeff Chapman II  

gcc/cp/
	* decl.c (duplicate_decls): Return original decl when
	attempting to redeclare a function owned by another module
	instead of clobbering the original decl.
	(grokfndecl): Error on member declarations of types owned by
	another module.

gcc/testsuite/
	* g++.dg/modules/redecl-1_[ab].C: New test.
	* g++.dg/modules/redecl-2_[ab].C: Ditto.
	* g++.dg/modules/redecl-3_[ab].C: Ditto.
---
 gcc/cp/decl.c | 14 ++
 gcc/testsuite/g++.dg/modules/redecl-1_a.C |  9 +
 gcc/testsuite/g++.dg/modules/redecl-1_b.C |  8 
 gcc/testsuite/g++.dg/modules/redecl-2_a.C | 10 ++
 gcc/testsuite/g++.dg/modules/redecl-2_b.C |  7 +++
 gcc/testsuite/g++.dg/modules/redecl-3_a.C |  6 ++
 gcc/testsuite/g++.dg/modules/redecl-3_b.C | 13 +
 7 files changed, 67 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/redecl-1_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/redecl-1_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/redecl-2_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/redecl-2_b.C
 create mode 100644 gcc/testsuite/g++.dg/modules/redecl-3_a.C
 create mode 100644 gcc/testsuite/g++.dg/modules/redecl-3_b.C

diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index decd58791ae..e8637dac7eb 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -2031,6 +2031,12 @@ duplicate_decls (tree newdecl, tree olddecl, bool newdecl_is_friend)
 	}
 	}
 }
+  /* FIXME Is there a better place to handle this? Without this, we end up
+   * potentially merging a decl owned by a separate module with a friend decl
+   * injected in the current module (see modules/redecl-3). Returning olddecl
+   * relies on code higher up handling the issue.  */
+  else if (modules_p () && !module_may_redeclare (olddecl))
+return olddecl;
 
   /* We have committed to returning OLDDECL at this point.  */
 
@@ -9517,6 +9523,14 @@ grokfndecl (tree ctype,
 
   if (TREE_CODE (type) == METHOD_TYPE)
 {
+  if (modules_p()
+	  && !module_may_redeclare (TYPE_NAME (ctype)))
+	{
+	  error_at (location, "declaration conflicts with import");
+	  inform (location_of (ctype), "import declared %q#T here", ctype);
+	  return NULL_TREE;
+	}
+
   tree parm = build_this_parm (decl, type, quals);
   DECL_CHAIN (parm) = parms;
   parms = parm;
diff --git a/gcc/testsuite/g++.dg/modules/redecl-1_a.C b/gcc/testsuite/g++.dg/modules/redecl-1_a.C
new file mode 100644
index 000..2646adf8327
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/redecl-1_a.C
@@ -0,0 +1,9 @@
+// { dg-additional-options -fmodules-ts }
+export module foo;
+// { dg-module-cmi foo }
+
+export struct Foo
+{
+  int foo();
+};
+
diff --git a/gcc/testsuite/g++.dg/modules/redecl-1_b.C b/gcc/testsuite/g++.dg/modules/redecl-1_b.C
new file mode 100644
index 000..b980bfe290c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/redecl-1_b.C
@@ -0,0 +1,8 @@
+// { dg-additional-options -fmodules-ts }
+import foo;
+
+int Foo::foo() // { dg-error "conflicts with import" }
+{
+  return 1;
+}
+
diff --git a/gcc/testsuite/g++.dg/modules/redecl-2_a.C b/gcc/testsuite/g++.dg/modules/redecl-2_a.C
new file mode 100644
index 000..eea99f2024e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/redecl-2_a.C
@@ -0,0 +1,10 @@
+// { dg-additional-options -fmodules-ts }
+export module foo;
+// { dg-module-cmi foo }
+
+export struct Foo
+{
+  int foo();
+  friend class Bar;
+};
+
diff --git a/gcc/testsuite/g++.dg/modules/redecl-2_b.C b/gcc/testsuite/g++.dg/modules/redecl-2_b.C
new file mode 100644
index 000..9c3b545f6d2
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/redecl-2_b.C
@@ -0,0 +1,7 @@
+// { dg-additional-options -fmodules-ts }
+import foo;
+
+struct Bar // { dg-error "cannot declare.*in a different module" }
+{
+};
+
diff --git 

[PATCH] modules: c++: Fix push_namespace ICE with modules

[Jeff Chapman via Gcc-patches]

Hello!

Attached is a patch that fixes an ICE on the devel/c++-modules branch caused
by a slot invalidation edge case in push_namespace.

It's been sitting around for a while and I wasn't sure if I should use the
original date or not. Feel free to adjust that to the commit date if that's
what it should be instead.


2020-05-15  Jeff Chapman II  

gcc/cp/
* name-lookup.c (push_namespace): Fix slot invalidation related
ICE when compiling with modules enabled.

gcc/testsuite/

* g++.dg/modules/string-view1.C: New test.
* g++.dg/modules/string-view2.C: Ditto.


Please let me know if there's anything more I can do,
Jeff Chapman II
From d33a239c187cb6cef1c39953c5f014bd266492de Mon Sep 17 00:00:00 2001
From: Jeff Chapman II 
Date: Fri, 15 May 2020 06:37:41 -0400
Subject: [PATCH 1/1] c++: Fix push_namespace ICE with modules

push_namespace was reusing an earlier find_namespace_slot result after
it was invalidated by pushdecl in corner cases.

2020-05-15  Jeff Chapman II  

gcc/cp/
	* name-lookup.c (push_namespace): Fix slot invalidation related
	ICE when compiling with modules enabled.

gcc/testsuite/

	* g++.dg/modules/string-view1.C: New test.
	* g++.dg/modules/string-view2.C: Ditto.
---
 gcc/cp/name-lookup.c|  2 +
 gcc/testsuite/g++.dg/modules/string-view1.C |  6 +++
 gcc/testsuite/g++.dg/modules/string-view2.C | 53 +
 3 files changed, 61 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/modules/string-view1.C
 create mode 100644 gcc/testsuite/g++.dg/modules/string-view2.C

diff --git a/gcc/cp/name-lookup.c b/gcc/cp/name-lookup.c
index e9495d2a282..462f028617c 100644
--- a/gcc/cp/name-lookup.c
+++ b/gcc/cp/name-lookup.c
@@ -8920,6 +8920,8 @@ push_namespace (tree name, bool make_inline)
 	{
 	  /* finish up making the namespace.  */
 	  add_decl_to_level (NAMESPACE_LEVEL (current_namespace), ns);
+	  /* pushdecl may invalidate slot, find name again.  */
+	  slot = find_namespace_slot (current_namespace, name, true);
 	  make_namespace_finish (ns, slot);
 
 	  /* Add the anon using-directive here, we don't do it in
diff --git a/gcc/testsuite/g++.dg/modules/string-view1.C b/gcc/testsuite/g++.dg/modules/string-view1.C
new file mode 100644
index 000..dabc16a8b01
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/string-view1.C
@@ -0,0 +1,6 @@
+// { dg-additional-options "-fmodules-ts" }
+module;
+#include 
+#include 
+export module foo;
+
diff --git a/gcc/testsuite/g++.dg/modules/string-view2.C b/gcc/testsuite/g++.dg/modules/string-view2.C
new file mode 100644
index 000..2e389eacd8f
--- /dev/null
+++ b/gcc/testsuite/g++.dg/modules/string-view2.C
@@ -0,0 +1,53 @@
+// { dg-additional-options "-fmodules-ts" }
+// reduced from string-view1 through cvise. Broken under c++2a too.
+namespace std {
+typedef int a;
+int b;
+decltype(nullptr) c;
+namespace xyz {}
+__builtin_va_list d;
+int n;
+int e;
+int f;
+int g;
+int h;
+int i;
+int j;
+int k;
+typedef struct l m;
+typedef struct aa w;
+typedef struct x o;
+typedef x p;
+long q;
+long r;
+typedef l s;
+extern p ab;
+void t();
+void v();
+extern p ac;
+void ad();
+int ae;
+int af;
+extern p ag;
+extern p ah;
+void ai();
+void y();
+int aj;
+int ak;
+int al;
+char am;
+int an;
+a ao;
+int ap;
+int aq;
+void z();
+int ar;
+int as;
+void at();
+void au();
+void av();
+void aw();
+int u;
+namespace zz {
+}
+}
-- 
2.27.0

Re: [PATCH] implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Ping. Any feedback would be appreciated :)

re: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549868.html
older reply: https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545339.html

On 7/10/20, Jeff Chapman  wrote:
> Hello again :)
>
> Attached is a new squashed revision of the patch sans ChangeLogs. The
> current work is now being done on github:
> https://github.com/lock3/gcc/tree/contracts-jac-alt
>
> Please let me know if there's a better way to share revisions.
>
 +  /* Check that assertions are null statements.  */
 +  if (attribute_contract_assert_p (contract_attrs))
 +if (token->type != CPP_SEMICOLON)
 +  error_at (token->location, "assertions must be followed by
 %<;%>");
>>>
>>> Better I think to handle this further down where [[fallthrough]] has the
>>> same requirement.
>>>
>>
>> I'm wondering if it would be better to move [[fallthrough]] up, since
>> the later check is not always executed and in the case of [[assert]] we
>> actually need to error.
>>
>>   [[fallthrough]] int x;
>>
>> for instance just gets a generic 'attribute ignored' warning. I'm not
>> entirely happy with how we prevent assert/pre/post from appearing in
>> invalid locations in general which I'll try to improve. If you have
>> concrete suggestions please let me know.
>
> Improvements have been made to handling contract attributes appearing in
> places they don't belong. Any feedback about moving the logic dealing
> with [[fallthrough]]?
>
>
>>> Why not leave the function the user declared as the unchecked function
>>> (just changing some linkage properties) and create a new function for
>>> the checked wrapper?
>>>
>>
>> This revision of the patch does not include changes to the
>> checked/unchecked function split. We're exploring an alternative rewrite
>> that leaves the original function declaration alone and should address
>> or sidestep a number of these comments.
>>
>> Specifically, we're exploring generating pre and post functions with
>> calls to them in the correct places (upon entering a guarded function,
>> wrapping the return value):
>>
>>   int f(int n) [[ pre: n > 0 ]] [[ post r: r < 0 ]] { return -n; }
>>
>> turns into
>>
>>   void __pre_f(int n) { [[ assert: n > 0 ]]; }
>>   int __post_f(int r) { [[ assert: r < 0 ]]; return r; }
>>   int f(int n) {
>> __pre_f(n);
>> return __post_f(-n);
>>   }
>>
>> with whatever hints we can give to optimize this as much as possible.
>
> This is the main change since the last submission. We now leave the
> original decl intact and instead generate a pre and post function.
> Naming and mangling of these pre/post functions needs addressed.
>
> Beyond that, more cleanup has been done. There's still outstanding
> cleanup work, mostly around investigating and improving performance.
> Feedback on the main direction of the patch would be appreciated, and
> any specific commentary or directions of investigation would be helpful.
>
>
 +/* Return the source text between two locations.  */
 +
 +static char *
 +get_source (location_t start, location_t end)
>>>
>>> This seems like it belongs in libcpp.  It also needs to
>>>
>>
>> This has been moved to input since it uses input functions, but needs
>> more work. Was there another comment you had that cutoff?
>>
>> ...snip...
>>
 +  /* We never want to accidentally instantiate templates.  */
 +  if (code == TEMPLATE_DECL)
 +return *tp; /* FIXME? */
>>>
>>> This seems unlikely to have the desired effect; we should see template
>>> instantiations as FUNCTION_DECL or VAR_DECL.  I'm also not sure what the
>>> cp_tree_defined_p check is trying to do; surely using defined functions
>>> and variables can also lead to runtime code?
>>>
>>
>> This is an result of the transform we do for axioms when they're enabled
>> that you may know of a better way to do. Essentially we turn this:
>>
>>   [[ assert axiom: f() > 0 ]];
>>
>> into this:
>>
>>   if (!(f() > 0))
>> __builtin_unreachable ();
>>
>> but we potentially run into issues later if f isn't (yet) defined or is
>> defined in a different translation unit, such as constexpr time. We're
>> avoiding those errors by generating a nop if there's any chance can't be
>> evaluated. We'd prefer something like
>>
>>   __builtin_assume (f() > 0);
>>
>> where the condition is simply ignored if enough information isn't
>> present to affect codegen. Is there a better way to handle this?
>>
>> I should mention that there are likely significant further changes
>> around axiom coming down the pike that may tie into or replace the
>> "is defined" check. Specifically for expressions that _cannot_ be
>> defined rather than ones that are simply not yet defined.
>>
>
> Not much has changed yet regarding axiom, but if you have any
> suggestions for handling any of the above then I'm all ears.
>

Re: [PATCH] implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Hello again :)

Attached is a new squashed revision of the patch sans ChangeLogs. The
current work is now being done on github:
https://github.com/lock3/gcc/tree/contracts-jac-alt

Please let me know if there's a better way to share revisions.

>>> +  /* Check that assertions are null statements.  */
>>> +  if (attribute_contract_assert_p (contract_attrs))
>>> +if (token->type != CPP_SEMICOLON)
>>> +  error_at (token->location, "assertions must be followed by
>>> %<;%>");
>>
>> Better I think to handle this further down where [[fallthrough]] has the
>> same requirement.
>>
>
> I'm wondering if it would be better to move [[fallthrough]] up, since
> the later check is not always executed and in the case of [[assert]] we
> actually need to error.
>
>   [[fallthrough]] int x;
>
> for instance just gets a generic 'attribute ignored' warning. I'm not
> entirely happy with how we prevent assert/pre/post from appearing in
> invalid locations in general which I'll try to improve. If you have
> concrete suggestions please let me know.

Improvements have been made to handling contract attributes appearing in
places they don't belong. Any feedback about moving the logic dealing
with [[fallthrough]]?


>> Why not leave the function the user declared as the unchecked function
>> (just changing some linkage properties) and create a new function for
>> the checked wrapper?
>>
>
> This revision of the patch does not include changes to the
> checked/unchecked function split. We're exploring an alternative rewrite
> that leaves the original function declaration alone and should address
> or sidestep a number of these comments.
>
> Specifically, we're exploring generating pre and post functions with
> calls to them in the correct places (upon entering a guarded function,
> wrapping the return value):
>
>   int f(int n) [[ pre: n > 0 ]] [[ post r: r < 0 ]] { return -n; }
>
> turns into
>
>   void __pre_f(int n) { [[ assert: n > 0 ]]; }
>   int __post_f(int r) { [[ assert: r < 0 ]]; return r; }
>   int f(int n) {
> __pre_f(n);
> return __post_f(-n);
>   }
>
> with whatever hints we can give to optimize this as much as possible.

This is the main change since the last submission. We now leave the
original decl intact and instead generate a pre and post function.
Naming and mangling of these pre/post functions needs addressed.

Beyond that, more cleanup has been done. There's still outstanding
cleanup work, mostly around investigating and improving performance.
Feedback on the main direction of the patch would be appreciated, and
any specific commentary or directions of investigation would be helpful.


>>> +/* Return the source text between two locations.  */
>>> +
>>> +static char *
>>> +get_source (location_t start, location_t end)
>>
>> This seems like it belongs in libcpp.  It also needs to
>>
>
> This has been moved to input since it uses input functions, but needs
> more work. Was there another comment you had that cutoff?
>
> ...snip...
>
>>> +  /* We never want to accidentally instantiate templates.  */
>>> +  if (code == TEMPLATE_DECL)
>>> +return *tp; /* FIXME? */
>>
>> This seems unlikely to have the desired effect; we should see template
>> instantiations as FUNCTION_DECL or VAR_DECL.  I'm also not sure what the
>> cp_tree_defined_p check is trying to do; surely using defined functions
>> and variables can also lead to runtime code?
>>
>
> This is an result of the transform we do for axioms when they're enabled
> that you may know of a better way to do. Essentially we turn this:
>
>   [[ assert axiom: f() > 0 ]];
>
> into this:
>
>   if (!(f() > 0))
> __builtin_unreachable ();
>
> but we potentially run into issues later if f isn't (yet) defined or is
> defined in a different translation unit, such as constexpr time. We're
> avoiding those errors by generating a nop if there's any chance can't be
> evaluated. We'd prefer something like
>
>   __builtin_assume (f() > 0);
>
> where the condition is simply ignored if enough information isn't
> present to affect codegen. Is there a better way to handle this?
>
> I should mention that there are likely significant further changes
> around axiom coming down the pike that may tie into or replace the
> "is defined" check. Specifically for expressions that _cannot_ be
> defined rather than ones that are simply not yet defined.
>

Not much has changed yet regarding axiom, but if you have any
suggestions for handling any of the above then I'm all ears.


0001-Implement-pre-c-20-contracts.patch.gz
Description: GNU Zip compressed data

Re: [PATCH] implement pre-c++20 contracts

[Jeff Chapman via Gcc-patches]

Hello,

On 12/10/19, Jason Merrill wrote:
> On 11/13/19, Jeff Chapman wrote:
>> Attached is a patch that implements pre-c++20 contracts. This comes
>> from a long running development branch which included ChangeLog entries
>> as we went, which are included in the patch itself. The repo and
>> initial wiki are located here:
>> https://gitlab.com/lock3/gcc-new/wikis/GCC-with-Contracts
>
> Thanks.  I've mostly been referring to the repo rather than the attached
> patch.  Below are a bunch of comments about the implementation, in no
> particular order.
>

I've attached a new squashed revision of the patch, and you can see the
changes I've made from your input on the contracts-jac-prep branch:
https://gitlab.com/lock3/gcc-new/-/tree/contracts-jac-prep . If there's
an easier format for you to review that I can produce please let me
know.

I'll address a few things inline below. Everything else should either be
handled or explained by Andrew's last email. If anything needs further
addressing or something hasn't been brought up yet please let me know :)


>> +handle_OPT_fcontract_build_level_ (const char *arg)
>> +{
>> +  if (contracts_p1332_default || contracts_p1332_review ||
>> contracts_p1429)
>> +{
>> +  error ("-fcontract-build-level= cannot be mixed with p1332/p1429");
>
> Hmm, P1429 includes the notion of build level, it's just checked after
> explicit semantics.  In general, P1429 seems like a compatible extension
> of the semantics previously in the working paper.
>
> P1332 could also be treated as compatible if we consider the P0542 build
> level to affect the default role as specified in P1429.  P1680 seems to
> suggest that this is what you had in mind.
>

These could possibly be made compatible, but in some cases the flags are
changing the same entries in the table. That would require deciding
whether flag ordering matters or whether a certain flags can't change
values set by other flags.

I'm not sure it's a worthwhile change. While it increases the valid
space of command line invocations, it doesn't actually increase the the
result space. I'd prefer an eventual solution that removed flags
entirely instead.


>> +  /* Check that assertions are null statements.  */
>> +  if (attribute_contract_assert_p (contract_attrs))
>> +if (token->type != CPP_SEMICOLON)
>> +  error_at (token->location, "assertions must be followed by
>> %<;%>");
>
> Better I think to handle this further down where [[fallthrough]] has the
> same requirement.
>

I'm wondering if it would be better to move [[fallthrough]] up, since
the later check is not always executed and in the case of [[assert]] we
actually need to error.

  [[fallthrough]] int x;

for instance just gets a generic 'attribute ignored' warning. I'm not
entirely happy with how we prevent assert/pre/post from appearing in
invalid locations in general which I'll try to improve. If you have
concrete suggestions please let me know.


> Why not leave the function the user declared as the unchecked function
> (just changing some linkage properties) and create a new function for
> the checked wrapper?
>

This revision of the patch does not include changes to the
checked/unchecked function split. We're exploring an alternative rewrite
that leaves the original function declaration alone and should address
or sidestep a number of these comments.

Specifically, we're exploring generating pre and post functions with
calls to them in the correct places (upon entering a guarded function,
wrapping the return value):

  int f(int n) [[ pre: n > 0 ]] [[ post r: r < 0 ]] { return -n; }

turns into

  void __pre_f(int n) { [[ assert: n > 0 ]]; }
  int __post_f(int r) { [[ assert: r < 0 ]]; return r; }
  int f(int n) {
__pre_f(n);
return __post_f(-n);
  }

with whatever hints we can give to optimize this as much as possible.


>> +/* Return the source text between two locations.  */
>> +
>> +static char *
>> +get_source (location_t start, location_t end)
>
> This seems like it belongs in libcpp.  It also needs to
>

This has been moved to input since it uses input functions, but needs
more work. Was there another comment you had that cutoff?


>> +  tree level_str = build_string_literal (strlen (level) + 1, level);
>> +  tree role_str = build_string_literal (strlen (role) + 1, role);
>
> Maybe combine these into a single string argument?
>

These are used separately in std::contract_violation and to my
understanding building them separately will collapse duplicate levels
and roles instead of duplicating them unless they both match -- is that
correct?


>> +  /* We never want to accidentally instantiate templates.  */
>> +  if (code == TEMPLATE_DECL)
>> +return *tp; /* FIXME? */
>
> This seems unlikely to have the desired effect; we should see template
> instantiations as FUNCTION_DECL or VAR_DECL.  I'm also not sure what the
> cp_tree_defined_p check is trying to do; surely using defined functions
> and variables can also lead to runtime code?
>

This is