Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Andrew Sutton via Gcc-patches]

> Is just using std::terminate as the handler viable? Or if we're sure
> contracts in some form will go into the IS eventually, and the
> signature won't change, we could just add it in __cxxabiv1:: as you
> suggested earlier.

No, the handler needs to be configurable (at least quietly) in order
to support experimentation by SG21. No idea if it will stay that way.

Andrew

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jonathan Wakely via Gcc-patches]

On Wed, 14 Jul 2021 at 04:56, Jason Merrill  wrote:
>
> On 7/12/21 3:58 PM, Jonathan Wakely wrote:
> > On Mon, 5 Jul 2021 at 20:07, Jason Merrill  wrote:
> >>
> >> On 6/26/21 10:23 AM, Andrew Sutton wrote:
> >>>
> >>> I ended up taking over this work from Jeff (CC'd on his existing email
> >>> address). I scraped all the contracts changes into one big patch
> >>> against master. See attached. The ChangeLog.contracts files list the
> >>> sum of changes for the patch, not the full history of the work.
> >>
> >> Jonathan, can you advise where the library support should go?
> >>
> >> In N4820  was part of the language-support clause, which makes
> >> sense, but it uses string_view, which brings in a lot of the rest of the
> >> library.  Did LWG talk about this when contracts went in?  How are
> >> freestanding implementations expected to support contracts?
> >
> > I don't recall that being discussed, but I think I was in another room
> > for much of the contracts review.
> >
> > If necessary we could make the std::char_traits specialization
> > available freestanding, without the primary template (or the other
> > specializations). But since C++20 std::string_view also depends on
> > quite a lot of ranges, which depends on iterators, which is not
> > freestanding. Some of those dependencies were added more recently than
> > contracts was reviewed and then yanked out, so maybe wasn't considered
> > a big problem back then. In any case, depending on std::string_view
> > (even without the rest of std::basic_string_view) is not currently
> > possible for freestanding.
>
> I guess I'll change string_view to const char* for now.

I think that's best. Making std::string_view usable would take some work.

> >> I imagine the header should be  for now.
> >
> > Agreed.
>
> And the type std::experimental::??::contract_violation.  Maybe
> contracts_v1 for the inline namespace?

LGTM

> Did you have any thoughts about the violation handler?  Is it OK to add
> a default definition to the library, in the above namespace?

I'd rather not have any std::experimental::* symbols go into the DSO.
For std::experimental::filesystem we added libstdc++fs.a, with no
corresponding .so library, which users need to link to explicitly to
use that TS. Would something like libstdc++contracts.a work here? Is
it just one symbol?

Aside: Ulrich Drepper suggested recently that the driver should have
been updated to automatically add -lstdc++fs so that using
 was seamless, as the archive contents
wouldn't be used unless something in the program referred to the
symbols in it.

Is just using std::terminate as the handler viable? Or if we're sure
contracts in some form will go into the IS eventually, and the
signature won't change, we could just add it in __cxxabiv1:: as you
suggested earlier.

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jason Merrill via Gcc-patches]

On 7/12/21 3:58 PM, Jonathan Wakely wrote:

On Mon, 5 Jul 2021 at 20:07, Jason Merrill  wrote:


On 6/26/21 10:23 AM, Andrew Sutton wrote:


I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.


Jonathan, can you advise where the library support should go?

In N4820  was part of the language-support clause, which makes
sense, but it uses string_view, which brings in a lot of the rest of the
library.  Did LWG talk about this when contracts went in?  How are
freestanding implementations expected to support contracts?


I don't recall that being discussed, but I think I was in another room
for much of the contracts review.

If necessary we could make the std::char_traits specialization
available freestanding, without the primary template (or the other
specializations). But since C++20 std::string_view also depends on
quite a lot of ranges, which depends on iterators, which is not
freestanding. Some of those dependencies were added more recently than
contracts was reviewed and then yanked out, so maybe wasn't considered
a big problem back then. In any case, depending on std::string_view
(even without the rest of std::basic_string_view) is not currently
possible for freestanding.


I guess I'll change string_view to const char* for now.


I imagine the header should be  for now.


Agreed.


And the type std::experimental::??::contract_violation.  Maybe 
contracts_v1 for the inline namespace?


Did you have any thoughts about the violation handler?  Is it OK to add 
a default definition to the library, in the above namespace?


Jason

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jonathan Wakely via Gcc-patches]

