Jason.
we currently clone constexpr function bodies when evaluating them (via the reuse
cache that caused such fun). The cloning is necessary so decls in different
(recursive) evaluations are distinct. We only need to clone for recursive
evaluations though -- the outermost evaluation could use the original function body.
That leaves us with needing some mechanism to know whether we're doing a
recursive call or not. This patch implements such an optimization.
get_fundef_copy is changed to use get-or-insert, and uses the 'existed' arg of
get_or_insert to know whether a new slot was created or not. If a new slot was
created, we know we're not currently evaluating the function, so we can use it
directly without copy. Otherwise, we look in the slot to see if there's a
cached body available. If there isn't we create a clone.
When saving the function for later reuse, the original fn is not distinguished
from copies. That's ok -- we'll just see it as a cached body available for
reuse on the next call. and if we run out of cached bodies, we'll create them
because the slot will not have just been created.
Manually instrumenting the new testcase shows things behaving as I expected.
The change in error behaviour of ubsan/pr63956.C was a surprise, but I think a
good one. That testcase boiled down to:
constexpr int
fn6 (const int &a, int b)
{
b = a; <-- problem here
return b;
}
constexpr int
fn7 (const int *a, int b)
{
return fn6 (*a, b);
}
constexpr int n4 = fn7 ((const int *) 0, 8); <-- diag here
and we emitted the following diagnostic:
pr63956.ii:15:24: in constexpr expansion of 'fn7(0u, 8)'
pr63956.ii:12:14: in constexpr expansion of 'fn6((* a), b)'
pr63956.ii:15:43: error: 'a' is not a constant expression
constexpr int n4 = fn7 ((const int *) 0, 8);
notice that final line is the assignment of 'n4' and not the location of the
problem.
With this patch we now produce:
pr63956.ii: At global scope:
pr63956.ii:14:24: in constexpr expansion of 'fn7(0u, 8)'
pr63956.ii:11:14: in constexpr expansion of 'fn6((* a), b)'
pr63956.ii:5:7: error: 'a' is not a constant expression
b = a;
which is better. There's something about the function cloning that throws off
the error location information. Changing fn6 to be conditionally recursive will
lead to the older error location behaviour when the problem is the cloned
instance. IMHO this is a collateral latent bug, and now it only affects
recursive constexpr fns.
ok for trunk?
nathan
2016-04-26 Nathan Sidwell <nat...@acm.org>
cp/
* constexpr.c (get_fundef_copy): Use the original function for
non-recursive evaluations.
(save_fundef_copy): Always expect a slot to be available.
testsuite/
* g++.dg/cpp0x/constexpr-recursion3.C: New.
* g++.dg/ubsan/pr63956.C: Adjust error location.
Index: cp/constexpr.c
===================================================================
--- cp/constexpr.c (revision 235364)
+++ cp/constexpr.c (working copy)
@@ -989,7 +989,8 @@ maybe_initialize_fundef_copies_table ()
}
/* Reuse a copy or create a new unshared copy of the function FUN.
- Return this copy. */
+ Return this copy. We use a TREE_LIST whose PURPOSE is body, VALUE
+ is parms, TYPE is result. */
static tree
get_fundef_copy (tree fun)
@@ -997,15 +998,26 @@ get_fundef_copy (tree fun)
maybe_initialize_fundef_copies_table ();
tree copy;
- tree *slot = fundef_copies_table->get (fun);
- if (slot == NULL || *slot == NULL_TREE)
+ bool existed;
+ tree *slot = &fundef_copies_table->get_or_insert (fun, &existed);
+
+ if (!existed)
+ {
+ /* There is no cached function available, or in use. We can use
+ the function directly. That the slot is now created records
+ that this function is now in use. */
+ copy = build_tree_list (DECL_SAVED_TREE (fun), DECL_ARGUMENTS (fun));
+ TREE_TYPE (copy) = DECL_RESULT (fun);
+ }
+ else if (*slot == NULL_TREE)
{
+ /* We've already used the function itself, so make a copy. */
copy = build_tree_list (NULL, NULL);
- /* PURPOSE is body, VALUE is parms, TYPE is result. */
TREE_PURPOSE (copy) = copy_fn (fun, TREE_VALUE (copy), TREE_TYPE (copy));
}
else
{
+ /* We have a cached function available. */
copy = *slot;
*slot = TREE_CHAIN (copy);
}
@@ -1013,12 +1025,14 @@ get_fundef_copy (tree fun)
return copy;
}
-/* Save the copy COPY of function FUN for later reuse by get_fundef_copy(). */
+/* Save the copy COPY of function FUN for later reuse by
+ get_fundef_copy(). By construction, there will always be an entry
+ to find. */
static void
save_fundef_copy (tree fun, tree copy)
{
- tree *slot = &fundef_copies_table->get_or_insert (fun, NULL);
+ tree *slot = fundef_copies_table->get (fun);
TREE_CHAIN (copy) = *slot;
*slot = copy;
}
Index: testsuite/g++.dg/cpp0x/constexpr-recursion3.C
===================================================================
--- testsuite/g++.dg/cpp0x/constexpr-recursion3.C (nonexistent)
+++ testsuite/g++.dg/cpp0x/constexpr-recursion3.C (working copy)
@@ -0,0 +1,14 @@
+// { dg-do compile { target c++11 } }
+
+constexpr int Foo (int i)
+{
+ return (i ? Foo (i - 1): 0) + i;
+}
+
+static int a = Foo (0);
+static int b = Foo (1);
+static int d = Foo (3);
+static int c = Foo (2);
+static int e = Foo (4);
+static int g = Foo (6);
+static int f = Foo (5);
Index: testsuite/g++.dg/ubsan/pr63956.C
===================================================================
--- testsuite/g++.dg/ubsan/pr63956.C (revision 235364)
+++ testsuite/g++.dg/ubsan/pr63956.C (working copy)
@@ -92,7 +92,7 @@ constexpr int
fn6 (const int &a, int b)
{
if (b != 2)
- b = a;
+ b = a; // { dg-error "is not a constant expression" }
return b;
}
@@ -106,7 +106,7 @@ fn7 (const int *a, int b)
constexpr int n1 = 7;
constexpr int n2 = fn7 (&n1, 5);
-constexpr int n3 = fn7 ((const int *) 0, 8); // { dg-error "is not a constant expression" }
+constexpr int n3 = fn7 ((const int *) 0, 8);
constexpr int
fn8 (int i)
