Ping. Any feedback would be appreciated :) re: https://gcc.gnu.org/pipermail/gcc-patches/2020-July/549868.html older reply: https://gcc.gnu.org/pipermail/gcc-patches/2020-May/545339.html
On 7/10/20, Jeff Chapman <jchap...@lock3software.com> wrote: > Hello again :) > > Attached is a new squashed revision of the patch sans ChangeLogs. The > current work is now being done on github: > https://github.com/lock3/gcc/tree/contracts-jac-alt > > Please let me know if there's a better way to share revisions. > >>>> + /* 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. > > Improvements have been made to handling contract attributes appearing in > places they don't belong. Any feedback about moving the logic dealing > with [[fallthrough]]? > > >>> 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. > > This is the main change since the last submission. We now leave the > original decl intact and instead generate a pre and post function. > Naming and mangling of these pre/post functions needs addressed. > > Beyond that, more cleanup has been done. There's still outstanding > cleanup work, mostly around investigating and improving performance. > Feedback on the main direction of the patch would be appreciated, and > any specific commentary or directions of investigation would be helpful. > > >>>> +/* 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? >> >> ...snip... >> >>>> + /* 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. >> > > Not much has changed yet regarding axiom, but if you have any > suggestions for handling any of the above then I'm all ears. >
