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.