On Mon, 5 Jul 2021 at 20:07, Jason Merrill  wrote:
>
> On 6/26/21 10:23 AM, Andrew Sutton wrote:
> >
> > I ended up taking over this work from Jeff (CC'd on his existing email
> > address). I scraped all the contracts changes into one big patch
> > against master. See attached. The ChangeLog.contracts files list the
> > sum of changes for the patch, not the full history of the work.
>
> Jonathan, can you advise where the library support should go?
>
> In N4820  was part of the language-support clause, which makes
> sense, but it uses string_view, which brings in a lot of the rest of the
> library.  Did LWG talk about this when contracts went in?  How are
> freestanding implementations expected to support contracts?

I don't recall that being discussed, but I think I was in another room
for much of the contracts review.

If necessary we could make the std::char_traits specialization
available freestanding, without the primary template (or the other
specializations). But since C++20 std::string_view also depends on
quite a lot of ranges, which depends on iterators, which is not
freestanding. Some of those dependencies were added more recently than
contracts was reviewed and then yanked out, so maybe wasn't considered
a big problem back then. In any case, depending on std::string_view
(even without the rest of std::basic_string_view) is not currently
possible for freestanding.

> I imagine the header should be  for now.

Agreed.

Re: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jason Merrill via Gcc-patches]

On 7/5/21 3:07 PM, Jason Merrill wrote:

On 6/26/21 10:23 AM, Andrew Sutton wrote:


I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.


Jonathan, can you advise where the library support should go?

In N4820  was part of the language-support clause, which makes 
sense, but it uses string_view, which brings in a lot of the rest of the 
library.  Did LWG talk about this when contracts went in?  How are 
freestanding implementations expected to support contracts?


I imagine the header should be  for now.

You've previously mentioned that various current experimental features 
don't appear in libstdc++.so; that is not true of the current patch.


I see that https://github.com/arcosuc3m/clang-contracts takes the 
approach, of teaching the compiler about std::contract_violation, 
building up an object, and passing it to the handler directly, much like 
we do for initializer_list.  Their equivalent of __on_contract_violation 
is an internal function emitted in each translation unit that needs it, 
so it doesn't need to affect the library ABI.  These both seem like 
improvements to me.


More complicated is the question of the default violation handler: the 
lock3 implementation calls it "handle_contract_violation" in the global 
namespace, and overriding it is done with ELF symbol interposition, much 
like the replaceable allocation functions.  That approach seems 
reasonable, but I'd think we should use a reserved name, e.g. 
::__handle_contract_violation or __cxxabiv1::__contract_violation_handler.


The clang implementation above involves specifying the name of the 
handler on the compiler command line, which seems problematic, as it 
would tend to mean multiple independent violation handlers active at the 
same time.  Their default handler is std::terminate, which does avoid 
needing to add the default handler to the library.


I've pushed the patch with my adjustments so far to devel/c++-contracts 
on gcc.gnu.org.  One of those adjustments was changing the 
contract_violation data members to be const char *, to make creating an 
object easier, and making the member functions inline, to reduce the 
number of symbols added to the library.


Jason

contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jason Merrill via Gcc-patches]

On 6/26/21 10:23 AM, Andrew Sutton wrote:


I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.


Jonathan, can you advise where the library support should go?

In N4820  was part of the language-support clause, which makes 
sense, but it uses string_view, which brings in a lot of the rest of the 
library.  Did LWG talk about this when contracts went in?  How are 
freestanding implementations expected to support contracts?


I imagine the header should be  for now.

You've previously mentioned that various current experimental features 
don't appear in libstdc++.so; that is not true of the current patch.


I see that https://github.com/arcosuc3m/clang-contracts takes the 
approach, of teaching the compiler about std::contract_violation, 
building up an object, and passing it to the handler directly, much like 
we do for initializer_list.  Their equivalent of __on_contract_violation 
is an internal function emitted in each translation unit that needs it, 
so it doesn't need to affect the library ABI.  These both seem like 
improvements to me.


More complicated is the question of the default violation handler: the 
lock3 implementation calls it "handle_contract_violation" in the global 
namespace, and overriding it is done with ELF symbol interposition, much 
like the replaceable allocation functions.  That approach seems 
reasonable, but I'd think we should use a reserved name, e.g. 
::__handle_contract_violation or __cxxabiv1::__contract_violation_handler.


The clang implementation above involves specifying the name of the 
handler on the compiler command line, which seems problematic, as it 
would tend to mean multiple independent violation handlers active at the 
same time.  Their default handler is std::terminate, which does avoid 
needing to add the default handler to the library.


