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 definitionYikes :( 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
