[PATCH] implement pre-c++20 contracts

[PATCH] implement pre-c++20 contracts

[Jeff Chapman]

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

Hello,

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

We've previously circulated a paper (P1680) which documents our
implementation experience which largely covers new flags and features.
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1680r0.pdf

That paper documents our changes in depth, barring the recently added
-fcontracts flag which is a global opt-in for contracts functionality.
As an overview of what we've done is included below for convenience.

The following switches have been added:

-fcontracts
  enable contract features in general

Flags from the old Working Draft:

-fcontract-build-level=[off|default|audit]
  specify max contract level to generate runtime checks for

-fcontract-continuation-mode=[on|off]
  toggle whether execution resumes after contract failure

Flags from P1290:

-fcontract-assumption-mode=[on|off]
  enable treating axiom level contracts as compile time assumptions
  (default on)

Flags from P1429:

-fcontract-mode=[on|off]
  enable or disable all contract facilities (default on)

-fcontract-semantic=<level>:<semantic>
  specify the concrete semantics for a level

Flags from P1332:

-fcontract-role=<name>:<semantics>
  specify semantics for all levels in a role (default, review, or a
    custom role)
  (ex: opt:assume,assume,assume)

Additional flags:

-fcontract-strict-declarations=[on|off]
  toggle warnings on generalized redecl of member functions
    without contracts (default off)


One assert contract may be present on any block scope empty statement:
  [[ assert contract-mode_opt : conditional-expression ]]

Function declarations have an optional, ordered, list of pre and post
contracts:
  [[ pre contract-mode_opt : conditional-expression ]]
  [[ post contract-mode_opt identifier_opt : conditional-expression ]]


The grammar for the contract-mode_opt portion which configures the
concrete semantics of the contracts is:

contract-mode
  contract-semantic
  contract-level_opt contract-role_opt

contract-semantic
  ignore
  check_never_continue
  check_maybe_continue
  check_always_continue
  assume

contract-level
  default
  audit
  axiom

contract-role
  %default
  %identifier


Contracts are configured via concrete semantics or by an optional
level and role which map to one of the concrete semantics:

  ignore – The contract does not affect the behavior of the program.
  assume – The condition may be used for optimization.
  never_continue – The program terminates if the contract is
                   not satisfied.
  maybe_continue – A user-defined violation handler may terminate the
                   program.
  always_continue – The program continues even if the contract is not
                    satisfied.


-fcontracts enables generalized member function redeclaration. That is,
non-defining declarations of member functions are allowed outside the
initial class definition.

Contracts are not required on the initial declaration of a
(non-virtual) function as long as all TUs that see a set of contracts
eventually see the same set of contracts. All contracts must be present
on the first declaration for virtual functions to ensure we know to
split the function in all overrides.


Explicit template specializations have an independent set of contracts
than any other explicit specializations.


Functions with contracts (which we call guarded functions) are split
just before genericization. A wrapper which checks pre and post
contracts is emitted with the original function name, while the
original function body is emitted under a new unchecked name. This
means that calls to functions in TUs which never see the contract list
still call a checked version of the function (see insulated contracts
in P1680).

Having a checked and unchecked version of the function makes it
potentially easier to inline the checks into the caller, and prevents
the need to repeat the post contracts at all return statements in the
original function.


A custom contract violation handler can be installed by defining
  void handle_contract_violation(const std::contract_violation &)

(you must #include <contract> for this to work) or by using LD_PRELOAD.
An example of defining the handler in the main TU can be found in the
testsuite g++.dg/cpp2a/contracts16.C .

Examples of how to use LD_PRELOAD to install a custom violation handler
are available in the contracts folder inside the testsuite.


Bootstrapped and tested on x86_64-pc-linux-gnu. cmcstl2 compiles cleanly
and has no `make test` failures.


Please let me know if you have any questions or know what the next
steps will be.


Thank you,
Jeff Chapman II

[-- Attachment #2: 0001-Implement-pre-c-20-contracts.patch.gz --]
[-- Type: application/gzip, Size: 95619 bytes --]

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

[Jason Merrill]

On 11/13/19 2:07 PM, 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.

> We've previously circulated a paper (P1680) which documents our
> implementation experience which largely covers new flags and features.
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1680r0.pdf
> 
> That paper documents our changes in depth, barring the recently added
> -fcontracts flag which is a global opt-in for contracts functionality.
> As an overview of what we've done is included below for convenience.
> 
> The following switches have been added:
> 
> -fcontracts
>    enable contract features in general
> 
> Flags from the old Working Draft:
> 
> -fcontract-build-level=[off|default|audit]
>    specify max contract level to generate runtime checks for
> 
> -fcontract-continuation-mode=[on|off]
>    toggle whether execution resumes after contract failure
> 
> Flags from P1290:
> 
> -fcontract-assumption-mode=[on|off]
>    enable treating axiom level contracts as compile time assumptions
>    (default on)
> 
> Flags from P1429:
> 
> -fcontract-mode=[on|off]
>    enable or disable all contract facilities (default on)
> 
> -fcontract-semantic=<level>:<semantic>
>    specify the concrete semantics for a level
> 
> Flags from P1332:
> 
> -fcontract-role=<name>:<semantics>
>    specify semantics for all levels in a role (default, review, or a
>      custom role)
>    (ex: opt:assume,assume,assume)
> 
> Additional flags:
> 
> -fcontract-strict-declarations=[on|off]
>    toggle warnings on generalized redecl of member functions
>      without contracts (default off)
> 
> 
> One assert contract may be present on any block scope empty statement:
>    [[ assert contract-mode_opt : conditional-expression ]]
> 
> Function declarations have an optional, ordered, list of pre and post
> contracts:
>    [[ pre contract-mode_opt : conditional-expression ]]
>    [[ post contract-mode_opt identifier_opt : conditional-expression ]]
> 
> 
> The grammar for the contract-mode_opt portion which configures the
> concrete semantics of the contracts is:
> 
> contract-mode
>    contract-semantic
>    contract-level_opt contract-role_opt
> 
> contract-semantic
>    ignore
>    check_never_continue
>    check_maybe_continue
>    check_always_continue
>    assume
> 
> contract-level
>    default
>    audit
>    axiom
> 
> contract-role
>    %default
>    %identifier
> 
> 
> Contracts are configured via concrete semantics or by an optional
> level and role which map to one of the concrete semantics:
> 
>    ignore – The contract does not affect the behavior of the program.
>    assume – The condition may be used for optimization.
>    never_continue – The program terminates if the contract is
>                     not satisfied.
>    maybe_continue – A user-defined violation handler may terminate the
>                     program.
>    always_continue – The program continues even if the contract is not
>                      satisfied.

I find the proposed always_continue semantics kind of nonsensical, as 
somewhat evidenced by the contortions the implementation gets into with 
marking the violation handler as pure.  The trick of assigning the 
result to a local variable won't work with optimization.

It also depends on the definition of a function that can be overridden 
to in fact never return.  This seems pretty fatal to it ever getting 
into the standard.

It's also unclear to me why anyone would want the described semantics. 
Why would you want a contract check that can be optimized away due to 
later undefined behavior?  The 0.2 use case from P1332 seems better 
suited to maybe_continue, because with always_continue such a check will 
have false negatives, leading to an unpleasant surprise when switching 
to never_continue.

I'd prefer to treat always_continue as equivalent to maybe_continue. 
Perhaps with ECF_NOTHROW|ECF_LEAF|ECF_NOVOPS to indicate that it doesn't 
clobber anything the caller can see, but that's risky if the handler is 
in fact defined in the same TU with anything that uses contracts.

> +  if (strcmp (name, "check_always_continue") == 0
> +      || strcmp (name, "always") == 0
> +      || strcmp (name, "continue") == 0)

Accordingly, "continue" should mean maybe_continue.

> +/* Definitions for C++ contract levels
> +   Copyright (C) 1987-2018 Free Software Foundation, Inc.

Should just be 2019 for a new file.

> +   Contributed by Michael Tiemann (tiemann@cygnus.com)

This seems inaccurate.  :)

It would also be good to have a reference to P1332 in this header.

> +/* Assertion role info.
> +
> +   FIXME: Why is this prefixed cpp?  */
> +struct cpp_contract_role

There seems to be no reason for it, since the struct definition is 
followed by a typedef; let's remove the prefix.

Any cpp_ prefixes we want to keep should change to cxx to avoid 
ambiguity with the preprocessor.

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

> +  // TODO: worry about leaking this?
> +  char *name = xstrdup (arg);

Yes, worry. :)
There's no reason to leak it.

> +++ b/gcc/c-family/contract.c
> @@ -0,0 +1,138 @@
> +#include "config.h"
> +#include "system.h"

This file needs the introductory copyright block.  It should probably be 
named cxx-contracts.c.  Can more of the option handling move into this file?

> +  return (contract_semantic) CONTRACT_CHECK (t)->base.u.bits.spare1;
> +  CONTRACT_CHECK (t)->base.u.bits.spare1 = semantic;

You can't use spare* for data; they are only for allocating bits out of 
for other named fields.  You could encode the semantic into multiple 
TREE_LANG_FLAG_* or make it an operand of the expression.

> @@ -2730,6 +2809,13 @@ struct GTY(()) lang_decl_fn {
>      tree GTY ((tag ("0"))) saved_auto_return_type;
>    } GTY ((desc ("%1.pending_inline_p"))) u;
>  
> +  /* For the checked version of a guarded function, this points to the var
> +     caputring the result of the unchecked function.  */
> +  tree unchecked_result;
> +  /* For a guarded function with contracts, this is a tree list where
> +     the purpose is the location of the contracts and the value is the list of
> +     contracts specified in original decl order.  */
> +  tree contracts;
>  };