Jason

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

[Andrew Sutton via Gcc-patches]

I think so, yes.

On Fri, Jul 2, 2021 at 11:09 AM Jason Merrill  wrote:
>
> On 7/1/21 12:27 PM, Andrew Sutton wrote:
> >>> I think this version addresses most of your concerns.
> >>
> >> Thanks, looking good.  I'll fuss with it a bit and commit it soon.
>
> Do you agree that this testcase should compile?

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

[Jason Merrill via Gcc-patches]

On 7/1/21 12:27 PM, Andrew Sutton wrote:

I think this version addresses most of your concerns.


Thanks, looking good.  I'll fuss with it a bit and commit it soon.


Do you agree that this testcase should compile?
>From 85400e1896a188892b1ebeb0c8e86ff3cd28cfa6 Mon Sep 17 00:00:00 2001
From: Jason Merrill 
Date: Wed, 30 Jun 2021 16:57:44 -0400
Subject: [PATCH] assume-cx
To: gcc-patches@gcc.gnu.org

---
 gcc/cp/constexpr.c| 26 +++
 .../g++.dg/contracts/contracts-constexpr3.C   | 10 +++
 2 files changed, 31 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C

diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 435bf530d68..66b3ccce713 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -7022,12 +7022,26 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t,
 	  break;
 
 	tree c = CONTRACT_CONDITION (t);
-	if (semantic == CCS_ASSUME && !cp_tree_defined_p (c))
-	  break;
+	if (semantic == CCS_ASSUME)
+	  {
+	/* For an assume contract, try evaluating it without instantiating
+	   anything.  If non-constant, assume it's satisfied.  */
 
-	/* Evaluate the generated check.  */
-	r = cxx_eval_constant_expression (ctx, c, false, non_constant_p,
-	  overflow_p);
+	if (!cp_tree_defined_p (c))
+	  break;
+
+	bool dummy_nc = false, dummy_ov = false;
+	constexpr_ctx new_ctx = *ctx;
+	new_ctx.quiet = true;
+	r = cxx_eval_constant_expression (_ctx, c, false,
+	  _nc, _ov);
+	if (dummy_nc)
+	  break;
+	  }
+	else
+	  /* Evaluate the generated check.  */
+	  r = cxx_eval_constant_expression (ctx, c, false, non_constant_p,
+	overflow_p);
 	if (r == boolean_false_node)
 	  {
 	if (!ctx->quiet)
@@ -8948,6 +8962,8 @@ potential_constant_expression_1 (tree t, bool want_rval, bool strict, bool now,
 case ASSERTION_STMT:
 case PRECONDITION_STMT:
 case POSTCONDITION_STMT:
+  if (!checked_contract_p (get_contract_semantic (t)))
+	return true;
   if (!RECUR (CONTRACT_CONDITION (t), rval))
 	return false;
   return true;
diff --git a/gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C b/gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C
new file mode 100644
index 000..8826220ef91
--- /dev/null
+++ b/gcc/testsuite/g++.dg/contracts/contracts-constexpr3.C
@@ -0,0 +1,10 @@
+// An assumed contract shouldn't break constant evaluation.
+
+// { dg-do compile { target c++20 } }
+// { dg-additional-options -fcontracts }
+
+bool b;
+
+constexpr int f() [[ pre assume: b ]] { return 42; }
+
+static_assert (f() > 0);
-- 
2.27.0

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

[Andrew Sutton via Gcc-patches]

> > I think this version addresses most of your concerns.
>
> Thanks, looking good.  I'll fuss with it a bit and commit it soon.

Awesome!

Andrew

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

[Jason Merrill via Gcc-patches]

On 6/26/21 10:23 AM, Andrew Sutton wrote:

Hi Jason,

I ended up taking over this work from Jeff (CC'd on his existing email
address). I scraped all the contracts changes into one big patch
against master. See attached. The ChangeLog.contracts files list the
sum of changes for the patch, not the full history of the work.

I think this version addresses most of your concerns.


Thanks, looking good.  I'll fuss with it a bit and commit it soon.


There are a few big changes.

The first is that we treat contracts like any other attribute, which
gets interesting at times. For example, in duplicate_decl, we have to
do a little dance to make sure the target merge_attributes doesn't
copy attributes between the new and old decls in seemingly arbitrary
order. Friends are also a bit funny because the attributes aren't
attached by cplus_decl_attributes at the point declarations are
merged, so we have to defer comparisons.

Contracts are always parsed where they appear, except on member
functions. For postconditions with result variables (e.g., [[post r:
...]]), we temporarily declare r as if 'auto r' and then update it
later when we've computed the function's return type. (I feel like
this was kind of overlooked in N4820... it's generally impossible to
assign a type to 'r' given the position of contract attributes in the
declarator: 'auto f(int n) [[post r: q]] -> int'. It's worse in GCC
since the return type is computed in grokdeclarator, well after
contract attributes have been parsed).

On a related note, the handling of postconditions involving deduced
return type was completely rewritten. Everything happens in
apply_deduced_return_type, which seems right. I think
check_return_expr is where the postcondition is actually invoked.

We no longer instantiate contract attributes until absolutely
necessary in regenerate_decl_from_tempalte. That seems to work well...
at least does after I discovered we were quietly rewriting contract
lists every time we removed contracts from an old declaration or from
a template specialization. This also gets rid of the need to have
unshare_templates, which had a FIXME note attached.

Lastly, we only ever generate pre/post checks for actual functions,
not function templates. I also simplified a lot of the logic around
associating pre/post check functions, so that it's only set exactly
once when start analyzing function definitions.

I believe Jeff addressed some of the ABI concerns and COMDAT-related questions.

On the issue of copy_fn_decl vs.v copy_fndecl_with_name... I didn't
change that. The latter sends the function to middle end for codegen,
which we really don't want at the point we make the copy.

I think we're probably still breaking NRVO. I didn't get a chance to
look at that.



On Fri, May 28, 2021 at 9:18 AM Jeff Chapman  wrote:


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 

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

[Jason Merrill via Gcc-patches]

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.


+  /* 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?


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.


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?


Smaller-scope comments:


+   /* If we didn't build a check, insert a NOP so we don't leak
+  contracts into GENERIC.  */
+   *stmt_p = build1 (NOP_EXPR, void_type_node, integer_zero_node);


You can use void_node for the NOP.


+  error_at (EXPR_LOCATION (new_contract),
+   "mismatched contract condition in %s",
+   ctx == cmc_declaration ? "declaration" : "override");


This sort of build-up of diagnostic messages by substring replacement 
doesn't work very well for translations.  In general, don't use %s for 
inserting English text, only code strings that will be the same in all 
languages.



+  /* Remove the associated contracts and unchecked result, if any.  */
+  if (flag_contracts && TREE_CODE (newdecl) == FUNCTION_DECL)
+    {
+  remove_contract_attributes (newdecl);
+  set_contract_functions (newdecl, NULL_TREE, NULL_TREE);
+    }


Why bother removing attributes on a decl that's about to be freed?

Why did we set the contract functions above only to clear them now?


   if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
  /* Aliases are definitions. */
- && !alias)
+ && !alias
+ && (DECL_VIRTUAL_P (decl) || !flag_contracts))
    permerror (declarator->id_loc,
   "declaration of %q#D outside of class is not 
definition",

   decl);
