| Message ID | 20200505194353.2254588-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: template instantiation during fold_for_warn [PR94038] | expand |
On Tue, 5 May 2020, Patrick Palka wrote: > Unfortunately, the previous fix to PR94038 is fragile. When the > argument to fold_for_warn is a bare CALL_EXPR, then all is well: the > result of maybe_constant_value from fold_for_warn (with > uid_sensitive=true) is reused via the cv_cache in the subsequent call to > maybe_constant_value from cp_fold (with uid_sensitive=false), so we > avoid instantiating bar<int>. > > But when the argument to fold_for_warn is more complex, e.g. an > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to bar<int>() > returning const int& which we need to decay to int) then from > fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and from > cp_fold we call it on the CALL_EXPR, so there is no reuse via the > cv_cache and we therefore end up instantiating bar<int>. > > So for a more robust solution to this general issue of warning flags > affecting code generation, it seems that we need a way to globally avoid > template instantiation during constexpr evaluation whenever we're > performing warning-dependent folding. > > To that end, this patch replaces the flag constexpr_ctx::uid_sensitive > with a global flag uid_sensitive_constexpr_evaluation_p, and enables it > during fold_for_warn using an RAII helper. > > The patch also adds a counter that keeps track of the number of times > uid_sensitive_constexpr_evaluation_p is called, and we use this to > determine whether the result of constexpr evaluation depended on the > flag's value. This lets us safely update the cv_cache and fold_cache > from fold_for_warn in the most common case where the flag's value was > irrelevant during constexpr evaluation. Whoops, shortly after I sent the patch it occurred to me that we only want to keep track of the number of times uid_sensitive_constexpr_evaluation_p was called _and returned true_. If the predicate was called and it returned false, then constexpr evaluation was not restricted, so we could safely cache the result. Please consider this patch instead, which also defines the various helper classse out-of-line in constexpr.c for improved encapsulation: -- >8 -- gcc/cp/ChangeLog: PR c++/94038 * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. (uid_sensitive_constexpr_evaluation_value): Define. (uid_sensitive_constexpr_evaluation_true_counter): Define. (uid_sensitive_constexpr_evaluation_p): Define. (uid_sensitive_constexpr_evaluation): Define its constructor and destructor. (uid_sensitive_constexpr_evaluation_marker): Define its constructor and its evaluation_restricted_p method. (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p instead of constexpr_ctx::uid_sensitive. (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it last. Adjust call to get_fundef_copy. (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the counter if necessary. (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' parameter. Adjust function body accordingly. Use uid_sensitive_constexpr_evaluation::value. (maybe_constant_value): Remove 'uid_sensitive' parameter and adjust function body accordingly. Set up a uid_sensitive_constexpr_evaluation_marker, and use it to conditionally update the cv_cache. * cp-gimplify.h (cp_fold): Set up a uid_sensitive_constexpr_evaluation_marker, and use it to conditionally update the fold_cache. * cp-tree.h (maybe_constant_value): Update declaration. (struct uid_sensitive_constexpr_evaluation): Define. (struct sensitive_constexpr_evaluation_marker): Define. * expr.c (fold_for_warn): Make constexpr evaluation uid-sensitive before calling the folding subroutines. Drop all but the first argument to maybe_constant_value. gcc/testsuite/ChangeLog: PR c++/94038 * g++.dg/warn/pr94038-2.C: New test. --- gcc/cp/constexpr.c | 96 ++++++++++++++++++++------- gcc/cp/cp-gimplify.c | 13 ++-- gcc/cp/cp-tree.h | 24 ++++++- gcc/cp/expr.c | 4 +- gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ 5 files changed, 136 insertions(+), 29 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 637cb746576..668e8b5d1b3 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1089,11 +1089,64 @@ struct constexpr_ctx { bool strict; /* Whether __builtin_is_constant_evaluated () should be true. */ bool manifestly_const_eval; - /* Whether we want to avoid doing anything that will cause extra DECL_UID - generation. */ - bool uid_sensitive; }; +/* This internal flag controls whether we should avoid doing anything during + constexpr evaluation that would cause extra DECL_UID generation, such as + template instantiation and function copying. */ + +static bool uid_sensitive_constexpr_evaluation_value; + +/* An internal counter that keeps track of the number of times + uid_sensitive_constexpr_evaluation_p returned true. */ + +static unsigned uid_sensitive_constexpr_evaluation_true_counter; + +/* The accessor for uid_sensitive_constexpr_evaluation::value which also + increments the corresponding counter. */ + +static bool +uid_sensitive_constexpr_evaluation_p () +{ + if (uid_sensitive_constexpr_evaluation_value) + { + ++uid_sensitive_constexpr_evaluation_true_counter; + return true; + } + else + return false; +} + +uid_sensitive_constexpr_evaluation +::uid_sensitive_constexpr_evaluation () + : saved_value (uid_sensitive_constexpr_evaluation_value) +{ + uid_sensitive_constexpr_evaluation_value = true; +} + +uid_sensitive_constexpr_evaluation +::~uid_sensitive_constexpr_evaluation () +{ + uid_sensitive_constexpr_evaluation_value = saved_value; +} + +uid_sensitive_constexpr_evaluation_marker +::uid_sensitive_constexpr_evaluation_marker () + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) +{ +} + +/* Returns true iff some constexpr evaluation was restricted due to + the value of uid_sensitive_constexpr_evaluation_p since this marker + object was constructed. */ + +bool +uid_sensitive_constexpr_evaluation_marker::evaluation_restricted_p () +{ + return saved_counter != uid_sensitive_constexpr_evaluation_true_counter; +} + + /* A table of all constexpr calls that have been evaluated by the compiler in this translation unit. */ @@ -1156,7 +1209,7 @@ static GTY(()) hash_map<tree, tree> *fundef_copies_table; is parms, TYPE is result. */ static tree -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) +get_fundef_copy (constexpr_fundef *fundef) { tree copy; bool existed; @@ -1173,7 +1226,7 @@ get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) } else if (*slot == NULL_TREE) { - if (ctx->uid_sensitive) + if (uid_sensitive_constexpr_evaluation_p ()) return NULL_TREE; /* We've already used the function itself, so make a copy. */ @@ -2292,8 +2345,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, /* We can't defer instantiating the function any longer. */ if (!DECL_INITIAL (fun) - && !ctx->uid_sensitive - && DECL_TEMPLOID_INSTANTIATION (fun)) + && DECL_TEMPLOID_INSTANTIATION (fun) + && !uid_sensitive_constexpr_evaluation_p ()) { location_t save_loc = input_location; input_location = loc; @@ -2454,7 +2507,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, gcc_assert (at_eof >= 2 && ctx->quiet); *non_constant_p = true; } - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) + else if (tree copy = get_fundef_copy (new_call.fundef)) { tree body, parms, res; releasing_vec ctors; @@ -6485,7 +6538,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, void */*data*/) && DECL_DECLARED_CONSTEXPR_P (*tp) && !DECL_INITIAL (*tp) && !trivial_fn_p (*tp) - && DECL_TEMPLOID_INSTANTIATION (*tp)) + && DECL_TEMPLOID_INSTANTIATION (*tp) + && !uid_sensitive_constexpr_evaluation_p ()) { ++function_depth; instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); @@ -6552,8 +6606,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, bool strict = true, bool manifestly_const_eval = false, bool constexpr_dtor = false, - tree object = NULL_TREE, - bool uid_sensitive = false) + tree object = NULL_TREE) { auto_timevar time (TV_CONSTEXPR); @@ -6569,8 +6622,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, constexpr_global_ctx global_ctx; constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, allow_non_constant, strict, - manifestly_const_eval || !allow_non_constant, - uid_sensitive }; + manifestly_const_eval || !allow_non_constant }; tree type = initialized_type (t); tree r = t; @@ -6660,7 +6712,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, auto_vec<tree, 16> cleanups; global_ctx.cleanups = &cleanups; - if (!uid_sensitive) + if (!uid_sensitive_constexpr_evaluation_value) instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); @@ -6909,14 +6961,12 @@ fold_simple (tree t) Otherwise, if T does not have TREE_CONSTANT set, returns T. Otherwise, returns a version of T without TREE_CONSTANT. MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated - as per P0595. UID_SENSITIVE is true if we can't do anything that - would affect DECL_UID ordering. */ + as per P0595. */ static GTY((deletable)) hash_map<tree, tree> *cv_cache; tree -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, - bool uid_sensitive) +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) { tree r; @@ -6934,8 +6984,7 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return t; if (manifestly_const_eval) - return cxx_eval_outermost_constant_expr (t, true, true, true, false, - decl, uid_sensitive); + return cxx_eval_outermost_constant_expr (t, true, true, true, false, decl); if (cv_cache == NULL) cv_cache = hash_map<tree, tree>::create_ggc (101); @@ -6950,14 +6999,15 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return r; } - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, - decl, uid_sensitive); + uid_sensitive_constexpr_evaluation_marker m; + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); gcc_checking_assert (r == t || CONVERT_EXPR_P (t) || TREE_CODE (t) == VIEW_CONVERT_EXPR || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) || !cp_tree_equal (r, t)); - cv_cache->put (t, r); + if (!m.evaluation_restricted_p ()) + cv_cache->put (t, r); return r; } diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index fc26a85f43a..3bcad665e8d 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -2443,6 +2443,8 @@ cp_fold (tree x) if (tree *cached = fold_cache->get (x)) return *cached; + uid_sensitive_constexpr_evaluation_marker m; + code = TREE_CODE (x); switch (code) { @@ -2925,10 +2927,13 @@ cp_fold (tree x) return org_x; } - fold_cache->put (org_x, x); - /* Prevent that we try to fold an already folded result again. */ - if (x != org_x) - fold_cache->put (x, x); + if (!m.evaluation_restricted_p ()) + { + fold_cache->put (org_x, x); + /* Prevent that we try to fold an already folded result again. */ + if (x != org_x) + fold_cache->put (x, x); + } return x; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index f4328403bc7..7f1c1ee8908 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7949,7 +7949,7 @@ extern bool require_potential_rvalue_constant_expression (tree); extern tree cxx_constant_value (tree, tree = NULL_TREE); extern void cxx_constant_dtor (tree, tree); extern tree cxx_constant_init (tree, tree = NULL_TREE); -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false, bool = false); +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false); extern tree maybe_constant_init (tree, tree = NULL_TREE, bool = false); extern tree fold_non_dependent_expr (tree, tsubst_flags_t = tf_warning_or_error, @@ -7968,6 +7968,28 @@ extern tree fold_sizeof_expr (tree); extern void clear_cv_and_fold_caches (bool = true); extern tree unshare_constructor (tree CXX_MEM_STAT_INFO); +/* An RAII sentinel used to restrict constexpr evaluation so that it + doesn't do anything that causes extra DECL_UID generation. */ + +struct uid_sensitive_constexpr_evaluation +{ + const bool saved_value; + uid_sensitive_constexpr_evaluation (); + ~uid_sensitive_constexpr_evaluation (); +}; + +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was + called and returned true, indicating that we've inhibited constexpr + evaluation in order to avoid UID generation. We use this to control + updates to the fold_cache and cv_cache. */ + +struct uid_sensitive_constexpr_evaluation_marker +{ + const unsigned saved_counter; + uid_sensitive_constexpr_evaluation_marker (); + bool evaluation_restricted_p (); +}; + /* In cp-ubsan.c */ extern void cp_ubsan_maybe_instrument_member_call (tree); extern void cp_ubsan_instrument_member_accesses (tree *); diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c index 9b535708c57..97671edf800 100644 --- a/gcc/cp/expr.c +++ b/gcc/cp/expr.c @@ -399,6 +399,8 @@ fold_for_warn (tree x) { /* C++ implementation. */ + uid_sensitive_constexpr_evaluation s; + /* It's not generally safe to fully fold inside of a template, so call fold_non_dependent_expr instead. */ if (processing_template_decl) @@ -410,7 +412,7 @@ fold_for_warn (tree x) return f; } else if (cxx_dialect >= cxx11) - x = maybe_constant_value (x, NULL_TREE, false, true); + x = maybe_constant_value (x); return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL); } diff --git a/gcc/testsuite/g++.dg/warn/pr94038-2.C b/gcc/testsuite/g++.dg/warn/pr94038-2.C new file mode 100644 index 00000000000..a468cc055eb --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/pr94038-2.C @@ -0,0 +1,28 @@ +// PR c++/94038 +// { dg-do compile { target c++11 } } +// { dg-additional-options "-O -Wall" } + +static constexpr int x = 0; + +template<typename T> +constexpr const int& +foo() +{ + static_assert(T(1) == 0, ""); + return x; +} + +template<typename T> +constexpr const int& +bar() +{ + return foo<T>(); +} + +constexpr int +baz(int a) +{ + return a; +} + +static_assert(decltype(baz(bar<int>())){} == 0, "");
On Tue, 5 May 2020, Patrick Palka wrote: > On Tue, 5 May 2020, Patrick Palka wrote: > > > Unfortunately, the previous fix to PR94038 is fragile. When the > > argument to fold_for_warn is a bare CALL_EXPR, then all is well: the > > result of maybe_constant_value from fold_for_warn (with > > uid_sensitive=true) is reused via the cv_cache in the subsequent call to > > maybe_constant_value from cp_fold (with uid_sensitive=false), so we > > avoid instantiating bar<int>. > > > > But when the argument to fold_for_warn is more complex, e.g. an > > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to bar<int>() > > returning const int& which we need to decay to int) then from > > fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and from > > cp_fold we call it on the CALL_EXPR, so there is no reuse via the > > cv_cache and we therefore end up instantiating bar<int>. > > > > So for a more robust solution to this general issue of warning flags > > affecting code generation, it seems that we need a way to globally avoid > > template instantiation during constexpr evaluation whenever we're > > performing warning-dependent folding. > > > > To that end, this patch replaces the flag constexpr_ctx::uid_sensitive > > with a global flag uid_sensitive_constexpr_evaluation_p, and enables it > > during fold_for_warn using an RAII helper. > > > > The patch also adds a counter that keeps track of the number of times > > uid_sensitive_constexpr_evaluation_p is called, and we use this to > > determine whether the result of constexpr evaluation depended on the > > flag's value. This lets us safely update the cv_cache and fold_cache > > from fold_for_warn in the most common case where the flag's value was > > irrelevant during constexpr evaluation. > > Whoops, shortly after I sent the patch it occurred to me that we only > want to keep track of the number of times > uid_sensitive_constexpr_evaluation_p was called _and returned true_. > If the predicate was called and it returned false, then constexpr > evaluation was not restricted, so we could safely cache the result. > > Please consider this patch instead, which also defines the various > helper classse out-of-line in constexpr.c for improved encapsulation: Here's another version of the patch which just polishes comments, marks the 'evaluation_restricted_p' method const, renames 'uid_sensitive_constexpr_evaluation' to 'uid_sensitive_constexpr_evaluation_sentinel', and renames 'uid_sensitive_constexpr_evaluation_marker' to 'uid_sensitive_constexpr_evaluation_checker' to better convey their purpose. The previous version passed bootstrap/regtest on x86_64-pc-linux-gnu, and bootstrap/regtest for this version is in progress. -- >8 -- gcc/cp/ChangeLog: PR c++/94038 * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. (uid_sensitive_constexpr_evaluation_value): Define. (uid_sensitive_constexpr_evaluation_true_counter): Define. (uid_sensitive_constexpr_evaluation_p): Define. (uid_sensitive_constexpr_evaluation_sentinel): Define its constructor and destructor. (uid_sensitive_constexpr_evaluation_checker): Define its constructor and its evaluation_restricted_p method. (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p instead of constexpr_ctx::uid_sensitive. (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it last. Adjust call to get_fundef_copy. (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the counter if necessary. (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' parameter. Adjust function body accordingly. Use uid_sensitive_constexpr_evaluation_value. (maybe_constant_value): Remove 'uid_sensitive' parameter and adjust function body accordingly. Set up a uid_sensitive_constexpr_evaluation_checker, and use it to conditionally update the cv_cache. * cp-gimplify.h (cp_fold): Set up a uid_sensitive_constexpr_evaluation_checker, and use it to conditionally update the fold_cache. * cp-tree.h (maybe_constant_value): Update declaration. (struct uid_sensitive_constexpr_evaluation_sentinel): Define. (struct sensitive_constexpr_evaluation_checker): Define. * expr.c (fold_for_warn): Set up a uid_sensitive_constexpr_evaluation_sentinel before calling the folding subroutines. Drop all but the first argument to maybe_constant_value. gcc/testsuite/ChangeLog: PR c++/94038 * g++.dg/warn/pr94038-2.C: New test. --- gcc/cp/constexpr.c | 96 ++++++++++++++++++++------- gcc/cp/cp-gimplify.c | 13 ++-- gcc/cp/cp-tree.h | 24 ++++++- gcc/cp/expr.c | 4 +- gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ 5 files changed, 136 insertions(+), 29 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 637cb746576..fb67d218ed8 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1089,11 +1089,64 @@ struct constexpr_ctx { bool strict; /* Whether __builtin_is_constant_evaluated () should be true. */ bool manifestly_const_eval; - /* Whether we want to avoid doing anything that will cause extra DECL_UID - generation. */ - bool uid_sensitive; }; +/* This internal flag controls whether we should avoid doing anything during + constexpr evaluation that would cause extra DECL_UID generation, such as + template instantiation and function copying. */ + +static bool uid_sensitive_constexpr_evaluation_value; + +/* An internal counter that keeps track of the number of times + uid_sensitive_constexpr_evaluation_p returned true. */ + +static unsigned uid_sensitive_constexpr_evaluation_true_counter; + +/* The accessor for uid_sensitive_constexpr_evaluation_value which also + increments the corresponding counter. */ + +static bool +uid_sensitive_constexpr_evaluation_p () +{ + if (uid_sensitive_constexpr_evaluation_value) + { + ++uid_sensitive_constexpr_evaluation_true_counter; + return true; + } + else + return false; +} + +uid_sensitive_constexpr_evaluation_sentinel +::uid_sensitive_constexpr_evaluation_sentinel () + : saved_value (uid_sensitive_constexpr_evaluation_value) +{ + uid_sensitive_constexpr_evaluation_value = true; +} + +uid_sensitive_constexpr_evaluation_sentinel +::~uid_sensitive_constexpr_evaluation_sentinel () +{ + uid_sensitive_constexpr_evaluation_value = saved_value; +} + +uid_sensitive_constexpr_evaluation_checker +::uid_sensitive_constexpr_evaluation_checker () + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) +{ +} + +/* Returns true iff some constexpr evaluation was restricted due to + uid_sensitive_constexpr_evaluation_p being called and returning true + after this marker object was constructed. */ + +bool +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () const +{ + return saved_counter != uid_sensitive_constexpr_evaluation_true_counter; +} + + /* A table of all constexpr calls that have been evaluated by the compiler in this translation unit. */ @@ -1156,7 +1209,7 @@ static GTY(()) hash_map<tree, tree> *fundef_copies_table; is parms, TYPE is result. */ static tree -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) +get_fundef_copy (constexpr_fundef *fundef) { tree copy; bool existed; @@ -1173,7 +1226,7 @@ get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) } else if (*slot == NULL_TREE) { - if (ctx->uid_sensitive) + if (uid_sensitive_constexpr_evaluation_p ()) return NULL_TREE; /* We've already used the function itself, so make a copy. */ @@ -2292,8 +2345,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, /* We can't defer instantiating the function any longer. */ if (!DECL_INITIAL (fun) - && !ctx->uid_sensitive - && DECL_TEMPLOID_INSTANTIATION (fun)) + && DECL_TEMPLOID_INSTANTIATION (fun) + && !uid_sensitive_constexpr_evaluation_p ()) { location_t save_loc = input_location; input_location = loc; @@ -2454,7 +2507,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, gcc_assert (at_eof >= 2 && ctx->quiet); *non_constant_p = true; } - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) + else if (tree copy = get_fundef_copy (new_call.fundef)) { tree body, parms, res; releasing_vec ctors; @@ -6485,7 +6538,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, void */*data*/) && DECL_DECLARED_CONSTEXPR_P (*tp) && !DECL_INITIAL (*tp) && !trivial_fn_p (*tp) - && DECL_TEMPLOID_INSTANTIATION (*tp)) + && DECL_TEMPLOID_INSTANTIATION (*tp) + && !uid_sensitive_constexpr_evaluation_p ()) { ++function_depth; instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); @@ -6552,8 +6606,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, bool strict = true, bool manifestly_const_eval = false, bool constexpr_dtor = false, - tree object = NULL_TREE, - bool uid_sensitive = false) + tree object = NULL_TREE) { auto_timevar time (TV_CONSTEXPR); @@ -6569,8 +6622,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, constexpr_global_ctx global_ctx; constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, allow_non_constant, strict, - manifestly_const_eval || !allow_non_constant, - uid_sensitive }; + manifestly_const_eval || !allow_non_constant }; tree type = initialized_type (t); tree r = t; @@ -6660,7 +6712,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, auto_vec<tree, 16> cleanups; global_ctx.cleanups = &cleanups; - if (!uid_sensitive) + if (!uid_sensitive_constexpr_evaluation_value) instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); @@ -6909,14 +6961,12 @@ fold_simple (tree t) Otherwise, if T does not have TREE_CONSTANT set, returns T. Otherwise, returns a version of T without TREE_CONSTANT. MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated - as per P0595. UID_SENSITIVE is true if we can't do anything that - would affect DECL_UID ordering. */ + as per P0595. */ static GTY((deletable)) hash_map<tree, tree> *cv_cache; tree -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, - bool uid_sensitive) +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) { tree r; @@ -6934,8 +6984,7 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return t; if (manifestly_const_eval) - return cxx_eval_outermost_constant_expr (t, true, true, true, false, - decl, uid_sensitive); + return cxx_eval_outermost_constant_expr (t, true, true, true, false, decl); if (cv_cache == NULL) cv_cache = hash_map<tree, tree>::create_ggc (101); @@ -6950,14 +6999,15 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return r; } - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, - decl, uid_sensitive); + uid_sensitive_constexpr_evaluation_checker c; + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); gcc_checking_assert (r == t || CONVERT_EXPR_P (t) || TREE_CODE (t) == VIEW_CONVERT_EXPR || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) || !cp_tree_equal (r, t)); - cv_cache->put (t, r); + if (!c.evaluation_restricted_p ()) + cv_cache->put (t, r); return r; } diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index fc26a85f43a..f298fa7cec1 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -2443,6 +2443,8 @@ cp_fold (tree x) if (tree *cached = fold_cache->get (x)) return *cached; + uid_sensitive_constexpr_evaluation_checker c; + code = TREE_CODE (x); switch (code) { @@ -2925,10 +2927,13 @@ cp_fold (tree x) return org_x; } - fold_cache->put (org_x, x); - /* Prevent that we try to fold an already folded result again. */ - if (x != org_x) - fold_cache->put (x, x); + if (!c.evaluation_restricted_p ()) + { + fold_cache->put (org_x, x); + /* Prevent that we try to fold an already folded result again. */ + if (x != org_x) + fold_cache->put (x, x); + } return x; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index c4b81428e14..9ae5112f37f 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7949,7 +7949,7 @@ extern bool require_potential_rvalue_constant_expression (tree); extern tree cxx_constant_value (tree, tree = NULL_TREE); extern void cxx_constant_dtor (tree, tree); extern tree cxx_constant_init (tree, tree = NULL_TREE); -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false, bool = false); +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false); extern tree maybe_constant_init (tree, tree = NULL_TREE, bool = false); extern tree fold_non_dependent_expr (tree, tsubst_flags_t = tf_warning_or_error, @@ -7968,6 +7968,28 @@ extern tree fold_sizeof_expr (tree); extern void clear_cv_and_fold_caches (bool = true); extern tree unshare_constructor (tree CXX_MEM_STAT_INFO); +/* An RAII sentinel used to restrict constexpr evaluation so that it + doesn't do anything that causes extra DECL_UID generation. */ + +struct uid_sensitive_constexpr_evaluation_sentinel +{ + const bool saved_value; + uid_sensitive_constexpr_evaluation_sentinel (); + ~uid_sensitive_constexpr_evaluation_sentinel (); +}; + +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was + called and returned true, indicating that we've restricted constexpr + evaluation in order to avoid UID generation. We use this to control + updates to the fold_cache and cv_cache. */ + +struct uid_sensitive_constexpr_evaluation_checker +{ + const unsigned saved_counter; + uid_sensitive_constexpr_evaluation_checker (); + bool evaluation_restricted_p () const; +}; + /* In cp-ubsan.c */ extern void cp_ubsan_maybe_instrument_member_call (tree); extern void cp_ubsan_instrument_member_accesses (tree *); diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c index 9b535708c57..0d68a738f8f 100644 --- a/gcc/cp/expr.c +++ b/gcc/cp/expr.c @@ -399,6 +399,8 @@ fold_for_warn (tree x) { /* C++ implementation. */ + uid_sensitive_constexpr_evaluation_sentinel s; + /* It's not generally safe to fully fold inside of a template, so call fold_non_dependent_expr instead. */ if (processing_template_decl) @@ -410,7 +412,7 @@ fold_for_warn (tree x) return f; } else if (cxx_dialect >= cxx11) - x = maybe_constant_value (x, NULL_TREE, false, true); + x = maybe_constant_value (x); return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL); } diff --git a/gcc/testsuite/g++.dg/warn/pr94038-2.C b/gcc/testsuite/g++.dg/warn/pr94038-2.C new file mode 100644 index 00000000000..a468cc055eb --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/pr94038-2.C @@ -0,0 +1,28 @@ +// PR c++/94038 +// { dg-do compile { target c++11 } } +// { dg-additional-options "-O -Wall" } + +static constexpr int x = 0; + +template<typename T> +constexpr const int& +foo() +{ + static_assert(T(1) == 0, ""); + return x; +} + +template<typename T> +constexpr const int& +bar() +{ + return foo<T>(); +} + +constexpr int +baz(int a) +{ + return a; +} + +static_assert(decltype(baz(bar<int>())){} == 0, "");
On Wed, 6 May 2020, Patrick Palka wrote: > On Tue, 5 May 2020, Patrick Palka wrote: > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > Unfortunately, the previous fix to PR94038 is fragile. When the > > > argument to fold_for_warn is a bare CALL_EXPR, then all is well: the > > > result of maybe_constant_value from fold_for_warn (with > > > uid_sensitive=true) is reused via the cv_cache in the subsequent call to > > > maybe_constant_value from cp_fold (with uid_sensitive=false), so we > > > avoid instantiating bar<int>. > > > > > > But when the argument to fold_for_warn is more complex, e.g. an > > > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to bar<int>() > > > returning const int& which we need to decay to int) then from > > > fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and from > > > cp_fold we call it on the CALL_EXPR, so there is no reuse via the > > > cv_cache and we therefore end up instantiating bar<int>. > > > > > > So for a more robust solution to this general issue of warning flags > > > affecting code generation, it seems that we need a way to globally avoid > > > template instantiation during constexpr evaluation whenever we're > > > performing warning-dependent folding. > > > > > > To that end, this patch replaces the flag constexpr_ctx::uid_sensitive > > > with a global flag uid_sensitive_constexpr_evaluation_p, and enables it > > > during fold_for_warn using an RAII helper. > > > > > > The patch also adds a counter that keeps track of the number of times > > > uid_sensitive_constexpr_evaluation_p is called, and we use this to > > > determine whether the result of constexpr evaluation depended on the > > > flag's value. This lets us safely update the cv_cache and fold_cache > > > from fold_for_warn in the most common case where the flag's value was > > > irrelevant during constexpr evaluation. > > > > Whoops, shortly after I sent the patch it occurred to me that we only > > want to keep track of the number of times > > uid_sensitive_constexpr_evaluation_p was called _and returned true_. > > If the predicate was called and it returned false, then constexpr > > evaluation was not restricted, so we could safely cache the result. > > > > Please consider this patch instead, which also defines the various > > helper classse out-of-line in constexpr.c for improved encapsulation: > > Here's another version of the patch which just polishes comments, marks > the 'evaluation_restricted_p' method const, renames > 'uid_sensitive_constexpr_evaluation' to > 'uid_sensitive_constexpr_evaluation_sentinel', and renames > 'uid_sensitive_constexpr_evaluation_marker' to > 'uid_sensitive_constexpr_evaluation_checker' to better convey their > purpose. > > The previous version passed bootstrap/regtest on x86_64-pc-linux-gnu, > and bootstrap/regtest for this version is in progress. > > -- >8 -- > > gcc/cp/ChangeLog: > > PR c++/94038 > * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. > (uid_sensitive_constexpr_evaluation_value): Define. > (uid_sensitive_constexpr_evaluation_true_counter): Define. > (uid_sensitive_constexpr_evaluation_p): Define. > (uid_sensitive_constexpr_evaluation_sentinel): Define its > constructor and destructor. > (uid_sensitive_constexpr_evaluation_checker): Define its > constructor and its evaluation_restricted_p method. > (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p > instead of constexpr_ctx::uid_sensitive. > (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it > last. Adjust call to get_fundef_copy. > (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the > counter if necessary. > (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' > parameter. Adjust function body accordingly. Use > uid_sensitive_constexpr_evaluation_value. > (maybe_constant_value): Remove 'uid_sensitive' parameter and > adjust function body accordingly. Set up a > uid_sensitive_constexpr_evaluation_checker, and use it to > conditionally update the cv_cache. > * cp-gimplify.h (cp_fold): Set up a > uid_sensitive_constexpr_evaluation_checker, and use it to > conditionally update the fold_cache. > * cp-tree.h (maybe_constant_value): Update declaration. > (struct uid_sensitive_constexpr_evaluation_sentinel): Define. > (struct sensitive_constexpr_evaluation_checker): Define. > * expr.c (fold_for_warn): Set up a > uid_sensitive_constexpr_evaluation_sentinel before calling > the folding subroutines. Drop all but the first argument to > maybe_constant_value. > > gcc/testsuite/ChangeLog: > > PR c++/94038 > * g++.dg/warn/pr94038-2.C: New test. > --- > gcc/cp/constexpr.c | 96 ++++++++++++++++++++------- > gcc/cp/cp-gimplify.c | 13 ++-- > gcc/cp/cp-tree.h | 24 ++++++- > gcc/cp/expr.c | 4 +- > gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ > 5 files changed, 136 insertions(+), 29 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index 637cb746576..fb67d218ed8 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -1089,11 +1089,64 @@ struct constexpr_ctx { > bool strict; > /* Whether __builtin_is_constant_evaluated () should be true. */ > bool manifestly_const_eval; > - /* Whether we want to avoid doing anything that will cause extra DECL_UID > - generation. */ > - bool uid_sensitive; > }; > > +/* This internal flag controls whether we should avoid doing anything during > + constexpr evaluation that would cause extra DECL_UID generation, such as > + template instantiation and function copying. */ > + > +static bool uid_sensitive_constexpr_evaluation_value; > + > +/* An internal counter that keeps track of the number of times > + uid_sensitive_constexpr_evaluation_p returned true. */ > + > +static unsigned uid_sensitive_constexpr_evaluation_true_counter; > + > +/* The accessor for uid_sensitive_constexpr_evaluation_value which also > + increments the corresponding counter. */ > + > +static bool > +uid_sensitive_constexpr_evaluation_p () > +{ > + if (uid_sensitive_constexpr_evaluation_value) > + { > + ++uid_sensitive_constexpr_evaluation_true_counter; > + return true; > + } > + else > + return false; > +} > + > +uid_sensitive_constexpr_evaluation_sentinel > +::uid_sensitive_constexpr_evaluation_sentinel () > + : saved_value (uid_sensitive_constexpr_evaluation_value) > +{ > + uid_sensitive_constexpr_evaluation_value = true; > +} > + > +uid_sensitive_constexpr_evaluation_sentinel > +::~uid_sensitive_constexpr_evaluation_sentinel () > +{ > + uid_sensitive_constexpr_evaluation_value = saved_value; > +} > + > +uid_sensitive_constexpr_evaluation_checker > +::uid_sensitive_constexpr_evaluation_checker () > + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) > +{ > +} > + > +/* Returns true iff some constexpr evaluation was restricted due to > + uid_sensitive_constexpr_evaluation_p being called and returning true > + after this marker object was constructed. */ > + > +bool > +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () const > +{ > + return saved_counter != uid_sensitive_constexpr_evaluation_true_counter; > +} > + > + > /* A table of all constexpr calls that have been evaluated by the > compiler in this translation unit. */ > > @@ -1156,7 +1209,7 @@ static GTY(()) hash_map<tree, tree> *fundef_copies_table; > is parms, TYPE is result. */ > > static tree > -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) > +get_fundef_copy (constexpr_fundef *fundef) > { > tree copy; > bool existed; > @@ -1173,7 +1226,7 @@ get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) > } > else if (*slot == NULL_TREE) > { > - if (ctx->uid_sensitive) > + if (uid_sensitive_constexpr_evaluation_p ()) > return NULL_TREE; > > /* We've already used the function itself, so make a copy. */ > @@ -2292,8 +2345,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > > /* We can't defer instantiating the function any longer. */ > if (!DECL_INITIAL (fun) > - && !ctx->uid_sensitive > - && DECL_TEMPLOID_INSTANTIATION (fun)) > + && DECL_TEMPLOID_INSTANTIATION (fun) > + && !uid_sensitive_constexpr_evaluation_p ()) > { > location_t save_loc = input_location; > input_location = loc; > @@ -2454,7 +2507,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > gcc_assert (at_eof >= 2 && ctx->quiet); > *non_constant_p = true; > } > - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) > + else if (tree copy = get_fundef_copy (new_call.fundef)) > { > tree body, parms, res; > releasing_vec ctors; > @@ -6485,7 +6538,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, void */*data*/) > && DECL_DECLARED_CONSTEXPR_P (*tp) > && !DECL_INITIAL (*tp) > && !trivial_fn_p (*tp) > - && DECL_TEMPLOID_INSTANTIATION (*tp)) > + && DECL_TEMPLOID_INSTANTIATION (*tp) > + && !uid_sensitive_constexpr_evaluation_p ()) > { > ++function_depth; > instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); > @@ -6552,8 +6606,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > bool strict = true, > bool manifestly_const_eval = false, > bool constexpr_dtor = false, > - tree object = NULL_TREE, > - bool uid_sensitive = false) > + tree object = NULL_TREE) > { > auto_timevar time (TV_CONSTEXPR); > > @@ -6569,8 +6622,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > constexpr_global_ctx global_ctx; > constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, > allow_non_constant, strict, > - manifestly_const_eval || !allow_non_constant, > - uid_sensitive }; > + manifestly_const_eval || !allow_non_constant }; > > tree type = initialized_type (t); > tree r = t; > @@ -6660,7 +6712,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > auto_vec<tree, 16> cleanups; > global_ctx.cleanups = &cleanups; > > - if (!uid_sensitive) > + if (!uid_sensitive_constexpr_evaluation_value) > instantiate_constexpr_fns (r); On second thought, we should call instantiate_constexpr_fns unconditionally here. I thought guarding the call with uid_sensitive_constexpr_evaluation_value would act as an optimization to avoid walking the expression when we are uid-sensitive, but we should call instantiate_constexpr_fns anyway (which calls instantiate_cx_fn_p, which checks uid_sensitive_constexpr_evaluation_p) so that evaluation_inhibited_p will later detect when we avoided instantiating a constexpr fn due to being uid-sensitive.
On Wed, 6 May 2020, Patrick Palka wrote: > On Wed, 6 May 2020, Patrick Palka wrote: > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > Unfortunately, the previous fix to PR94038 is fragile. When the > > > > argument to fold_for_warn is a bare CALL_EXPR, then all is well: the > > > > result of maybe_constant_value from fold_for_warn (with > > > > uid_sensitive=true) is reused via the cv_cache in the subsequent call to > > > > maybe_constant_value from cp_fold (with uid_sensitive=false), so we > > > > avoid instantiating bar<int>. > > > > > > > > But when the argument to fold_for_warn is more complex, e.g. an > > > > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to bar<int>() > > > > returning const int& which we need to decay to int) then from > > > > fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and from > > > > cp_fold we call it on the CALL_EXPR, so there is no reuse via the > > > > cv_cache and we therefore end up instantiating bar<int>. > > > > > > > > So for a more robust solution to this general issue of warning flags > > > > affecting code generation, it seems that we need a way to globally avoid > > > > template instantiation during constexpr evaluation whenever we're > > > > performing warning-dependent folding. > > > > > > > > To that end, this patch replaces the flag constexpr_ctx::uid_sensitive > > > > with a global flag uid_sensitive_constexpr_evaluation_p, and enables it > > > > during fold_for_warn using an RAII helper. > > > > > > > > The patch also adds a counter that keeps track of the number of times > > > > uid_sensitive_constexpr_evaluation_p is called, and we use this to > > > > determine whether the result of constexpr evaluation depended on the > > > > flag's value. This lets us safely update the cv_cache and fold_cache > > > > from fold_for_warn in the most common case where the flag's value was > > > > irrelevant during constexpr evaluation. Here's some statistics about about the fold cache and cv cache that were collected when building the testsuite of range-v3 (with the default build flags which include -O -Wall -Wextra, so warning-dependent folding applies). The "naive solution" below refers to the one in which we would refrain from updating the caches at the end of cp_fold/maybe_constant_value whenever we're in uid-sensitive constexpr evaluation mode (i.e. whenever we're called from fold_for_warn), regardless of whether constexpr evaluation was actually restricted. FOLD CACHE | cache hit | cache miss | cache miss | | | cache updated | cache not updated | ------------+-----------+---------------+-------------------+ naive sol'n | 5060779 | 11374667 | 12887790 | ------------------------------------------------------------+ this patch | 6665242 | 19688975 | 199137 | ------------+-----------+---------------+-------------------+ CV CACHE | cache hit | cache miss | cache miss | | | cache updated | cache not updated | ------------+-----------+---------------+-------------------+ naive sol'n | 76214 | 3637218 | 6889213 | ------------------------------------------------------------+ this patch | 215980 | 9882310 | 405513 | ------------+-----------+---------------+-------------------+ -- >8 -- gcc/cp/ChangeLog: PR c++/94038 * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. (uid_sensitive_constexpr_evaluation_value): Define. (uid_sensitive_constexpr_evaluation_true_counter): Define. (uid_sensitive_constexpr_evaluation_p): Define. (uid_sensitive_constexpr_evaluation_sentinel): Define its constructor. (uid_sensitive_constexpr_evaluation_checker): Define its constructor and its evaluation_restricted_p method. (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p instead of constexpr_ctx::uid_sensitive. (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it last. Adjust call to get_fundef_copy. (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the counter if necessary. (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' parameter. Adjust function body accordingly. (maybe_constant_value): Remove 'uid_sensitive' parameter and adjust function body accordingly. Set up a uid_sensitive_constexpr_evaluation_checker, and use it to conditionally update the cv_cache. * cp-gimplify.h (cp_fold): Set up a uid_sensitive_constexpr_evaluation_checker, and use it to conditionally update the fold_cache. * cp-tree.h (maybe_constant_value): Update declaration. (struct uid_sensitive_constexpr_evaluation_sentinel): Define. (struct sensitive_constexpr_evaluation_checker): Define. * expr.c (fold_for_warn): Set up a uid_sensitive_constexpr_evaluation_sentinel before calling the folding subroutines. Drop all but the first argument to maybe_constant_value. gcc/testsuite/ChangeLog: PR c++/94038 * g++.dg/warn/pr94038-2.C: New test. --- gcc/cp/constexpr.c | 92 ++++++++++++++++++++------- gcc/cp/cp-gimplify.c | 13 ++-- gcc/cp/cp-tree.h | 23 ++++++- gcc/cp/expr.c | 4 +- gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ 5 files changed, 130 insertions(+), 30 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 706d8a13d8e..d2b75ddaa19 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1089,11 +1089,59 @@ struct constexpr_ctx { bool strict; /* Whether __builtin_is_constant_evaluated () should be true. */ bool manifestly_const_eval; - /* Whether we want to avoid doing anything that will cause extra DECL_UID - generation. */ - bool uid_sensitive; }; +/* This internal flag controls whether we should avoid doing anything during + constexpr evaluation that would cause extra DECL_UID generation, such as + template instantiation and function copying. */ + +static bool uid_sensitive_constexpr_evaluation_value; + +/* An internal counter that keeps track of the number of times + uid_sensitive_constexpr_evaluation_p returned true. */ + +static unsigned uid_sensitive_constexpr_evaluation_true_counter; + +/* The accessor for uid_sensitive_constexpr_evaluation_value which also + increments the corresponding counter. */ + +static bool +uid_sensitive_constexpr_evaluation_p () +{ + if (uid_sensitive_constexpr_evaluation_value) + { + ++uid_sensitive_constexpr_evaluation_true_counter; + return true; + } + else + return false; +} + +uid_sensitive_constexpr_evaluation_sentinel +::uid_sensitive_constexpr_evaluation_sentinel () + : ovr (uid_sensitive_constexpr_evaluation_value, true) +{ +} + +uid_sensitive_constexpr_evaluation_checker +::uid_sensitive_constexpr_evaluation_checker () + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) +{ +} + +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, + and some constexpr evaluation was restricted due to u_s_c_e_p + being called and returning true after this checker object was + constructed. */ + +bool +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () const +{ + return (uid_sensitive_constexpr_evaluation_value + && saved_counter != uid_sensitive_constexpr_evaluation_true_counter); +} + + /* A table of all constexpr calls that have been evaluated by the compiler in this translation unit. */ @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> *fundef_copies_table; is parms, TYPE is result. */ static tree -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) +get_fundef_copy (constexpr_fundef *fundef) { tree copy; bool existed; @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) } else if (*slot == NULL_TREE) { - if (ctx->uid_sensitive) + if (uid_sensitive_constexpr_evaluation_p ()) return NULL_TREE; /* We've already used the function itself, so make a copy. */ @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, /* We can't defer instantiating the function any longer. */ if (!DECL_INITIAL (fun) - && !ctx->uid_sensitive - && DECL_TEMPLOID_INSTANTIATION (fun)) + && DECL_TEMPLOID_INSTANTIATION (fun) + && !uid_sensitive_constexpr_evaluation_p ()) { location_t save_loc = input_location; input_location = loc; @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, gcc_assert (at_eof >= 2 && ctx->quiet); *non_constant_p = true; } - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) + else if (tree copy = get_fundef_copy (new_call.fundef)) { tree body, parms, res; releasing_vec ctors; @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, void */*data*/) && DECL_DECLARED_CONSTEXPR_P (*tp) && !DECL_INITIAL (*tp) && !trivial_fn_p (*tp) - && DECL_TEMPLOID_INSTANTIATION (*tp)) + && DECL_TEMPLOID_INSTANTIATION (*tp) + && !uid_sensitive_constexpr_evaluation_p ()) { ++function_depth; instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, bool strict = true, bool manifestly_const_eval = false, bool constexpr_dtor = false, - tree object = NULL_TREE, - bool uid_sensitive = false) + tree object = NULL_TREE) { auto_timevar time (TV_CONSTEXPR); @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, constexpr_global_ctx global_ctx; constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, allow_non_constant, strict, - manifestly_const_eval || !allow_non_constant, - uid_sensitive }; + manifestly_const_eval || !allow_non_constant }; tree type = initialized_type (t); tree r = t; @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, auto_vec<tree, 16> cleanups; global_ctx.cleanups = &cleanups; - if (!uid_sensitive) - instantiate_constexpr_fns (r); + instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); @@ -6909,14 +6955,12 @@ fold_simple (tree t) Otherwise, if T does not have TREE_CONSTANT set, returns T. Otherwise, returns a version of T without TREE_CONSTANT. MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated - as per P0595. UID_SENSITIVE is true if we can't do anything that - would affect DECL_UID ordering. */ + as per P0595. */ static GTY((deletable)) hash_map<tree, tree> *cv_cache; tree -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, - bool uid_sensitive) +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) { tree r; @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return t; if (manifestly_const_eval) - return cxx_eval_outermost_constant_expr (t, true, true, true, false, - decl, uid_sensitive); + return cxx_eval_outermost_constant_expr (t, true, true, true, false, decl); if (cv_cache == NULL) cv_cache = hash_map<tree, tree>::create_ggc (101); @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return r; } - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, - decl, uid_sensitive); + uid_sensitive_constexpr_evaluation_checker c; + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); gcc_checking_assert (r == t || CONVERT_EXPR_P (t) || TREE_CODE (t) == VIEW_CONVERT_EXPR || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) || !cp_tree_equal (r, t)); - cv_cache->put (t, r); + if (!c.evaluation_restricted_p ()) + cv_cache->put (t, r); return r; } diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index fc26a85f43a..f298fa7cec1 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -2443,6 +2443,8 @@ cp_fold (tree x) if (tree *cached = fold_cache->get (x)) return *cached; + uid_sensitive_constexpr_evaluation_checker c; + code = TREE_CODE (x); switch (code) { @@ -2925,10 +2927,13 @@ cp_fold (tree x) return org_x; } - fold_cache->put (org_x, x); - /* Prevent that we try to fold an already folded result again. */ - if (x != org_x) - fold_cache->put (x, x); + if (!c.evaluation_restricted_p ()) + { + fold_cache->put (org_x, x); + /* Prevent that we try to fold an already folded result again. */ + if (x != org_x) + fold_cache->put (x, x); + } return x; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index deb000babc1..c2511196de4 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7949,7 +7949,7 @@ extern bool require_potential_rvalue_constant_expression (tree); extern tree cxx_constant_value (tree, tree = NULL_TREE); extern void cxx_constant_dtor (tree, tree); extern tree cxx_constant_init (tree, tree = NULL_TREE); -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false, bool = false); +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false); extern tree maybe_constant_init (tree, tree = NULL_TREE, bool = false); extern tree fold_non_dependent_expr (tree, tsubst_flags_t = tf_warning_or_error, @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr (tree); extern void clear_cv_and_fold_caches (bool = true); extern tree unshare_constructor (tree CXX_MEM_STAT_INFO); +/* An RAII sentinel used to restrict constexpr evaluation so that it + doesn't do anything that causes extra DECL_UID generation. */ + +struct uid_sensitive_constexpr_evaluation_sentinel +{ + temp_override<bool> ovr; + uid_sensitive_constexpr_evaluation_sentinel (); +}; + +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was + called and returned true, indicating that we've restricted constexpr + evaluation in order to avoid UID generation. We use this to control + updates to the fold_cache and cv_cache. */ + +struct uid_sensitive_constexpr_evaluation_checker +{ + const unsigned saved_counter; + uid_sensitive_constexpr_evaluation_checker (); + bool evaluation_restricted_p () const; +}; + /* In cp-ubsan.c */ extern void cp_ubsan_maybe_instrument_member_call (tree); extern void cp_ubsan_instrument_member_accesses (tree *); diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c index 9b535708c57..0d68a738f8f 100644 --- a/gcc/cp/expr.c +++ b/gcc/cp/expr.c @@ -399,6 +399,8 @@ fold_for_warn (tree x) { /* C++ implementation. */ + uid_sensitive_constexpr_evaluation_sentinel s; + /* It's not generally safe to fully fold inside of a template, so call fold_non_dependent_expr instead. */ if (processing_template_decl) @@ -410,7 +412,7 @@ fold_for_warn (tree x) return f; } else if (cxx_dialect >= cxx11) - x = maybe_constant_value (x, NULL_TREE, false, true); + x = maybe_constant_value (x); return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL); } diff --git a/gcc/testsuite/g++.dg/warn/pr94038-2.C b/gcc/testsuite/g++.dg/warn/pr94038-2.C new file mode 100644 index 00000000000..a468cc055eb --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/pr94038-2.C @@ -0,0 +1,28 @@ +// PR c++/94038 +// { dg-do compile { target c++11 } } +// { dg-additional-options "-O -Wall" } + +static constexpr int x = 0; + +template<typename T> +constexpr const int& +foo() +{ + static_assert(T(1) == 0, ""); + return x; +} + +template<typename T> +constexpr const int& +bar() +{ + return foo<T>(); +} + +constexpr int +baz(int a) +{ + return a; +} + +static_assert(decltype(baz(bar<int>())){} == 0, "");
On 5/8/20 11:42 AM, Patrick Palka wrote: > On Wed, 6 May 2020, Patrick Palka wrote: > >> On Wed, 6 May 2020, Patrick Palka wrote: >> >>> On Tue, 5 May 2020, Patrick Palka wrote: >>> >>>> On Tue, 5 May 2020, Patrick Palka wrote: >>>> >>>>> Unfortunately, the previous fix to PR94038 is fragile. When the >>>>> argument to fold_for_warn is a bare CALL_EXPR, then all is well: the >>>>> result of maybe_constant_value from fold_for_warn (with >>>>> uid_sensitive=true) is reused via the cv_cache in the subsequent call to >>>>> maybe_constant_value from cp_fold (with uid_sensitive=false), so we >>>>> avoid instantiating bar<int>. >>>>> >>>>> But when the argument to fold_for_warn is more complex, e.g. an >>>>> INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to bar<int>() >>>>> returning const int& which we need to decay to int) then from >>>>> fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and from >>>>> cp_fold we call it on the CALL_EXPR, so there is no reuse via the >>>>> cv_cache and we therefore end up instantiating bar<int>. >>>>> >>>>> So for a more robust solution to this general issue of warning flags >>>>> affecting code generation, it seems that we need a way to globally avoid >>>>> template instantiation during constexpr evaluation whenever we're >>>>> performing warning-dependent folding. >>>>> >>>>> To that end, this patch replaces the flag constexpr_ctx::uid_sensitive >>>>> with a global flag uid_sensitive_constexpr_evaluation_p, and enables it >>>>> during fold_for_warn using an RAII helper. >>>>> >>>>> The patch also adds a counter that keeps track of the number of times >>>>> uid_sensitive_constexpr_evaluation_p is called, and we use this to >>>>> determine whether the result of constexpr evaluation depended on the >>>>> flag's value. This lets us safely update the cv_cache and fold_cache >>>>> from fold_for_warn in the most common case where the flag's value was >>>>> irrelevant during constexpr evaluation. > > Here's some statistics about about the fold cache and cv cache that were > collected when building the testsuite of range-v3 (with the default > build flags which include -O -Wall -Wextra, so warning-dependent folding > applies). > > The "naive solution" below refers to the one in which we would refrain > from updating the caches at the end of cp_fold/maybe_constant_value > whenever we're in uid-sensitive constexpr evaluation mode (i.e. whenever > we're called from fold_for_warn), regardless of whether constexpr > evaluation was actually restricted. > > > FOLD CACHE | cache hit | cache miss | cache miss | > | | cache updated | cache not updated | > ------------+-----------+---------------+-------------------+ > naive sol'n | 5060779 | 11374667 | 12887790 | > ------------------------------------------------------------+ > this patch | 6665242 | 19688975 | 199137 | > ------------+-----------+---------------+-------------------+ > > > CV CACHE | cache hit | cache miss | cache miss | > | | cache updated | cache not updated | > ------------+-----------+---------------+-------------------+ > naive sol'n | 76214 | 3637218 | 6889213 | > ------------------------------------------------------------+ > this patch | 215980 | 9882310 | 405513 | > ------------+-----------+---------------+-------------------+ > > -- >8 -- > > gcc/cp/ChangeLog: > > PR c++/94038 > * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. > (uid_sensitive_constexpr_evaluation_value): Define. > (uid_sensitive_constexpr_evaluation_true_counter): Define. > (uid_sensitive_constexpr_evaluation_p): Define. > (uid_sensitive_constexpr_evaluation_sentinel): Define its > constructor. > (uid_sensitive_constexpr_evaluation_checker): Define its > constructor and its evaluation_restricted_p method. > (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p > instead of constexpr_ctx::uid_sensitive. > (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it > last. Adjust call to get_fundef_copy. > (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the > counter if necessary. > (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' > parameter. Adjust function body accordingly. > (maybe_constant_value): Remove 'uid_sensitive' parameter and > adjust function body accordingly. Set up a > uid_sensitive_constexpr_evaluation_checker, and use it to > conditionally update the cv_cache. > * cp-gimplify.h (cp_fold): Set up a > uid_sensitive_constexpr_evaluation_checker, and use it to > conditionally update the fold_cache. > * cp-tree.h (maybe_constant_value): Update declaration. > (struct uid_sensitive_constexpr_evaluation_sentinel): Define. > (struct sensitive_constexpr_evaluation_checker): Define. > * expr.c (fold_for_warn): Set up a > uid_sensitive_constexpr_evaluation_sentinel before calling > the folding subroutines. Drop all but the first argument to > maybe_constant_value. > > gcc/testsuite/ChangeLog: > > PR c++/94038 > * g++.dg/warn/pr94038-2.C: New test. > --- > gcc/cp/constexpr.c | 92 ++++++++++++++++++++------- > gcc/cp/cp-gimplify.c | 13 ++-- > gcc/cp/cp-tree.h | 23 ++++++- > gcc/cp/expr.c | 4 +- > gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ > 5 files changed, 130 insertions(+), 30 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index 706d8a13d8e..d2b75ddaa19 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -1089,11 +1089,59 @@ struct constexpr_ctx { > bool strict; > /* Whether __builtin_is_constant_evaluated () should be true. */ > bool manifestly_const_eval; > - /* Whether we want to avoid doing anything that will cause extra DECL_UID > - generation. */ > - bool uid_sensitive; > }; > > +/* This internal flag controls whether we should avoid doing anything during > + constexpr evaluation that would cause extra DECL_UID generation, such as > + template instantiation and function copying. */ > + > +static bool uid_sensitive_constexpr_evaluation_value; > + > +/* An internal counter that keeps track of the number of times > + uid_sensitive_constexpr_evaluation_p returned true. */ > + > +static unsigned uid_sensitive_constexpr_evaluation_true_counter; > + > +/* The accessor for uid_sensitive_constexpr_evaluation_value which also > + increments the corresponding counter. */ > + > +static bool > +uid_sensitive_constexpr_evaluation_p () > +{ > + if (uid_sensitive_constexpr_evaluation_value) > + { > + ++uid_sensitive_constexpr_evaluation_true_counter; > + return true; > + } > + else > + return false; > +} > + > +uid_sensitive_constexpr_evaluation_sentinel > +::uid_sensitive_constexpr_evaluation_sentinel () > + : ovr (uid_sensitive_constexpr_evaluation_value, true) > +{ > +} > + > +uid_sensitive_constexpr_evaluation_checker > +::uid_sensitive_constexpr_evaluation_checker () > + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) > +{ > +} These two constructors need comments. > +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, > + and some constexpr evaluation was restricted due to u_s_c_e_p > + being called and returning true after this checker object was > + constructed. */ > + > +bool > +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () const > +{ > + return (uid_sensitive_constexpr_evaluation_value > + && saved_counter != uid_sensitive_constexpr_evaluation_true_counter); > +} > + > + > /* A table of all constexpr calls that have been evaluated by the > compiler in this translation unit. */ > > @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> *fundef_copies_table; > is parms, TYPE is result. */ > > static tree > -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) > +get_fundef_copy (constexpr_fundef *fundef) > { > tree copy; > bool existed; > @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) > } > else if (*slot == NULL_TREE) > { > - if (ctx->uid_sensitive) > + if (uid_sensitive_constexpr_evaluation_p ()) > return NULL_TREE; > > /* We've already used the function itself, so make a copy. */ > @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > > /* We can't defer instantiating the function any longer. */ > if (!DECL_INITIAL (fun) > - && !ctx->uid_sensitive > - && DECL_TEMPLOID_INSTANTIATION (fun)) > + && DECL_TEMPLOID_INSTANTIATION (fun) > + && !uid_sensitive_constexpr_evaluation_p ()) > { > location_t save_loc = input_location; > input_location = loc; > @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, > gcc_assert (at_eof >= 2 && ctx->quiet); > *non_constant_p = true; > } > - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) > + else if (tree copy = get_fundef_copy (new_call.fundef)) > { > tree body, parms, res; > releasing_vec ctors; > @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, void */*data*/) > && DECL_DECLARED_CONSTEXPR_P (*tp) > && !DECL_INITIAL (*tp) > && !trivial_fn_p (*tp) > - && DECL_TEMPLOID_INSTANTIATION (*tp)) > + && DECL_TEMPLOID_INSTANTIATION (*tp) > + && !uid_sensitive_constexpr_evaluation_p ()) > { > ++function_depth; > instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); > @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > bool strict = true, > bool manifestly_const_eval = false, > bool constexpr_dtor = false, > - tree object = NULL_TREE, > - bool uid_sensitive = false) > + tree object = NULL_TREE) > { > auto_timevar time (TV_CONSTEXPR); > > @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > constexpr_global_ctx global_ctx; > constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, > allow_non_constant, strict, > - manifestly_const_eval || !allow_non_constant, > - uid_sensitive }; > + manifestly_const_eval || !allow_non_constant }; > > tree type = initialized_type (t); > tree r = t; > @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, > auto_vec<tree, 16> cleanups; > global_ctx.cleanups = &cleanups; > > - if (!uid_sensitive) > - instantiate_constexpr_fns (r); > + instantiate_constexpr_fns (r); > r = cxx_eval_constant_expression (&ctx, r, > false, &non_constant_p, &overflow_p); > > @@ -6909,14 +6955,12 @@ fold_simple (tree t) > Otherwise, if T does not have TREE_CONSTANT set, returns T. > Otherwise, returns a version of T without TREE_CONSTANT. > MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated > - as per P0595. UID_SENSITIVE is true if we can't do anything that > - would affect DECL_UID ordering. */ > + as per P0595. */ > > static GTY((deletable)) hash_map<tree, tree> *cv_cache; > > tree > -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, > - bool uid_sensitive) > +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) > { > tree r; > > @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, > return t; > > if (manifestly_const_eval) > - return cxx_eval_outermost_constant_expr (t, true, true, true, false, > - decl, uid_sensitive); > + return cxx_eval_outermost_constant_expr (t, true, true, true, false, decl); > > if (cv_cache == NULL) > cv_cache = hash_map<tree, tree>::create_ggc (101); > @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, > return r; > } > > - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, > - decl, uid_sensitive); > + uid_sensitive_constexpr_evaluation_checker c; > + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); > gcc_checking_assert (r == t > || CONVERT_EXPR_P (t) > || TREE_CODE (t) == VIEW_CONVERT_EXPR > || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) > || !cp_tree_equal (r, t)); > - cv_cache->put (t, r); > + if (!c.evaluation_restricted_p ()) > + cv_cache->put (t, r); > return r; > } > > diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c > index fc26a85f43a..f298fa7cec1 100644 > --- a/gcc/cp/cp-gimplify.c > +++ b/gcc/cp/cp-gimplify.c > @@ -2443,6 +2443,8 @@ cp_fold (tree x) > if (tree *cached = fold_cache->get (x)) > return *cached; > > + uid_sensitive_constexpr_evaluation_checker c; > + > code = TREE_CODE (x); > switch (code) > { > @@ -2925,10 +2927,13 @@ cp_fold (tree x) > return org_x; > } > > - fold_cache->put (org_x, x); > - /* Prevent that we try to fold an already folded result again. */ > - if (x != org_x) > - fold_cache->put (x, x); > + if (!c.evaluation_restricted_p ()) > + { > + fold_cache->put (org_x, x); > + /* Prevent that we try to fold an already folded result again. */ > + if (x != org_x) > + fold_cache->put (x, x); > + } > > return x; > } > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index deb000babc1..c2511196de4 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -7949,7 +7949,7 @@ extern bool require_potential_rvalue_constant_expression (tree); > extern tree cxx_constant_value (tree, tree = NULL_TREE); > extern void cxx_constant_dtor (tree, tree); > extern tree cxx_constant_init (tree, tree = NULL_TREE); > -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false, bool = false); > +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false); > extern tree maybe_constant_init (tree, tree = NULL_TREE, bool = false); > extern tree fold_non_dependent_expr (tree, > tsubst_flags_t = tf_warning_or_error, > @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr (tree); > extern void clear_cv_and_fold_caches (bool = true); > extern tree unshare_constructor (tree CXX_MEM_STAT_INFO); > > +/* An RAII sentinel used to restrict constexpr evaluation so that it > + doesn't do anything that causes extra DECL_UID generation. */ > + > +struct uid_sensitive_constexpr_evaluation_sentinel > +{ > + temp_override<bool> ovr; > + uid_sensitive_constexpr_evaluation_sentinel (); > +}; > + > +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was > + called and returned true, indicating that we've restricted constexpr > + evaluation in order to avoid UID generation. We use this to control > + updates to the fold_cache and cv_cache. */ > + > +struct uid_sensitive_constexpr_evaluation_checker > +{ > + const unsigned saved_counter; > + uid_sensitive_constexpr_evaluation_checker (); > + bool evaluation_restricted_p () const; > +}; > + > /* In cp-ubsan.c */ > extern void cp_ubsan_maybe_instrument_member_call (tree); > extern void cp_ubsan_instrument_member_accesses (tree *); > diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c > index 9b535708c57..0d68a738f8f 100644 > --- a/gcc/cp/expr.c > +++ b/gcc/cp/expr.c > @@ -399,6 +399,8 @@ fold_for_warn (tree x) > { > /* C++ implementation. */ > > + uid_sensitive_constexpr_evaluation_sentinel s; Please add a comment about why we need this. OK with these comments added. > /* It's not generally safe to fully fold inside of a template, so > call fold_non_dependent_expr instead. */ > if (processing_template_decl) > @@ -410,7 +412,7 @@ fold_for_warn (tree x) > return f; > } > else if (cxx_dialect >= cxx11) > - x = maybe_constant_value (x, NULL_TREE, false, true); > + x = maybe_constant_value (x); > > return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL); > } > diff --git a/gcc/testsuite/g++.dg/warn/pr94038-2.C b/gcc/testsuite/g++.dg/warn/pr94038-2.C > new file mode 100644 > index 00000000000..a468cc055eb > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/pr94038-2.C > @@ -0,0 +1,28 @@ > +// PR c++/94038 > +// { dg-do compile { target c++11 } } > +// { dg-additional-options "-O -Wall" } > + > +static constexpr int x = 0; > + > +template<typename T> > +constexpr const int& > +foo() > +{ > + static_assert(T(1) == 0, ""); > + return x; > +} > + > +template<typename T> > +constexpr const int& > +bar() > +{ > + return foo<T>(); > +} > + > +constexpr int > +baz(int a) > +{ > + return a; > +} > + > +static_assert(decltype(baz(bar<int>())){} == 0, ""); >
On Tue, 19 May 2020, Jason Merrill wrote: > On 5/8/20 11:42 AM, Patrick Palka wrote: > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > > > Unfortunately, the previous fix to PR94038 is fragile. When the > > > > > > argument to fold_for_warn is a bare CALL_EXPR, then all is well: the > > > > > > result of maybe_constant_value from fold_for_warn (with > > > > > > uid_sensitive=true) is reused via the cv_cache in the subsequent > > > > > > call to > > > > > > maybe_constant_value from cp_fold (with uid_sensitive=false), so we > > > > > > avoid instantiating bar<int>. > > > > > > > > > > > > But when the argument to fold_for_warn is more complex, e.g. an > > > > > > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to > > > > > > bar<int>() > > > > > > returning const int& which we need to decay to int) then from > > > > > > fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and > > > > > > from > > > > > > cp_fold we call it on the CALL_EXPR, so there is no reuse via the > > > > > > cv_cache and we therefore end up instantiating bar<int>. > > > > > > > > > > > > So for a more robust solution to this general issue of warning flags > > > > > > affecting code generation, it seems that we need a way to globally > > > > > > avoid > > > > > > template instantiation during constexpr evaluation whenever we're > > > > > > performing warning-dependent folding. > > > > > > > > > > > > To that end, this patch replaces the flag > > > > > > constexpr_ctx::uid_sensitive > > > > > > with a global flag uid_sensitive_constexpr_evaluation_p, and enables > > > > > > it > > > > > > during fold_for_warn using an RAII helper. > > > > > > > > > > > > The patch also adds a counter that keeps track of the number of > > > > > > times > > > > > > uid_sensitive_constexpr_evaluation_p is called, and we use this to > > > > > > determine whether the result of constexpr evaluation depended on the > > > > > > flag's value. This lets us safely update the cv_cache and > > > > > > fold_cache > > > > > > from fold_for_warn in the most common case where the flag's value > > > > > > was > > > > > > irrelevant during constexpr evaluation. > > > > Here's some statistics about about the fold cache and cv cache that were > > collected when building the testsuite of range-v3 (with the default > > build flags which include -O -Wall -Wextra, so warning-dependent folding > > applies). > > > > The "naive solution" below refers to the one in which we would refrain > > from updating the caches at the end of cp_fold/maybe_constant_value > > whenever we're in uid-sensitive constexpr evaluation mode (i.e. whenever > > we're called from fold_for_warn), regardless of whether constexpr > > evaluation was actually restricted. > > > > > > FOLD CACHE | cache hit | cache miss | cache miss | > > | | cache updated | cache not updated | > > ------------+-----------+---------------+-------------------+ > > naive sol'n | 5060779 | 11374667 | 12887790 | > > ------------------------------------------------------------+ > > this patch | 6665242 | 19688975 | 199137 | > > ------------+-----------+---------------+-------------------+ > > > > > > CV CACHE | cache hit | cache miss | cache miss | > > | | cache updated | cache not updated | > > ------------+-----------+---------------+-------------------+ > > naive sol'n | 76214 | 3637218 | 6889213 | > > ------------------------------------------------------------+ > > this patch | 215980 | 9882310 | 405513 | > > ------------+-----------+---------------+-------------------+ > > > > -- >8 -- > > > > gcc/cp/ChangeLog: > > > > PR c++/94038 > > * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. > > (uid_sensitive_constexpr_evaluation_value): Define. > > (uid_sensitive_constexpr_evaluation_true_counter): Define. > > (uid_sensitive_constexpr_evaluation_p): Define. > > (uid_sensitive_constexpr_evaluation_sentinel): Define its > > constructor. > > (uid_sensitive_constexpr_evaluation_checker): Define its > > constructor and its evaluation_restricted_p method. > > (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p > > instead of constexpr_ctx::uid_sensitive. > > (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it > > last. Adjust call to get_fundef_copy. > > (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the > > counter if necessary. > > (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' > > parameter. Adjust function body accordingly. > > (maybe_constant_value): Remove 'uid_sensitive' parameter and > > adjust function body accordingly. Set up a > > uid_sensitive_constexpr_evaluation_checker, and use it to > > conditionally update the cv_cache. > > * cp-gimplify.h (cp_fold): Set up a > > uid_sensitive_constexpr_evaluation_checker, and use it to > > conditionally update the fold_cache. > > * cp-tree.h (maybe_constant_value): Update declaration. > > (struct uid_sensitive_constexpr_evaluation_sentinel): Define. > > (struct sensitive_constexpr_evaluation_checker): Define. > > * expr.c (fold_for_warn): Set up a > > uid_sensitive_constexpr_evaluation_sentinel before calling > > the folding subroutines. Drop all but the first argument to > > maybe_constant_value. > > > > gcc/testsuite/ChangeLog: > > > > PR c++/94038 > > * g++.dg/warn/pr94038-2.C: New test. > > --- > > gcc/cp/constexpr.c | 92 ++++++++++++++++++++------- > > gcc/cp/cp-gimplify.c | 13 ++-- > > gcc/cp/cp-tree.h | 23 ++++++- > > gcc/cp/expr.c | 4 +- > > gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ > > 5 files changed, 130 insertions(+), 30 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > index 706d8a13d8e..d2b75ddaa19 100644 > > --- a/gcc/cp/constexpr.c > > +++ b/gcc/cp/constexpr.c > > @@ -1089,11 +1089,59 @@ struct constexpr_ctx { > > bool strict; > > /* Whether __builtin_is_constant_evaluated () should be true. */ > > bool manifestly_const_eval; > > - /* Whether we want to avoid doing anything that will cause extra DECL_UID > > - generation. */ > > - bool uid_sensitive; > > }; > > +/* This internal flag controls whether we should avoid doing anything > > during > > + constexpr evaluation that would cause extra DECL_UID generation, such as > > + template instantiation and function copying. */ > > + > > +static bool uid_sensitive_constexpr_evaluation_value; > > + > > +/* An internal counter that keeps track of the number of times > > + uid_sensitive_constexpr_evaluation_p returned true. */ > > + > > +static unsigned uid_sensitive_constexpr_evaluation_true_counter; > > + > > +/* The accessor for uid_sensitive_constexpr_evaluation_value which also > > + increments the corresponding counter. */ > > + > > +static bool > > +uid_sensitive_constexpr_evaluation_p () > > +{ > > + if (uid_sensitive_constexpr_evaluation_value) > > + { > > + ++uid_sensitive_constexpr_evaluation_true_counter; > > + return true; > > + } > > + else > > + return false; > > +} > > + > > +uid_sensitive_constexpr_evaluation_sentinel > > +::uid_sensitive_constexpr_evaluation_sentinel () > > + : ovr (uid_sensitive_constexpr_evaluation_value, true) > > +{ > > +} > > + > > +uid_sensitive_constexpr_evaluation_checker > > +::uid_sensitive_constexpr_evaluation_checker () > > + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) > > +{ > > +} > > These two constructors need comments. > > > +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, > > + and some constexpr evaluation was restricted due to u_s_c_e_p > > + being called and returning true after this checker object was > > + constructed. */ > > + > > +bool > > +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () > > const > > +{ > > + return (uid_sensitive_constexpr_evaluation_value > > + && saved_counter != > > uid_sensitive_constexpr_evaluation_true_counter); > > +} > > + > > + > > /* A table of all constexpr calls that have been evaluated by the > > compiler in this translation unit. */ > > @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> > > *fundef_copies_table; > > is parms, TYPE is result. */ > > static tree > > -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) > > +get_fundef_copy (constexpr_fundef *fundef) > > { > > tree copy; > > bool existed; > > @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, > > constexpr_fundef *fundef) > > } > > else if (*slot == NULL_TREE) > > { > > - if (ctx->uid_sensitive) > > + if (uid_sensitive_constexpr_evaluation_p ()) > > return NULL_TREE; > > /* We've already used the function itself, so make a copy. */ > > @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, > > tree t, > > /* We can't defer instantiating the function any longer. */ > > if (!DECL_INITIAL (fun) > > - && !ctx->uid_sensitive > > - && DECL_TEMPLOID_INSTANTIATION (fun)) > > + && DECL_TEMPLOID_INSTANTIATION (fun) > > + && !uid_sensitive_constexpr_evaluation_p ()) > > { > > location_t save_loc = input_location; > > input_location = loc; > > @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, > > tree t, > > gcc_assert (at_eof >= 2 && ctx->quiet); > > *non_constant_p = true; > > } > > - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) > > + else if (tree copy = get_fundef_copy (new_call.fundef)) > > { > > tree body, parms, res; > > releasing_vec ctors; > > @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, > > void */*data*/) > > && DECL_DECLARED_CONSTEXPR_P (*tp) > > && !DECL_INITIAL (*tp) > > && !trivial_fn_p (*tp) > > - && DECL_TEMPLOID_INSTANTIATION (*tp)) > > + && DECL_TEMPLOID_INSTANTIATION (*tp) > > + && !uid_sensitive_constexpr_evaluation_p ()) > > { > > ++function_depth; > > instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); > > @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > allow_non_constant, > > bool strict = true, > > bool manifestly_const_eval = false, > > bool constexpr_dtor = false, > > - tree object = NULL_TREE, > > - bool uid_sensitive = false) > > + tree object = NULL_TREE) > > { > > auto_timevar time (TV_CONSTEXPR); > > @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > allow_non_constant, > > constexpr_global_ctx global_ctx; > > constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, > > allow_non_constant, strict, > > - manifestly_const_eval || !allow_non_constant, > > - uid_sensitive }; > > + manifestly_const_eval || !allow_non_constant }; > > tree type = initialized_type (t); > > tree r = t; > > @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > allow_non_constant, > > auto_vec<tree, 16> cleanups; > > global_ctx.cleanups = &cleanups; > > - if (!uid_sensitive) > > - instantiate_constexpr_fns (r); > > + instantiate_constexpr_fns (r); > > r = cxx_eval_constant_expression (&ctx, r, > > false, &non_constant_p, &overflow_p); > > @@ -6909,14 +6955,12 @@ fold_simple (tree t) > > Otherwise, if T does not have TREE_CONSTANT set, returns T. > > Otherwise, returns a version of T without TREE_CONSTANT. > > MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated > > - as per P0595. UID_SENSITIVE is true if we can't do anything that > > - would affect DECL_UID ordering. */ > > + as per P0595. */ > > static GTY((deletable)) hash_map<tree, tree> *cv_cache; > > tree > > -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, > > - bool uid_sensitive) > > +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) > > { > > tree r; > > @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, bool > > manifestly_const_eval, > > return t; > > if (manifestly_const_eval) > > - return cxx_eval_outermost_constant_expr (t, true, true, true, false, > > - decl, uid_sensitive); > > + return cxx_eval_outermost_constant_expr (t, true, true, true, false, > > decl); > > if (cv_cache == NULL) > > cv_cache = hash_map<tree, tree>::create_ggc (101); > > @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, bool > > manifestly_const_eval, > > return r; > > } > > - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, > > - decl, uid_sensitive); > > + uid_sensitive_constexpr_evaluation_checker c; > > + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); > > gcc_checking_assert (r == t > > || CONVERT_EXPR_P (t) > > || TREE_CODE (t) == VIEW_CONVERT_EXPR > > || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) > > || !cp_tree_equal (r, t)); > > - cv_cache->put (t, r); > > + if (!c.evaluation_restricted_p ()) > > + cv_cache->put (t, r); > > return r; > > } > > diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c > > index fc26a85f43a..f298fa7cec1 100644 > > --- a/gcc/cp/cp-gimplify.c > > +++ b/gcc/cp/cp-gimplify.c > > @@ -2443,6 +2443,8 @@ cp_fold (tree x) > > if (tree *cached = fold_cache->get (x)) > > return *cached; > > + uid_sensitive_constexpr_evaluation_checker c; > > + > > code = TREE_CODE (x); > > switch (code) > > { > > @@ -2925,10 +2927,13 @@ cp_fold (tree x) > > return org_x; > > } > > - fold_cache->put (org_x, x); > > - /* Prevent that we try to fold an already folded result again. */ > > - if (x != org_x) > > - fold_cache->put (x, x); > > + if (!c.evaluation_restricted_p ()) > > + { > > + fold_cache->put (org_x, x); > > + /* Prevent that we try to fold an already folded result again. */ > > + if (x != org_x) > > + fold_cache->put (x, x); > > + } > > return x; > > } > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > index deb000babc1..c2511196de4 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -7949,7 +7949,7 @@ extern bool > > require_potential_rvalue_constant_expression (tree); > > extern tree cxx_constant_value (tree, tree = > > NULL_TREE); > > extern void cxx_constant_dtor (tree, tree); > > extern tree cxx_constant_init (tree, tree = > > NULL_TREE); > > -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool > > = false, bool = false); > > +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool > > = false); > > extern tree maybe_constant_init (tree, tree = > > NULL_TREE, bool = false); > > extern tree fold_non_dependent_expr (tree, > > tsubst_flags_t = > > tf_warning_or_error, > > @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr > > (tree); > > extern void clear_cv_and_fold_caches (bool = true); > > extern tree unshare_constructor (tree > > CXX_MEM_STAT_INFO); > > +/* An RAII sentinel used to restrict constexpr evaluation so that it > > + doesn't do anything that causes extra DECL_UID generation. */ > > + > > +struct uid_sensitive_constexpr_evaluation_sentinel > > +{ > > + temp_override<bool> ovr; > > + uid_sensitive_constexpr_evaluation_sentinel (); > > +}; > > + > > +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was > > + called and returned true, indicating that we've restricted constexpr > > + evaluation in order to avoid UID generation. We use this to control > > + updates to the fold_cache and cv_cache. */ > > + > > +struct uid_sensitive_constexpr_evaluation_checker > > +{ > > + const unsigned saved_counter; > > + uid_sensitive_constexpr_evaluation_checker (); > > + bool evaluation_restricted_p () const; > > +}; > > + > > /* In cp-ubsan.c */ > > extern void cp_ubsan_maybe_instrument_member_call (tree); > > extern void cp_ubsan_instrument_member_accesses (tree *); > > diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c > > index 9b535708c57..0d68a738f8f 100644 > > --- a/gcc/cp/expr.c > > +++ b/gcc/cp/expr.c > > @@ -399,6 +399,8 @@ fold_for_warn (tree x) > > { > > /* C++ implementation. */ > > + uid_sensitive_constexpr_evaluation_sentinel s; > > Please add a comment about why we need this. Thanks for the review. Here's the final patch (with the added comments) that I've committed: -- >8 -- Subject: [PATCH] c++: template instantiation during fold_for_warn [PR94038] Unfortunately, the previous fix to PR94038 is fragile. When the argument to fold_for_warn is a bare CALL_EXPR, then all is well: the result of maybe_constant_value from fold_for_warn (with uid_sensitive=true) is reused via the cv_cache in the subsequent call to maybe_constant_value from cp_fold (with uid_sensitive=false), so we avoid instantiating bar<int>. But when the argument to fold_for_warn is more complex, e.g. an INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to bar<int>() returning const int& which we need to decay to int) then from fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and from cp_fold we call it on the CALL_EXPR, so there is no reuse via the cv_cache and we therefore end up instantiating bar<int>. So for a more robust solution to this general issue of warning flags affecting code generation, it seems that we need a way to globally avoid template instantiation during constexpr evaluation whenever we're performing warning-dependent folding. To that end, this patch replaces the flag constexpr_ctx::uid_sensitive with a global flag uid_sensitive_constexpr_evaluation_p, and enables it during fold_for_warn using an RAII helper. The patch also adds a counter that keeps track of the number of times uid_sensitive_constexpr_evaluation_p is called and returned true, and we use this to determine whether the result of constexpr evaluation was restricted by the flag. This lets us safely update the cv_cache and fold_cache from fold_for_warn in the most common case where the flag did not restrict constexpr evaluation. gcc/cp/ChangeLog: PR c++/94038 * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. (uid_sensitive_constexpr_evaluation_value): Define. (uid_sensitive_constexpr_evaluation_true_counter): Define. (uid_sensitive_constexpr_evaluation_p): Define. (uid_sensitive_constexpr_evaluation_sentinel): Define its constructor. (uid_sensitive_constexpr_evaluation_checker): Define its constructor and its evaluation_restricted_p method. (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p instead of constexpr_ctx::uid_sensitive. (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it last. Adjust call to get_fundef_copy. (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the counter if necessary. (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' parameter. Adjust function body accordingly. (maybe_constant_value): Remove 'uid_sensitive' parameter and adjust function body accordingly. Set up a uid_sensitive_constexpr_evaluation_checker, and use it to conditionally update the cv_cache. * cp-gimplify.h (cp_fold): Set up a uid_sensitive_constexpr_evaluation_checker, and use it to conditionally update the fold_cache. * cp-tree.h (maybe_constant_value): Update declaration. (struct uid_sensitive_constexpr_evaluation_sentinel): Define. (struct sensitive_constexpr_evaluation_checker): Define. * expr.c (fold_for_warn): Set up a uid_sensitive_constexpr_evaluation_sentinel before calling the folding subroutines. Drop all but the first argument to maybe_constant_value. gcc/testsuite/ChangeLog: PR c++/94038 * g++.dg/warn/pr94038-2.C: New test. --- gcc/cp/constexpr.c | 100 +++++++++++++++++++------- gcc/cp/cp-gimplify.c | 13 ++-- gcc/cp/cp-tree.h | 23 +++++- gcc/cp/expr.c | 7 +- gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ 5 files changed, 141 insertions(+), 30 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 04faabc0258..d8e524b0aa7 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1089,11 +1089,67 @@ struct constexpr_ctx { bool strict; /* Whether __builtin_is_constant_evaluated () should be true. */ bool manifestly_const_eval; - /* Whether we want to avoid doing anything that will cause extra DECL_UID - generation. */ - bool uid_sensitive; }; +/* This internal flag controls whether we should avoid doing anything during + constexpr evaluation that would cause extra DECL_UID generation, such as + template instantiation and function body copying. */ + +static bool uid_sensitive_constexpr_evaluation_value; + +/* An internal counter that keeps track of the number of times + uid_sensitive_constexpr_evaluation_p returned true. */ + +static unsigned uid_sensitive_constexpr_evaluation_true_counter; + +/* The accessor for uid_sensitive_constexpr_evaluation_value which also + increments the corresponding counter. */ + +static bool +uid_sensitive_constexpr_evaluation_p () +{ + if (uid_sensitive_constexpr_evaluation_value) + { + ++uid_sensitive_constexpr_evaluation_true_counter; + return true; + } + else + return false; +} + +/* The default constructor for uid_sensitive_constexpr_evaluation_sentinel + sets the internal flag for uid_sensitive_constexpr_evaluation_p to true + during the lifetime of the sentinel object. Upon its destruction, the + previous value of uid_sensitive_constexpr_evaluation_p is restored. */ + +uid_sensitive_constexpr_evaluation_sentinel +::uid_sensitive_constexpr_evaluation_sentinel () + : ovr (uid_sensitive_constexpr_evaluation_value, true) +{ +} + +/* The default constructor for uid_sensitive_constexpr_evaluation_checker + records the current number of times that uid_sensitive_constexpr_evaluation_p + has been called and returned true. */ + +uid_sensitive_constexpr_evaluation_checker +::uid_sensitive_constexpr_evaluation_checker () + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) +{ +} + +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, and + some constexpr evaluation was restricted due to u_s_c_e_p being called + and returning true during the lifetime of this checker object. */ + +bool +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () const +{ + return (uid_sensitive_constexpr_evaluation_value + && saved_counter != uid_sensitive_constexpr_evaluation_true_counter); +} + + /* A table of all constexpr calls that have been evaluated by the compiler in this translation unit. */ @@ -1156,7 +1212,7 @@ static GTY(()) hash_map<tree, tree> *fundef_copies_table; is parms, TYPE is result. */ static tree -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) +get_fundef_copy (constexpr_fundef *fundef) { tree copy; bool existed; @@ -1173,7 +1229,7 @@ get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) } else if (*slot == NULL_TREE) { - if (ctx->uid_sensitive) + if (uid_sensitive_constexpr_evaluation_p ()) return NULL_TREE; /* We've already used the function itself, so make a copy. */ @@ -2292,8 +2348,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, /* We can't defer instantiating the function any longer. */ if (!DECL_INITIAL (fun) - && !ctx->uid_sensitive - && DECL_TEMPLOID_INSTANTIATION (fun)) + && DECL_TEMPLOID_INSTANTIATION (fun) + && !uid_sensitive_constexpr_evaluation_p ()) { location_t save_loc = input_location; input_location = loc; @@ -2454,7 +2510,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, gcc_assert (at_eof >= 2 && ctx->quiet); *non_constant_p = true; } - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) + else if (tree copy = get_fundef_copy (new_call.fundef)) { tree body, parms, res; releasing_vec ctors; @@ -6485,7 +6541,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, void */*data*/) && DECL_DECLARED_CONSTEXPR_P (*tp) && !DECL_INITIAL (*tp) && !trivial_fn_p (*tp) - && DECL_TEMPLOID_INSTANTIATION (*tp)) + && DECL_TEMPLOID_INSTANTIATION (*tp) + && !uid_sensitive_constexpr_evaluation_p ()) { ++function_depth; instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); @@ -6552,8 +6609,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, bool strict = true, bool manifestly_const_eval = false, bool constexpr_dtor = false, - tree object = NULL_TREE, - bool uid_sensitive = false) + tree object = NULL_TREE) { auto_timevar time (TV_CONSTEXPR); @@ -6569,8 +6625,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, constexpr_global_ctx global_ctx; constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, allow_non_constant, strict, - manifestly_const_eval || !allow_non_constant, - uid_sensitive }; + manifestly_const_eval || !allow_non_constant }; tree type = initialized_type (t); tree r = t; @@ -6660,8 +6715,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, auto_vec<tree, 16> cleanups; global_ctx.cleanups = &cleanups; - if (!uid_sensitive) - instantiate_constexpr_fns (r); + instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); @@ -6909,14 +6963,12 @@ fold_simple (tree t) Otherwise, if T does not have TREE_CONSTANT set, returns T. Otherwise, returns a version of T without TREE_CONSTANT. MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated - as per P0595. UID_SENSITIVE is true if we can't do anything that - would affect DECL_UID ordering. */ + as per P0595. */ static GTY((deletable)) hash_map<tree, tree> *cv_cache; tree -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, - bool uid_sensitive) +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) { tree r; @@ -6934,8 +6986,7 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return t; if (manifestly_const_eval) - return cxx_eval_outermost_constant_expr (t, true, true, true, false, - decl, uid_sensitive); + return cxx_eval_outermost_constant_expr (t, true, true, true, false, decl); if (cv_cache == NULL) cv_cache = hash_map<tree, tree>::create_ggc (101); @@ -6950,14 +7001,15 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return r; } - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, - decl, uid_sensitive); + uid_sensitive_constexpr_evaluation_checker c; + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); gcc_checking_assert (r == t || CONVERT_EXPR_P (t) || TREE_CODE (t) == VIEW_CONVERT_EXPR || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) || !cp_tree_equal (r, t)); - cv_cache->put (t, r); + if (!c.evaluation_restricted_p ()) + cv_cache->put (t, r); return r; } diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index ede98929179..2804958c246 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -2447,6 +2447,8 @@ cp_fold (tree x) if (tree *cached = fold_cache->get (x)) return *cached; + uid_sensitive_constexpr_evaluation_checker c; + code = TREE_CODE (x); switch (code) { @@ -2929,10 +2931,13 @@ cp_fold (tree x) return org_x; } - fold_cache->put (org_x, x); - /* Prevent that we try to fold an already folded result again. */ - if (x != org_x) - fold_cache->put (x, x); + if (!c.evaluation_restricted_p ()) + { + fold_cache->put (org_x, x); + /* Prevent that we try to fold an already folded result again. */ + if (x != org_x) + fold_cache->put (x, x); + } return x; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 27707abaadc..07c16144c98 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7950,7 +7950,7 @@ extern bool require_potential_rvalue_constant_expression (tree); extern tree cxx_constant_value (tree, tree = NULL_TREE); extern void cxx_constant_dtor (tree, tree); extern tree cxx_constant_init (tree, tree = NULL_TREE); -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false, bool = false); +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false); extern tree maybe_constant_init (tree, tree = NULL_TREE, bool = false); extern tree fold_non_dependent_expr (tree, tsubst_flags_t = tf_warning_or_error, @@ -7969,6 +7969,27 @@ extern tree fold_sizeof_expr (tree); extern void clear_cv_and_fold_caches (bool = true); extern tree unshare_constructor (tree CXX_MEM_STAT_INFO); +/* An RAII sentinel used to restrict constexpr evaluation so that it + doesn't do anything that causes extra DECL_UID generation. */ + +struct uid_sensitive_constexpr_evaluation_sentinel +{ + temp_override<bool> ovr; + uid_sensitive_constexpr_evaluation_sentinel (); +}; + +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was + called and returned true, indicating that we've restricted constexpr + evaluation in order to avoid UID generation. We use this to control + updates to the fold_cache and cv_cache. */ + +struct uid_sensitive_constexpr_evaluation_checker +{ + const unsigned saved_counter; + uid_sensitive_constexpr_evaluation_checker (); + bool evaluation_restricted_p () const; +}; + /* In cp-ubsan.c */ extern void cp_ubsan_maybe_instrument_member_call (tree); extern void cp_ubsan_instrument_member_accesses (tree *); diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c index 9b535708c57..2be2d897f5e 100644 --- a/gcc/cp/expr.c +++ b/gcc/cp/expr.c @@ -399,6 +399,11 @@ fold_for_warn (tree x) { /* C++ implementation. */ + /* Prevent warning-dependent constexpr evaluation from changing + DECL_UID (which breaks -fcompare-debug) and from instantiating + uninstantiated templates. */ + uid_sensitive_constexpr_evaluation_sentinel s; + /* It's not generally safe to fully fold inside of a template, so call fold_non_dependent_expr instead. */ if (processing_template_decl) @@ -410,7 +415,7 @@ fold_for_warn (tree x) return f; } else if (cxx_dialect >= cxx11) - x = maybe_constant_value (x, NULL_TREE, false, true); + x = maybe_constant_value (x); return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL); } diff --git a/gcc/testsuite/g++.dg/warn/pr94038-2.C b/gcc/testsuite/g++.dg/warn/pr94038-2.C new file mode 100644 index 00000000000..a468cc055eb --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/pr94038-2.C @@ -0,0 +1,28 @@ +// PR c++/94038 +// { dg-do compile { target c++11 } } +// { dg-additional-options "-O -Wall" } + +static constexpr int x = 0; + +template<typename T> +constexpr const int& +foo() +{ + static_assert(T(1) == 0, ""); + return x; +} + +template<typename T> +constexpr const int& +bar() +{ + return foo<T>(); +} + +constexpr int +baz(int a) +{ + return a; +} + +static_assert(decltype(baz(bar<int>())){} == 0, "");
On Wed, 20 May 2020, Patrick Palka wrote: > On Tue, 19 May 2020, Jason Merrill wrote: > > > On 5/8/20 11:42 AM, Patrick Palka wrote: > > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > Unfortunately, the previous fix to PR94038 is fragile. When the > > > > > > > argument to fold_for_warn is a bare CALL_EXPR, then all is well: the > > > > > > > result of maybe_constant_value from fold_for_warn (with > > > > > > > uid_sensitive=true) is reused via the cv_cache in the subsequent > > > > > > > call to > > > > > > > maybe_constant_value from cp_fold (with uid_sensitive=false), so we > > > > > > > avoid instantiating bar<int>. > > > > > > > > > > > > > > But when the argument to fold_for_warn is more complex, e.g. an > > > > > > > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to > > > > > > > bar<int>() > > > > > > > returning const int& which we need to decay to int) then from > > > > > > > fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and > > > > > > > from > > > > > > > cp_fold we call it on the CALL_EXPR, so there is no reuse via the > > > > > > > cv_cache and we therefore end up instantiating bar<int>. > > > > > > > > > > > > > > So for a more robust solution to this general issue of warning flags > > > > > > > affecting code generation, it seems that we need a way to globally > > > > > > > avoid > > > > > > > template instantiation during constexpr evaluation whenever we're > > > > > > > performing warning-dependent folding. > > > > > > > > > > > > > > To that end, this patch replaces the flag > > > > > > > constexpr_ctx::uid_sensitive > > > > > > > with a global flag uid_sensitive_constexpr_evaluation_p, and enables > > > > > > > it > > > > > > > during fold_for_warn using an RAII helper. > > > > > > > > > > > > > > The patch also adds a counter that keeps track of the number of > > > > > > > times > > > > > > > uid_sensitive_constexpr_evaluation_p is called, and we use this to > > > > > > > determine whether the result of constexpr evaluation depended on the > > > > > > > flag's value. This lets us safely update the cv_cache and > > > > > > > fold_cache > > > > > > > from fold_for_warn in the most common case where the flag's value > > > > > > > was > > > > > > > irrelevant during constexpr evaluation. > > > > > > Here's some statistics about about the fold cache and cv cache that were > > > collected when building the testsuite of range-v3 (with the default > > > build flags which include -O -Wall -Wextra, so warning-dependent folding > > > applies). > > > > > > The "naive solution" below refers to the one in which we would refrain > > > from updating the caches at the end of cp_fold/maybe_constant_value > > > whenever we're in uid-sensitive constexpr evaluation mode (i.e. whenever > > > we're called from fold_for_warn), regardless of whether constexpr > > > evaluation was actually restricted. > > > > > > > > > FOLD CACHE | cache hit | cache miss | cache miss | > > > | | cache updated | cache not updated | > > > ------------+-----------+---------------+-------------------+ > > > naive sol'n | 5060779 | 11374667 | 12887790 | > > > ------------------------------------------------------------+ > > > this patch | 6665242 | 19688975 | 199137 | > > > ------------+-----------+---------------+-------------------+ > > > > > > > > > CV CACHE | cache hit | cache miss | cache miss | > > > | | cache updated | cache not updated | > > > ------------+-----------+---------------+-------------------+ > > > naive sol'n | 76214 | 3637218 | 6889213 | > > > ------------------------------------------------------------+ > > > this patch | 215980 | 9882310 | 405513 | > > > ------------+-----------+---------------+-------------------+ > > > > > > -- >8 -- > > > > > > gcc/cp/ChangeLog: > > > > > > PR c++/94038 > > > * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. > > > (uid_sensitive_constexpr_evaluation_value): Define. > > > (uid_sensitive_constexpr_evaluation_true_counter): Define. > > > (uid_sensitive_constexpr_evaluation_p): Define. > > > (uid_sensitive_constexpr_evaluation_sentinel): Define its > > > constructor. > > > (uid_sensitive_constexpr_evaluation_checker): Define its > > > constructor and its evaluation_restricted_p method. > > > (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p > > > instead of constexpr_ctx::uid_sensitive. > > > (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it > > > last. Adjust call to get_fundef_copy. > > > (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the > > > counter if necessary. > > > (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' > > > parameter. Adjust function body accordingly. > > > (maybe_constant_value): Remove 'uid_sensitive' parameter and > > > adjust function body accordingly. Set up a > > > uid_sensitive_constexpr_evaluation_checker, and use it to > > > conditionally update the cv_cache. > > > * cp-gimplify.h (cp_fold): Set up a > > > uid_sensitive_constexpr_evaluation_checker, and use it to > > > conditionally update the fold_cache. > > > * cp-tree.h (maybe_constant_value): Update declaration. > > > (struct uid_sensitive_constexpr_evaluation_sentinel): Define. > > > (struct sensitive_constexpr_evaluation_checker): Define. > > > * expr.c (fold_for_warn): Set up a > > > uid_sensitive_constexpr_evaluation_sentinel before calling > > > the folding subroutines. Drop all but the first argument to > > > maybe_constant_value. > > > > > > gcc/testsuite/ChangeLog: > > > > > > PR c++/94038 > > > * g++.dg/warn/pr94038-2.C: New test. > > > --- > > > gcc/cp/constexpr.c | 92 ++++++++++++++++++++------- > > > gcc/cp/cp-gimplify.c | 13 ++-- > > > gcc/cp/cp-tree.h | 23 ++++++- > > > gcc/cp/expr.c | 4 +- > > > gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ > > > 5 files changed, 130 insertions(+), 30 deletions(-) > > > create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > index 706d8a13d8e..d2b75ddaa19 100644 > > > --- a/gcc/cp/constexpr.c > > > +++ b/gcc/cp/constexpr.c > > > @@ -1089,11 +1089,59 @@ struct constexpr_ctx { > > > bool strict; > > > /* Whether __builtin_is_constant_evaluated () should be true. */ > > > bool manifestly_const_eval; > > > - /* Whether we want to avoid doing anything that will cause extra DECL_UID > > > - generation. */ > > > - bool uid_sensitive; > > > }; > > > +/* This internal flag controls whether we should avoid doing anything > > > during > > > + constexpr evaluation that would cause extra DECL_UID generation, such as > > > + template instantiation and function copying. */ > > > + > > > +static bool uid_sensitive_constexpr_evaluation_value; > > > + > > > +/* An internal counter that keeps track of the number of times > > > + uid_sensitive_constexpr_evaluation_p returned true. */ > > > + > > > +static unsigned uid_sensitive_constexpr_evaluation_true_counter; > > > + > > > +/* The accessor for uid_sensitive_constexpr_evaluation_value which also > > > + increments the corresponding counter. */ > > > + > > > +static bool > > > +uid_sensitive_constexpr_evaluation_p () > > > +{ > > > + if (uid_sensitive_constexpr_evaluation_value) > > > + { > > > + ++uid_sensitive_constexpr_evaluation_true_counter; > > > + return true; > > > + } > > > + else > > > + return false; > > > +} > > > + > > > +uid_sensitive_constexpr_evaluation_sentinel > > > +::uid_sensitive_constexpr_evaluation_sentinel () > > > + : ovr (uid_sensitive_constexpr_evaluation_value, true) > > > +{ > > > +} > > > + > > > +uid_sensitive_constexpr_evaluation_checker > > > +::uid_sensitive_constexpr_evaluation_checker () > > > + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) > > > +{ > > > +} > > > > These two constructors need comments. > > > > > +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, > > > + and some constexpr evaluation was restricted due to u_s_c_e_p > > > + being called and returning true after this checker object was > > > + constructed. */ > > > + > > > +bool > > > +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () > > > const > > > +{ > > > + return (uid_sensitive_constexpr_evaluation_value > > > + && saved_counter != > > > uid_sensitive_constexpr_evaluation_true_counter); > > > +} > > > + > > > + > > > /* A table of all constexpr calls that have been evaluated by the > > > compiler in this translation unit. */ > > > @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> > > > *fundef_copies_table; > > > is parms, TYPE is result. */ > > > static tree > > > -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) > > > +get_fundef_copy (constexpr_fundef *fundef) > > > { > > > tree copy; > > > bool existed; > > > @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, > > > constexpr_fundef *fundef) > > > } > > > else if (*slot == NULL_TREE) > > > { > > > - if (ctx->uid_sensitive) > > > + if (uid_sensitive_constexpr_evaluation_p ()) > > > return NULL_TREE; > > > /* We've already used the function itself, so make a copy. */ > > > @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, > > > tree t, > > > /* We can't defer instantiating the function any longer. */ > > > if (!DECL_INITIAL (fun) > > > - && !ctx->uid_sensitive > > > - && DECL_TEMPLOID_INSTANTIATION (fun)) > > > + && DECL_TEMPLOID_INSTANTIATION (fun) > > > + && !uid_sensitive_constexpr_evaluation_p ()) > > > { > > > location_t save_loc = input_location; > > > input_location = loc; > > > @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, > > > tree t, > > > gcc_assert (at_eof >= 2 && ctx->quiet); > > > *non_constant_p = true; > > > } > > > - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) > > > + else if (tree copy = get_fundef_copy (new_call.fundef)) > > > { > > > tree body, parms, res; > > > releasing_vec ctors; > > > @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, > > > void */*data*/) > > > && DECL_DECLARED_CONSTEXPR_P (*tp) > > > && !DECL_INITIAL (*tp) > > > && !trivial_fn_p (*tp) > > > - && DECL_TEMPLOID_INSTANTIATION (*tp)) > > > + && DECL_TEMPLOID_INSTANTIATION (*tp) > > > + && !uid_sensitive_constexpr_evaluation_p ()) > > > { > > > ++function_depth; > > > instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); > > > @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > > allow_non_constant, > > > bool strict = true, > > > bool manifestly_const_eval = false, > > > bool constexpr_dtor = false, > > > - tree object = NULL_TREE, > > > - bool uid_sensitive = false) > > > + tree object = NULL_TREE) > > > { > > > auto_timevar time (TV_CONSTEXPR); > > > @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > > allow_non_constant, > > > constexpr_global_ctx global_ctx; > > > constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, > > > allow_non_constant, strict, > > > - manifestly_const_eval || !allow_non_constant, > > > - uid_sensitive }; > > > + manifestly_const_eval || !allow_non_constant }; > > > tree type = initialized_type (t); > > > tree r = t; > > > @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > > allow_non_constant, > > > auto_vec<tree, 16> cleanups; > > > global_ctx.cleanups = &cleanups; > > > - if (!uid_sensitive) > > > - instantiate_constexpr_fns (r); > > > + instantiate_constexpr_fns (r); > > > r = cxx_eval_constant_expression (&ctx, r, > > > false, &non_constant_p, &overflow_p); > > > @@ -6909,14 +6955,12 @@ fold_simple (tree t) > > > Otherwise, if T does not have TREE_CONSTANT set, returns T. > > > Otherwise, returns a version of T without TREE_CONSTANT. > > > MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated > > > - as per P0595. UID_SENSITIVE is true if we can't do anything that > > > - would affect DECL_UID ordering. */ > > > + as per P0595. */ > > > static GTY((deletable)) hash_map<tree, tree> *cv_cache; > > > tree > > > -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, > > > - bool uid_sensitive) > > > +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) > > > { > > > tree r; > > > @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, bool > > > manifestly_const_eval, > > > return t; > > > if (manifestly_const_eval) > > > - return cxx_eval_outermost_constant_expr (t, true, true, true, false, > > > - decl, uid_sensitive); > > > + return cxx_eval_outermost_constant_expr (t, true, true, true, false, > > > decl); > > > if (cv_cache == NULL) > > > cv_cache = hash_map<tree, tree>::create_ggc (101); > > > @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, bool > > > manifestly_const_eval, > > > return r; > > > } > > > - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, > > > - decl, uid_sensitive); > > > + uid_sensitive_constexpr_evaluation_checker c; > > > + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); > > > gcc_checking_assert (r == t > > > || CONVERT_EXPR_P (t) > > > || TREE_CODE (t) == VIEW_CONVERT_EXPR > > > || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) > > > || !cp_tree_equal (r, t)); > > > - cv_cache->put (t, r); > > > + if (!c.evaluation_restricted_p ()) > > > + cv_cache->put (t, r); > > > return r; > > > } > > > diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c > > > index fc26a85f43a..f298fa7cec1 100644 > > > --- a/gcc/cp/cp-gimplify.c > > > +++ b/gcc/cp/cp-gimplify.c > > > @@ -2443,6 +2443,8 @@ cp_fold (tree x) > > > if (tree *cached = fold_cache->get (x)) > > > return *cached; > > > + uid_sensitive_constexpr_evaluation_checker c; > > > + > > > code = TREE_CODE (x); > > > switch (code) > > > { > > > @@ -2925,10 +2927,13 @@ cp_fold (tree x) > > > return org_x; > > > } > > > - fold_cache->put (org_x, x); > > > - /* Prevent that we try to fold an already folded result again. */ > > > - if (x != org_x) > > > - fold_cache->put (x, x); > > > + if (!c.evaluation_restricted_p ()) > > > + { > > > + fold_cache->put (org_x, x); > > > + /* Prevent that we try to fold an already folded result again. */ > > > + if (x != org_x) > > > + fold_cache->put (x, x); > > > + } > > > return x; > > > } > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > > index deb000babc1..c2511196de4 100644 > > > --- a/gcc/cp/cp-tree.h > > > +++ b/gcc/cp/cp-tree.h > > > @@ -7949,7 +7949,7 @@ extern bool > > > require_potential_rvalue_constant_expression (tree); > > > extern tree cxx_constant_value (tree, tree = > > > NULL_TREE); > > > extern void cxx_constant_dtor (tree, tree); > > > extern tree cxx_constant_init (tree, tree = > > > NULL_TREE); > > > -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool > > > = false, bool = false); > > > +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool > > > = false); > > > extern tree maybe_constant_init (tree, tree = > > > NULL_TREE, bool = false); > > > extern tree fold_non_dependent_expr (tree, > > > tsubst_flags_t = > > > tf_warning_or_error, > > > @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr > > > (tree); > > > extern void clear_cv_and_fold_caches (bool = true); > > > extern tree unshare_constructor (tree > > > CXX_MEM_STAT_INFO); > > > +/* An RAII sentinel used to restrict constexpr evaluation so that it > > > + doesn't do anything that causes extra DECL_UID generation. */ > > > + > > > +struct uid_sensitive_constexpr_evaluation_sentinel > > > +{ > > > + temp_override<bool> ovr; > > > + uid_sensitive_constexpr_evaluation_sentinel (); > > > +}; > > > + > > > +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was > > > + called and returned true, indicating that we've restricted constexpr > > > + evaluation in order to avoid UID generation. We use this to control > > > + updates to the fold_cache and cv_cache. */ > > > + > > > +struct uid_sensitive_constexpr_evaluation_checker > > > +{ > > > + const unsigned saved_counter; > > > + uid_sensitive_constexpr_evaluation_checker (); > > > + bool evaluation_restricted_p () const; > > > +}; > > > + > > > /* In cp-ubsan.c */ > > > extern void cp_ubsan_maybe_instrument_member_call (tree); > > > extern void cp_ubsan_instrument_member_accesses (tree *); > > > diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c > > > index 9b535708c57..0d68a738f8f 100644 > > > --- a/gcc/cp/expr.c > > > +++ b/gcc/cp/expr.c > > > @@ -399,6 +399,8 @@ fold_for_warn (tree x) > > > { > > > /* C++ implementation. */ > > > + uid_sensitive_constexpr_evaluation_sentinel s; > > > > Please add a comment about why we need this. > > Thanks for the review. Here's the final patch (with the added comments) > that I've committed: Whoops, I spoke too soon. Attempting to push this to master fails with: Enumerating objects: 26, done. Counting objects: 100% (26/26), done. Delta compression using up to 8 threads Compressing objects: 100% (14/14), done. Writing objects: 100% (14/14), 4.55 KiB | 32.00 KiB/s, done. Total 14 (delta 12), reused 1 (delta 0), pack-reused 0 remote: Traceback (most recent call last): remote: File "hooks/update.py", line 13, in <module> remote: from updates.factory import new_update remote: File "/sourceware1/home/gccadmin/git-hooks/hooks/updates/__init__.py", line 8, in <module> remote: from pre_commit_checks import (check_revision_history, style_check_commit, remote: File "/sourceware1/home/gccadmin/git-hooks/hooks/pre_commit_checks.py", line 6, in <module> remote: from git_commit import GitCommit remote: ImportError: No module named git_commit remote: error: hook declined to update refs/heads/master To git+ssh://gcc.gnu.org/git/gcc.git ! [remote rejected] HEAD -> master (hook declined) error: failed to push some refs to 'git+ssh://gcc.gnu.org/git/gcc.git' I will try again tomorrow.
On 5/20/20 10:08 PM, Patrick Palka wrote: > On Wed, 20 May 2020, Patrick Palka wrote: >> On Tue, 19 May 2020, Jason Merrill wrote: >> >>> On 5/8/20 11:42 AM, Patrick Palka wrote: >>>> On Wed, 6 May 2020, Patrick Palka wrote: >>>> >>>>> On Wed, 6 May 2020, Patrick Palka wrote: >>>>> >>>>>> On Tue, 5 May 2020, Patrick Palka wrote: >>>>>> >>>>>>> On Tue, 5 May 2020, Patrick Palka wrote: >>>>>>> >>>>>>>> Unfortunately, the previous fix to PR94038 is fragile. When the >>>>>>>> argument to fold_for_warn is a bare CALL_EXPR, then all is well: the >>>>>>>> result of maybe_constant_value from fold_for_warn (with >>>>>>>> uid_sensitive=true) is reused via the cv_cache in the subsequent >>>>>>>> call to >>>>>>>> maybe_constant_value from cp_fold (with uid_sensitive=false), so we >>>>>>>> avoid instantiating bar<int>. >>>>>>>> >>>>>>>> But when the argument to fold_for_warn is more complex, e.g. an >>>>>>>> INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to >>>>>>>> bar<int>() >>>>>>>> returning const int& which we need to decay to int) then from >>>>>>>> fold_for_warn we call maybe_constant_value on the INDIRECT_REF, and >>>>>>>> from >>>>>>>> cp_fold we call it on the CALL_EXPR, so there is no reuse via the >>>>>>>> cv_cache and we therefore end up instantiating bar<int>. >>>>>>>> >>>>>>>> So for a more robust solution to this general issue of warning flags >>>>>>>> affecting code generation, it seems that we need a way to globally >>>>>>>> avoid >>>>>>>> template instantiation during constexpr evaluation whenever we're >>>>>>>> performing warning-dependent folding. >>>>>>>> >>>>>>>> To that end, this patch replaces the flag >>>>>>>> constexpr_ctx::uid_sensitive >>>>>>>> with a global flag uid_sensitive_constexpr_evaluation_p, and enables >>>>>>>> it >>>>>>>> during fold_for_warn using an RAII helper. >>>>>>>> >>>>>>>> The patch also adds a counter that keeps track of the number of >>>>>>>> times >>>>>>>> uid_sensitive_constexpr_evaluation_p is called, and we use this to >>>>>>>> determine whether the result of constexpr evaluation depended on the >>>>>>>> flag's value. This lets us safely update the cv_cache and >>>>>>>> fold_cache >>>>>>>> from fold_for_warn in the most common case where the flag's value >>>>>>>> was >>>>>>>> irrelevant during constexpr evaluation. >>>> >>>> Here's some statistics about about the fold cache and cv cache that were >>>> collected when building the testsuite of range-v3 (with the default >>>> build flags which include -O -Wall -Wextra, so warning-dependent folding >>>> applies). >>>> >>>> The "naive solution" below refers to the one in which we would refrain >>>> from updating the caches at the end of cp_fold/maybe_constant_value >>>> whenever we're in uid-sensitive constexpr evaluation mode (i.e. whenever >>>> we're called from fold_for_warn), regardless of whether constexpr >>>> evaluation was actually restricted. >>>> >>>> >>>> FOLD CACHE | cache hit | cache miss | cache miss | >>>> | | cache updated | cache not updated | >>>> ------------+-----------+---------------+-------------------+ >>>> naive sol'n | 5060779 | 11374667 | 12887790 | >>>> ------------------------------------------------------------+ >>>> this patch | 6665242 | 19688975 | 199137 | >>>> ------------+-----------+---------------+-------------------+ >>>> >>>> >>>> CV CACHE | cache hit | cache miss | cache miss | >>>> | | cache updated | cache not updated | >>>> ------------+-----------+---------------+-------------------+ >>>> naive sol'n | 76214 | 3637218 | 6889213 | >>>> ------------------------------------------------------------+ >>>> this patch | 215980 | 9882310 | 405513 | >>>> ------------+-----------+---------------+-------------------+ >>>> >>>> -- >8 -- >>>> >>>> gcc/cp/ChangeLog: >>>> >>>> PR c++/94038 >>>> * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. >>>> (uid_sensitive_constexpr_evaluation_value): Define. >>>> (uid_sensitive_constexpr_evaluation_true_counter): Define. >>>> (uid_sensitive_constexpr_evaluation_p): Define. >>>> (uid_sensitive_constexpr_evaluation_sentinel): Define its >>>> constructor. >>>> (uid_sensitive_constexpr_evaluation_checker): Define its >>>> constructor and its evaluation_restricted_p method. >>>> (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p >>>> instead of constexpr_ctx::uid_sensitive. >>>> (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it >>>> last. Adjust call to get_fundef_copy. >>>> (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the >>>> counter if necessary. >>>> (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' >>>> parameter. Adjust function body accordingly. >>>> (maybe_constant_value): Remove 'uid_sensitive' parameter and >>>> adjust function body accordingly. Set up a >>>> uid_sensitive_constexpr_evaluation_checker, and use it to >>>> conditionally update the cv_cache. >>>> * cp-gimplify.h (cp_fold): Set up a >>>> uid_sensitive_constexpr_evaluation_checker, and use it to >>>> conditionally update the fold_cache. >>>> * cp-tree.h (maybe_constant_value): Update declaration. >>>> (struct uid_sensitive_constexpr_evaluation_sentinel): Define. >>>> (struct sensitive_constexpr_evaluation_checker): Define. >>>> * expr.c (fold_for_warn): Set up a >>>> uid_sensitive_constexpr_evaluation_sentinel before calling >>>> the folding subroutines. Drop all but the first argument to >>>> maybe_constant_value. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR c++/94038 >>>> * g++.dg/warn/pr94038-2.C: New test. >>>> --- >>>> gcc/cp/constexpr.c | 92 ++++++++++++++++++++------- >>>> gcc/cp/cp-gimplify.c | 13 ++-- >>>> gcc/cp/cp-tree.h | 23 ++++++- >>>> gcc/cp/expr.c | 4 +- >>>> gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ >>>> 5 files changed, 130 insertions(+), 30 deletions(-) >>>> create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C >>>> >>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>>> index 706d8a13d8e..d2b75ddaa19 100644 >>>> --- a/gcc/cp/constexpr.c >>>> +++ b/gcc/cp/constexpr.c >>>> @@ -1089,11 +1089,59 @@ struct constexpr_ctx { >>>> bool strict; >>>> /* Whether __builtin_is_constant_evaluated () should be true. */ >>>> bool manifestly_const_eval; >>>> - /* Whether we want to avoid doing anything that will cause extra DECL_UID >>>> - generation. */ >>>> - bool uid_sensitive; >>>> }; >>>> +/* This internal flag controls whether we should avoid doing anything >>>> during >>>> + constexpr evaluation that would cause extra DECL_UID generation, such as >>>> + template instantiation and function copying. */ >>>> + >>>> +static bool uid_sensitive_constexpr_evaluation_value; >>>> + >>>> +/* An internal counter that keeps track of the number of times >>>> + uid_sensitive_constexpr_evaluation_p returned true. */ >>>> + >>>> +static unsigned uid_sensitive_constexpr_evaluation_true_counter; >>>> + >>>> +/* The accessor for uid_sensitive_constexpr_evaluation_value which also >>>> + increments the corresponding counter. */ >>>> + >>>> +static bool >>>> +uid_sensitive_constexpr_evaluation_p () >>>> +{ >>>> + if (uid_sensitive_constexpr_evaluation_value) >>>> + { >>>> + ++uid_sensitive_constexpr_evaluation_true_counter; >>>> + return true; >>>> + } >>>> + else >>>> + return false; >>>> +} >>>> + >>>> +uid_sensitive_constexpr_evaluation_sentinel >>>> +::uid_sensitive_constexpr_evaluation_sentinel () >>>> + : ovr (uid_sensitive_constexpr_evaluation_value, true) >>>> +{ >>>> +} >>>> + >>>> +uid_sensitive_constexpr_evaluation_checker >>>> +::uid_sensitive_constexpr_evaluation_checker () >>>> + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) >>>> +{ >>>> +} >>> >>> These two constructors need comments. >>> >>>> +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, >>>> + and some constexpr evaluation was restricted due to u_s_c_e_p >>>> + being called and returning true after this checker object was >>>> + constructed. */ >>>> + >>>> +bool >>>> +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p () >>>> const >>>> +{ >>>> + return (uid_sensitive_constexpr_evaluation_value >>>> + && saved_counter != >>>> uid_sensitive_constexpr_evaluation_true_counter); >>>> +} >>>> + >>>> + >>>> /* A table of all constexpr calls that have been evaluated by the >>>> compiler in this translation unit. */ >>>> @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> >>>> *fundef_copies_table; >>>> is parms, TYPE is result. */ >>>> static tree >>>> -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) >>>> +get_fundef_copy (constexpr_fundef *fundef) >>>> { >>>> tree copy; >>>> bool existed; >>>> @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, >>>> constexpr_fundef *fundef) >>>> } >>>> else if (*slot == NULL_TREE) >>>> { >>>> - if (ctx->uid_sensitive) >>>> + if (uid_sensitive_constexpr_evaluation_p ()) >>>> return NULL_TREE; >>>> /* We've already used the function itself, so make a copy. */ >>>> @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, >>>> tree t, >>>> /* We can't defer instantiating the function any longer. */ >>>> if (!DECL_INITIAL (fun) >>>> - && !ctx->uid_sensitive >>>> - && DECL_TEMPLOID_INSTANTIATION (fun)) >>>> + && DECL_TEMPLOID_INSTANTIATION (fun) >>>> + && !uid_sensitive_constexpr_evaluation_p ()) >>>> { >>>> location_t save_loc = input_location; >>>> input_location = loc; >>>> @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, >>>> tree t, >>>> gcc_assert (at_eof >= 2 && ctx->quiet); >>>> *non_constant_p = true; >>>> } >>>> - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) >>>> + else if (tree copy = get_fundef_copy (new_call.fundef)) >>>> { >>>> tree body, parms, res; >>>> releasing_vec ctors; >>>> @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, >>>> void */*data*/) >>>> && DECL_DECLARED_CONSTEXPR_P (*tp) >>>> && !DECL_INITIAL (*tp) >>>> && !trivial_fn_p (*tp) >>>> - && DECL_TEMPLOID_INSTANTIATION (*tp)) >>>> + && DECL_TEMPLOID_INSTANTIATION (*tp) >>>> + && !uid_sensitive_constexpr_evaluation_p ()) >>>> { >>>> ++function_depth; >>>> instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); >>>> @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, bool >>>> allow_non_constant, >>>> bool strict = true, >>>> bool manifestly_const_eval = false, >>>> bool constexpr_dtor = false, >>>> - tree object = NULL_TREE, >>>> - bool uid_sensitive = false) >>>> + tree object = NULL_TREE) >>>> { >>>> auto_timevar time (TV_CONSTEXPR); >>>> @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree t, bool >>>> allow_non_constant, >>>> constexpr_global_ctx global_ctx; >>>> constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, >>>> allow_non_constant, strict, >>>> - manifestly_const_eval || !allow_non_constant, >>>> - uid_sensitive }; >>>> + manifestly_const_eval || !allow_non_constant }; >>>> tree type = initialized_type (t); >>>> tree r = t; >>>> @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, bool >>>> allow_non_constant, >>>> auto_vec<tree, 16> cleanups; >>>> global_ctx.cleanups = &cleanups; >>>> - if (!uid_sensitive) >>>> - instantiate_constexpr_fns (r); >>>> + instantiate_constexpr_fns (r); >>>> r = cxx_eval_constant_expression (&ctx, r, >>>> false, &non_constant_p, &overflow_p); >>>> @@ -6909,14 +6955,12 @@ fold_simple (tree t) >>>> Otherwise, if T does not have TREE_CONSTANT set, returns T. >>>> Otherwise, returns a version of T without TREE_CONSTANT. >>>> MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated >>>> - as per P0595. UID_SENSITIVE is true if we can't do anything that >>>> - would affect DECL_UID ordering. */ >>>> + as per P0595. */ >>>> static GTY((deletable)) hash_map<tree, tree> *cv_cache; >>>> tree >>>> -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, >>>> - bool uid_sensitive) >>>> +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) >>>> { >>>> tree r; >>>> @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, bool >>>> manifestly_const_eval, >>>> return t; >>>> if (manifestly_const_eval) >>>> - return cxx_eval_outermost_constant_expr (t, true, true, true, false, >>>> - decl, uid_sensitive); >>>> + return cxx_eval_outermost_constant_expr (t, true, true, true, false, >>>> decl); >>>> if (cv_cache == NULL) >>>> cv_cache = hash_map<tree, tree>::create_ggc (101); >>>> @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, bool >>>> manifestly_const_eval, >>>> return r; >>>> } >>>> - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, >>>> - decl, uid_sensitive); >>>> + uid_sensitive_constexpr_evaluation_checker c; >>>> + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); >>>> gcc_checking_assert (r == t >>>> || CONVERT_EXPR_P (t) >>>> || TREE_CODE (t) == VIEW_CONVERT_EXPR >>>> || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) >>>> || !cp_tree_equal (r, t)); >>>> - cv_cache->put (t, r); >>>> + if (!c.evaluation_restricted_p ()) >>>> + cv_cache->put (t, r); >>>> return r; >>>> } >>>> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c >>>> index fc26a85f43a..f298fa7cec1 100644 >>>> --- a/gcc/cp/cp-gimplify.c >>>> +++ b/gcc/cp/cp-gimplify.c >>>> @@ -2443,6 +2443,8 @@ cp_fold (tree x) >>>> if (tree *cached = fold_cache->get (x)) >>>> return *cached; >>>> + uid_sensitive_constexpr_evaluation_checker c; >>>> + >>>> code = TREE_CODE (x); >>>> switch (code) >>>> { >>>> @@ -2925,10 +2927,13 @@ cp_fold (tree x) >>>> return org_x; >>>> } >>>> - fold_cache->put (org_x, x); >>>> - /* Prevent that we try to fold an already folded result again. */ >>>> - if (x != org_x) >>>> - fold_cache->put (x, x); >>>> + if (!c.evaluation_restricted_p ()) >>>> + { >>>> + fold_cache->put (org_x, x); >>>> + /* Prevent that we try to fold an already folded result again. */ >>>> + if (x != org_x) >>>> + fold_cache->put (x, x); >>>> + } >>>> return x; >>>> } >>>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h >>>> index deb000babc1..c2511196de4 100644 >>>> --- a/gcc/cp/cp-tree.h >>>> +++ b/gcc/cp/cp-tree.h >>>> @@ -7949,7 +7949,7 @@ extern bool >>>> require_potential_rvalue_constant_expression (tree); >>>> extern tree cxx_constant_value (tree, tree = >>>> NULL_TREE); >>>> extern void cxx_constant_dtor (tree, tree); >>>> extern tree cxx_constant_init (tree, tree = >>>> NULL_TREE); >>>> -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool >>>> = false, bool = false); >>>> +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool >>>> = false); >>>> extern tree maybe_constant_init (tree, tree = >>>> NULL_TREE, bool = false); >>>> extern tree fold_non_dependent_expr (tree, >>>> tsubst_flags_t = >>>> tf_warning_or_error, >>>> @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr >>>> (tree); >>>> extern void clear_cv_and_fold_caches (bool = true); >>>> extern tree unshare_constructor (tree >>>> CXX_MEM_STAT_INFO); >>>> +/* An RAII sentinel used to restrict constexpr evaluation so that it >>>> + doesn't do anything that causes extra DECL_UID generation. */ >>>> + >>>> +struct uid_sensitive_constexpr_evaluation_sentinel >>>> +{ >>>> + temp_override<bool> ovr; >>>> + uid_sensitive_constexpr_evaluation_sentinel (); >>>> +}; >>>> + >>>> +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was >>>> + called and returned true, indicating that we've restricted constexpr >>>> + evaluation in order to avoid UID generation. We use this to control >>>> + updates to the fold_cache and cv_cache. */ >>>> + >>>> +struct uid_sensitive_constexpr_evaluation_checker >>>> +{ >>>> + const unsigned saved_counter; >>>> + uid_sensitive_constexpr_evaluation_checker (); >>>> + bool evaluation_restricted_p () const; >>>> +}; >>>> + >>>> /* In cp-ubsan.c */ >>>> extern void cp_ubsan_maybe_instrument_member_call (tree); >>>> extern void cp_ubsan_instrument_member_accesses (tree *); >>>> diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c >>>> index 9b535708c57..0d68a738f8f 100644 >>>> --- a/gcc/cp/expr.c >>>> +++ b/gcc/cp/expr.c >>>> @@ -399,6 +399,8 @@ fold_for_warn (tree x) >>>> { >>>> /* C++ implementation. */ >>>> + uid_sensitive_constexpr_evaluation_sentinel s; >>> >>> Please add a comment about why we need this. >> >> Thanks for the review. Here's the final patch (with the added comments) >> that I've committed: This seems to have broken cmcstl2: > Building CXX object test/iterator/CMakeFiles/make_range.dir/make_range.cpp.o > In file included from /home/jason/src/cmcstl2/include/stl2/type_traits.hpp:16, > from /home/jason/src/cmcstl2/include/stl2/iterator.hpp:18, > from /home/jason/src/cmcstl2/test/iterator/make_range.cpp:1: > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits: In instantiation of ‘struct std::is_nothrow_destructible<int*>’: > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:3170:35: required from ‘constexpr const bool std::is_nothrow_destructible_v<int*>’ > /home/jason/src/cmcstl2/include/stl2/detail/concepts/object/move_constructible.hpp:34:35: required from here > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: error: non-constant condition for static assertion > 895 | static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}), > | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: error: ‘constexpr std::true_type std::__is_complete_or_unbounded(std::__type_identity<_Tp>) [with _Tp = int*; long unsigned int <anonymous> = 8; std::true_type = std::integral_constant<bool, true>]’ used before its definition Jason
On Fri, 22 May 2020, Jason Merrill wrote: > On 5/20/20 10:08 PM, Patrick Palka wrote: > > On Wed, 20 May 2020, Patrick Palka wrote: > > > On Tue, 19 May 2020, Jason Merrill wrote: > > > > > > > On 5/8/20 11:42 AM, Patrick Palka wrote: > > > > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > > > > > > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > > > Unfortunately, the previous fix to PR94038 is fragile. When > > > > > > > > > the > > > > > > > > > argument to fold_for_warn is a bare CALL_EXPR, then all is > > > > > > > > > well: the > > > > > > > > > result of maybe_constant_value from fold_for_warn (with > > > > > > > > > uid_sensitive=true) is reused via the cv_cache in the > > > > > > > > > subsequent > > > > > > > > > call to > > > > > > > > > maybe_constant_value from cp_fold (with uid_sensitive=false), > > > > > > > > > so we > > > > > > > > > avoid instantiating bar<int>. > > > > > > > > > > > > > > > > > > But when the argument to fold_for_warn is more complex, e.g. > > > > > > > > > an > > > > > > > > > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to > > > > > > > > > bar<int>() > > > > > > > > > returning const int& which we need to decay to int) then from > > > > > > > > > fold_for_warn we call maybe_constant_value on the > > > > > > > > > INDIRECT_REF, and > > > > > > > > > from > > > > > > > > > cp_fold we call it on the CALL_EXPR, so there is no reuse via > > > > > > > > > the > > > > > > > > > cv_cache and we therefore end up instantiating bar<int>. > > > > > > > > > > > > > > > > > > So for a more robust solution to this general issue of warning > > > > > > > > > flags > > > > > > > > > affecting code generation, it seems that we need a way to > > > > > > > > > globally > > > > > > > > > avoid > > > > > > > > > template instantiation during constexpr evaluation whenever > > > > > > > > > we're > > > > > > > > > performing warning-dependent folding. > > > > > > > > > > > > > > > > > > To that end, this patch replaces the flag > > > > > > > > > constexpr_ctx::uid_sensitive > > > > > > > > > with a global flag uid_sensitive_constexpr_evaluation_p, and > > > > > > > > > enables > > > > > > > > > it > > > > > > > > > during fold_for_warn using an RAII helper. > > > > > > > > > > > > > > > > > > The patch also adds a counter that keeps track of the number > > > > > > > > > of > > > > > > > > > times > > > > > > > > > uid_sensitive_constexpr_evaluation_p is called, and we use > > > > > > > > > this to > > > > > > > > > determine whether the result of constexpr evaluation depended > > > > > > > > > on the > > > > > > > > > flag's value. This lets us safely update the cv_cache and > > > > > > > > > fold_cache > > > > > > > > > from fold_for_warn in the most common case where the flag's > > > > > > > > > value > > > > > > > > > was > > > > > > > > > irrelevant during constexpr evaluation. > > > > > > > > > > Here's some statistics about about the fold cache and cv cache that > > > > > were > > > > > collected when building the testsuite of range-v3 (with the default > > > > > build flags which include -O -Wall -Wextra, so warning-dependent > > > > > folding > > > > > applies). > > > > > > > > > > The "naive solution" below refers to the one in which we would refrain > > > > > from updating the caches at the end of cp_fold/maybe_constant_value > > > > > whenever we're in uid-sensitive constexpr evaluation mode (i.e. > > > > > whenever > > > > > we're called from fold_for_warn), regardless of whether constexpr > > > > > evaluation was actually restricted. > > > > > > > > > > > > > > > FOLD CACHE | cache hit | cache miss | cache miss | > > > > > | | cache updated | cache not updated | > > > > > ------------+-----------+---------------+-------------------+ > > > > > naive sol'n | 5060779 | 11374667 | 12887790 | > > > > > ------------------------------------------------------------+ > > > > > this patch | 6665242 | 19688975 | 199137 | > > > > > ------------+-----------+---------------+-------------------+ > > > > > > > > > > > > > > > CV CACHE | cache hit | cache miss | cache miss | > > > > > | | cache updated | cache not updated | > > > > > ------------+-----------+---------------+-------------------+ > > > > > naive sol'n | 76214 | 3637218 | 6889213 | > > > > > ------------------------------------------------------------+ > > > > > this patch | 215980 | 9882310 | 405513 | > > > > > ------------+-----------+---------------+-------------------+ > > > > > > > > > > -- >8 -- > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > PR c++/94038 > > > > > * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. > > > > > (uid_sensitive_constexpr_evaluation_value): Define. > > > > > (uid_sensitive_constexpr_evaluation_true_counter): Define. > > > > > (uid_sensitive_constexpr_evaluation_p): Define. > > > > > (uid_sensitive_constexpr_evaluation_sentinel): Define its > > > > > constructor. > > > > > (uid_sensitive_constexpr_evaluation_checker): Define its > > > > > constructor and its evaluation_restricted_p method. > > > > > (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p > > > > > instead of constexpr_ctx::uid_sensitive. > > > > > (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it > > > > > last. Adjust call to get_fundef_copy. > > > > > (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the > > > > > counter if necessary. > > > > > (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' > > > > > parameter. Adjust function body accordingly. > > > > > (maybe_constant_value): Remove 'uid_sensitive' parameter and > > > > > adjust function body accordingly. Set up a > > > > > uid_sensitive_constexpr_evaluation_checker, and use it to > > > > > conditionally update the cv_cache. > > > > > * cp-gimplify.h (cp_fold): Set up a > > > > > uid_sensitive_constexpr_evaluation_checker, and use it to > > > > > conditionally update the fold_cache. > > > > > * cp-tree.h (maybe_constant_value): Update declaration. > > > > > (struct uid_sensitive_constexpr_evaluation_sentinel): Define. > > > > > (struct sensitive_constexpr_evaluation_checker): Define. > > > > > * expr.c (fold_for_warn): Set up a > > > > > uid_sensitive_constexpr_evaluation_sentinel before calling > > > > > the folding subroutines. Drop all but the first argument to > > > > > maybe_constant_value. > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > PR c++/94038 > > > > > * g++.dg/warn/pr94038-2.C: New test. > > > > > --- > > > > > gcc/cp/constexpr.c | 92 > > > > > ++++++++++++++++++++------- > > > > > gcc/cp/cp-gimplify.c | 13 ++-- > > > > > gcc/cp/cp-tree.h | 23 ++++++- > > > > > gcc/cp/expr.c | 4 +- > > > > > gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ > > > > > 5 files changed, 130 insertions(+), 30 deletions(-) > > > > > create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C > > > > > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > > > index 706d8a13d8e..d2b75ddaa19 100644 > > > > > --- a/gcc/cp/constexpr.c > > > > > +++ b/gcc/cp/constexpr.c > > > > > @@ -1089,11 +1089,59 @@ struct constexpr_ctx { > > > > > bool strict; > > > > > /* Whether __builtin_is_constant_evaluated () should be true. */ > > > > > bool manifestly_const_eval; > > > > > - /* Whether we want to avoid doing anything that will cause extra > > > > > DECL_UID > > > > > - generation. */ > > > > > - bool uid_sensitive; > > > > > }; > > > > > +/* This internal flag controls whether we should avoid doing > > > > > anything > > > > > during > > > > > + constexpr evaluation that would cause extra DECL_UID generation, > > > > > such as > > > > > + template instantiation and function copying. */ > > > > > + > > > > > +static bool uid_sensitive_constexpr_evaluation_value; > > > > > + > > > > > +/* An internal counter that keeps track of the number of times > > > > > + uid_sensitive_constexpr_evaluation_p returned true. */ > > > > > + > > > > > +static unsigned uid_sensitive_constexpr_evaluation_true_counter; > > > > > + > > > > > +/* The accessor for uid_sensitive_constexpr_evaluation_value which > > > > > also > > > > > + increments the corresponding counter. */ > > > > > + > > > > > +static bool > > > > > +uid_sensitive_constexpr_evaluation_p () > > > > > +{ > > > > > + if (uid_sensitive_constexpr_evaluation_value) > > > > > + { > > > > > + ++uid_sensitive_constexpr_evaluation_true_counter; > > > > > + return true; > > > > > + } > > > > > + else > > > > > + return false; > > > > > +} > > > > > + > > > > > +uid_sensitive_constexpr_evaluation_sentinel > > > > > +::uid_sensitive_constexpr_evaluation_sentinel () > > > > > + : ovr (uid_sensitive_constexpr_evaluation_value, true) > > > > > +{ > > > > > +} > > > > > + > > > > > +uid_sensitive_constexpr_evaluation_checker > > > > > +::uid_sensitive_constexpr_evaluation_checker () > > > > > + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) > > > > > +{ > > > > > +} > > > > > > > > These two constructors need comments. > > > > > > > > > +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, > > > > > + and some constexpr evaluation was restricted due to u_s_c_e_p > > > > > + being called and returning true after this checker object was > > > > > + constructed. */ > > > > > + > > > > > +bool > > > > > +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p > > > > > () > > > > > const > > > > > +{ > > > > > + return (uid_sensitive_constexpr_evaluation_value > > > > > + && saved_counter != > > > > > uid_sensitive_constexpr_evaluation_true_counter); > > > > > +} > > > > > + > > > > > + > > > > > /* A table of all constexpr calls that have been evaluated by the > > > > > compiler in this translation unit. */ > > > > > @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> > > > > > *fundef_copies_table; > > > > > is parms, TYPE is result. */ > > > > > static tree > > > > > -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) > > > > > +get_fundef_copy (constexpr_fundef *fundef) > > > > > { > > > > > tree copy; > > > > > bool existed; > > > > > @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, > > > > > constexpr_fundef *fundef) > > > > > } > > > > > else if (*slot == NULL_TREE) > > > > > { > > > > > - if (ctx->uid_sensitive) > > > > > + if (uid_sensitive_constexpr_evaluation_p ()) > > > > > return NULL_TREE; > > > > > /* We've already used the function itself, so make a copy. > > > > > */ > > > > > @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const constexpr_ctx > > > > > *ctx, > > > > > tree t, > > > > > /* We can't defer instantiating the function any longer. */ > > > > > if (!DECL_INITIAL (fun) > > > > > - && !ctx->uid_sensitive > > > > > - && DECL_TEMPLOID_INSTANTIATION (fun)) > > > > > + && DECL_TEMPLOID_INSTANTIATION (fun) > > > > > + && !uid_sensitive_constexpr_evaluation_p ()) > > > > > { > > > > > location_t save_loc = input_location; > > > > > input_location = loc; > > > > > @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const constexpr_ctx > > > > > *ctx, > > > > > tree t, > > > > > gcc_assert (at_eof >= 2 && ctx->quiet); > > > > > *non_constant_p = true; > > > > > } > > > > > - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) > > > > > + else if (tree copy = get_fundef_copy (new_call.fundef)) > > > > > { > > > > > tree body, parms, res; > > > > > releasing_vec ctors; > > > > > @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int > > > > > *walk_subtrees, > > > > > void */*data*/) > > > > > && DECL_DECLARED_CONSTEXPR_P (*tp) > > > > > && !DECL_INITIAL (*tp) > > > > > && !trivial_fn_p (*tp) > > > > > - && DECL_TEMPLOID_INSTANTIATION (*tp)) > > > > > + && DECL_TEMPLOID_INSTANTIATION (*tp) > > > > > + && !uid_sensitive_constexpr_evaluation_p ()) > > > > > { > > > > > ++function_depth; > > > > > instantiate_decl (*tp, /*defer_ok*/false, > > > > > /*expl_inst*/false); > > > > > @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > > > > allow_non_constant, > > > > > bool strict = true, > > > > > bool manifestly_const_eval = false, > > > > > bool constexpr_dtor = false, > > > > > - tree object = NULL_TREE, > > > > > - bool uid_sensitive = false) > > > > > + tree object = NULL_TREE) > > > > > { > > > > > auto_timevar time (TV_CONSTEXPR); > > > > > @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree t, > > > > > bool > > > > > allow_non_constant, > > > > > constexpr_global_ctx global_ctx; > > > > > constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, > > > > > NULL, > > > > > allow_non_constant, strict, > > > > > - manifestly_const_eval || !allow_non_constant, > > > > > - uid_sensitive }; > > > > > + manifestly_const_eval || !allow_non_constant > > > > > }; > > > > > tree type = initialized_type (t); > > > > > tree r = t; > > > > > @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, bool > > > > > allow_non_constant, > > > > > auto_vec<tree, 16> cleanups; > > > > > global_ctx.cleanups = &cleanups; > > > > > - if (!uid_sensitive) > > > > > - instantiate_constexpr_fns (r); > > > > > + instantiate_constexpr_fns (r); > > > > > r = cxx_eval_constant_expression (&ctx, r, > > > > > false, &non_constant_p, > > > > > &overflow_p); > > > > > @@ -6909,14 +6955,12 @@ fold_simple (tree t) > > > > > Otherwise, if T does not have TREE_CONSTANT set, returns T. > > > > > Otherwise, returns a version of T without TREE_CONSTANT. > > > > > MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated > > > > > - as per P0595. UID_SENSITIVE is true if we can't do anything that > > > > > - would affect DECL_UID ordering. */ > > > > > + as per P0595. */ > > > > > static GTY((deletable)) hash_map<tree, tree> *cv_cache; > > > > > tree > > > > > -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, > > > > > - bool uid_sensitive) > > > > > +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) > > > > > { > > > > > tree r; > > > > > @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, bool > > > > > manifestly_const_eval, > > > > > return t; > > > > > if (manifestly_const_eval) > > > > > - return cxx_eval_outermost_constant_expr (t, true, true, true, > > > > > false, > > > > > - decl, uid_sensitive); > > > > > + return cxx_eval_outermost_constant_expr (t, true, true, true, > > > > > false, > > > > > decl); > > > > > if (cv_cache == NULL) > > > > > cv_cache = hash_map<tree, tree>::create_ggc (101); > > > > > @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, bool > > > > > manifestly_const_eval, > > > > > return r; > > > > > } > > > > > - r = cxx_eval_outermost_constant_expr (t, true, true, false, > > > > > false, > > > > > - decl, uid_sensitive); > > > > > + uid_sensitive_constexpr_evaluation_checker c; > > > > > + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, > > > > > decl); > > > > > gcc_checking_assert (r == t > > > > > || CONVERT_EXPR_P (t) > > > > > || TREE_CODE (t) == VIEW_CONVERT_EXPR > > > > > || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) > > > > > || !cp_tree_equal (r, t)); > > > > > - cv_cache->put (t, r); > > > > > + if (!c.evaluation_restricted_p ()) > > > > > + cv_cache->put (t, r); > > > > > return r; > > > > > } > > > > > diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c > > > > > index fc26a85f43a..f298fa7cec1 100644 > > > > > --- a/gcc/cp/cp-gimplify.c > > > > > +++ b/gcc/cp/cp-gimplify.c > > > > > @@ -2443,6 +2443,8 @@ cp_fold (tree x) > > > > > if (tree *cached = fold_cache->get (x)) > > > > > return *cached; > > > > > + uid_sensitive_constexpr_evaluation_checker c; > > > > > + > > > > > code = TREE_CODE (x); > > > > > switch (code) > > > > > { > > > > > @@ -2925,10 +2927,13 @@ cp_fold (tree x) > > > > > return org_x; > > > > > } > > > > > - fold_cache->put (org_x, x); > > > > > - /* Prevent that we try to fold an already folded result again. */ > > > > > - if (x != org_x) > > > > > - fold_cache->put (x, x); > > > > > + if (!c.evaluation_restricted_p ()) > > > > > + { > > > > > + fold_cache->put (org_x, x); > > > > > + /* Prevent that we try to fold an already folded result again. > > > > > */ > > > > > + if (x != org_x) > > > > > + fold_cache->put (x, x); > > > > > + } > > > > > return x; > > > > > } > > > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > > > > index deb000babc1..c2511196de4 100644 > > > > > --- a/gcc/cp/cp-tree.h > > > > > +++ b/gcc/cp/cp-tree.h > > > > > @@ -7949,7 +7949,7 @@ extern bool > > > > > require_potential_rvalue_constant_expression (tree); > > > > > extern tree cxx_constant_value (tree, tree = > > > > > NULL_TREE); > > > > > extern void cxx_constant_dtor (tree, tree); > > > > > extern tree cxx_constant_init (tree, tree = > > > > > NULL_TREE); > > > > > -extern tree maybe_constant_value (tree, tree = > > > > > NULL_TREE, bool > > > > > = false, bool = false); > > > > > +extern tree maybe_constant_value (tree, tree = > > > > > NULL_TREE, bool > > > > > = false); > > > > > extern tree maybe_constant_init (tree, tree = > > > > > NULL_TREE, bool = false); > > > > > extern tree fold_non_dependent_expr (tree, > > > > > tsubst_flags_t = > > > > > tf_warning_or_error, > > > > > @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr > > > > > (tree); > > > > > extern void clear_cv_and_fold_caches (bool = true); > > > > > extern tree unshare_constructor (tree > > > > > CXX_MEM_STAT_INFO); > > > > > +/* An RAII sentinel used to restrict constexpr evaluation so that > > > > > it > > > > > + doesn't do anything that causes extra DECL_UID generation. */ > > > > > + > > > > > +struct uid_sensitive_constexpr_evaluation_sentinel > > > > > +{ > > > > > + temp_override<bool> ovr; > > > > > + uid_sensitive_constexpr_evaluation_sentinel (); > > > > > +}; > > > > > + > > > > > +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was > > > > > + called and returned true, indicating that we've restricted > > > > > constexpr > > > > > + evaluation in order to avoid UID generation. We use this to > > > > > control > > > > > + updates to the fold_cache and cv_cache. */ > > > > > + > > > > > +struct uid_sensitive_constexpr_evaluation_checker > > > > > +{ > > > > > + const unsigned saved_counter; > > > > > + uid_sensitive_constexpr_evaluation_checker (); > > > > > + bool evaluation_restricted_p () const; > > > > > +}; > > > > > + > > > > > /* In cp-ubsan.c */ > > > > > extern void cp_ubsan_maybe_instrument_member_call (tree); > > > > > extern void cp_ubsan_instrument_member_accesses (tree *); > > > > > diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c > > > > > index 9b535708c57..0d68a738f8f 100644 > > > > > --- a/gcc/cp/expr.c > > > > > +++ b/gcc/cp/expr.c > > > > > @@ -399,6 +399,8 @@ fold_for_warn (tree x) > > > > > { > > > > > /* C++ implementation. */ > > > > > + uid_sensitive_constexpr_evaluation_sentinel s; > > > > > > > > Please add a comment about why we need this. > > > > > > Thanks for the review. Here's the final patch (with the added comments) > > > that I've committed: > > This seems to have broken cmcstl2: > > > Building CXX object test/iterator/CMakeFiles/make_range.dir/make_range.cpp.o > > In file included from > > /home/jason/src/cmcstl2/include/stl2/type_traits.hpp:16, > > from /home/jason/src/cmcstl2/include/stl2/iterator.hpp:18, > > from > > /home/jason/src/cmcstl2/test/iterator/make_range.cpp:1: > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits: In > > instantiation of ‘struct std::is_nothrow_destructible<int*>’: > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:3170:35: > > required from ‘constexpr const bool std::is_nothrow_destructible_v<int*>’ > > /home/jason/src/cmcstl2/include/stl2/detail/concepts/object/move_constructible.hpp:34:35: > > required from here > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: > > error: non-constant condition for static assertion > > 895 | > > static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}), > > | > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: > > error: ‘constexpr std::true_type > > std::__is_complete_or_unbounded(std::__type_identity<_Tp>) [with _Tp = int*; > > long unsigned int <anonymous> = 8; std::true_type = > > std::integral_constant<bool, true>]’ used before its definition Yikes :( The following fixlet seems to restore the cmcstl2 build: diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 98c974e657f..4e441ac8d2f 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -6486,7 +6486,8 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, break; } - if (!processing_template_decl) + if (!processing_template_decl + && !uid_sensitive_constexpr_evaluation_p ()) r = evaluate_concept_check (t, tf_warning_or_error); else *non_constant_p = true; Concept evaluation may entail DECL_UID generation and/or template instantiation, so in general it seems we can't perform it during uid-sensitive constexpr evaluation. > > Jason > >
On 5/22/20 9:18 PM, Patrick Palka wrote: > On Fri, 22 May 2020, Jason Merrill wrote: >> On 5/20/20 10:08 PM, Patrick Palka wrote: >>> On Wed, 20 May 2020, Patrick Palka wrote: >>>> On Tue, 19 May 2020, Jason Merrill wrote: >>>> >>>>> On 5/8/20 11:42 AM, Patrick Palka wrote: >>>>>> On Wed, 6 May 2020, Patrick Palka wrote: >>>>>> >>>>>>> On Wed, 6 May 2020, Patrick Palka wrote: >>>>>>> >>>>>>>> On Tue, 5 May 2020, Patrick Palka wrote: >>>>>>>> >>>>>>>>> On Tue, 5 May 2020, Patrick Palka wrote: >>>>>>>>> >>>>>>>>>> Unfortunately, the previous fix to PR94038 is fragile. When >>>>>>>>>> the >>>>>>>>>> argument to fold_for_warn is a bare CALL_EXPR, then all is >>>>>>>>>> well: the >>>>>>>>>> result of maybe_constant_value from fold_for_warn (with >>>>>>>>>> uid_sensitive=true) is reused via the cv_cache in the >>>>>>>>>> subsequent >>>>>>>>>> call to >>>>>>>>>> maybe_constant_value from cp_fold (with uid_sensitive=false), >>>>>>>>>> so we >>>>>>>>>> avoid instantiating bar<int>. >>>>>>>>>> >>>>>>>>>> But when the argument to fold_for_warn is more complex, e.g. >>>>>>>>>> an >>>>>>>>>> INDIRECT_REF of a CALL_EXPR, as in the testcase below (due to >>>>>>>>>> bar<int>() >>>>>>>>>> returning const int& which we need to decay to int) then from >>>>>>>>>> fold_for_warn we call maybe_constant_value on the >>>>>>>>>> INDIRECT_REF, and >>>>>>>>>> from >>>>>>>>>> cp_fold we call it on the CALL_EXPR, so there is no reuse via >>>>>>>>>> the >>>>>>>>>> cv_cache and we therefore end up instantiating bar<int>. >>>>>>>>>> >>>>>>>>>> So for a more robust solution to this general issue of warning >>>>>>>>>> flags >>>>>>>>>> affecting code generation, it seems that we need a way to >>>>>>>>>> globally >>>>>>>>>> avoid >>>>>>>>>> template instantiation during constexpr evaluation whenever >>>>>>>>>> we're >>>>>>>>>> performing warning-dependent folding. >>>>>>>>>> >>>>>>>>>> To that end, this patch replaces the flag >>>>>>>>>> constexpr_ctx::uid_sensitive >>>>>>>>>> with a global flag uid_sensitive_constexpr_evaluation_p, and >>>>>>>>>> enables >>>>>>>>>> it >>>>>>>>>> during fold_for_warn using an RAII helper. >>>>>>>>>> >>>>>>>>>> The patch also adds a counter that keeps track of the number >>>>>>>>>> of >>>>>>>>>> times >>>>>>>>>> uid_sensitive_constexpr_evaluation_p is called, and we use >>>>>>>>>> this to >>>>>>>>>> determine whether the result of constexpr evaluation depended >>>>>>>>>> on the >>>>>>>>>> flag's value. This lets us safely update the cv_cache and >>>>>>>>>> fold_cache >>>>>>>>>> from fold_for_warn in the most common case where the flag's >>>>>>>>>> value >>>>>>>>>> was >>>>>>>>>> irrelevant during constexpr evaluation. >>>>>> >>>>>> Here's some statistics about about the fold cache and cv cache that >>>>>> were >>>>>> collected when building the testsuite of range-v3 (with the default >>>>>> build flags which include -O -Wall -Wextra, so warning-dependent >>>>>> folding >>>>>> applies). >>>>>> >>>>>> The "naive solution" below refers to the one in which we would refrain >>>>>> from updating the caches at the end of cp_fold/maybe_constant_value >>>>>> whenever we're in uid-sensitive constexpr evaluation mode (i.e. >>>>>> whenever >>>>>> we're called from fold_for_warn), regardless of whether constexpr >>>>>> evaluation was actually restricted. >>>>>> >>>>>> >>>>>> FOLD CACHE | cache hit | cache miss | cache miss | >>>>>> | | cache updated | cache not updated | >>>>>> ------------+-----------+---------------+-------------------+ >>>>>> naive sol'n | 5060779 | 11374667 | 12887790 | >>>>>> ------------------------------------------------------------+ >>>>>> this patch | 6665242 | 19688975 | 199137 | >>>>>> ------------+-----------+---------------+-------------------+ >>>>>> >>>>>> >>>>>> CV CACHE | cache hit | cache miss | cache miss | >>>>>> | | cache updated | cache not updated | >>>>>> ------------+-----------+---------------+-------------------+ >>>>>> naive sol'n | 76214 | 3637218 | 6889213 | >>>>>> ------------------------------------------------------------+ >>>>>> this patch | 215980 | 9882310 | 405513 | >>>>>> ------------+-----------+---------------+-------------------+ >>>>>> >>>>>> -- >8 -- >>>>>> >>>>>> gcc/cp/ChangeLog: >>>>>> >>>>>> PR c++/94038 >>>>>> * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. >>>>>> (uid_sensitive_constexpr_evaluation_value): Define. >>>>>> (uid_sensitive_constexpr_evaluation_true_counter): Define. >>>>>> (uid_sensitive_constexpr_evaluation_p): Define. >>>>>> (uid_sensitive_constexpr_evaluation_sentinel): Define its >>>>>> constructor. >>>>>> (uid_sensitive_constexpr_evaluation_checker): Define its >>>>>> constructor and its evaluation_restricted_p method. >>>>>> (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p >>>>>> instead of constexpr_ctx::uid_sensitive. >>>>>> (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it >>>>>> last. Adjust call to get_fundef_copy. >>>>>> (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the >>>>>> counter if necessary. >>>>>> (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' >>>>>> parameter. Adjust function body accordingly. >>>>>> (maybe_constant_value): Remove 'uid_sensitive' parameter and >>>>>> adjust function body accordingly. Set up a >>>>>> uid_sensitive_constexpr_evaluation_checker, and use it to >>>>>> conditionally update the cv_cache. >>>>>> * cp-gimplify.h (cp_fold): Set up a >>>>>> uid_sensitive_constexpr_evaluation_checker, and use it to >>>>>> conditionally update the fold_cache. >>>>>> * cp-tree.h (maybe_constant_value): Update declaration. >>>>>> (struct uid_sensitive_constexpr_evaluation_sentinel): Define. >>>>>> (struct sensitive_constexpr_evaluation_checker): Define. >>>>>> * expr.c (fold_for_warn): Set up a >>>>>> uid_sensitive_constexpr_evaluation_sentinel before calling >>>>>> the folding subroutines. Drop all but the first argument to >>>>>> maybe_constant_value. >>>>>> >>>>>> gcc/testsuite/ChangeLog: >>>>>> >>>>>> PR c++/94038 >>>>>> * g++.dg/warn/pr94038-2.C: New test. >>>>>> --- >>>>>> gcc/cp/constexpr.c | 92 >>>>>> ++++++++++++++++++++------- >>>>>> gcc/cp/cp-gimplify.c | 13 ++-- >>>>>> gcc/cp/cp-tree.h | 23 ++++++- >>>>>> gcc/cp/expr.c | 4 +- >>>>>> gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ >>>>>> 5 files changed, 130 insertions(+), 30 deletions(-) >>>>>> create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C >>>>>> >>>>>> diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c >>>>>> index 706d8a13d8e..d2b75ddaa19 100644 >>>>>> --- a/gcc/cp/constexpr.c >>>>>> +++ b/gcc/cp/constexpr.c >>>>>> @@ -1089,11 +1089,59 @@ struct constexpr_ctx { >>>>>> bool strict; >>>>>> /* Whether __builtin_is_constant_evaluated () should be true. */ >>>>>> bool manifestly_const_eval; >>>>>> - /* Whether we want to avoid doing anything that will cause extra >>>>>> DECL_UID >>>>>> - generation. */ >>>>>> - bool uid_sensitive; >>>>>> }; >>>>>> +/* This internal flag controls whether we should avoid doing >>>>>> anything >>>>>> during >>>>>> + constexpr evaluation that would cause extra DECL_UID generation, >>>>>> such as >>>>>> + template instantiation and function copying. */ >>>>>> + >>>>>> +static bool uid_sensitive_constexpr_evaluation_value; >>>>>> + >>>>>> +/* An internal counter that keeps track of the number of times >>>>>> + uid_sensitive_constexpr_evaluation_p returned true. */ >>>>>> + >>>>>> +static unsigned uid_sensitive_constexpr_evaluation_true_counter; >>>>>> + >>>>>> +/* The accessor for uid_sensitive_constexpr_evaluation_value which >>>>>> also >>>>>> + increments the corresponding counter. */ >>>>>> + >>>>>> +static bool >>>>>> +uid_sensitive_constexpr_evaluation_p () >>>>>> +{ >>>>>> + if (uid_sensitive_constexpr_evaluation_value) >>>>>> + { >>>>>> + ++uid_sensitive_constexpr_evaluation_true_counter; >>>>>> + return true; >>>>>> + } >>>>>> + else >>>>>> + return false; >>>>>> +} >>>>>> + >>>>>> +uid_sensitive_constexpr_evaluation_sentinel >>>>>> +::uid_sensitive_constexpr_evaluation_sentinel () >>>>>> + : ovr (uid_sensitive_constexpr_evaluation_value, true) >>>>>> +{ >>>>>> +} >>>>>> + >>>>>> +uid_sensitive_constexpr_evaluation_checker >>>>>> +::uid_sensitive_constexpr_evaluation_checker () >>>>>> + : saved_counter (uid_sensitive_constexpr_evaluation_true_counter) >>>>>> +{ >>>>>> +} >>>>> >>>>> These two constructors need comments. >>>>> >>>>>> +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, >>>>>> + and some constexpr evaluation was restricted due to u_s_c_e_p >>>>>> + being called and returning true after this checker object was >>>>>> + constructed. */ >>>>>> + >>>>>> +bool >>>>>> +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p >>>>>> () >>>>>> const >>>>>> +{ >>>>>> + return (uid_sensitive_constexpr_evaluation_value >>>>>> + && saved_counter != >>>>>> uid_sensitive_constexpr_evaluation_true_counter); >>>>>> +} >>>>>> + >>>>>> + >>>>>> /* A table of all constexpr calls that have been evaluated by the >>>>>> compiler in this translation unit. */ >>>>>> @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> >>>>>> *fundef_copies_table; >>>>>> is parms, TYPE is result. */ >>>>>> static tree >>>>>> -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) >>>>>> +get_fundef_copy (constexpr_fundef *fundef) >>>>>> { >>>>>> tree copy; >>>>>> bool existed; >>>>>> @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, >>>>>> constexpr_fundef *fundef) >>>>>> } >>>>>> else if (*slot == NULL_TREE) >>>>>> { >>>>>> - if (ctx->uid_sensitive) >>>>>> + if (uid_sensitive_constexpr_evaluation_p ()) >>>>>> return NULL_TREE; >>>>>> /* We've already used the function itself, so make a copy. >>>>>> */ >>>>>> @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const constexpr_ctx >>>>>> *ctx, >>>>>> tree t, >>>>>> /* We can't defer instantiating the function any longer. */ >>>>>> if (!DECL_INITIAL (fun) >>>>>> - && !ctx->uid_sensitive >>>>>> - && DECL_TEMPLOID_INSTANTIATION (fun)) >>>>>> + && DECL_TEMPLOID_INSTANTIATION (fun) >>>>>> + && !uid_sensitive_constexpr_evaluation_p ()) >>>>>> { >>>>>> location_t save_loc = input_location; >>>>>> input_location = loc; >>>>>> @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const constexpr_ctx >>>>>> *ctx, >>>>>> tree t, >>>>>> gcc_assert (at_eof >= 2 && ctx->quiet); >>>>>> *non_constant_p = true; >>>>>> } >>>>>> - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) >>>>>> + else if (tree copy = get_fundef_copy (new_call.fundef)) >>>>>> { >>>>>> tree body, parms, res; >>>>>> releasing_vec ctors; >>>>>> @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int >>>>>> *walk_subtrees, >>>>>> void */*data*/) >>>>>> && DECL_DECLARED_CONSTEXPR_P (*tp) >>>>>> && !DECL_INITIAL (*tp) >>>>>> && !trivial_fn_p (*tp) >>>>>> - && DECL_TEMPLOID_INSTANTIATION (*tp)) >>>>>> + && DECL_TEMPLOID_INSTANTIATION (*tp) >>>>>> + && !uid_sensitive_constexpr_evaluation_p ()) >>>>>> { >>>>>> ++function_depth; >>>>>> instantiate_decl (*tp, /*defer_ok*/false, >>>>>> /*expl_inst*/false); >>>>>> @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, bool >>>>>> allow_non_constant, >>>>>> bool strict = true, >>>>>> bool manifestly_const_eval = false, >>>>>> bool constexpr_dtor = false, >>>>>> - tree object = NULL_TREE, >>>>>> - bool uid_sensitive = false) >>>>>> + tree object = NULL_TREE) >>>>>> { >>>>>> auto_timevar time (TV_CONSTEXPR); >>>>>> @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree t, >>>>>> bool >>>>>> allow_non_constant, >>>>>> constexpr_global_ctx global_ctx; >>>>>> constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, >>>>>> NULL, >>>>>> allow_non_constant, strict, >>>>>> - manifestly_const_eval || !allow_non_constant, >>>>>> - uid_sensitive }; >>>>>> + manifestly_const_eval || !allow_non_constant >>>>>> }; >>>>>> tree type = initialized_type (t); >>>>>> tree r = t; >>>>>> @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, bool >>>>>> allow_non_constant, >>>>>> auto_vec<tree, 16> cleanups; >>>>>> global_ctx.cleanups = &cleanups; >>>>>> - if (!uid_sensitive) >>>>>> - instantiate_constexpr_fns (r); >>>>>> + instantiate_constexpr_fns (r); >>>>>> r = cxx_eval_constant_expression (&ctx, r, >>>>>> false, &non_constant_p, >>>>>> &overflow_p); >>>>>> @@ -6909,14 +6955,12 @@ fold_simple (tree t) >>>>>> Otherwise, if T does not have TREE_CONSTANT set, returns T. >>>>>> Otherwise, returns a version of T without TREE_CONSTANT. >>>>>> MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated >>>>>> - as per P0595. UID_SENSITIVE is true if we can't do anything that >>>>>> - would affect DECL_UID ordering. */ >>>>>> + as per P0595. */ >>>>>> static GTY((deletable)) hash_map<tree, tree> *cv_cache; >>>>>> tree >>>>>> -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, >>>>>> - bool uid_sensitive) >>>>>> +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) >>>>>> { >>>>>> tree r; >>>>>> @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, bool >>>>>> manifestly_const_eval, >>>>>> return t; >>>>>> if (manifestly_const_eval) >>>>>> - return cxx_eval_outermost_constant_expr (t, true, true, true, >>>>>> false, >>>>>> - decl, uid_sensitive); >>>>>> + return cxx_eval_outermost_constant_expr (t, true, true, true, >>>>>> false, >>>>>> decl); >>>>>> if (cv_cache == NULL) >>>>>> cv_cache = hash_map<tree, tree>::create_ggc (101); >>>>>> @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, bool >>>>>> manifestly_const_eval, >>>>>> return r; >>>>>> } >>>>>> - r = cxx_eval_outermost_constant_expr (t, true, true, false, >>>>>> false, >>>>>> - decl, uid_sensitive); >>>>>> + uid_sensitive_constexpr_evaluation_checker c; >>>>>> + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, >>>>>> decl); >>>>>> gcc_checking_assert (r == t >>>>>> || CONVERT_EXPR_P (t) >>>>>> || TREE_CODE (t) == VIEW_CONVERT_EXPR >>>>>> || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) >>>>>> || !cp_tree_equal (r, t)); >>>>>> - cv_cache->put (t, r); >>>>>> + if (!c.evaluation_restricted_p ()) >>>>>> + cv_cache->put (t, r); >>>>>> return r; >>>>>> } >>>>>> diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c >>>>>> index fc26a85f43a..f298fa7cec1 100644 >>>>>> --- a/gcc/cp/cp-gimplify.c >>>>>> +++ b/gcc/cp/cp-gimplify.c >>>>>> @@ -2443,6 +2443,8 @@ cp_fold (tree x) >>>>>> if (tree *cached = fold_cache->get (x)) >>>>>> return *cached; >>>>>> + uid_sensitive_constexpr_evaluation_checker c; >>>>>> + >>>>>> code = TREE_CODE (x); >>>>>> switch (code) >>>>>> { >>>>>> @@ -2925,10 +2927,13 @@ cp_fold (tree x) >>>>>> return org_x; >>>>>> } >>>>>> - fold_cache->put (org_x, x); >>>>>> - /* Prevent that we try to fold an already folded result again. */ >>>>>> - if (x != org_x) >>>>>> - fold_cache->put (x, x); >>>>>> + if (!c.evaluation_restricted_p ()) >>>>>> + { >>>>>> + fold_cache->put (org_x, x); >>>>>> + /* Prevent that we try to fold an already folded result again. >>>>>> */ >>>>>> + if (x != org_x) >>>>>> + fold_cache->put (x, x); >>>>>> + } >>>>>> return x; >>>>>> } >>>>>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h >>>>>> index deb000babc1..c2511196de4 100644 >>>>>> --- a/gcc/cp/cp-tree.h >>>>>> +++ b/gcc/cp/cp-tree.h >>>>>> @@ -7949,7 +7949,7 @@ extern bool >>>>>> require_potential_rvalue_constant_expression (tree); >>>>>> extern tree cxx_constant_value (tree, tree = >>>>>> NULL_TREE); >>>>>> extern void cxx_constant_dtor (tree, tree); >>>>>> extern tree cxx_constant_init (tree, tree = >>>>>> NULL_TREE); >>>>>> -extern tree maybe_constant_value (tree, tree = >>>>>> NULL_TREE, bool >>>>>> = false, bool = false); >>>>>> +extern tree maybe_constant_value (tree, tree = >>>>>> NULL_TREE, bool >>>>>> = false); >>>>>> extern tree maybe_constant_init (tree, tree = >>>>>> NULL_TREE, bool = false); >>>>>> extern tree fold_non_dependent_expr (tree, >>>>>> tsubst_flags_t = >>>>>> tf_warning_or_error, >>>>>> @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr >>>>>> (tree); >>>>>> extern void clear_cv_and_fold_caches (bool = true); >>>>>> extern tree unshare_constructor (tree >>>>>> CXX_MEM_STAT_INFO); >>>>>> +/* An RAII sentinel used to restrict constexpr evaluation so that >>>>>> it >>>>>> + doesn't do anything that causes extra DECL_UID generation. */ >>>>>> + >>>>>> +struct uid_sensitive_constexpr_evaluation_sentinel >>>>>> +{ >>>>>> + temp_override<bool> ovr; >>>>>> + uid_sensitive_constexpr_evaluation_sentinel (); >>>>>> +}; >>>>>> + >>>>>> +/* Used to determine whether uid_sensitive_constexpr_evaluation_p was >>>>>> + called and returned true, indicating that we've restricted >>>>>> constexpr >>>>>> + evaluation in order to avoid UID generation. We use this to >>>>>> control >>>>>> + updates to the fold_cache and cv_cache. */ >>>>>> + >>>>>> +struct uid_sensitive_constexpr_evaluation_checker >>>>>> +{ >>>>>> + const unsigned saved_counter; >>>>>> + uid_sensitive_constexpr_evaluation_checker (); >>>>>> + bool evaluation_restricted_p () const; >>>>>> +}; >>>>>> + >>>>>> /* In cp-ubsan.c */ >>>>>> extern void cp_ubsan_maybe_instrument_member_call (tree); >>>>>> extern void cp_ubsan_instrument_member_accesses (tree *); >>>>>> diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c >>>>>> index 9b535708c57..0d68a738f8f 100644 >>>>>> --- a/gcc/cp/expr.c >>>>>> +++ b/gcc/cp/expr.c >>>>>> @@ -399,6 +399,8 @@ fold_for_warn (tree x) >>>>>> { >>>>>> /* C++ implementation. */ >>>>>> + uid_sensitive_constexpr_evaluation_sentinel s; >>>>> >>>>> Please add a comment about why we need this. >>>> >>>> Thanks for the review. Here's the final patch (with the added comments) >>>> that I've committed: >> >> This seems to have broken cmcstl2: >> >>> Building CXX object test/iterator/CMakeFiles/make_range.dir/make_range.cpp.o >>> In file included from >>> /home/jason/src/cmcstl2/include/stl2/type_traits.hpp:16, >>> from /home/jason/src/cmcstl2/include/stl2/iterator.hpp:18, >>> from >>> /home/jason/src/cmcstl2/test/iterator/make_range.cpp:1: >>> /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits: In >>> instantiation of ‘struct std::is_nothrow_destructible<int*>’: >>> /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:3170:35: >>> required from ‘constexpr const bool std::is_nothrow_destructible_v<int*>’ >>> /home/jason/src/cmcstl2/include/stl2/detail/concepts/object/move_constructible.hpp:34:35: >>> required from here >>> /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: >>> error: non-constant condition for static assertion >>> 895 | >>> static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}), >>> | >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ >>> /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: >>> error: ‘constexpr std::true_type >>> std::__is_complete_or_unbounded(std::__type_identity<_Tp>) [with _Tp = int*; >>> long unsigned int <anonymous> = 8; std::true_type = >>> std::integral_constant<bool, true>]’ used before its definition > > Yikes :( The following fixlet seems to restore the cmcstl2 build: > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > index 98c974e657f..4e441ac8d2f 100644 > --- a/gcc/cp/constexpr.c > +++ b/gcc/cp/constexpr.c > @@ -6486,7 +6486,8 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, > break; > } > > - if (!processing_template_decl) > + if (!processing_template_decl > + && !uid_sensitive_constexpr_evaluation_p ()) > r = evaluate_concept_check (t, tf_warning_or_error); > else > *non_constant_p = true; > > Concept evaluation may entail DECL_UID generation and/or template > instantiation, so in general it seems we can't perform it during > uid-sensitive constexpr evaluation. Makes sense. The patch is OK. Jason
On Fri, 22 May 2020, Jason Merrill wrote: > On 5/22/20 9:18 PM, Patrick Palka wrote: > > On Fri, 22 May 2020, Jason Merrill wrote: > > > On 5/20/20 10:08 PM, Patrick Palka wrote: > > > > On Wed, 20 May 2020, Patrick Palka wrote: > > > > > On Tue, 19 May 2020, Jason Merrill wrote: > > > > > > > > > > > On 5/8/20 11:42 AM, Patrick Palka wrote: > > > > > > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > On Wed, 6 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > > > > > On Tue, 5 May 2020, Patrick Palka wrote: > > > > > > > > > > > > > > > > > > > > > Unfortunately, the previous fix to PR94038 is fragile. > > > > > > > > > > > When > > > > > > > > > > > the > > > > > > > > > > > argument to fold_for_warn is a bare CALL_EXPR, then all is > > > > > > > > > > > well: the > > > > > > > > > > > result of maybe_constant_value from fold_for_warn (with > > > > > > > > > > > uid_sensitive=true) is reused via the cv_cache in the > > > > > > > > > > > subsequent > > > > > > > > > > > call to > > > > > > > > > > > maybe_constant_value from cp_fold (with > > > > > > > > > > > uid_sensitive=false), > > > > > > > > > > > so we > > > > > > > > > > > avoid instantiating bar<int>. > > > > > > > > > > > > > > > > > > > > > > But when the argument to fold_for_warn is more complex, > > > > > > > > > > > e.g. > > > > > > > > > > > an > > > > > > > > > > > INDIRECT_REF of a CALL_EXPR, as in the testcase below (due > > > > > > > > > > > to > > > > > > > > > > > bar<int>() > > > > > > > > > > > returning const int& which we need to decay to int) then > > > > > > > > > > > from > > > > > > > > > > > fold_for_warn we call maybe_constant_value on the > > > > > > > > > > > INDIRECT_REF, and > > > > > > > > > > > from > > > > > > > > > > > cp_fold we call it on the CALL_EXPR, so there is no reuse > > > > > > > > > > > via > > > > > > > > > > > the > > > > > > > > > > > cv_cache and we therefore end up instantiating bar<int>. > > > > > > > > > > > > > > > > > > > > > > So for a more robust solution to this general issue of > > > > > > > > > > > warning > > > > > > > > > > > flags > > > > > > > > > > > affecting code generation, it seems that we need a way to > > > > > > > > > > > globally > > > > > > > > > > > avoid > > > > > > > > > > > template instantiation during constexpr evaluation > > > > > > > > > > > whenever > > > > > > > > > > > we're > > > > > > > > > > > performing warning-dependent folding. > > > > > > > > > > > > > > > > > > > > > > To that end, this patch replaces the flag > > > > > > > > > > > constexpr_ctx::uid_sensitive > > > > > > > > > > > with a global flag uid_sensitive_constexpr_evaluation_p, > > > > > > > > > > > and > > > > > > > > > > > enables > > > > > > > > > > > it > > > > > > > > > > > during fold_for_warn using an RAII helper. > > > > > > > > > > > > > > > > > > > > > > The patch also adds a counter that keeps track of the > > > > > > > > > > > number > > > > > > > > > > > of > > > > > > > > > > > times > > > > > > > > > > > uid_sensitive_constexpr_evaluation_p is called, and we use > > > > > > > > > > > this to > > > > > > > > > > > determine whether the result of constexpr evaluation > > > > > > > > > > > depended > > > > > > > > > > > on the > > > > > > > > > > > flag's value. This lets us safely update the cv_cache and > > > > > > > > > > > fold_cache > > > > > > > > > > > from fold_for_warn in the most common case where the > > > > > > > > > > > flag's > > > > > > > > > > > value > > > > > > > > > > > was > > > > > > > > > > > irrelevant during constexpr evaluation. > > > > > > > > > > > > > > Here's some statistics about about the fold cache and cv cache > > > > > > > that > > > > > > > were > > > > > > > collected when building the testsuite of range-v3 (with the > > > > > > > default > > > > > > > build flags which include -O -Wall -Wextra, so warning-dependent > > > > > > > folding > > > > > > > applies). > > > > > > > > > > > > > > The "naive solution" below refers to the one in which we would > > > > > > > refrain > > > > > > > from updating the caches at the end of > > > > > > > cp_fold/maybe_constant_value > > > > > > > whenever we're in uid-sensitive constexpr evaluation mode (i.e. > > > > > > > whenever > > > > > > > we're called from fold_for_warn), regardless of whether constexpr > > > > > > > evaluation was actually restricted. > > > > > > > > > > > > > > > > > > > > > FOLD CACHE | cache hit | cache miss | cache miss | > > > > > > > | | cache updated | cache not updated | > > > > > > > ------------+-----------+---------------+-------------------+ > > > > > > > naive sol'n | 5060779 | 11374667 | 12887790 | > > > > > > > ------------------------------------------------------------+ > > > > > > > this patch | 6665242 | 19688975 | 199137 | > > > > > > > ------------+-----------+---------------+-------------------+ > > > > > > > > > > > > > > > > > > > > > CV CACHE | cache hit | cache miss | cache miss | > > > > > > > | | cache updated | cache not updated | > > > > > > > ------------+-----------+---------------+-------------------+ > > > > > > > naive sol'n | 76214 | 3637218 | 6889213 | > > > > > > > ------------------------------------------------------------+ > > > > > > > this patch | 215980 | 9882310 | 405513 | > > > > > > > ------------+-----------+---------------+-------------------+ > > > > > > > > > > > > > > -- >8 -- > > > > > > > > > > > > > > gcc/cp/ChangeLog: > > > > > > > > > > > > > > PR c++/94038 > > > > > > > * constexpr.c (constexpr_ctx::uid_sensitive): Remove field. > > > > > > > (uid_sensitive_constexpr_evaluation_value): Define. > > > > > > > (uid_sensitive_constexpr_evaluation_true_counter): Define. > > > > > > > (uid_sensitive_constexpr_evaluation_p): Define. > > > > > > > (uid_sensitive_constexpr_evaluation_sentinel): Define its > > > > > > > constructor. > > > > > > > (uid_sensitive_constexpr_evaluation_checker): Define its > > > > > > > constructor and its evaluation_restricted_p method. > > > > > > > (get_fundef_copy): Remove 'ctx' parameter. Use u_s_c_e_p > > > > > > > instead of constexpr_ctx::uid_sensitive. > > > > > > > (cxx_eval_call_expression): Use u_s_c_e_p instead, and test it > > > > > > > last. Adjust call to get_fundef_copy. > > > > > > > (instantiate_cx_fn_r): Test u_s_c_e_p so that we increment the > > > > > > > counter if necessary. > > > > > > > (cxx_eval_outermost_constant_expr): Remove 'uid_sensitive' > > > > > > > parameter. Adjust function body accordingly. > > > > > > > (maybe_constant_value): Remove 'uid_sensitive' parameter and > > > > > > > adjust function body accordingly. Set up a > > > > > > > uid_sensitive_constexpr_evaluation_checker, and use it to > > > > > > > conditionally update the cv_cache. > > > > > > > * cp-gimplify.h (cp_fold): Set up a > > > > > > > uid_sensitive_constexpr_evaluation_checker, and use it to > > > > > > > conditionally update the fold_cache. > > > > > > > * cp-tree.h (maybe_constant_value): Update declaration. > > > > > > > (struct uid_sensitive_constexpr_evaluation_sentinel): Define. > > > > > > > (struct sensitive_constexpr_evaluation_checker): Define. > > > > > > > * expr.c (fold_for_warn): Set up a > > > > > > > uid_sensitive_constexpr_evaluation_sentinel before calling > > > > > > > the folding subroutines. Drop all but the first argument to > > > > > > > maybe_constant_value. > > > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > > > > > > > PR c++/94038 > > > > > > > * g++.dg/warn/pr94038-2.C: New test. > > > > > > > --- > > > > > > > gcc/cp/constexpr.c | 92 > > > > > > > ++++++++++++++++++++------- > > > > > > > gcc/cp/cp-gimplify.c | 13 ++-- > > > > > > > gcc/cp/cp-tree.h | 23 ++++++- > > > > > > > gcc/cp/expr.c | 4 +- > > > > > > > gcc/testsuite/g++.dg/warn/pr94038-2.C | 28 ++++++++ > > > > > > > 5 files changed, 130 insertions(+), 30 deletions(-) > > > > > > > create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-2.C > > > > > > > > > > > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > > > > > > index 706d8a13d8e..d2b75ddaa19 100644 > > > > > > > --- a/gcc/cp/constexpr.c > > > > > > > +++ b/gcc/cp/constexpr.c > > > > > > > @@ -1089,11 +1089,59 @@ struct constexpr_ctx { > > > > > > > bool strict; > > > > > > > /* Whether __builtin_is_constant_evaluated () should be > > > > > > > true. */ > > > > > > > bool manifestly_const_eval; > > > > > > > - /* Whether we want to avoid doing anything that will cause > > > > > > > extra > > > > > > > DECL_UID > > > > > > > - generation. */ > > > > > > > - bool uid_sensitive; > > > > > > > }; > > > > > > > +/* This internal flag controls whether we should avoid doing > > > > > > > anything > > > > > > > during > > > > > > > + constexpr evaluation that would cause extra DECL_UID > > > > > > > generation, > > > > > > > such as > > > > > > > + template instantiation and function copying. */ > > > > > > > + > > > > > > > +static bool uid_sensitive_constexpr_evaluation_value; > > > > > > > + > > > > > > > +/* An internal counter that keeps track of the number of times > > > > > > > + uid_sensitive_constexpr_evaluation_p returned true. */ > > > > > > > + > > > > > > > +static unsigned uid_sensitive_constexpr_evaluation_true_counter; > > > > > > > + > > > > > > > +/* The accessor for uid_sensitive_constexpr_evaluation_value > > > > > > > which > > > > > > > also > > > > > > > + increments the corresponding counter. */ > > > > > > > + > > > > > > > +static bool > > > > > > > +uid_sensitive_constexpr_evaluation_p () > > > > > > > +{ > > > > > > > + if (uid_sensitive_constexpr_evaluation_value) > > > > > > > + { > > > > > > > + ++uid_sensitive_constexpr_evaluation_true_counter; > > > > > > > + return true; > > > > > > > + } > > > > > > > + else > > > > > > > + return false; > > > > > > > +} > > > > > > > + > > > > > > > +uid_sensitive_constexpr_evaluation_sentinel > > > > > > > +::uid_sensitive_constexpr_evaluation_sentinel () > > > > > > > + : ovr (uid_sensitive_constexpr_evaluation_value, true) > > > > > > > +{ > > > > > > > +} > > > > > > > + > > > > > > > +uid_sensitive_constexpr_evaluation_checker > > > > > > > +::uid_sensitive_constexpr_evaluation_checker () > > > > > > > + : saved_counter > > > > > > > (uid_sensitive_constexpr_evaluation_true_counter) > > > > > > > +{ > > > > > > > +} > > > > > > > > > > > > These two constructors need comments. > > > > > > > > > > > > > +/* Returns true iff uid_sensitive_constexpr_evaluation_p is true, > > > > > > > + and some constexpr evaluation was restricted due to u_s_c_e_p > > > > > > > + being called and returning true after this checker object was > > > > > > > + constructed. */ > > > > > > > + > > > > > > > +bool > > > > > > > +uid_sensitive_constexpr_evaluation_checker::evaluation_restricted_p > > > > > > > () > > > > > > > const > > > > > > > +{ > > > > > > > + return (uid_sensitive_constexpr_evaluation_value > > > > > > > + && saved_counter != > > > > > > > uid_sensitive_constexpr_evaluation_true_counter); > > > > > > > +} > > > > > > > + > > > > > > > + > > > > > > > /* A table of all constexpr calls that have been evaluated by > > > > > > > the > > > > > > > compiler in this translation unit. */ > > > > > > > @@ -1156,7 +1204,7 @@ static GTY(()) hash_map<tree, tree> > > > > > > > *fundef_copies_table; > > > > > > > is parms, TYPE is result. */ > > > > > > > static tree > > > > > > > -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef > > > > > > > *fundef) > > > > > > > +get_fundef_copy (constexpr_fundef *fundef) > > > > > > > { > > > > > > > tree copy; > > > > > > > bool existed; > > > > > > > @@ -1173,7 +1221,7 @@ get_fundef_copy (const constexpr_ctx *ctx, > > > > > > > constexpr_fundef *fundef) > > > > > > > } > > > > > > > else if (*slot == NULL_TREE) > > > > > > > { > > > > > > > - if (ctx->uid_sensitive) > > > > > > > + if (uid_sensitive_constexpr_evaluation_p ()) > > > > > > > return NULL_TREE; > > > > > > > /* We've already used the function itself, so make a > > > > > > > copy. > > > > > > > */ > > > > > > > @@ -2292,8 +2340,8 @@ cxx_eval_call_expression (const > > > > > > > constexpr_ctx > > > > > > > *ctx, > > > > > > > tree t, > > > > > > > /* We can't defer instantiating the function any longer. > > > > > > > */ > > > > > > > if (!DECL_INITIAL (fun) > > > > > > > - && !ctx->uid_sensitive > > > > > > > - && DECL_TEMPLOID_INSTANTIATION (fun)) > > > > > > > + && DECL_TEMPLOID_INSTANTIATION (fun) > > > > > > > + && !uid_sensitive_constexpr_evaluation_p ()) > > > > > > > { > > > > > > > location_t save_loc = input_location; > > > > > > > input_location = loc; > > > > > > > @@ -2454,7 +2502,7 @@ cxx_eval_call_expression (const > > > > > > > constexpr_ctx > > > > > > > *ctx, > > > > > > > tree t, > > > > > > > gcc_assert (at_eof >= 2 && ctx->quiet); > > > > > > > *non_constant_p = true; > > > > > > > } > > > > > > > - else if (tree copy = get_fundef_copy (ctx, > > > > > > > new_call.fundef)) > > > > > > > + else if (tree copy = get_fundef_copy (new_call.fundef)) > > > > > > > { > > > > > > > tree body, parms, res; > > > > > > > releasing_vec ctors; > > > > > > > @@ -6485,7 +6533,8 @@ instantiate_cx_fn_r (tree *tp, int > > > > > > > *walk_subtrees, > > > > > > > void */*data*/) > > > > > > > && DECL_DECLARED_CONSTEXPR_P (*tp) > > > > > > > && !DECL_INITIAL (*tp) > > > > > > > && !trivial_fn_p (*tp) > > > > > > > - && DECL_TEMPLOID_INSTANTIATION (*tp)) > > > > > > > + && DECL_TEMPLOID_INSTANTIATION (*tp) > > > > > > > + && !uid_sensitive_constexpr_evaluation_p ()) > > > > > > > { > > > > > > > ++function_depth; > > > > > > > instantiate_decl (*tp, /*defer_ok*/false, > > > > > > > /*expl_inst*/false); > > > > > > > @@ -6552,8 +6601,7 @@ cxx_eval_outermost_constant_expr (tree t, > > > > > > > bool > > > > > > > allow_non_constant, > > > > > > > bool strict = true, > > > > > > > bool manifestly_const_eval = > > > > > > > false, > > > > > > > bool constexpr_dtor = false, > > > > > > > - tree object = NULL_TREE, > > > > > > > - bool uid_sensitive = false) > > > > > > > + tree object = NULL_TREE) > > > > > > > { > > > > > > > auto_timevar time (TV_CONSTEXPR); > > > > > > > @@ -6569,8 +6617,7 @@ cxx_eval_outermost_constant_expr (tree > > > > > > > t, > > > > > > > bool > > > > > > > allow_non_constant, > > > > > > > constexpr_global_ctx global_ctx; > > > > > > > constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, > > > > > > > NULL, > > > > > > > NULL, > > > > > > > allow_non_constant, strict, > > > > > > > - manifestly_const_eval || !allow_non_constant, > > > > > > > - uid_sensitive }; > > > > > > > + manifestly_const_eval || !allow_non_constant > > > > > > > }; > > > > > > > tree type = initialized_type (t); > > > > > > > tree r = t; > > > > > > > @@ -6660,8 +6707,7 @@ cxx_eval_outermost_constant_expr (tree t, > > > > > > > bool > > > > > > > allow_non_constant, > > > > > > > auto_vec<tree, 16> cleanups; > > > > > > > global_ctx.cleanups = &cleanups; > > > > > > > - if (!uid_sensitive) > > > > > > > - instantiate_constexpr_fns (r); > > > > > > > + instantiate_constexpr_fns (r); > > > > > > > r = cxx_eval_constant_expression (&ctx, r, > > > > > > > false, &non_constant_p, > > > > > > > &overflow_p); > > > > > > > @@ -6909,14 +6955,12 @@ fold_simple (tree t) > > > > > > > Otherwise, if T does not have TREE_CONSTANT set, returns T. > > > > > > > Otherwise, returns a version of T without TREE_CONSTANT. > > > > > > > MANIFESTLY_CONST_EVAL is true if T is manifestly > > > > > > > const-evaluated > > > > > > > - as per P0595. UID_SENSITIVE is true if we can't do anything > > > > > > > that > > > > > > > - would affect DECL_UID ordering. */ > > > > > > > + as per P0595. */ > > > > > > > static GTY((deletable)) hash_map<tree, tree> *cv_cache; > > > > > > > tree > > > > > > > -maybe_constant_value (tree t, tree decl, bool > > > > > > > manifestly_const_eval, > > > > > > > - bool uid_sensitive) > > > > > > > +maybe_constant_value (tree t, tree decl, bool > > > > > > > manifestly_const_eval) > > > > > > > { > > > > > > > tree r; > > > > > > > @@ -6934,8 +6978,7 @@ maybe_constant_value (tree t, tree decl, > > > > > > > bool > > > > > > > manifestly_const_eval, > > > > > > > return t; > > > > > > > if (manifestly_const_eval) > > > > > > > - return cxx_eval_outermost_constant_expr (t, true, true, true, > > > > > > > false, > > > > > > > - decl, uid_sensitive); > > > > > > > + return cxx_eval_outermost_constant_expr (t, true, true, true, > > > > > > > false, > > > > > > > decl); > > > > > > > if (cv_cache == NULL) > > > > > > > cv_cache = hash_map<tree, tree>::create_ggc (101); > > > > > > > @@ -6950,14 +6993,15 @@ maybe_constant_value (tree t, tree decl, > > > > > > > bool > > > > > > > manifestly_const_eval, > > > > > > > return r; > > > > > > > } > > > > > > > - r = cxx_eval_outermost_constant_expr (t, true, true, false, > > > > > > > false, > > > > > > > - decl, uid_sensitive); > > > > > > > + uid_sensitive_constexpr_evaluation_checker c; > > > > > > > + r = cxx_eval_outermost_constant_expr (t, true, true, false, > > > > > > > false, > > > > > > > decl); > > > > > > > gcc_checking_assert (r == t > > > > > > > || CONVERT_EXPR_P (t) > > > > > > > || TREE_CODE (t) == VIEW_CONVERT_EXPR > > > > > > > || (TREE_CONSTANT (t) && !TREE_CONSTANT > > > > > > > (r)) > > > > > > > || !cp_tree_equal (r, t)); > > > > > > > - cv_cache->put (t, r); > > > > > > > + if (!c.evaluation_restricted_p ()) > > > > > > > + cv_cache->put (t, r); > > > > > > > return r; > > > > > > > } > > > > > > > diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c > > > > > > > index fc26a85f43a..f298fa7cec1 100644 > > > > > > > --- a/gcc/cp/cp-gimplify.c > > > > > > > +++ b/gcc/cp/cp-gimplify.c > > > > > > > @@ -2443,6 +2443,8 @@ cp_fold (tree x) > > > > > > > if (tree *cached = fold_cache->get (x)) > > > > > > > return *cached; > > > > > > > + uid_sensitive_constexpr_evaluation_checker c; > > > > > > > + > > > > > > > code = TREE_CODE (x); > > > > > > > switch (code) > > > > > > > { > > > > > > > @@ -2925,10 +2927,13 @@ cp_fold (tree x) > > > > > > > return org_x; > > > > > > > } > > > > > > > - fold_cache->put (org_x, x); > > > > > > > - /* Prevent that we try to fold an already folded result again. > > > > > > > */ > > > > > > > - if (x != org_x) > > > > > > > - fold_cache->put (x, x); > > > > > > > + if (!c.evaluation_restricted_p ()) > > > > > > > + { > > > > > > > + fold_cache->put (org_x, x); > > > > > > > + /* Prevent that we try to fold an already folded result > > > > > > > again. > > > > > > > */ > > > > > > > + if (x != org_x) > > > > > > > + fold_cache->put (x, x); > > > > > > > + } > > > > > > > return x; > > > > > > > } > > > > > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > > > > > > index deb000babc1..c2511196de4 100644 > > > > > > > --- a/gcc/cp/cp-tree.h > > > > > > > +++ b/gcc/cp/cp-tree.h > > > > > > > @@ -7949,7 +7949,7 @@ extern bool > > > > > > > require_potential_rvalue_constant_expression (tree); > > > > > > > extern tree cxx_constant_value (tree, tree = > > > > > > > NULL_TREE); > > > > > > > extern void cxx_constant_dtor (tree, tree); > > > > > > > extern tree cxx_constant_init (tree, tree = > > > > > > > NULL_TREE); > > > > > > > -extern tree maybe_constant_value (tree, tree = > > > > > > > NULL_TREE, bool > > > > > > > = false, bool = false); > > > > > > > +extern tree maybe_constant_value (tree, tree = > > > > > > > NULL_TREE, bool > > > > > > > = false); > > > > > > > extern tree maybe_constant_init (tree, tree = > > > > > > > NULL_TREE, bool = false); > > > > > > > extern tree fold_non_dependent_expr (tree, > > > > > > > tsubst_flags_t > > > > > > > = > > > > > > > tf_warning_or_error, > > > > > > > @@ -7968,6 +7968,27 @@ extern tree fold_sizeof_expr > > > > > > > (tree); > > > > > > > extern void clear_cv_and_fold_caches (bool = true); > > > > > > > extern tree unshare_constructor (tree > > > > > > > CXX_MEM_STAT_INFO); > > > > > > > +/* An RAII sentinel used to restrict constexpr evaluation so > > > > > > > that > > > > > > > it > > > > > > > + doesn't do anything that causes extra DECL_UID generation. */ > > > > > > > + > > > > > > > +struct uid_sensitive_constexpr_evaluation_sentinel > > > > > > > +{ > > > > > > > + temp_override<bool> ovr; > > > > > > > + uid_sensitive_constexpr_evaluation_sentinel (); > > > > > > > +}; > > > > > > > + > > > > > > > +/* Used to determine whether uid_sensitive_constexpr_evaluation_p > > > > > > > was > > > > > > > + called and returned true, indicating that we've restricted > > > > > > > constexpr > > > > > > > + evaluation in order to avoid UID generation. We use this to > > > > > > > control > > > > > > > + updates to the fold_cache and cv_cache. */ > > > > > > > + > > > > > > > +struct uid_sensitive_constexpr_evaluation_checker > > > > > > > +{ > > > > > > > + const unsigned saved_counter; > > > > > > > + uid_sensitive_constexpr_evaluation_checker (); > > > > > > > + bool evaluation_restricted_p () const; > > > > > > > +}; > > > > > > > + > > > > > > > /* In cp-ubsan.c */ > > > > > > > extern void cp_ubsan_maybe_instrument_member_call (tree); > > > > > > > extern void cp_ubsan_instrument_member_accesses (tree *); > > > > > > > diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c > > > > > > > index 9b535708c57..0d68a738f8f 100644 > > > > > > > --- a/gcc/cp/expr.c > > > > > > > +++ b/gcc/cp/expr.c > > > > > > > @@ -399,6 +399,8 @@ fold_for_warn (tree x) > > > > > > > { > > > > > > > /* C++ implementation. */ > > > > > > > + uid_sensitive_constexpr_evaluation_sentinel s; > > > > > > > > > > > > Please add a comment about why we need this. > > > > > > > > > > Thanks for the review. Here's the final patch (with the added > > > > > comments) > > > > > that I've committed: > > > > > > This seems to have broken cmcstl2: > > > > > > > Building CXX object > > > > test/iterator/CMakeFiles/make_range.dir/make_range.cpp.o > > > > In file included from > > > > /home/jason/src/cmcstl2/include/stl2/type_traits.hpp:16, > > > > from > > > > /home/jason/src/cmcstl2/include/stl2/iterator.hpp:18, > > > > from > > > > /home/jason/src/cmcstl2/test/iterator/make_range.cpp:1: > > > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits: > > > > In > > > > instantiation of ‘struct std::is_nothrow_destructible<int*>’: > > > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:3170:35: > > > > required from ‘constexpr const bool > > > > std::is_nothrow_destructible_v<int*>’ > > > > /home/jason/src/cmcstl2/include/stl2/detail/concepts/object/move_constructible.hpp:34:35: > > > > required from here > > > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: > > > > error: non-constant condition for static assertion > > > > 895 | > > > > static_assert(std::__is_complete_or_unbounded(__type_identity<_Tp>{}), > > > > | > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~ > > > > /home/jason/s/gcc/x86_64-pc-linux-gnu/libstdc++-v3/include/type_traits:895:52: > > > > error: ‘constexpr std::true_type > > > > std::__is_complete_or_unbounded(std::__type_identity<_Tp>) [with _Tp = > > > > int*; > > > > long unsigned int <anonymous> = 8; std::true_type = > > > > std::integral_constant<bool, true>]’ used before its definition > > > > Yikes :( The following fixlet seems to restore the cmcstl2 build: > > > > diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c > > index 98c974e657f..4e441ac8d2f 100644 > > --- a/gcc/cp/constexpr.c > > +++ b/gcc/cp/constexpr.c > > @@ -6486,7 +6486,8 @@ cxx_eval_constant_expression (const constexpr_ctx > > *ctx, tree t, > > break; > > } > > - if (!processing_template_decl) > > + if (!processing_template_decl > > + && !uid_sensitive_constexpr_evaluation_p ()) > > r = evaluate_concept_check (t, tf_warning_or_error); > > else > > *non_constant_p = true; > > > > Concept evaluation may entail DECL_UID generation and/or template > > instantiation, so in general it seems we can't perform it during > > uid-sensitive constexpr evaluation. > > Makes sense. The patch is OK. I've just committed the following after bootstrap/regtest on x86_64-pc-linux-gnu, and after verifying that the build of cmcstl2 is restored: -- >8 -- Subject: [PATCH] c++: Avoid concept evaluation when uid-sensitive [PR94038] Concept evaluation may entail DECL_UID generation and/or template instantiation, so in general we can't perform it during uid-sensitive constexpr evaluation. gcc/cp/ChangeLog: PR c++/94038 * constexpr.c (cxx_eval_constant_expression) <case TEMPLATE_ID_EXPR>: Don't evaluate the concept when constexpr evaluation is uid-sensitive. gcc/testsuite/ChangeLog: PR c++/94038 * g++.dg/warn/pr94038-3.C: New test. --- gcc/cp/constexpr.c | 3 ++- gcc/testsuite/g++.dg/warn/pr94038-3.C | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/warn/pr94038-3.C diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 98c974e657f..4e441ac8d2f 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -6486,7 +6486,8 @@ cxx_eval_constant_expression (const constexpr_ctx *ctx, tree t, break; } - if (!processing_template_decl) + if (!processing_template_decl + && !uid_sensitive_constexpr_evaluation_p ()) r = evaluate_concept_check (t, tf_warning_or_error); else *non_constant_p = true; diff --git a/gcc/testsuite/g++.dg/warn/pr94038-3.C b/gcc/testsuite/g++.dg/warn/pr94038-3.C new file mode 100644 index 00000000000..49b6d133f08 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/pr94038-3.C @@ -0,0 +1,15 @@ +// PR c++/94038 +// { dg-do compile { target c++20 } } +// { dg-additional-options "-Wall" } + +template<typename T> +constexpr int foo() { + return T::x; +} + +constexpr bool bar(bool a) { return a; } + +template<typename T> +concept C = foo<T>() == 0; + +static_assert(decltype(bar(C<int>)){} == false);
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c index 637cb746576..7bce9e6e9b0 100644 --- a/gcc/cp/constexpr.c +++ b/gcc/cp/constexpr.c @@ -1089,11 +1089,28 @@ struct constexpr_ctx { bool strict; /* Whether __builtin_is_constant_evaluated () should be true. */ bool manifestly_const_eval; - /* Whether we want to avoid doing anything that will cause extra DECL_UID - generation. */ - bool uid_sensitive; }; +/* This internal flag controls whether we should avoid doing anything during + constexpr evaluation that would cause extra DECL_UID generation, such as + template instantiation and function copying. */ + +bool uid_sensitive_constexpr_evaluation::value; + +/* An internal counter that keeps track of the number of times + uid_sensitive_constexpr_evaluation_p was called. */ + +unsigned uid_sensitive_constexpr_evaluation::counter; + +/* The public accessor for uid_sensitive_constexpr_evaluation::value. */ + +bool +uid_sensitive_constexpr_evaluation_p () +{ + ++uid_sensitive_constexpr_evaluation::counter; + return uid_sensitive_constexpr_evaluation::value; +} + /* A table of all constexpr calls that have been evaluated by the compiler in this translation unit. */ @@ -1156,7 +1173,7 @@ static GTY(()) hash_map<tree, tree> *fundef_copies_table; is parms, TYPE is result. */ static tree -get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) +get_fundef_copy (constexpr_fundef *fundef) { tree copy; bool existed; @@ -1173,7 +1190,7 @@ get_fundef_copy (const constexpr_ctx *ctx, constexpr_fundef *fundef) } else if (*slot == NULL_TREE) { - if (ctx->uid_sensitive) + if (uid_sensitive_constexpr_evaluation_p ()) return NULL_TREE; /* We've already used the function itself, so make a copy. */ @@ -2292,8 +2309,8 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, /* We can't defer instantiating the function any longer. */ if (!DECL_INITIAL (fun) - && !ctx->uid_sensitive - && DECL_TEMPLOID_INSTANTIATION (fun)) + && DECL_TEMPLOID_INSTANTIATION (fun) + && !uid_sensitive_constexpr_evaluation_p ()) { location_t save_loc = input_location; input_location = loc; @@ -2454,7 +2471,7 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t, gcc_assert (at_eof >= 2 && ctx->quiet); *non_constant_p = true; } - else if (tree copy = get_fundef_copy (ctx, new_call.fundef)) + else if (tree copy = get_fundef_copy (new_call.fundef)) { tree body, parms, res; releasing_vec ctors; @@ -6485,7 +6502,8 @@ instantiate_cx_fn_r (tree *tp, int *walk_subtrees, void */*data*/) && DECL_DECLARED_CONSTEXPR_P (*tp) && !DECL_INITIAL (*tp) && !trivial_fn_p (*tp) - && DECL_TEMPLOID_INSTANTIATION (*tp)) + && DECL_TEMPLOID_INSTANTIATION (*tp) + && !uid_sensitive_constexpr_evaluation_p ()) { ++function_depth; instantiate_decl (*tp, /*defer_ok*/false, /*expl_inst*/false); @@ -6552,8 +6570,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, bool strict = true, bool manifestly_const_eval = false, bool constexpr_dtor = false, - tree object = NULL_TREE, - bool uid_sensitive = false) + tree object = NULL_TREE) { auto_timevar time (TV_CONSTEXPR); @@ -6569,8 +6586,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, constexpr_global_ctx global_ctx; constexpr_ctx ctx = { &global_ctx, NULL, NULL, NULL, NULL, NULL, NULL, allow_non_constant, strict, - manifestly_const_eval || !allow_non_constant, - uid_sensitive }; + manifestly_const_eval || !allow_non_constant }; tree type = initialized_type (t); tree r = t; @@ -6660,7 +6676,7 @@ cxx_eval_outermost_constant_expr (tree t, bool allow_non_constant, auto_vec<tree, 16> cleanups; global_ctx.cleanups = &cleanups; - if (!uid_sensitive) + if (!uid_sensitive_constexpr_evaluation::value) instantiate_constexpr_fns (r); r = cxx_eval_constant_expression (&ctx, r, false, &non_constant_p, &overflow_p); @@ -6909,14 +6925,12 @@ fold_simple (tree t) Otherwise, if T does not have TREE_CONSTANT set, returns T. Otherwise, returns a version of T without TREE_CONSTANT. MANIFESTLY_CONST_EVAL is true if T is manifestly const-evaluated - as per P0595. UID_SENSITIVE is true if we can't do anything that - would affect DECL_UID ordering. */ + as per P0595. */ static GTY((deletable)) hash_map<tree, tree> *cv_cache; tree -maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, - bool uid_sensitive) +maybe_constant_value (tree t, tree decl, bool manifestly_const_eval) { tree r; @@ -6934,8 +6948,7 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return t; if (manifestly_const_eval) - return cxx_eval_outermost_constant_expr (t, true, true, true, false, - decl, uid_sensitive); + return cxx_eval_outermost_constant_expr (t, true, true, true, false, decl); if (cv_cache == NULL) cv_cache = hash_map<tree, tree>::create_ggc (101); @@ -6950,14 +6963,15 @@ maybe_constant_value (tree t, tree decl, bool manifestly_const_eval, return r; } - r = cxx_eval_outermost_constant_expr (t, true, true, false, false, - decl, uid_sensitive); + uid_sensitive_constexpr_evaluation_marker m; + r = cxx_eval_outermost_constant_expr (t, true, true, false, false, decl); gcc_checking_assert (r == t || CONVERT_EXPR_P (t) || TREE_CODE (t) == VIEW_CONVERT_EXPR || (TREE_CONSTANT (t) && !TREE_CONSTANT (r)) || !cp_tree_equal (r, t)); - cv_cache->put (t, r); + if (!m.changed_p ()) + cv_cache->put (t, r); return r; } diff --git a/gcc/cp/cp-gimplify.c b/gcc/cp/cp-gimplify.c index fc26a85f43a..2f04b41b582 100644 --- a/gcc/cp/cp-gimplify.c +++ b/gcc/cp/cp-gimplify.c @@ -2443,6 +2443,8 @@ cp_fold (tree x) if (tree *cached = fold_cache->get (x)) return *cached; + uid_sensitive_constexpr_evaluation_marker m; + code = TREE_CODE (x); switch (code) { @@ -2925,10 +2927,13 @@ cp_fold (tree x) return org_x; } - fold_cache->put (org_x, x); - /* Prevent that we try to fold an already folded result again. */ - if (x != org_x) - fold_cache->put (x, x); + if (!m.changed_p ()) + { + fold_cache->put (org_x, x); + /* Prevent that we try to fold an already folded result again. */ + if (x != org_x) + fold_cache->put (x, x); + } return x; } diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index f4328403bc7..784b6057080 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -7949,7 +7949,7 @@ extern bool require_potential_rvalue_constant_expression (tree); extern tree cxx_constant_value (tree, tree = NULL_TREE); extern void cxx_constant_dtor (tree, tree); extern tree cxx_constant_init (tree, tree = NULL_TREE); -extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false, bool = false); +extern tree maybe_constant_value (tree, tree = NULL_TREE, bool = false); extern tree maybe_constant_init (tree, tree = NULL_TREE, bool = false); extern tree fold_non_dependent_expr (tree, tsubst_flags_t = tf_warning_or_error, @@ -7968,6 +7968,48 @@ extern tree fold_sizeof_expr (tree); extern void clear_cv_and_fold_caches (bool = true); extern tree unshare_constructor (tree CXX_MEM_STAT_INFO); +extern bool uid_sensitive_constexpr_evaluation_p (); + +/* An RAII sentinel to temporarily have uid_sensitive_constexpr_evaluation_p + return true. */ + +struct uid_sensitive_constexpr_evaluation +{ + const bool saved_value; + uid_sensitive_constexpr_evaluation () + : saved_value (value) + { + value = true; + } + + ~uid_sensitive_constexpr_evaluation () + { + value = saved_value; + } + + static bool value; + static unsigned counter; +}; + +/* A class used to determine whether uid_sensitive_constexpr_evaluation_p was + called within some scope. We use this to avoid updating fold_cache and + cv_cache when the result of constexpr evaluation depended on the value of + uid_sensitive_constexpr_evaluation_p. */ + +struct uid_sensitive_constexpr_evaluation_marker +{ + const unsigned saved_counter; + uid_sensitive_constexpr_evaluation_marker () + : saved_counter (uid_sensitive_constexpr_evaluation::counter) + { + } + + bool changed_p () + { + return uid_sensitive_constexpr_evaluation::counter != saved_counter; + } +}; + /* In cp-ubsan.c */ extern void cp_ubsan_maybe_instrument_member_call (tree); extern void cp_ubsan_instrument_member_accesses (tree *); diff --git a/gcc/cp/expr.c b/gcc/cp/expr.c index 9b535708c57..97671edf800 100644 --- a/gcc/cp/expr.c +++ b/gcc/cp/expr.c @@ -399,6 +399,8 @@ fold_for_warn (tree x) { /* C++ implementation. */ + uid_sensitive_constexpr_evaluation s; + /* It's not generally safe to fully fold inside of a template, so call fold_non_dependent_expr instead. */ if (processing_template_decl) @@ -410,7 +412,7 @@ fold_for_warn (tree x) return f; } else if (cxx_dialect >= cxx11) - x = maybe_constant_value (x, NULL_TREE, false, true); + x = maybe_constant_value (x); return c_fully_fold (x, /*for_init*/false, /*maybe_constp*/NULL); } diff --git a/gcc/testsuite/g++.dg/warn/pr94038-2.C b/gcc/testsuite/g++.dg/warn/pr94038-2.C new file mode 100644 index 00000000000..a468cc055eb --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/pr94038-2.C @@ -0,0 +1,28 @@ +// PR c++/94038 +// { dg-do compile { target c++11 } } +// { dg-additional-options "-O -Wall" } + +static constexpr int x = 0; + +template<typename T> +constexpr const int& +foo() +{ + static_assert(T(1) == 0, ""); + return x; +} + +template<typename T> +constexpr const int& +bar() +{ + return foo<T>(); +} + +constexpr int +baz(int a) +{ + return a; +} + +static_assert(decltype(baz(bar<int>())){} == 0, "");