Let's not make all functions two words larger to support contracts; this 
information can go either in an attribute or a separate hash table.  An 
attribute seems natural for the contracts since that's already the 
syntax contracts use.  I wouldn't expect you to need a pointer to the 
unchecked result in the FUNCTION_DECL, that seems like a detail local to 
the implementation of the checked function.

> @@ -6036,6 +6157,8 @@ struct cp_declarator {
>       declarator is a pointer or a reference, these attributes apply
>       to the pointer, rather than to the type pointed to.  */
>    tree std_attributes;
> +  /* The contracts, if any. */
> +  tree contracts;

Adding to cp_declarator isn't a problem, but again it seems like 
contracts could be represented as attributes.  In general, it seems like 
you are working to subvert the attribute mechanisms rather than using 
them normally, and I'm not sure why.

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

> +++ b/gcc/c-family/c-common.c
> @@ -50,6 +50,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "spellcheck.h"
>  #include "c-spellcheck.h"
>  #include "selftest.h"
> +#include "print-tree.h"

I don't see anything that needs this.

> +  /* Compare the conditions of the contracts.  */
> +  tree old_cond = cp_fully_fold_init (CONTRACT_CONDITION (old_contract));
> +  tree new_cond = cp_fully_fold_init (CONTRACT_CONDITION (new_contract));
> +  if (!cp_tree_equal (old_cond, new_cond))

Why fold before comparison?  I would think that we want the conditions 
to be equivalent before folding.

> +/* Compare the contract attributes of OLDDECL and NEWDECL. Returns false 
> +   if the contracts match, and true if they differ.  */
> +
> +static bool
> +match_contract_conditions (location_t oldloc, tree old_attrs,
...
> +match_contracts (tree olddecl, location_t newloc, tree new_attrs)

These names suggest to me that they would return true if they match, 
rather than true if there's a problem.  Changing "match" to "mismatched" 
would be better.

>        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);

This change to member function redeclaration seems undesirable; your 
rationale in P1680 is "for generality", but member functions are already 
stricter about redeclaration than non-member functions; I don't see a 
reason to relax this rule just for contracts.  With member functions, 
there *is* always a canonical first declaration, and people expect the 
class definition to have its complete interface, of which contracts are 
a part.

For non-member functions, we still need to complain if contracts are 
added after we've seen an ODR-use of the function, just like with 
explicit specializations.

> +      /* TODO is there any way to relax this requirement?  */
> +      if (DECL_VIRTUAL_P (olddecl) && !TYPE_BEING_DEFINED (DECL_CONTEXT (olddecl)))

Relatedly, I don't think we want to relax this requirement.

> +  /* FIXME part of this is taken from store_parm_decls -- is there an existing
> +     method that does only this subset of work?  */

I think you're looking for inject_parm_decls; that's what deferred 
noexcept parsing uses.

> +  /* If we're entering a class member; ensure current_class_ptr and
> +     current_class_ref point to the correct place.  */

And this is inject_this_parameter.

> +/* Build and return a new string representing the unchecked function name
> +   corresponding to the name in IDENT. */
> +
> +// FIXME: are we sure we shouldn't just mangle or use some existing machinery?
> +static const char *
> +get_contracts_internal_decl_name (const char *ident)

You absolutely should use the mangler, and should propose a mangling to 
the ABI committee at https://github.com/itanium-cxx-abi/cxx-abi/issues

For comparison, transactional memory extra entry points are encoded by 
inserting "GTt" after the _Z in the mangling of a function; something 
similar seems appropriate here.

> +/* Like copy_fn; rebuild UNCHECKED's DECL_SAVED_TREE to have its own locals
> +   and point to its own DECL_ARGUMENTS and DECL_RESULT instead of pointing at
> +   CHECKED's.
> +
> +   Unlike copy_fn, UNCHECKED (the target) does not need to be
> +   current_function_decl.  */

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?

> +         && (declarator->kind != cdk_function
> +             || !inner_declarator || inner_declarator->kind != cdk_id))

I think you want function_declarator_p.

> +  // TODO: can this contextually be ECF_NOTHROW if inside a noexcept?

That doesn't sound worthwhile to me.

> +/* Parse a conditional-expression.  */
> +/* FIXME: should callers use cp_parser_constant_expression?  */

It doesn't really matter since C++11 generalized constant expressions; 
we don't need to enforce their rules in the parser anymore.

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

> +  /* FIXME how should we handle newlines and runs of spaces?  */
> +  char buf[1024 + 4]{};
> +  char *res = buf;
> +  size_t len = 1024;

Obstacks work very well for temporary buffers like this.

> +/* Pretty print an expression and return the result as a char*.  */
> +
> +static char*
> +stringify_tree (tree expr)

Is there a reason not to use expr_to_string?

> +  /* TODO: If this is a postcondition and the return type is deduced,
> +     then we need to cache tokens and parse when the function is
> +     defined.

Yes.

> +     Would it be okay to declare the result decl as having a deduced
> +     return type also?

For a template, yes.  For a non-templated function, no.

> +     NOTE: You can't declare a deduced return type function with
> +     postconditions and not define it.  */

Well, you can, but you can't call it, so it doesn't matter if we haven't 
figured out the contracts.

> +   FIXME: There are probably earlier exits than end of file. What
> +   about a semicolon?  */

You could use cp_parser_skip_to_closing_parenthesis_1 to look for an 
un-nested ] and then complain if it isn't followed by another ].

> +/* Build and return an argument list using all the arguments passed to the
> +   (presumably guarded) FUNCTION_DECL FN.  This can be used to forward all of
> +   FN's arguments to a function taking the same list of arguments -- namely
> +   the unchecked form of FN. */

You will want to use forward_parm in this function.

> +      if (VOID_TYPE_P (t))
> +       continue;

There are no void parameters in DECL_ARGUMENTS (only in TYPE_ARG_TYPES).