+  else if (DECL_EXTERNAL (decl) && ! 

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

[Marek Polacek via Gcc-patches]

On Fri, May 14, 2021 at 04:54:10PM -0400, Jason Merrill via Gcc-patches wrote:
> 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.

And I think let's please name the file contracts.cc.

Marek

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

[Jason Merrill via Gcc-patches]

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.



+  /* 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?


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.


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?


Smaller-scope comments:


+   /* If we didn't build a check, insert a NOP so we don't leak
+  contracts into GENERIC.  */
+   *stmt_p = build1 (NOP_EXPR, void_type_node, integer_zero_node);


You can use void_node for the NOP.


+  error_at (EXPR_LOCATION (new_contract),
+   "mismatched contract condition in %s",
+   ctx == cmc_declaration ? "declaration" : "override");


This sort of build-up of diagnostic messages by substring replacement 
doesn't work very well for translations.  In general, don't use %s for 
inserting English text, only code strings that will be the same in all 
languages.



+  /* Remove the associated contracts and unchecked result, if any.  */
+  if (flag_contracts && TREE_CODE (newdecl) == FUNCTION_DECL)
+{
+  remove_contract_attributes (newdecl);
+  set_contract_functions (newdecl, NULL_TREE, NULL_TREE);
+}


Why bother removing attributes on a decl that's about to be freed?

Why did we set the contract functions above only to clear them now?


   if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
  /* Aliases are definitions. */
- && !alias)
+ && !alias
+ && (DECL_VIRTUAL_P (decl) || !flag_contracts))
permerror (declarator->id_loc,
   "declaration of %q#D outside of class is not definition",
   decl);
