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 > >