> +      error ("guarded function %q+D overriding non-guarded function",

The term "guarded" doesn't seem to appear in any of the contracts papers 
other than yours, so let's word this differently.

> +      error ("guarded function %q+D overriding non-guarded function",
> +            overrider);
> +      inform (DECL_SOURCE_LOCATION (basefn),
> +             "overridden function is %qD", basefn);
> +      return 0; // FIXME?

What's to fix?

> +  /* FIXME: Why do we have two unrelated statement lists?  */
> +  tree def = push_stmt_list();
> +  tree body = begin_compound_stmt (BCS_NORMAL);

You probably want begin_function_body instead of push_stmt_list.

> +  finish_function_body (TREE_VALUE (def));

And then a finish_compound_stmt before the finish_function_body.

> +  /* FIXME we're marking the unchecked fn decl as not virtual, so why do we
> +     need to disallow virtual when building the call? */

Indeed, the disallow_virtual argument only matters for virtual 
functions, so either value is fine.

> +  /* FIXME it may make more sense to have the checked function be concrete
> +     with the unchecked be virtual since all checked function overrides must
> +     be equivalent due to the contract matching requirement.  */

Doing it that way would allow better inlining of the checking wrapper, 
but would require some way to specify virtual or non-virtual calling 
from the checking wrapper to the unchecked function.

> +  /* FIXME temporarily inlined from finish_compound_stmt except for actually
> +     adding the compound statement to the current statement list. We need
> +     several parts of this logic to handle normal and template parsing for
> +     postconditions, but is there something better we can use instead?  */

You could push/pop_stmt_list around the call to finish_compound_stmt to 
capture the result.

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

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

> +  /* FIXME: This code gen will be moved to gimple.  */

It should be simple enough to move to cp_genericize_r.

> +  /* The condition is converted to bool.
> +
> +     FIXME: When we instantiate this, the input location is in entirely
> +     the wrong place.  */

It should work to attach the location to the contract _STMT.

> @@ -14156,6 +14156,7 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
>               goto dont_recalculate;
>  
>             default:
> +              debug_tree (*expr_p);

Leftover debugging code.

> +      /* If we don't have contracts and haven't already set the deferred
> +        contracts to a "no initial contracts" sentinel, do so now to support
> +        flag_contract_strict_declarations.  */

What is the advantage of this sentinel over NULL_TREE?

> +// TODO FIXME override stops parsing of attrs silently?

Any more information about this?

> +    /* FIXME: should the contracts on a friend decl be a complete class
> +       context for the type being currently defined? */
> +  /* FIXME contracts can be parsed inside of a class when the decl is a friend? */

I would expect all contract attributes in a class to be complete class 
contexts, whether the function is a member or friend.

> +  /* FIXME Is this right for friends? Can a friend refer to a static member?
> +     Can a friend's contracts refer to our privates? Turning them into
> +     [[assert]]s inside the body of the friend itself certainly lets them do
> +     so. */

P0542 says contracts are limited to the accessibility of the function 
itself, so a friend cannot refer to private members.

> +  /* FIXME Do instantiations mean anything in the context of contracts?  */

No; I'm not sure what they mean for default arguments.

> +      DECL_DEFERRED_CONTRACTS (decl) = chainon (
> +         DECL_DEFERRED_CONTRACTS (decl),
> +         build_tree_list (original_loc, fn->contracts));

This indentation pattern occurs pretty often in the patch, but doesn't 
match the usual style in GCC sources; there's only one occurrence of a 
left paren at the end of a line.  Please move it to the beginning of the 
argument list on the next line.  The following lines also seem more 
indented than necessary.

Jason

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

[Jason Merrill]

On 12/10/19 12:58 AM, Jason Merrill wrote:
> On 11/13/19 2:07 PM, 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.
> 
>> We've previously circulated a paper (P1680) which documents our
>> implementation experience which largely covers new flags and features.
>> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1680r0.pdf
>>
>> That paper documents our changes in depth, barring the recently added
>> -fcontracts flag which is a global opt-in for contracts functionality.
>> As an overview of what we've done is included below for convenience.
>>
>> The following switches have been added:
>>
>> -fcontracts
>>    enable contract features in general
>>
>> Flags from the old Working Draft:
>>
>> -fcontract-build-level=[off|default|audit]
>>    specify max contract level to generate runtime checks for
>>
>> -fcontract-continuation-mode=[on|off]
>>    toggle whether execution resumes after contract failure
>>
>> Flags from P1290:
>>
>> -fcontract-assumption-mode=[on|off]
>>    enable treating axiom level contracts as compile time assumptions
>>    (default on)
>>
>> Flags from P1429:
>>
>> -fcontract-mode=[on|off]
>>    enable or disable all contract facilities (default on)
>>
>> -fcontract-semantic=<level>:<semantic>
>>    specify the concrete semantics for a level
>>
>> Flags from P1332:
>>
>> -fcontract-role=<name>:<semantics>
>>    specify semantics for all levels in a role (default, review, or a
>>      custom role)
>>    (ex: opt:assume,assume,assume)
>>
>> Additional flags:
>>
>> -fcontract-strict-declarations=[on|off]
>>    toggle warnings on generalized redecl of member functions
>>      without contracts (default off)
>>
>>
>> One assert contract may be present on any block scope empty statement:
>>    [[ assert contract-mode_opt : conditional-expression ]]
>>
>> Function declarations have an optional, ordered, list of pre and post
>> contracts:
>>    [[ pre contract-mode_opt : conditional-expression ]]
>>    [[ post contract-mode_opt identifier_opt : conditional-expression ]]
>>
>>
>> The grammar for the contract-mode_opt portion which configures the
>> concrete semantics of the contracts is:
>>
>> contract-mode
>>    contract-semantic
>>    contract-level_opt contract-role_opt
>>
>> contract-semantic
>>    ignore
>>    check_never_continue
>>    check_maybe_continue
>>    check_always_continue
>>    assume
>>
>> contract-level
>>    default
>>    audit
>>    axiom
>>
>> contract-role
>>    %default
>>    %identifier
>>
>>
>> Contracts are configured via concrete semantics or by an optional
>> level and role which map to one of the concrete semantics:
>>
>>    ignore – The contract does not affect the behavior of the program.
>>    assume – The condition may be used for optimization.
>>    never_continue – The program terminates if the contract is
>>                     not satisfied.
>>    maybe_continue – A user-defined violation handler may terminate the
>>                     program.
>>    always_continue – The program continues even if the contract is not
>>                      satisfied.
> 
> I find the proposed always_continue semantics kind of nonsensical, as 
> somewhat evidenced by the contortions the implementation gets into with 
> marking the violation handler as pure.  The trick of assigning the 
> result to a local variable won't work with optimization.
> 
> It also depends on the definition of a function that can be overridden 
> to in fact never return.  This seems pretty fatal to it ever getting 
> into the standard.
> 
> It's also unclear to me why anyone would want the described semantics. 
> Why would you want a contract check that can be optimized away due to 
> later undefined behavior?  The 0.2 use case from P1332 seems better 
> suited to maybe_continue, because with always_continue such a check will 
> have false negatives, leading to an unpleasant surprise when switching 
> to never_continue.
> 
> I'd prefer to treat always_continue as equivalent to maybe_continue. 
> Perhaps with ECF_NOTHROW|ECF_LEAF|ECF_NOVOPS to indicate that it doesn't 
> clobber anything the caller can see, but that's risky if the handler is 
> in fact defined in the same TU with anything that uses contracts.
> 
>> +  if (strcmp (name, "check_always_continue") == 0
>> +      || strcmp (name, "always") == 0
>> +      || strcmp (name, "continue") == 0)
> 
> Accordingly, "continue" should mean maybe_continue.
> 
>> +/* Definitions for C++ contract levels
>> +   Copyright (C) 1987-2018 Free Software Foundation, Inc.
> 
> Should just be 2019 for a new file.
> 
>> +   Contributed by Michael Tiemann (tiemann@cygnus.com)
> 
> This seems inaccurate.  :)
> 
> It would also be good to have a reference to P1332 in this header.
> 
>> +/* Assertion role info.
>> +
>> +   FIXME: Why is this prefixed cpp?  */
>> +struct cpp_contract_role
> 
> There seems to be no reason for it, since the struct definition is 
> followed by a typedef; let's remove the prefix.
> 
> Any cpp_ prefixes we want to keep should change to cxx to avoid 
> ambiguity with the preprocessor.
> 
>> +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.
> 
>> +  // TODO: worry about leaking this?
>> +  char *name = xstrdup (arg);
> 
> Yes, worry. :)
> There's no reason to leak it.
> 
>> +++ b/gcc/c-family/contract.c
>> @@ -0,0 +1,138 @@
>> +#include "config.h"
>> +#include "system.h"
> 
> This file needs the introductory copyright block.  It should probably be 
> named cxx-contracts.c.  Can more of the option handling move into this 
> file?
> 
>> +  return (contract_semantic) CONTRACT_CHECK (t)->base.u.bits.spare1;
>> +  CONTRACT_CHECK (t)->base.u.bits.spare1 = semantic;
> 
> You can't use spare* for data; they are only for allocating bits out of 
> for other named fields.  You could encode the semantic into multiple 
> TREE_LANG_FLAG_* or make it an operand of the expression.
> 
>> @@ -2730,6 +2809,13 @@ struct GTY(()) lang_decl_fn {
>>      tree GTY ((tag ("0"))) saved_auto_return_type;
>>    } GTY ((desc ("%1.pending_inline_p"))) u;
>>
>> +  /* For the checked version of a guarded function, this points to 
>> the var
>> +     caputring the result of the unchecked function.  */
>> +  tree unchecked_result;
>> +  /* For a guarded function with contracts, this is a tree list where
>> +     the purpose is the location of the contracts and the value is 
>> the list of
>> +     contracts specified in original decl order.  */
>> +  tree contracts;
>>  };
> 
> Let's not make all functions two words larger to support contracts; this 
> information can go either in an attribute or a separate hash table.  An 
> attribute seems natural for the contracts since that's already the 
> syntax contracts use.  I wouldn't expect you to need a pointer to the 
> unchecked result in the FUNCTION_DECL, that seems like a detail local to 
> the implementation of the checked function.
> 
>> @@ -6036,6 +6157,8 @@ struct cp_declarator {
>>       declarator is a pointer or a reference, these attributes apply
>>       to the pointer, rather than to the type pointed to.  */
>>    tree std_attributes;
>> +  /* The contracts, if any. */
>> +  tree contracts;
> 
> Adding to cp_declarator isn't a problem, but again it seems like 
> contracts could be represented as attributes.  In general, it seems like 
> you are working to subvert the attribute mechanisms rather than using 
> them normally, and I'm not sure why.
> 
>> +  /* 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.
> 
>> +++ b/gcc/c-family/c-common.c
>> @@ -50,6 +50,7 @@ along with GCC; see the file COPYING3.  If not see
>>  #include "spellcheck.h"
>>  #include "c-spellcheck.h"
>>  #include "selftest.h"
>> +#include "print-tree.h"
> 
> I don't see anything that needs this.
> 
>> +  /* Compare the conditions of the contracts.  */
>> +  tree old_cond = cp_fully_fold_init (CONTRACT_CONDITION 
>> (old_contract));
>> +  tree new_cond = cp_fully_fold_init (CONTRACT_CONDITION 
>> (new_contract));
>> +  if (!cp_tree_equal (old_cond, new_cond))
> 
> Why fold before comparison?  I would think that we want the conditions 
> to be equivalent before folding.
> 
>> +/* Compare the contract attributes of OLDDECL and NEWDECL. Returns 
>> false +   if the contracts match, and true if they differ.  */
>> +
>> +static bool
>> +match_contract_conditions (location_t oldloc, tree old_attrs,
> ...
>> +match_contracts (tree olddecl, location_t newloc, tree new_attrs)
> 
> These names suggest to me that they would return true if they match, 
> rather than true if there's a problem.  Changing "match" to "mismatched" 
> would be better.
> 
>>        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);
> 
> This change to member function redeclaration seems undesirable; your 
> rationale in P1680 is "for generality", but member functions are already 
> stricter about redeclaration than non-member functions; I don't see a 
> reason to relax this rule just for contracts.  With member functions, 
> there *is* always a canonical first declaration, and people expect the 
> class definition to have its complete interface, of which contracts are 
> a part.
> 
> For non-member functions, we still need to complain if contracts are 
> added after we've seen an ODR-use of the function, just like with 
> explicit specializations.
> 
>> +      /* TODO is there any way to relax this requirement?  */
>> +      if (DECL_VIRTUAL_P (olddecl) && !TYPE_BEING_DEFINED 
>> (DECL_CONTEXT (olddecl)))
> 
> Relatedly, I don't think we want to relax this requirement.
> 
>> +  /* FIXME part of this is taken from store_parm_decls -- is there an 
>> existing
>> +     method that does only this subset of work?  */
> 
> I think you're looking for inject_parm_decls; that's what deferred 
> noexcept parsing uses.
> 
>> +  /* If we're entering a class member; ensure current_class_ptr and
>> +     current_class_ref point to the correct place.  */
> 
> And this is inject_this_parameter.
> 
>> +/* Build and return a new string representing the unchecked function 
>> name
>> +   corresponding to the name in IDENT. */
>> +
>> +// FIXME: are we sure we shouldn't just mangle or use some existing 
>> machinery?
>> +static const char *
>> +get_contracts_internal_decl_name (const char *ident)
> 
> You absolutely should use the mangler, and should propose a mangling to 
> the ABI committee at https://github.com/itanium-cxx-abi/cxx-abi/issues
> 
> For comparison, transactional memory extra entry points are encoded by 
> inserting "GTt" after the _Z in the mangling of a function; something 
> similar seems appropriate here.
> 
>> +/* Like copy_fn; rebuild UNCHECKED's DECL_SAVED_TREE to have its own 
>> locals
>> +   and point to its own DECL_ARGUMENTS and DECL_RESULT instead of 
>> pointing at
>> +   CHECKED's.
>> +
>> +   Unlike copy_fn, UNCHECKED (the target) does not need to be
>> +   current_function_decl.  */
> 
> 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?
> 
>> +         && (declarator->kind != cdk_function
>> +             || !inner_declarator || inner_declarator->kind != cdk_id))
> 
> I think you want function_declarator_p.
> 
>> +  // TODO: can this contextually be ECF_NOTHROW if inside a noexcept?
> 
> That doesn't sound worthwhile to me.
> 
>> +/* Parse a conditional-expression.  */
>> +/* FIXME: should callers use cp_parser_constant_expression?  */
> 
> It doesn't really matter since C++11 generalized constant expressions; 
> we don't need to enforce their rules in the parser anymore.
> 
>> +/* 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
> 
>> +  /* FIXME how should we handle newlines and runs of spaces?  */
>> +  char buf[1024 + 4]{};
>> +  char *res = buf;
>> +  size_t len = 1024;
> 
> Obstacks work very well for temporary buffers like this.
> 
>> +/* Pretty print an expression and return the result as a char*.  */
>> +
>> +static char*
>> +stringify_tree (tree expr)
> 
> Is there a reason not to use expr_to_string?
> 
>> +  /* TODO: If this is a postcondition and the return type is deduced,
>> +     then we need to cache tokens and parse when the function is
>> +     defined.
> 
> Yes.
> 
>> +     Would it be okay to declare the result decl as having a deduced
>> +     return type also?
> 
> For a template, yes.  For a non-templated function, no.
> 
>> +     NOTE: You can't declare a deduced return type function with
>> +     postconditions and not define it.  */
> 
> Well, you can, but you can't call it, so it doesn't matter if we haven't 
> figured out the contracts.
> 
>> +   FIXME: There are probably earlier exits than end of file. What
>> +   about a semicolon?  */
> 
> You could use cp_parser_skip_to_closing_parenthesis_1 to look for an 
> un-nested ] and then complain if it isn't followed by another ].
> 
>> +/* Build and return an argument list using all the arguments passed 
>> to the
>> +   (presumably guarded) FUNCTION_DECL FN.  This can be used to 
>> forward all of
>> +   FN's arguments to a function taking the same list of arguments -- 
>> namely
>> +   the unchecked form of FN. */
> 
> You will want to use forward_parm in this function.
> 
>> +      if (VOID_TYPE_P (t))
>> +       continue;
> 
> There are no void parameters in DECL_ARGUMENTS (only in TYPE_ARG_TYPES).
> 
>> +      error ("guarded function %q+D overriding non-guarded function",
> 
> The term "guarded" doesn't seem to appear in any of the contracts papers 
> other than yours, so let's word this differently.
> 
>> +      error ("guarded function %q+D overriding non-guarded function",
>> +            overrider);
>> +      inform (DECL_SOURCE_LOCATION (basefn),
>> +             "overridden function is %qD", basefn);
>> +      return 0; // FIXME?
> 
> What's to fix?
> 
>> +  /* FIXME: Why do we have two unrelated statement lists?  */
>> +  tree def = push_stmt_list();
>> +  tree body = begin_compound_stmt (BCS_NORMAL);
> 
> You probably want begin_function_body instead of push_stmt_list.
> 
>> +  finish_function_body (TREE_VALUE (def));
> 
> And then a finish_compound_stmt before the finish_function_body.
> 
>> +  /* FIXME we're marking the unchecked fn decl as not virtual, so why 
>> do we
>> +     need to disallow virtual when building the call? */
> 
> Indeed, the disallow_virtual argument only matters for virtual 
> functions, so either value is fine.
> 
>> +  /* FIXME it may make more sense to have the checked function be 
>> concrete
>> +     with the unchecked be virtual since all checked function 
>> overrides must
>> +     be equivalent due to the contract matching requirement.  */
> 
> Doing it that way would allow better inlining of the checking wrapper, 
> but would require some way to specify virtual or non-virtual calling 
> from the checking wrapper to the unchecked function.
> 
>> +  /* FIXME temporarily inlined from finish_compound_stmt except for 
>> actually
>> +     adding the compound statement to the current statement list. We 
>> need
>> +     several parts of this logic to handle normal and template 
>> parsing for
>> +     postconditions, but is there something better we can use 
>> instead?  */
> 
> You could push/pop_stmt_list around the call to finish_compound_stmt to 
> capture the result.
> 
>> +  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?
> 
>> +  /* 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?
> 
>> +  /* FIXME: This code gen will be moved to gimple.  */
> 
> It should be simple enough to move to cp_genericize_r.
> 
>> +  /* The condition is converted to bool.
>> +
>> +     FIXME: When we instantiate this, the input location is in entirely
>> +     the wrong place.  */
> 
> It should work to attach the location to the contract _STMT.
> 
>> @@ -14156,6 +14156,7 @@ gimplify_expr (tree *expr_p, gimple_seq 
>> *pre_p, gimple_seq *post_p,
>>               goto dont_recalculate;
>>
>>             default:
>> +              debug_tree (*expr_p);
> 
> Leftover debugging code.
> 
>> +      /* If we don't have contracts and haven't already set the deferred
>> +        contracts to a "no initial contracts" sentinel, do so now to 
>> support
>> +        flag_contract_strict_declarations.  */
> 
> What is the advantage of this sentinel over NULL_TREE?
> 
>> +// TODO FIXME override stops parsing of attrs silently?
> 
> Any more information about this?
> 
>> +    /* FIXME: should the contracts on a friend decl be a complete class
>> +       context for the type being currently defined? */
>> +  /* FIXME contracts can be parsed inside of a class when the decl is 
>> a friend? */
> 
> I would expect all contract attributes in a class to be complete class 
> contexts, whether the function is a member or friend.
> 
>> +  /* FIXME Is this right for friends? Can a friend refer to a static 
>> member?
>> +     Can a friend's contracts refer to our privates? Turning them into
>> +     [[assert]]s inside the body of the friend itself certainly lets 
>> them do
>> +     so. */
> 
> P0542 says contracts are limited to the accessibility of the function 
> itself, so a friend cannot refer to private members.
> 
>> +  /* FIXME Do instantiations mean anything in the context of 
>> contracts?  */
> 
> No; I'm not sure what they mean for default arguments.

Now I understand again what they mean for default arguments: if we need 
to instantiate a member function before its default arguments have been 
parsed, we stick the same DEFERRED_PARSE in the default arguments of the 
instantiation, and then replace any of those with the parsed result once 
we have it.  Since you wait to substitute contracts until you are 
instantiating the function definition, you shouldn't need anything like 
that.

>> +      DECL_DEFERRED_CONTRACTS (decl) = chainon (
>> +         DECL_DEFERRED_CONTRACTS (decl),
>> +         build_tree_list (original_loc, fn->contracts));
> 
> This indentation pattern occurs pretty often in the patch, but doesn't 
> match the usual style in GCC sources; there's only one occurrence of a 
> left paren at the end of a line.  Please move it to the beginning of the 
> argument list on the next line.  The following lines also seem more 
> indented than necessary.
> 
> Jason

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

[Andrew Sutton]

Hi Jason,

Sorry I haven't been able to get back to this. I've been swamped with
other work, and we haven't had the resources to properly address this.
Jeff Chapman will be working on cleaning this up for when master/trunk
re-opens.


> I find the proposed always_continue semantics kind of nonsensical, as
> somewhat evidenced by the contortions the implementation gets into with
> marking the violation handler as pure.  The trick of assigning the
> result to a local variable won't work with optimization.

We tend to agree and think it's practically non-implementable. IIRC,
later contracts proposals steered away from adding this as a concrete
semantic. I suspect killing this would be fine.

> > +/* Definitions for C++ contract levels
> > +   Copyright (C) 1987-2018 Free Software Foundation, Inc.
>
> Should just be 2019 for a new file.

Probably 2020 now :)

> > +   Contributed by Michael Tiemann (tiemann@cygnus.com)
>
> This seems inaccurate.  :)

Indeed :)


> This change to member function redeclaration seems undesirable; your
> rationale in P1680 is "for generality", but member functions are already
> stricter about redeclaration than non-member functions; I don't see a
> reason to relax this rule just for contracts.  With member functions,
> there *is* always a canonical first declaration, and people expect the
> class definition to have its complete interface, of which contracts are
> a part.

There was a proposal that gave more motivation than just "for
generality". Apparently, I was a co-author -- I think I just helped
write the wording.

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1320r2.html


> For non-member functions, we still need to complain if contracts are
> added after we've seen an ODR-use of the function, just like with
> explicit specializations.

We could do that, but it doesn't seem necessary with this model.
Whether the contracts have been seen or not doesn't affect which
function is called.


> > +      /* TODO is there any way to relax this requirement?  */
> > +      if (DECL_VIRTUAL_P (olddecl) && !TYPE_BEING_DEFINED (DECL_CONTEXT (olddecl)))
>
> Relatedly, I don't think we want to relax this requirement.

Probably. Virtual functions and contracts have complex interactions.
There are going to be more EWG papers about this, I'm sure.


> > +  /* FIXME Is this right for friends? Can a friend refer to a static member?
> > +     Can a friend's contracts refer to our privates? Turning them into
> > +     [[assert]]s inside the body of the friend itself certainly lets them do
> > +     so. */
>
> P0542 says contracts are limited to the accessibility of the function
> itself, so a friend cannot refer to private members.

That will almost certainly be changed. This was the proposal:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1289r0.pdf

There was near-unanimous support for removing that (19 for, 1 against).

Andrew

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

[Jeff Chapman]

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

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

[-- Attachment #2: 0001-Implement-pre-c-20-contracts.patch.gz --]
[-- Type: application/x-gzip, Size: 102273 bytes --]

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

[Jeff Chapman]

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

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.

[-- Attachment #2: 0001-Implement-pre-c-20-contracts.patch.gz --]
[-- Type: application/x-gzip, Size: 75262 bytes --]

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

[Jeff Chapman]

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 <jchapman@lock3software.com> 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

[Jason Merrill]

On 7/10/20 1:53 PM, 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

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.

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

[Andrew Sutton]

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

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 :)

Andrew

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

[Jason Merrill]

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

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

[Jeff Chapman]

> 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 <jason@redhat.com> 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
> >     <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] PING implement pre-c++20 contracts

[Jeff Chapman]

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] PING implement pre-c++20 contracts

[Jason Merrill]

On 1/4/21 9:58 AM, Jeff Chapman wrote:
> Ping. re: 
> https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html 
> <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 
> <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

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

[Jason Merrill]

On 1/4/21 9:58 AM, Jeff Chapman wrote:
> Ping. re: 
> https://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html 
> <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 
> <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", contract_attrs))
> +    error_at (EXPR_LOCATION (TREE_VALUE (pre)),
> +             "preconditions cannot be statements");
> +  else if (tree post = lookup_attribute ("post", contract_attrs))
> +    error_at (EXPR_LOCATION (TREE_VALUE (post)),
> +             "postconditions cannot be statements");
> +
> +  /* FIXME: move fallthrough up here so it applies to decls/etc?  */

That would make sense.

> +  temp_override<tree> saved_cct(current_class_type);
> +  temp_override<tree> saved_ccp(current_class_ptr);
> +  temp_override<tree> saved_ccr(current_class_ref);

Are these actually needed?

> +++ b/gcc/tree-inline.c
> @@ -31,6 +31,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "ssa.h"
>  #include "cgraph.h"
>  #include "tree-pretty-print.h"
> +#include "cp/cp-tree.h"

Can't include cp-tree.h in a language-independent file.  We already call 
the tree-inline machinery from the front-end in clone_body; can't you do 
the same for contracts?

> +build_arg_list (tree fn)
> +{
> +  gcc_assert (TREE_CODE (fn) == FUNCTION_DECL);
> +
> +  vec<tree, va_gc> *args = make_tree_vector ();
> +  for (tree t = DECL_ARGUMENTS (fn); t != NULL_TREE; t = TREE_CHAIN (t))
> +    {
> +      if (TREE_CODE (TREE_TYPE (t)) == POINTER_TYPE
> +         && DECL_NAME (t) != NULL_TREE
> +         && IDENTIFIER_POINTER (DECL_NAME (t)) != NULL
> +         && id_equal (DECL_NAME (t), "this"))

You can use is_this_parameter here.

> +             /* 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: are we sure we shouldn't just mangle or use some existing machinery?
> +static const char *
> +get_contracts_internal_decl_name (const char *ident)

Definitely use the mangling machinery.

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

More soon.

Jason

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

[Jeff Chapman]

On 1/18/21, Jason Merrill <jason@redhat.com> 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://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
>>
>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>> <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]

On 3/1/21 8:12 AM, Jeff Chapman wrote:
> On 1/18/21, Jason Merrill <jason@redhat.com> 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://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
>>>
>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>> <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]

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

On 3/25/21, Jason Merrill <jason@redhat.com> wrote:
> On 3/1/21 8:12 AM, Jeff Chapman wrote:
>> On 1/18/21, Jason Merrill <jason@redhat.com> 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://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
>>>>
>>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>>> <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]

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 <jason@redhat.com> wrote:
>> On 3/1/21 8:12 AM, Jeff Chapman wrote:
>>> On 1/18/21, Jason Merrill <jason@redhat.com> 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://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
>>>>>
>>>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>>>> <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. */
>> +         && !alias
>> +         && flag_contract_strict_declarations)
>> +       warning_at (declarator->id_loc, OPT_fcontract_strict_declarations_,
>> +                   "declaration of %q#D outside of class is not definition",
>> +                   decl);

Let's share the common predicates.

>> +  /* FIXME: move fallthrough up here so it applies to decls/etc?  */

That seems unnecessary, we already warn and ignore fallthrough on a decl.

>> +int cp_contract_operand = 0;
>> +tree cp_contract_return_value = NULL_TREE;

These need to be saved/restored by push_to/pop_from_top_level somehow, 
so that they don't stay set when parsing the contract triggers template 
instantiation.  The best way is probably to add them to struct saved_scope.

> +      token = cp_lexer_peek_token (parser->lexer);
> +      if (token->type == CPP_NAME)
> +       {
> +         attr_name = token->u.value;
> +         attr_name = canonicalize_attr_name (attr_name);
> +       }
> +
> +      /* Handle contract-attribute-specs specially.  */
> +      if (attr_name && contract_attribute_p (attr_name))
> +       {
> +         attributes
> +           = cp_parser_contract_attribute_spec (parser, attr_name);
> +         goto finish_attrs;
> +       }

Let's move the name checking inside cp_parser_contract_attribute_spec, 
and make that function return null for a non-contract attribute.

> +  /* FIXME we can do this a lot more efficiently? Once per function for all of
> +   * its pre contracts, and then once per post contract? Is there an
> +   * appreciable difference? Or a way to simply rename the post ret val parm? *

You might want to share the context handling with late parsing of noexcept.

> +  /* Don't do access checking if it is a templated function.  */
> +  if (processing_template_decl)
> +    push_deferring_access_checks (dk_no_check);

Don't we already defer access checking for templated functions?

> +/* Return a copy of the FUNCTION_DECL IDECL with its own unshared 
> +   PARM_DECL and DECL_ATTRIBUTEs.  */
> +
> +static tree
> +copy_fn_decl (tree idecl)

Can you use, or at least share code with, the existing 
copy_fndecl_with_name?

> +  /* FIXME will later optimizations delete unused args to prevent extra arg
> +   * passing? do we care? */

It might be possible to leverage the optimizer's partial-inlining code 
for this, I don't know.  Probably not worth messing with now.

More soon.

Jason

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

[Marek Polacek]

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]

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 <jason@redhat.com> wrote:
>>> On 3/1/21 8:12 AM, Jeff Chapman wrote:
>>>> On 1/18/21, Jason Merrill <jason@redhat.com> 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://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
>>>>>>
>>>>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>>>>> <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. */
>>> +         && !alias
>>> +         && flag_contract_strict_declarations)
>>> +       warning_at (declarator->id_loc, 
>>> OPT_fcontract_strict_declarations_,
>>> +                   "declaration of %q#D outside of class is not 
>>> definition",
>>> +                   decl);
> 
> Let's share the common predicates.
> 
>>> +  /* FIXME: move fallthrough up here so it applies to decls/etc?  */
> 
> That seems unnecessary, we already warn and ignore fallthrough on a decl.
> 
>>> +int cp_contract_operand = 0;
>>> +tree cp_contract_return_value = NULL_TREE;
> 
> These need to be saved/restored by push_to/pop_from_top_level somehow, 
> so that they don't stay set when parsing the contract triggers template 
> instantiation.  The best way is probably to add them to struct saved_scope.
> 
>> +      token = cp_lexer_peek_token (parser->lexer);
>> +      if (token->type == CPP_NAME)
>> +       {
>> +         attr_name = token->u.value;
>> +         attr_name = canonicalize_attr_name (attr_name);
>> +       }
>> +
>> +      /* Handle contract-attribute-specs specially.  */
>> +      if (attr_name && contract_attribute_p (attr_name))
>> +       {
>> +         attributes
>> +           = cp_parser_contract_attribute_spec (parser, attr_name);
>> +         goto finish_attrs;
>> +       }
> 
> Let's move the name checking inside cp_parser_contract_attribute_spec, 
> and make that function return null for a non-contract attribute.
> 
>> +  /* FIXME we can do this a lot more efficiently? Once per function 
>> for all of
>> +   * its pre contracts, and then once per post contract? Is there an
>> +   * appreciable difference? Or a way to simply rename the post ret 
>> val parm? *
> 
> You might want to share the context handling with late parsing of noexcept.
> 
>> +  /* Don't do access checking if it is a templated function.  */
>> +  if (processing_template_decl)
>> +    push_deferring_access_checks (dk_no_check);
> 
> Don't we already defer access checking for templated functions?
> 
>> +/* Return a copy of the FUNCTION_DECL IDECL with its own unshared +   
>> PARM_DECL and DECL_ATTRIBUTEs.  */
>> +
>> +static tree
>> +copy_fn_decl (tree idecl)
> 
> Can you use, or at least share code with, the existing 
> copy_fndecl_with_name?
> 
>> +  /* FIXME will later optimizations delete unused args to prevent 
>> extra arg
>> +   * passing? do we care? */
> 
> It might be possible to leverage the optimizer's partial-inlining code 
> for this, I don't know.  Probably not worth messing with now.
> 
> More soon.