+  else if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
+ /* Aliases are definitions. */

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

[Jason Merrill via Gcc-patches]

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.



+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

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

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

[Jason Merrill via Gcc-patches]

On 1/4/21 9:58 AM, Jeff Chapman wrote:
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.


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/.


And then much of the code you add to decl.c could also move to the 
contracts file, and some of the contracts stuff in cp-tree.h could move 
to cxx-contracts.h?



+extern bool cxx23_contract_attribute_p (const_tree);


This name seems optimistic.  :)
Let's call it cxx_contract_attribute_p.


+/* Return TRUE iff ATTR has been parsed by the fornt-end as a c++2a contract


"front"


@@ -566,7 +566,11 @@ decl_attributes (tree *node, tree attributes, int flags,
{
  if (!(flags & (int) ATTR_FLAG_BUILT_IN))
{
- if (ns == NULL_TREE || !cxx11_attr_p)
+ if (cxx23_contract_attribute_p (attr))
+   {
+ ; /* Do not warn about contract "attributes".  */
+   }


I don't want the language-independent code to have to know about this. 
If you want decl_attributes to ignore these attributes, you could give 
these attributes a dummy spec that just returns?



+set_decl_contracts (tree decl, tree contract_attrs)
+{
+  remove_contract_attributes (decl);
+  if (!DECL_ATTRIBUTES (decl))
+{
+  DECL_ATTRIBUTES (decl) = contract_attrs;
+  return;
+}
+  tree last_attr = DECL_ATTRIBUTES (decl);
+  while (TREE_CHAIN (last_attr))
+last_attr = TREE_CHAIN (last_attr);
+  TREE_CHAIN (last_attr) = contract_attrs;


I think you want to use 'chainon' here.


@@ -5498,10 +5863,17 @@ start_decl (const cp_declarator *declarator,
 
   if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)

  /* Aliases are definitions. */
- && !alias)
+ && !alias
+ && (DECL_VIRTUAL_P (decl) || !flag_contracts))
permerror (declarator->id_loc,
   "declaration of %q#D outside of class is not definition",
   decl);
+  else if (DECL_EXTERNAL (decl) && ! DECL_TEMPLATE_SPECIALIZATION (decl)
+ /* Aliases are definitions. */
+ && !alias
+ && flag_contract_strict_declarations)
+   warning_at (declarator->id_loc, OPT_fcontract_strict_declarations_,
+   "non-defining declaration of %q#D outside of class", decl);


Let's keep the same message for the two cases.


+void
+finish_function_contracts (tree fndecl, bool is_inline)


This function needs a comment.


+/* cp_tree_defined_p helper -- returns TP if TP is undefined.  */
+
+static tree
+cp_tree_defined_p_r (tree *tp, int *, void *)
+{
+  enum tree_code code = TREE_CODE (*tp);
+  if ((code == FUNCTION_DECL || code == VAR_DECL)
+  && !decl_defined_p (*tp))
+return *tp;
+  /* We never want to accidentally instantiate templates.  */
+  if (code == TEMPLATE_DECL)
+return *tp; /* FIXME? */


In what context are you getting a TEMPLATE_DECL here?  I don't see how 
this would have an effect on instantiations.



+/* Parse a conditional-expression.  */
+/* FIXME: should callers use cp_parser_constant_expression?  */
+
+static cp_expr
+cp_parser_conditional_expression (cp_parser *parser)

...

+  /* FIXME: can we use constant_expression for this?  */
+  cp_expr cond = cp_parser_conditional_expression (parser);


I don't think we want to use cp_parser_constant_expression for 
expressions that are not intended to be constant.



+  bool finishing_guarded_p = true//!processing_template_decl


?


+/* FIXME: Is this going to leak?  */
+comment_str = xstrdup (expr_to_string (cond));


There's no need to strdup here (and free a few lines later); 
build_string_literal copies the bytes.  The return value of 
expr_to_string is in GC memory.



+  /* 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?


+  if (tree pre = lookup_attribute ("pre", 

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

[Jason Merrill via Gcc-patches]

On 1/4/21 9:58 AM, Jeff Chapman wrote:
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?


Either way I'm reviewing by diff against the most recent merged trunk 
revision, so it doesn't really matter.


But you probably want to do one merge at least, to make sure that 
modules and contracts coexist well.


Jason

[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