On 7/10/20 1:53 PM, Jeff Chapman 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
I'm starting to review this now, sorry for the delay. Is this still the
branch you want me to consider for GCC 11? I notice that the -constexpr
and -mangled-config branches are newer.
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.