More:

> +       if (semantic == CCS_ASSUME && !cp_tree_defined_p (c))
> +         break;

Another approach to this would be to evaluate an assume with a separate 
non_constant_p and within the lifetime of a 
uid_sensitive_constexpr_evaluation_sentinel.  But the way you have it is 
fine.

> +  /* Ignored contracts are never checked or assumed.  */
> +  if (semantic == CCS_IGNORE)
> +    return build1 (NOP_EXPR, void_type_node, integer_zero_node);

Another place you can use void_node.

> +static void
> +build_contract_handler_fn (tree contract,
> +                          contract_continuation cmode)

This function needs a comment, and a better name.  Maybe just 
build_contract_handler_fn_call.

> +/* Rewrite the post function decl of FNDECL, replacing the original undeduced
> +   return type with RETURN_TYPE.  */
> +
> +static void
> +apply_post_deduced_return_type (tree fndecl, tree return_type)

Can this share more code with apply_deduced_return_type?

> +  if (needs_post && DECL_POST_FN (current_function_decl) != error_mark_node)
> +    {
> +      vec<tree, va_gc> *args = build_arg_list (current_function_decl);
> +      if (DECL_UNCHECKED_RESULT (current_function_decl))
> +       vec_safe_push (args, expr); // FIXME do we need forward_parm or similar?
> +
> +      if (undeduced_auto_decl (DECL_POST_FN (current_function_decl)))
> +       apply_post_deduced_return_type (current_function_decl,
> +                                       TREE_TYPE (expr));
> +
> +      push_deferring_access_checks (dk_no_check);
> +      tree call = finish_call_expr (DECL_POST_FN (current_function_decl), &args,
> +                                   /*disallow_virtual=*/true,
> +                                   /*koenig_p=*/false,
> +                                   /*complain=*/tf_warning_or_error);
> +      gcc_assert (call != error_mark_node);
> +      pop_deferring_access_checks ();
> +
> +      /* Replace returned expression with call to post function.  */
> +      expr = call;
> +    }

