[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