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 <jchap...@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 <ja...@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 <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.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.ccHmm. 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. JasonPlease 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