This looks like you pass the returned expression as an argument to the 
post function instead of using it to initialize the return value.  That 
will break copy elision.

What if instead (for a class return type at least) you initialize the 
return value, then pass a reference to the return value to the post 
function?  Then you could also share the call to the post function 
between all returns.

> +       if (tree check = build_contract_check (stmt))
> +         {
> +           /* Mark the current function as possibly throwing exceptions
> +              (through invocation of the contract violation handler).  */
> +           current_function_returns_abnormally = 1;
> +           TREE_NOTHROW (current_function_decl) = 0;

These shouldn't be set here, they should be set as part of building the 
call to the handler.  You should get that by changing build_call_expr to 
build_call_n...

> +  tree call = build_call_expr (violation_fn, 8, continue_mode, line_number,
> +                              file_name, function_name, comment,
> +                              level_str, role_str,
> +                              continuation);

...here.

> +  /* FIXME: It looks  like we have two bits of information for
> +     continuing.  Is this right?  */

I think this is no longer true since you dropped "always continue".

> +  char buf[1024 + 4]{};
> +  char *res = buf;
> +  size_t len = 1024;

This seems like a good place to use obstacks.

> +      int s = expstart.column - 1;
> +      int l = expend.column - expstart.column + 1;

So, l = expend.column - s?

> +++ b/libstdc++-v3/src/c++17/contract.cc

Hmm.  Contracts library support was in clause 17, which would mean this 
should go in libstdc++-v3/libsupc++ instead.  But it depends on 
string_view, which is not in clause 17.  This seems like a problem in 
the standardese.

> +      /* If this is the pre/post function for a guarded function, append
> +        pre/post in the vendor specific portion of the mangling.
> +
> +        TODO: this likely needs standardizing.
> +        TODO: do we need special handling in other tools like the demangler? */
> +      if (DECL_IS_PRE_FN_P (decl))
> +       write_string (".pre");
> +      else if (DECL_IS_POST_FN_P (decl))
> +       write_string (".post");

Do you have in mind that these functions are part of the ABI, or 
internal implementation details of the functions?  If the former, we 
need to give them a proper mangling.  If the latter, we can make them 
hidden symbols in the same COMDAT section with their guarded function 
like with constructor variants, and their mangling isn't important.  I'm 
still wondering why you split them out from the guarded function in the 
first place.

> +      remap_overrider_contracts (overrider, basefn);
> +      remap_overrider_contracts (DECL_PRE_FN (overrider), DECL_PRE_FN (basefn));
> +      remap_overrider_contracts (DECL_POST_FN (overrider), DECL_POST_FN (basefn));

Why three times?  Why copy the contracts from the guarded function to 
the pre/post functions?

> +/* Assertion role info.  */

This needs elaboration about what a role is.

A lot of the code in decl.c could move to cxx-contracts.c.

Jason

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

[Jeff Chapman]

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 <jason@redhat.com> 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 <jason@redhat.com> wrote:
>>>> On 3/1/21 8:12 AM, Jeff Chapman wrote:
>>>>> On 1/18/21, Jason Merrill <jason@redhat.com> 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://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
>>>>>>>
>>>>>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>>>>>> <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
>>>> +          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?

Because the pre/post FUNCTION_DECLs are in a parallel map rather than being a
part of the original FUNCTION_DECL itself, this was to prevent the potentially
reused newdecl from already being associated with the wrong functions if it
also had contracts. Moot now though :)

