On 11/2/23 11:28, Marek Polacek wrote:
On Sat, Oct 14, 2023 at 12:56:11AM -0400, Jason Merrill wrote:
On 10/10/23 13:20, Marek Polacek wrote:
On Tue, Aug 29, 2023 at 03:26:46PM -0400, Jason Merrill wrote:
On 8/23/23 15:49, Marek Polacek wrote:
+struct A {
+ int x;
+ int y = id(x);
+};
+
+template<class T>
+constexpr int k(int) { // k<int> is not an immediate function because
A(42) is a
+ return A(42).y; // constant expression and thus not
immediate-escalating
+}
Needs use(s) of k to test the comment.
True, and that revealed what I think is a bug in the standard.
In the test I'm saying:
// ??? [expr.const]#example-9 says:
// k<int> is not an immediate function because A(42) is a
// constant expression and thus not immediate-escalating
// But I think the call to id(x) is *not* a constant expression and thus
// it is an immediate-escalating expression. Therefore k<int> *is*
// an immediate function. So we get the error below. clang++ agrees.
id(x) is not a constant expression because x isn't constant.
Not when considering id(x) by itself, but in the evaluation of A(42), the
member x has just been initialized to constant 42. And A(42) is
constant-evaluated because "An aggregate initialization is an immediate
invocation if it evaluates a default member initializer that has a
subexpression that is an immediate-escalating expression."
I assume clang doesn't handle this passage properly yet because it was added
during core review of the paper, for parity between aggregate initialization
and constructor escalation.
This can be a follow-up patch.
I see. So the fix will be to, for the aggregate initialization case, pass
the whole A(42).y thing to cxx_constant_eval, not just id(x).
Well, A(42) is the immediate invocation, the .y is not part of it.
I suppose some
functions cannot possibly be promoted because they don't contain
any CALL_EXPRs. So we may be able to rule them out while doing
cp_fold_r early.
Yes. Or, the only immediate-escalating functions referenced have already
been checked.
It looks like you haven't pursued this yet? One implementation thought:
maybe_store_cfun... could stop skipping immediate_escalating_function_p
(current_function_decl), and after we're done folding if the current
function isn't in the hash_set we can go ahead and set
DECL_ESCALATION_CHECKED_P?
We can also do some escalation during constexpr evaluation: all the
functions involved need to be instantiated for the evaluation, and if we
encounter an immediate-escalating expression while evaluating a call to an
immediate-escalating function, we can promote it then. Though we can't
necessarily mark it as not needing promotion, as there might be i-e exprs in
branches that the particular evaluation doesn't take.
I've tried but I didn't get anywhere. The patch was basically
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -2983,7 +2983,13 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree
t,
} fb (new_call.bindings);
if (*non_constant_p)
- return t;
+ {
+ if (cxx_dialect >= cxx20
+ && ctx->manifestly_const_eval == mce_false
+ && DECL_IMMEDIATE_FUNCTION_P (fun))
+ maybe_promote_function_to_consteval (current_function_decl);
+ return t;
+ }
/* We can't defer instantiating the function any longer. */
if (!DECL_INITIAL (fun)
but since I have to check mce_false, it didn't do anything useful
in practice (that is, it wouldn't escalate anything in my tests).
Makes sense.
@@ -55,6 +64,8 @@ enum fold_flags {
ff_mce_false = 1 << 1,
/* Whether we're being called from cp_fold_immediate. */
ff_fold_immediate = 1 << 2,
+ /* Whether we're escalating immediate-escalating functions. */
+ ff_escalating = 1 << 3,
If we always escalate when we see a known immediate invocation, this means
recurse. And maybe we could use at_eof for that?
Yes. Though, at_eof == 1 doesn't imply that all templates have been
instantiated, only that we got to c_parse_final_cleanups. Rather than
keeping the ff_ flag, I've updated at_eof to mean that 2 signals that
all templates have been instantiated, and 3 that we're into cgraph
territory. (Might should use an enum rather than magic numbers.)
Sounds good.
};
using fold_flags_t = int;
@@ -428,6 +439,176 @@ lvalue_has_side_effects (tree e)
return TREE_SIDE_EFFECTS (e);
}
+/* Return true if FN is an immediate-escalating function. */
+
+static bool
+immediate_escalating_function_p (tree fn)
+{
+ if (!fn || !flag_immediate_escalation)
+ return false;
+
+ gcc_checking_assert (TREE_CODE (fn) == FUNCTION_DECL);
Maybe check DECL_IMMEDIATE_FUNCTION_P early, rather than multiple times
below...
+ /* An immediate-escalating function is
+ -- the call operator of a lambda that is not declared with the consteval
+ specifier */
+ if (LAMBDA_FUNCTION_P (fn) && !DECL_IMMEDIATE_FUNCTION_P (fn))
+ return true;
+ /* -- a defaulted special member function that is not declared with the
+ consteval specifier */
+ special_function_kind sfk = special_memfn_p (fn);
+ if (sfk != sfk_none
+ && DECL_DEFAULTED_FN (fn)
+ && !DECL_IMMEDIATE_FUNCTION_P (fn))
+ return true;
+ /* -- a function that results from the instantiation of a templated entity
+ defined with the constexpr specifier. */
+ return is_instantiation_of_constexpr (fn);
+}
+
+/* Promote FN to an immediate function, including its clones, if it is
+ an immediate-escalating function. Return true if we did promote;
+ false otherwise. */
+
+static bool
+maybe_promote_function_to_consteval (tree fn)
+{
+ if (fn
+ && !DECL_IMMEDIATE_FUNCTION_P (fn)
...and in all the callers?
Ah, beautiful. And likewise for DECL_ESCALATION_CHECKED_P.
Agreed, but the function comment should mention that it returns false
for an immediate-escalating function that is no longer a candidate for
escalation. And maybe the function name should be something like
unchecked_immediate_escalating_function_p to clarify that it implements
something different from the term in the standard?
+ return;
+
+ vec_safe_push (deferred_escalating_exprs, current_function_decl);
+}
+
+/* Find an immediate-escalating expression or conversion in *TP.
+ If DATA_ is non-null, this function will promote function to
+ consteval as it goes; otherwise, we're just looking for what
+ made a function consteval, for diagnostic purposes. This
+ function assumes that *TP was instantiated. */
+
+static tree
+find_escalating_expr_r (tree *tp, int *walk_subtrees, void *data)
Can this merge with cp_fold_immediate_r? They seem to be doing essentially
the same thing.
I did not do it, sorry. I thought it would not make things more readable
as I don't see a whole lot of code to share.
The code is significantly different, sure, but both are walking the tree
to look for immediate-escalating expressions. Why do we need two of those?
+ if (code == ADDR_EXPR || !data->pset.add (stmt))
+ {
+ taking_address_of_imm_fn_error (stmt, decl);
+ *stmt_p = build_zero_cst (TREE_TYPE (stmt));
+ }
+ /* If we're giving hard errors, continue the walk rather than
+ bailing out after the first error. */
+ break;
+ }
+ return error_mark_node;
+ }
+ /* Not consteval yet, but could become one, in which case it's invalid
+ to take its address. */
+ else if (!(data->flags & ff_fold_immediate)
Why check ff_fold_immediate? Don't we want to do deferred checking of
immediate invocations in discarded branches?
On reflection, I was papering over a different problem here.
Inserting a new element to a hash table while we're traversing it is
dangerous because a new element might mean calling hash_table::expand
which creates a new table, std::moves the elements, and calls ggc_free
on the old table. Then iterating to the next element is going to crash
because it has been freed.
We were trying to add a new element while being called from
process_pending_immediate_escalating_fns. This should not happen.
The problem was that the DECL_ESCALATION_CHECKED_P flag may not have
been properly set for functions without any CALL_EXPRs in them, and that
immediate_escalating_function_p was not checking it, either.
The code is just
+ else if (immediate_escalating_function_p (decl))
+ remember_escalating_expr (stmt);
now.
The problem triggered in Wfree-nonheap-object-3.C. Unfortunately, I could
not reduce it very far so I don't have a test suitable for the testsuite.
The tricky thing is that the problem only manifests when we expand() a hash
table which is unlikely to happen in a small test and I don't see a --param
I could use.
You could save the value of deferred_escalating_exprs->elements() before
the loop and if it changes, abort in checking mode or goto back before
the loop when not checking?
It also seems odd that the ADDR_EXPR case calls vec_safe_push
(deferred_escalating_exprs, while the CALL_EXPR case calls
maybe_store_cfun_for_late_checking, why the different handling?
maybe_store_cfun_for_late_checking saves current_function_decl
so that we can check:
void g (int i) {
fn (i); // error if fn promotes to consteval
}
Yes, but why don't we want the same handling for ADDR_EXPR?
Jason
