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 (tiem...@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
