On 7/12/24 1:03 PM, Iain Sandoe wrote:
HI Jason,
On 9 Jul 2024, at 22:55, Jason Merrill <ja...@redhat.com> wrote:
On 7/9/24 11:52 AM, Iain Sandoe wrote:
Hi Folks
On 8 Jul 2024, at 20:57, Jason Merrill <ja...@redhat.com> wrote:
On 7/8/24 3:37 PM, Iain Sandoe wrote:
On 8 Jul 2024, at 20:19, Jason Merrill <ja...@redhat.com> wrote:
On 6/17/24 8:15 AM, Iain Sandoe wrote:
potentially_transformed_function_body ();
}
finally
{
handle_post_condition_checking ();
}
else [only if the function is not marked noexcept(true) ]
{
__rethrow ();
This sounds undesirable, EH normally continues unwinding after running a
cleanup. Why would the exception be considered caught in a way that needs a
rethrow?
We need to implement that the no-EH cleanup [i.e. the postcondition] does _not_
run when an exception is thrown. Right now there’s no HANDLER type that does
that - we have EH_ONLY but no NON_EH one (otherwise I'd have built a C++ try).
however, I could not figure out how to make this arm of the EH_ELSE_EXPR work
as an empty construct (the gimplifier does not seem to expect this).
You can't just put void_node in the else arm?
I had tried a number of permutations (void_node, empty_stmt, empty statement
list etc). Unfortunately, at present that causes the EH_ELSE expression to be
silently dropped in gimplication. The fact that you think it should work (as
did i) makes me think either we have a gimplifier bug or a misunderstanding:
@Alex;
we have (gcc/gimplify.cc:18423):
if (!gimple_seq_empty_p (n) && !gimple_seq_empty_p (e))
{
geh_else *stmt = gimple_build_eh_else (n, e);
gimple_seq_add_stmt (&cleanup, stmt);
}
Which essentially says “if either of the sub-expressions to this are empty,
then do not build it”
Was there a reason for this, or is it a typo?
If I replace ‘&&' by ‘||' (i.e. “if either of the sub-expressions is present,
then build it” then things behave as I expect.
IMO it the current action _is_ intended
(a) it should at least emit a checking diagnostic
(b) we need to figure out how to extend it so that we can implement what’s
needed (non-EH only cleanups)
So I patched the gimplifier as above and initial testing says that this change
does not cause any C++ regressions - but I need to do Ada, Objective-C etc too.
thoughts?
That sounds good to me, it does seem like a typo.
Here is the revised patch - which implements the gimplifier change and thus
simplifes the rest.
OK for trunk?
thanks
Iain
P.S. I did not figure out how to find the right message-id to reply to from git
send-email, so attaching.
I tend to need to "view source" for that, which varies with mail readers.
- The original decl is left alone and instead calls are generated to pre/post
- functions within the body:
- void fun.pre(int v)
- {
- [[ assert: v > 0 ]];
- }
- int fun.post(int v, int __r)
{
- [[ assert: __r < 0 ]];
- return __r;
}
- int fun(int v)
{
- fun.pre(v);
- return fun.post(v, -v);
}
-
- If fun returns in memory, the return value is not passed through the post
- function; instead, the return object is initialized directly and then passed
- to the post function by invisible reference.
-
- This sides steps a number of issues with having to rewrite the bodies or
- rewrite the parsed conditions as the parameters to the original function
- changes (as happens during redeclaration). The ultimate goal is to get
- something that optimizes well along the lines of
-
- int fun(int v)
{
- [[ assert: v > 0 ]];
- auto &&__r = -v;
- goto out;
- out:
- [[ assert: __r < 0 ]];
- return __r;
}
- With the idea being that multiple return statements could collapse the
- function epilogue after inlining the pre/post functions. clang is able
- to collapse common function epilogues, while gcc needs -O3 -Os combined.
-
- Directly laying the pre contracts down in the function body doesn't have
- many issues. The post contracts may need to be repeated multiple times, once
- for each return, or a goto epilogue would need to be generated.
- For this initial implementation, generating function calls and letting
- later optimizations decide whether to inline and duplicate the actual
- checks or whether to collapse the shared epilogue was chosen.
-
- For cdtors a post contract is implemented using a CLEANUP_STMT.
-
- FIXME the compiler already shores cleanup code on multiple exit paths, so
- this outlining seems unnecessary if we represent the postcondition as a
- cleanup for all functions.
-
- More helpful for optimization might be to make the contracts a wrapper
- function (for non-variadic functions), that could be inlined into a
- caller while preserving the call to the actual function? Either that or
- mirror a never-continue post contract with an assume in the caller. */
Much the old documentation about outlining conditions still seems
relevant. In particular I'd keep something like
"FIXME outlining contract checks into separate functions was motivated
partly by wanting to call the postcondition function at each return
statement, which we no longer do; at this point outlining doesn't seem
to have any advantage over emitting the contracts directly in the
function body.
More helpful for optimization might be to make the contracts a wrapper
function that could be inlined into the caller, the callee, or both."
+ gcc_checking_assert (DECL_PRE_FN (fndecl)
+ && DECL_PRE_FN (fndecl) != error_mark_node);
...
+ gcc_checking_assert (DECL_POST_FN (fndecl)
+ && DECL_POST_FN (fndecl) != error_mark_node);
The && should line up past the ( on the previous line.
-/* Emit statements for postcondition attributes. */
+/* Emit statements for precondition attributes. */
static void
-emit_postconditions_cleanup (tree contracts)
+emit_postconditions (tree attr)
You changed the comment from post to pre, but the function is still post.
+ bool coro_p = flag_coroutines
+ && !processing_template_decl
+ && DECL_COROUTINE_P (fndecl);
+ bool coro_emit_helpers = false;
+ bool do_contracts = DECL_HAS_CONTRACTS_P (fndecl)
+ && !processing_template_decl;
Let's add parens to help preserve the indentation.
- current_function_decl = NULL_TREE;
+ gcc_checking_assert (current_function_decl == NULL_TREE);
I think we can just drop this line, it's redundant with the clearing a
few lines above.
OK with these tweaks.
Jason
