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.
