Hello, On 12/10/19, Jason Merrill wrote: > On 11/13/19, 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. >
I've attached a new squashed revision of the patch, and you can see the changes I've made from your input on the contracts-jac-prep branch: https://gitlab.com/lock3/gcc-new/-/tree/contracts-jac-prep . If there's an easier format for you to review that I can produce please let me know. I'll address a few things inline below. Everything else should either be handled or explained by Andrew's last email. If anything needs further addressing or something hasn't been brought up yet please let me know :) >> +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. > These could possibly be made compatible, but in some cases the flags are changing the same entries in the table. That would require deciding whether flag ordering matters or whether a certain flags can't change values set by other flags. I'm not sure it's a worthwhile change. While it increases the valid space of command line invocations, it doesn't actually increase the the result space. I'd prefer an eventual solution that removed flags entirely instead. >> + /* 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. > I'm wondering if it would be better to move [[fallthrough]] up, since the later check is not always executed and in the case of [[assert]] we actually need to error. [[fallthrough]] int x; for instance just gets a generic 'attribute ignored' warning. I'm not entirely happy with how we prevent assert/pre/post from appearing in invalid locations in general which I'll try to improve. If you have concrete suggestions please let me know. > 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? > This revision of the patch does not include changes to the checked/unchecked function split. We're exploring an alternative rewrite that leaves the original function declaration alone and should address or sidestep a number of these comments. Specifically, we're exploring generating pre and post functions with calls to them in the correct places (upon entering a guarded function, wrapping the return value): int f(int n) [[ pre: n > 0 ]] [[ post r: r < 0 ]] { return -n; } turns into void __pre_f(int n) { [[ assert: n > 0 ]]; } int __post_f(int r) { [[ assert: r < 0 ]]; return r; } int f(int n) { __pre_f(n); return __post_f(-n); } with whatever hints we can give to optimize this as much as possible. >> +/* 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 > This has been moved to input since it uses input functions, but needs more work. Was there another comment you had that cutoff? >> + 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? > These are used separately in std::contract_violation and to my understanding building them separately will collapse duplicate levels and roles instead of duplicating them unless they both match -- is that correct? >> + /* 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? > This is an result of the transform we do for axioms when they're enabled that you may know of a better way to do. Essentially we turn this: [[ assert axiom: f() > 0 ]]; into this: if (!(f() > 0)) __builtin_unreachable (); but we potentially run into issues later if f isn't (yet) defined or is defined in a different translation unit, such as constexpr time. We're avoiding those errors by generating a nop if there's any chance can't be evaluated. We'd prefer something like __builtin_assume (f() > 0); where the condition is simply ignored if enough information isn't present to affect codegen. Is there a better way to handle this? I should mention that there are likely significant further changes around axiom coming down the pike that may tie into or replace the "is defined" check. Specifically for expressions that _cannot_ be defined rather than ones that are simply not yet defined.
0001-Implement-pre-c-20-contracts.patch.gz
Description: GNU Zip compressed data
