[PR86218] handle ck_aggr in compare_ics in both and either conversion

Message ID orzhrirvfc.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • [PR86218] handle ck_aggr in compare_ics in both and either conversion
Related show

Commit Message

Alexandre Oliva Jan. 30, 2019, 3:56 p.m.
Because of rank compares, and checks for ck_list, we know that if we
see user_conv_p or ck_list in ics1, we'll also see it in ics2.  This
reasoning does not extend to ck_aggr, however, so we might have
ck_aggr conversions starting both ics1 and ics2, which we handle
correctly, or either, which we likely handle by crashing on whatever
path we take depending on whether ck_aggr is in ics1 or ics2.

We crash because, as we search the conversion sequences, we may very
well fail to find what we are looking for, and reach the end of the
sequence, which is unexpected in all paths.

This patch arranges for us to take the same path when ck_aggr is in
ics2 only that we would if it was in ics1 (regardless of ics2), and it
deals with not finding the kind of conversion we look for there.

I've changed the type of the literal constant in the testcase, so as
to hopefully make it well-formed.  We'd fail to reject the narrowing
conversion in the original testcase, but that's a separate bug.

Regstrapped on x86_64- and i686-linux-gnu.  Ok to install?


for  gcc/cp/ChangeLog

	PR c++/86218
	* call.c (compare_ics): Deal with ck_aggr in either cs.

for  gcc/testsuite/ChangeLog

	PR c++/86218
	* g++.dg/cpp0x/pr86218.C: New.
---
 gcc/cp/call.c                        |    9 +++++----
 gcc/testsuite/g++.dg/cpp0x/pr86218.C |   11 +++++++++++
 2 files changed, 16 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/pr86218.C



-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free!         FSF Latin America board member
GNU Toolchain Engineer                Free Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe

Comments

Alexandre Oliva Feb. 5, 2019, 6:08 a.m. | #1
On Jan 30, 2019, Alexandre Oliva <aoliva@redhat.com> wrote:

> Because of rank compares, and checks for ck_list, we know that if we

> see user_conv_p or ck_list in ics1, we'll also see it in ics2.  This

> reasoning does not extend to ck_aggr, however, so we might have

> ck_aggr conversions starting both ics1 and ics2, which we handle

> correctly, or either, which we likely handle by crashing on whatever

> path we take depending on whether ck_aggr is in ics1 or ics2.

[...]

Ping?  https://gcc.gnu.org/ml/gcc-patches/2019-01/msg01731.html

> for  gcc/cp/ChangeLog


> 	PR c++/86218

> 	* call.c (compare_ics): Deal with ck_aggr in either cs.


> for  gcc/testsuite/ChangeLog


> 	PR c++/86218

> 	* g++.dg/cpp0x/pr86218.C: New.


-- 
Alexandre Oliva, freedom fighter   https://FSFLA.org/blogs/lxo
Be the change, be Free!         FSF Latin America board member
GNU Toolchain Engineer                Free Software Evangelist
Hay que enGNUrecerse, pero sin perder la terGNUra jamás-GNUChe
Jason Merrill Feb. 5, 2019, 6:58 p.m. | #2
On 1/30/19 10:56 AM, Alexandre Oliva wrote:
> Because of rank compares, and checks for ck_list, we know that if we

> see user_conv_p or ck_list in ics1, we'll also see it in ics2.  This

> reasoning does not extend to ck_aggr, however, so we might have

> ck_aggr conversions starting both ics1 and ics2, which we handle

> correctly, or either, which we likely handle by crashing on whatever

> path we take depending on whether ck_aggr is in ics1 or ics2.

> 

> We crash because, as we search the conversion sequences, we may very

> well fail to find what we are looking for, and reach the end of the

> sequence, which is unexpected in all paths.

> 

> This patch arranges for us to take the same path when ck_aggr is in

> ics2 only that we would if it was in ics1 (regardless of ics2), and it

> deals with not finding the kind of conversion we look for there.

> 

> I've changed the type of the literal constant in the testcase, so as

> to hopefully make it well-formed.  We'd fail to reject the narrowing

> conversion in the original testcase, but that's a separate bug.

> 

> Regstrapped on x86_64- and i686-linux-gnu.  Ok to install?

> 

> 

> for  gcc/cp/ChangeLog

> 

> 	PR c++/86218

> 	* call.c (compare_ics): Deal with ck_aggr in either cs.


OK.

Jason

Patch

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index c74d1b4ebdf60..7ae67004b9359 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -10033,21 +10033,22 @@  compare_ics (conversion *ics1, conversion *ics2)
      Specifically, we need to do the reference binding comparison at the
      end of this function.  */
 
