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