>>
>>>>        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_,
>>>> +                   "declaration of %q#D outside of class is not
>>>> definition",
>>>> +                   decl);
>>
>> Let's share the common predicates.
>>
>>>> +  /* FIXME: move fallthrough up here so it applies to decls/etc?  */
>>
>> That seems unnecessary, we already warn and ignore fallthrough on a decl.
>>
>>>> +int cp_contract_operand = 0;
>>>> +tree cp_contract_return_value = NULL_TREE;
>>
>> These need to be saved/restored by push_to/pop_from_top_level somehow,
>> so that they don't stay set when parsing the contract triggers template
>> instantiation.  The best way is probably to add them to struct
>> saved_scope.
>>
>>> +      token = cp_lexer_peek_token (parser->lexer);
>>> +      if (token->type == CPP_NAME)
>>> +       {
>>> +         attr_name = token->u.value;
>>> +         attr_name = canonicalize_attr_name (attr_name);
>>> +       }
>>> +
>>> +      /* Handle contract-attribute-specs specially.  */
>>> +      if (attr_name && contract_attribute_p (attr_name))
>>> +       {
>>> +         attributes
>>> +           = cp_parser_contract_attribute_spec (parser, attr_name);
>>> +         goto finish_attrs;
>>> +       }
>>
>> Let's move the name checking inside cp_parser_contract_attribute_spec,
>> and make that function return null for a non-contract attribute.
>>
>>> +  /* FIXME we can do this a lot more efficiently? Once per function
>>> for all of
>>> +   * its pre contracts, and then once per post contract? Is there an
>>> +   * appreciable difference? Or a way to simply rename the post ret
>>> val parm? *
>>
>> You might want to share the context handling with late parsing of
>> noexcept.
>>
>>> +  /* Don't do access checking if it is a templated function.  */
>>> +  if (processing_template_decl)
>>> +    push_deferring_access_checks (dk_no_check);
>>
>> Don't we already defer access checking for templated functions?
>>
>>> +/* Return a copy of the FUNCTION_DECL IDECL with its own unshared +
>>> PARM_DECL and DECL_ATTRIBUTEs.  */
>>> +
>>> +static tree
>>> +copy_fn_decl (tree idecl)
>>
>> Can you use, or at least share code with, the existing
>> copy_fndecl_with_name?
>>
>>> +  /* FIXME will later optimizations delete unused args to prevent
>>> extra arg
>>> +   * passing? do we care? */
>>
>> It might be possible to leverage the optimizer's partial-inlining code
>> for this, I don't know.  Probably not worth messing with now.
>>
>> More soon.
>
> More:
>
>> +       if (semantic == CCS_ASSUME && !cp_tree_defined_p (c))
>> +         break;
>
> Another approach to this would be to evaluate an assume with a separate
> non_constant_p and within the lifetime of a
> uid_sensitive_constexpr_evaluation_sentinel.  But the way you have it is
> fine.
>
>> +  /* Ignored contracts are never checked or assumed.  */
>> +  if (semantic == CCS_IGNORE)
>> +    return build1 (NOP_EXPR, void_type_node, integer_zero_node);
>
> Another place you can use void_node.
>
>> +static void
>> +build_contract_handler_fn (tree contract,
>> +                          contract_continuation cmode)
>
> This function needs a comment, and a better name.  Maybe just
> build_contract_handler_fn_call.
>
>> +/* Rewrite the post function decl of FNDECL, replacing the original
>> undeduced
>> +   return type with RETURN_TYPE.  */
>> +
>> +static void
>> +apply_post_deduced_return_type (tree fndecl, tree return_type)
>
> Can this share more code with apply_deduced_return_type?
>
>> +  if (needs_post && DECL_POST_FN (current_function_decl) !=
>> error_mark_node)
>> +    {
>> +      vec<tree, va_gc> *args = build_arg_list (current_function_decl);
>> +      if (DECL_UNCHECKED_RESULT (current_function_decl))
>> +       vec_safe_push (args, expr); // FIXME do we need forward_parm or
>> similar?
>> +
>> +      if (undeduced_auto_decl (DECL_POST_FN (current_function_decl)))
>> +       apply_post_deduced_return_type (current_function_decl,
>> +                                       TREE_TYPE (expr));
>> +
>> +      push_deferring_access_checks (dk_no_check);
>> +      tree call = finish_call_expr (DECL_POST_FN (current_function_decl),
>> &args,
>> +                                   /*disallow_virtual=*/true,
>> +                                   /*koenig_p=*/false,
>> +                                   /*complain=*/tf_warning_or_error);
>> +      gcc_assert (call != error_mark_node);
>> +      pop_deferring_access_checks ();
>> +
>> +      /* Replace returned expression with call to post function.  */
>> +      expr = call;
>> +    }
>
> This looks like you pass the returned expression as an argument to the
> post function instead of using it to initialize the return value.  That
> will break copy elision.
>
> What if instead (for a class return type at least) you initialize the
> return value, then pass a reference to the return value to the post
> function?  Then you could also share the call to the post function
> between all returns.
>
>> +       if (tree check = build_contract_check (stmt))
>> +         {
>> +           /* Mark the current function as possibly throwing exceptions
>> +              (through invocation of the contract violation handler).
>> */
>> +           current_function_returns_abnormally = 1;
>> +           TREE_NOTHROW (current_function_decl) = 0;
>
> These shouldn't be set here, they should be set as part of building the
> call to the handler.  You should get that by changing build_call_expr to
> build_call_n...
>
>> +  tree call = build_call_expr (violation_fn, 8, continue_mode,
>> line_number,
>> +                              file_name, function_name, comment,
>> +                              level_str, role_str,
>> +                              continuation);
>
> ...here.
>
>> +  /* FIXME: It looks  like we have two bits of information for
>> +     continuing.  Is this right?  */
>
> I think this is no longer true since you dropped "always continue".
>
>> +  char buf[1024 + 4]{};
>> +  char *res = buf;
>> +  size_t len = 1024;
>
> This seems like a good place to use obstacks.
>
>> +      int s = expstart.column - 1;
>> +      int l = expend.column - expstart.column + 1;
>
> So, l = expend.column - s?
>

Changes to get_source to use obstacks and cleanup some of the math are also
sitting on a branch and will be cherry-picked once the rewrite is stable.

>> +++ b/libstdc++-v3/src/c++17/contract.cc
>
> Hmm.  Contracts library support was in clause 17, which would mean this
> should go in libstdc++-v3/libsupc++ instead.  But it depends on
> string_view, which is not in clause 17.  This seems like a problem in
> the standardese.
>
>> +      /* If this is the pre/post function for a guarded function, append
>> +        pre/post in the vendor specific portion of the mangling.
>> +
>> +        TODO: this likely needs standardizing.
>> +        TODO: do we need special handling in other tools like the
>> demangler? */
>> +      if (DECL_IS_PRE_FN_P (decl))
>> +       write_string (".pre");
>> +      else if (DECL_IS_POST_FN_P (decl))
>> +       write_string (".post");
>
> Do you have in mind that these functions are part of the ABI, or
> internal implementation details of the functions?  If the former, we
> need to give them a proper mangling.  If the latter, we can make them
> hidden symbols in the same COMDAT section with their guarded function
> like with constructor variants, and their mangling isn't important.  I'm
> still wondering why you split them out from the guarded function in the
> first place.
>

Ditto. These are currently internal implementation details so they've been made
hidden. Future extensions to contracts might result in this being revisited.

>> +      remap_overrider_contracts (overrider, basefn);
>> +      remap_overrider_contracts (DECL_PRE_FN (overrider), DECL_PRE_FN
>> (basefn));
>> +      remap_overrider_contracts (DECL_POST_FN (overrider), DECL_POST_FN
>> (basefn));
>
> Why three times?  Why copy the contracts from the guarded function to
> the pre/post functions?

This is fixed in the rewrite branch.

>
>> +/* Assertion role info.  */
>
> This needs elaboration about what a role is.
>
> A lot of the code in decl.c could move to cxx-contracts.c.
>
> Jason
>
>

Please let us know if there are other issues, or if it sounds like we're not
headed in the correct direction.

Thank you,
Jeff Chapman

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

[Jason Merrill]

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 <jchapman@lock3software.com> 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 <jason@redhat.com> 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 <jason@redhat.com> wrote:
>>>>>> On 3/1/21 8:12 AM, Jeff Chapman wrote:
>>>>>>> On 1/18/21, Jason Merrill <jason@redhat.com> 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://gcc.gnu.org/pipermail/gcc-patches/2020-December/561135.html>
>>>>>>>>>
>>>>>>>>> https://github.com/lock3/gcc/tree/contracts-jac-alt
>>>>>>>>> <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
>>>>>> +          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?
>>
>> Because the pre/post FUNCTION_DECLs are in a parallel map rather than being a
>> part of the original FUNCTION_DECL itself, this was to prevent the potentially
>> reused newdecl from already being associated with the wrong functions if it
>> also had contracts. Moot now though :)
>>
>>>>
>>>>>>         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_,
>>>>>> +                   "declaration of %q#D outside of class is not
>>>>>> definition",
>>>>>> +                   decl);
>>>>
>>>> Let's share the common predicates.
>>>>
>>>>>> +  /* FIXME: move fallthrough up here so it applies to decls/etc?  */
>>>>
>>>> That seems unnecessary, we already warn and ignore fallthrough on a decl.
>>>>
>>>>>> +int cp_contract_operand = 0;
>>>>>> +tree cp_contract_return_value = NULL_TREE;
>>>>
>>>> These need to be saved/restored by push_to/pop_from_top_level somehow,
>>>> so that they don't stay set when parsing the contract triggers template
>>>> instantiation.  The best way is probably to add them to struct
>>>> saved_scope.
>>>>
>>>>> +      token = cp_lexer_peek_token (parser->lexer);
>>>>> +      if (token->type == CPP_NAME)
>>>>> +       {
>>>>> +         attr_name = token->u.value;
>>>>> +         attr_name = canonicalize_attr_name (attr_name);
>>>>> +       }
>>>>> +
>>>>> +      /* Handle contract-attribute-specs specially.  */
>>>>> +      if (attr_name && contract_attribute_p (attr_name))
>>>>> +       {
>>>>> +         attributes
>>>>> +           = cp_parser_contract_attribute_spec (parser, attr_name);
>>>>> +         goto finish_attrs;
>>>>> +       }
>>>>
>>>> Let's move the name checking inside cp_parser_contract_attribute_spec,
>>>> and make that function return null for a non-contract attribute.
>>>>
>>>>> +  /* FIXME we can do this a lot more efficiently? Once per function
>>>>> for all of
>>>>> +   * its pre contracts, and then once per post contract? Is there an
>>>>> +   * appreciable difference? Or a way to simply rename the post ret
>>>>> val parm? *
>>>>
>>>> You might want to share the context handling with late parsing of
>>>> noexcept.
>>>>
>>>>> +  /* Don't do access checking if it is a templated function.  */
>>>>> +  if (processing_template_decl)
>>>>> +    push_deferring_access_checks (dk_no_check);
>>>>
>>>> Don't we already defer access checking for templated functions?
>>>>
>>>>> +/* Return a copy of the FUNCTION_DECL IDECL with its own unshared +
>>>>> PARM_DECL and DECL_ATTRIBUTEs.  */
>>>>> +
>>>>> +static tree
>>>>> +copy_fn_decl (tree idecl)
>>>>
>>>> Can you use, or at least share code with, the existing
>>>> copy_fndecl_with_name?
>>>>
>>>>> +  /* FIXME will later optimizations delete unused args to prevent
>>>>> extra arg
>>>>> +   * passing? do we care? */
>>>>
>>>> It might be possible to leverage the optimizer's partial-inlining code
>>>> for this, I don't know.  Probably not worth messing with now.
>>>>
>>>> More soon.
>>>
>>> More:
>>>
>>>> +       if (semantic == CCS_ASSUME && !cp_tree_defined_p (c))
>>>> +         break;
>>>
>>> Another approach to this would be to evaluate an assume with a separate
>>> non_constant_p and within the lifetime of a
>>> uid_sensitive_constexpr_evaluation_sentinel.  But the way you have it is
>>> fine.
>>>
>>>> +  /* Ignored contracts are never checked or assumed.  */
>>>> +  if (semantic == CCS_IGNORE)
>>>> +    return build1 (NOP_EXPR, void_type_node, integer_zero_node);
>>>
>>> Another place you can use void_node.
>>>
>>>> +static void
>>>> +build_contract_handler_fn (tree contract,
>>>> +                          contract_continuation cmode)
>>>
>>> This function needs a comment, and a better name.  Maybe just
>>> build_contract_handler_fn_call.
>>>
>>>> +/* Rewrite the post function decl of FNDECL, replacing the original
>>>> undeduced
>>>> +   return type with RETURN_TYPE.  */
>>>> +
>>>> +static void
>>>> +apply_post_deduced_return_type (tree fndecl, tree return_type)
>>>
>>> Can this share more code with apply_deduced_return_type?
>>>
>>>> +  if (needs_post && DECL_POST_FN (current_function_decl) !=
>>>> error_mark_node)
>>>> +    {
>>>> +      vec<tree, va_gc> *args = build_arg_list (current_function_decl);
>>>> +      if (DECL_UNCHECKED_RESULT (current_function_decl))
>>>> +       vec_safe_push (args, expr); // FIXME do we need forward_parm or
>>>> similar?
>>>> +
>>>> +      if (undeduced_auto_decl (DECL_POST_FN (current_function_decl)))
>>>> +       apply_post_deduced_return_type (current_function_decl,
>>>> +                                       TREE_TYPE (expr));
>>>> +
>>>> +      push_deferring_access_checks (dk_no_check);
>>>> +      tree call = finish_call_expr (DECL_POST_FN (current_function_decl),
>>>> &args,
>>>> +                                   /*disallow_virtual=*/true,
>>>> +                                   /*koenig_p=*/false,
>>>> +                                   /*complain=*/tf_warning_or_error);
>>>> +      gcc_assert (call != error_mark_node);
>>>> +      pop_deferring_access_checks ();
>>>> +
>>>> +      /* Replace returned expression with call to post function.  */
>>>> +      expr = call;
>>>> +    }
>>>
>>> This looks like you pass the returned expression as an argument to the
>>> post function instead of using it to initialize the return value.  That
>>> will break copy elision.
>>>
>>> What if instead (for a class return type at least) you initialize the
>>> return value, then pass a reference to the return value to the post
>>> function?  Then you could also share the call to the post function
>>> between all returns.
>>>
>>>> +       if (tree check = build_contract_check (stmt))
>>>> +         {
>>>> +           /* Mark the current function as possibly throwing exceptions
>>>> +              (through invocation of the contract violation handler).
>>>> */
>>>> +           current_function_returns_abnormally = 1;
>>>> +           TREE_NOTHROW (current_function_decl) = 0;
>>>
>>> These shouldn't be set here, they should be set as part of building the
>>> call to the handler.  You should get that by changing build_call_expr to
>>> build_call_n...
>>>
>>>> +  tree call = build_call_expr (violation_fn, 8, continue_mode,
>>>> line_number,
>>>> +                              file_name, function_name, comment,
>>>> +                              level_str, role_str,
>>>> +                              continuation);
>>>
>>> ...here.
>>>
>>>> +  /* FIXME: It looks  like we have two bits of information for
>>>> +     continuing.  Is this right?  */
>>>
>>> I think this is no longer true since you dropped "always continue".
>>>
>>>> +  char buf[1024 + 4]{};
>>>> +  char *res = buf;
>>>> +  size_t len = 1024;
>>>
>>> This seems like a good place to use obstacks.
>>>
>>>> +      int s = expstart.column - 1;
>>>> +      int l = expend.column - expstart.column + 1;
>>>
>>> So, l = expend.column - s?
>>>
>>
>> Changes to get_source to use obstacks and cleanup some of the math are also
>> sitting on a branch and will be cherry-picked once the rewrite is stable.
>>
>>>> +++ b/libstdc++-v3/src/c++17/contract.cc
>>>
>>> Hmm.  Contracts library support was in clause 17, which would mean this
>>> should go in libstdc++-v3/libsupc++ instead.  But it depends on
>>> string_view, which is not in clause 17.  This seems like a problem in
>>> the standardese.
>>>
>>>> +      /* If this is the pre/post function for a guarded function, append
>>>> +        pre/post in the vendor specific portion of the mangling.
>>>> +
>>>> +        TODO: this likely needs standardizing.
>>>> +        TODO: do we need special handling in other tools like the
>>>> demangler? */
>>>> +      if (DECL_IS_PRE_FN_P (decl))
>>>> +       write_string (".pre");
>>>> +      else if (DECL_IS_POST_FN_P (decl))
>>>> +       write_string (".post");
>>>
>>> Do you have in mind that these functions are part of the ABI, or
>>> internal implementation details of the functions?  If the former, we
>>> need to give them a proper mangling.  If the latter, we can make them
>>> hidden symbols in the same COMDAT section with their guarded function
>>> like with constructor variants, and their mangling isn't important.  I'm
>>> still wondering why you split them out from the guarded function in the
>>> first place.
>>>
>>
>> Ditto. These are currently internal implementation details so they've been made
>> hidden. Future extensions to contracts might result in this being revisited.
>>
>>>> +      remap_overrider_contracts (overrider, basefn);
>>>> +      remap_overrider_contracts (DECL_PRE_FN (overrider), DECL_PRE_FN
>>>> (basefn));
>>>> +      remap_overrider_contracts (DECL_POST_FN (overrider), DECL_POST_FN
>>>> (basefn));
>>>
>>> Why three times?  Why copy the contracts from the guarded function to
>>> the pre/post functions?
>>
>> This is fixed in the rewrite branch.
>>
>>>
>>>> +/* Assertion role info.  */
>>>
>>> This needs elaboration about what a role is.
>>>
>>> A lot of the code in decl.c could move to cxx-contracts.c.
>>>
>>> Jason
>>>
>>>
>>
>> Please let us know if there are other issues, or if it sounds like we're not
>> headed in the correct direction.
>>
>> Thank you,
>> Jeff Chapman

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

[Andrew Sutton]

> > 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]

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

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?

