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 <ja...@redhat.com> wrote:
On 3/1/21 8:12 AM, Jeff Chapman wrote:
On 1/18/21, Jason Merrill <ja...@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