-  if (ics1->user_conv_p || ics1->kind == ck_list || ics1->kind == ck_aggr)
+  if (ics1->user_conv_p || ics1->kind == ck_list
+      || ics1->kind == ck_aggr || ics2->kind == ck_aggr)
     {
       conversion *t1;
       conversion *t2;
 
-      for (t1 = ics1; t1->kind != ck_user; t1 = next_conversion (t1))
+      for (t1 = ics1; t1 && t1->kind != ck_user; t1 = next_conversion (t1))
 	if (t1->kind == ck_ambig || t1->kind == ck_aggr
 	    || t1->kind == ck_list)
 	  break;
-      for (t2 = ics2; t2->kind != ck_user; t2 = next_conversion (t2))
+      for (t2 = ics2; t2 && t2->kind != ck_user; t2 = next_conversion (t2))
 	if (t2->kind == ck_ambig || t2->kind == ck_aggr
 	    || t2->kind == ck_list)
 	  break;
 
-      if (t1->kind != t2->kind)
+      if (!t1 || !t2 || t1->kind != t2->kind)
 	return 0;
       else if (t1->kind == ck_user)
 	{
diff --git a/gcc/testsuite/g++.dg/cpp0x/pr86218.C b/gcc/testsuite/g++.dg/cpp0x/pr86218.C
new file mode 100644
index 0000000000000..9892ccde5be9c
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/pr86218.C
@@ -0,0 +1,11 @@ 
+// { dg-do compile { target c++11 } }
+
+template <int a>
+void f (const char (&)[a]) { }
+void f (int) { }
+template <class...a>
+void
+g ()
+{
+  f ({2u});
+}



Before getting to the simpler formulation above, I tried to model more
closely what's specified in the standard for braced initializer lists
used to initialize arrays and complex types, namely, to record the worst
conversion sequence in the ck_aggr conversion, and use that for
comparison purposes instead.  It turned out to not be of much use,
because we already keep track of ranks and that's pretty much the only
relevant data for the compare, but I post it below, for the record, just
in case someone finds it useful.


[PR86218] save worst aggr conv in ck_aggr non-class conversions

---
 gcc/cp/call.c |   29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/gcc/cp/call.c b/gcc/cp/call.c
index 7ae67004b9359..c6beda585327c 100644
--- a/gcc/cp/call.c
+++ b/gcc/cp/call.c
@@ -128,9 +128,14 @@  struct conversion {
        variant is used only when KIN D is ck_list.  */
     conversion **list;
   } u;
-  /* The function candidate corresponding to this conversion
-     sequence.  This field is only used if KIND is ck_user.  */
-  struct z_candidate *cand;
+  union {
+    /* The function candidate corresponding to this conversion
+       sequence.  This field is only used if KIND is ck_user.  */
+    struct z_candidate *cand;
+    /* The worst conversion sequence for the aggregate.  This field is
+       onl used if KIND is ck_aggr.  */
+    conversion *aggr_worst;
+  };
 };
 
 #define CONVERSION_RANK(NODE)			\
@@ -963,6 +968,7 @@  build_aggr_conv (tree type, tree ctor, int flags, tsubst_flags_t complain)
   c->user_conv_p = true;
   c->check_narrowing = true;
   c->u.next = NULL;
+  c->aggr_worst = NULL;
   return c;
 }
 
@@ -994,6 +1000,8 @@  build_array_conv (tree type, tree ctor, int flags, tsubst_flags_t complain)
 
   flags = LOOKUP_IMPLICIT|LOOKUP_NO_NARROWING;
 
+  conversion *worst = NULL;
+
   FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (ctor), i, val)
     {
       conversion *sub
@@ -1008,6 +1016,9 @@  build_array_conv (tree type, tree ctor, int flags, tsubst_flags_t complain)
 	user = true;
       if (sub->bad_p)
 	bad = true;
+
+      if (!worst || compare_ics (worst, sub) > 0)
+	worst = sub;
     }
 
   c = alloc_conversion (ck_aggr);
@@ -1016,6 +1027,7 @@  build_array_conv (tree type, tree ctor, int flags, tsubst_flags_t complain)
   c->user_conv_p = user;
   c->bad_p = bad;
   c->u.next = NULL;
+  c->aggr_worst = worst;
   return c;
 }
 
@@ -1040,6 +1052,8 @@  build_complex_conv (tree type, tree ctor, int flags,
 
   flags = LOOKUP_IMPLICIT|LOOKUP_NO_NARROWING;
 
+  conversion *worst = NULL;
+
   FOR_EACH_CONSTRUCTOR_VALUE (CONSTRUCTOR_ELTS (ctor), i, val)
     {
       conversion *sub
@@ -1054,6 +1068,9 @@  build_complex_conv (tree type, tree ctor, int flags,
 	user = true;
       if (sub->bad_p)
 	bad = true;
+
+      if (!worst || compare_ics (worst, sub) > 0)
+	worst = sub;
     }
 
   c = alloc_conversion (ck_aggr);
@@ -1062,6 +1079,7 @@  build_complex_conv (tree type, tree ctor, int flags,
   c->user_conv_p = user;
   c->bad_p = bad;
   c->u.next = NULL;
+  c->aggr_worst = worst;
   return c;
 }
 
@@ -10021,6 +10039,11 @@  compare_ics (conversion *ics1, conversion *ics2)
     /* Both conversions are ellipsis conversions.  */
     return 0;
 
+  while (ics1->kind == ck_aggr && ics1->aggr_worst)
+    ics1 = ics1->aggr_worst;
+  while (ics2->kind == ck_aggr && ics2->aggr_worst)
+    ics2 = ics2->aggr_worst;
+
   /* User-defined  conversion sequence U1 is a better conversion sequence
      than another user-defined conversion sequence U2 if they contain the
      same user-defined conversion operator or constructor and if the sec-