[PATCH] c++: template instantiation during fold_for_warn [PR94038]
Jason Merrill
jason@redhat.com
Fri May 22 20:17:54 GMT 2020
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
More information about the Gcc-patches
mailing list