[-- Attachment #2: 0001-assume-cx.patch --]
[-- Type: text/x-patch, Size: 2475 bytes --]

From 85400e1896a188892b1ebeb0c8e86ff3cd28cfa6 Mon Sep 17 00:00:00 2001
From: Jason Merrill <jason@redhat.com>
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 (&new_ctx, c, false,
+					      &dummy_nc, &dummy_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 00000000000..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]

I think so, yes.

On Fri, Jul 2, 2021 at 11:09 AM Jason Merrill <jason@redhat.com> 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?

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

[Jason Merrill]

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 <contract> 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 <experimental/contract> 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: contracts library support (was Re: [PATCH] PING implement pre-c++20 contracts)

[Jason Merrill]

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 <contract> 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 <experimental/contract> 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

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

[Jonathan Wakely]

On Mon, 5 Jul 2021 at 20:07, Jason Merrill <jason@redhat.com> 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 <contract> 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<char> 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 <experimental/contract> for now.

Agreed.

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

[Jason Merrill]

On 7/12/21 3:58 PM, Jonathan Wakely wrote:
> On Mon, 5 Jul 2021 at 20:07, Jason Merrill <jason@redhat.com> 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 <contract> 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<char> 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 <experimental/contract> 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]

On Wed, 14 Jul 2021 at 04:56, Jason Merrill <jason@redhat.com> wrote:
>
> On 7/12/21 3:58 PM, Jonathan Wakely wrote:
> > On Mon, 5 Jul 2021 at 20:07, Jason Merrill <jason@redhat.com> 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 <contract> 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<char> 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 <experimental/contract> 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
<experimental/filesystem> 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)

[Andrew Sutton]

> 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