* [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
@ 2020-03-10 23:38 Marek Polacek
2020-03-11 2:04 ` Jason Merrill
2020-03-11 6:52 ` Jakub Jelinek
0 siblings, 2 replies; 9+ messages in thread
From: Marek Polacek @ 2020-03-10 23:38 UTC (permalink / raw)
To: Jason Merrill, GCC Patches
This is a bad interaction between sharing a constructor for an array
and stripping its trailing zero-initializers. Here we reuse a ctor
and then strip its 0s. This breaks overload resolution in this test:
D can be initialized from {} but not from {0}, so if we truncate the
constructor not to include the zero, the F(D) overload becomes valid
and then we get the ambiguous conversion error.
Bootstrapped/regtested on x86_64-linux, ok for trunk?
PR c++/94124 - wrong conversion error with non-viable overload.
* decl.c (reshape_init_array_1): Unshare a constructor if we
stripped trailing zero-initializers.
* g++.dg/cpp0x/initlist-overload1.C: New test.
---
gcc/cp/decl.c | 7 +++++++
gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C | 15 +++++++++++++++
2 files changed, 22 insertions(+)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C
diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
index bb242743074..aa58e5f88ae 100644
--- a/gcc/cp/decl.c
+++ b/gcc/cp/decl.c
@@ -6062,6 +6062,13 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d,
else if (last_nonzero < nelts - 1)
nelts = last_nonzero + 1;
+ /* Sharing a stripped constructor can get in the way of
+ overload resolution. E.g., initializing a class from
+ {{0}} might be invalid while initializing the same class
+ from {{}} might be valid. */
+ if (reuse)
+ new_init = unshare_constructor (new_init);
+
vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
}
diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C b/gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C
new file mode 100644
index 00000000000..12bb606ce67
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C
@@ -0,0 +1,15 @@
+// PR c++/94124 - wrong conversion error with non-viable overload.
+// { dg-do compile { target c++11 } }
+
+template <int N> struct A { typedef int _Type[N]; };
+template <int N> struct B { typename A<N>::_Type _M_elems; };
+class C { };
+struct D {
+ D(C);
+};
+
+struct F {
+ F(B<2>);
+ F(D); // This overload should not be viable.
+};
+F fn1() { return {{{0}}}; }
base-commit: 0b7f1e24316cfc1f85408918d1734d3266d65089
--
Marek Polacek • Red Hat, Inc. • 300 A St, Boston, MA
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-10 23:38 [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124] Marek Polacek
@ 2020-03-11 2:04 ` Jason Merrill
2020-03-11 6:52 ` Jakub Jelinek
1 sibling, 0 replies; 9+ messages in thread
From: Jason Merrill @ 2020-03-11 2:04 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
On 3/10/20 7:38 PM, Marek Polacek wrote:
> This is a bad interaction between sharing a constructor for an array
> and stripping its trailing zero-initializers. Here we reuse a ctor
> and then strip its 0s. This breaks overload resolution in this test:
> D can be initialized from {} but not from {0}, so if we truncate the
> constructor not to include the zero, the F(D) overload becomes valid
> and then we get the ambiguous conversion error.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
OK.
> PR c++/94124 - wrong conversion error with non-viable overload.
> * decl.c (reshape_init_array_1): Unshare a constructor if we
> stripped trailing zero-initializers.
>
> * g++.dg/cpp0x/initlist-overload1.C: New test.
> ---
> gcc/cp/decl.c | 7 +++++++
> gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C | 15 +++++++++++++++
> 2 files changed, 22 insertions(+)
> create mode 100644 gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C
>
> diff --git a/gcc/cp/decl.c b/gcc/cp/decl.c
> index bb242743074..aa58e5f88ae 100644
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -6062,6 +6062,13 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d,
> else if (last_nonzero < nelts - 1)
> nelts = last_nonzero + 1;
>
> + /* Sharing a stripped constructor can get in the way of
> + overload resolution. E.g., initializing a class from
> + {{0}} might be invalid while initializing the same class
> + from {{}} might be valid. */
> + if (reuse)
> + new_init = unshare_constructor (new_init);
> +
> vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
> }
>
> diff --git a/gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C b/gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C
> new file mode 100644
> index 00000000000..12bb606ce67
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/initlist-overload1.C
> @@ -0,0 +1,15 @@
> +// PR c++/94124 - wrong conversion error with non-viable overload.
> +// { dg-do compile { target c++11 } }
> +
> +template <int N> struct A { typedef int _Type[N]; };
> +template <int N> struct B { typename A<N>::_Type _M_elems; };
> +class C { };
> +struct D {
> + D(C);
> +};
> +
> +struct F {
> + F(B<2>);
> + F(D); // This overload should not be viable.
> +};
> +F fn1() { return {{{0}}}; }
>
> base-commit: 0b7f1e24316cfc1f85408918d1734d3266d65089
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-10 23:38 [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124] Marek Polacek
2020-03-11 2:04 ` Jason Merrill
@ 2020-03-11 6:52 ` Jakub Jelinek
2020-03-11 15:44 ` Marek Polacek
1 sibling, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2020-03-11 6:52 UTC (permalink / raw)
To: Marek Polacek; +Cc: Jason Merrill, GCC Patches
On Tue, Mar 10, 2020 at 07:38:17PM -0400, Marek Polacek via Gcc-patches wrote:
> --- a/gcc/cp/decl.c
> +++ b/gcc/cp/decl.c
> @@ -6062,6 +6062,13 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d,
> else if (last_nonzero < nelts - 1)
> nelts = last_nonzero + 1;
>
> + /* Sharing a stripped constructor can get in the way of
> + overload resolution. E.g., initializing a class from
> + {{0}} might be invalid while initializing the same class
> + from {{}} might be valid. */
> + if (reuse)
> + new_init = unshare_constructor (new_init);
> +
> vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
Isn't it wasteful to first copy perhaps a large constructor (recursively)
and then truncate it to very few elts (zero in this case)?
So, perhaps doing instead:
if (reuse)
{
vec<constructor_elt, va_gc> *v = NULL;
if (nelts)
vec_alloc (v, nelts);
for (unsigned int i = 0; i < nelts; i++)
{
constructor_elt elt = CONSTRUCTOR_ELT (new_init, i);
if (TREE_CODE (elt.value) == CONSTRUCTOR)
elt.value = unshare_constructor (elt.value);
v->quick_push (elt);
}
new_init = build_constructor (TREE_TYPE (new_init), v);
}
else
vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
?
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-11 6:52 ` Jakub Jelinek
@ 2020-03-11 15:44 ` Marek Polacek
2020-03-11 20:02 ` Jason Merrill
0 siblings, 1 reply; 9+ messages in thread
From: Marek Polacek @ 2020-03-11 15:44 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: GCC Patches, Jason Merrill
On Wed, Mar 11, 2020 at 07:52:15AM +0100, Jakub Jelinek via Gcc-patches wrote:
> On Tue, Mar 10, 2020 at 07:38:17PM -0400, Marek Polacek via Gcc-patches wrote:
> > --- a/gcc/cp/decl.c
> > +++ b/gcc/cp/decl.c
> > @@ -6062,6 +6062,13 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d,
> > else if (last_nonzero < nelts - 1)
> > nelts = last_nonzero + 1;
> >
> > + /* Sharing a stripped constructor can get in the way of
> > + overload resolution. E.g., initializing a class from
> > + {{0}} might be invalid while initializing the same class
> > + from {{}} might be valid. */
> > + if (reuse)
> > + new_init = unshare_constructor (new_init);
> > +
> > vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
>
> Isn't it wasteful to first copy perhaps a large constructor (recursively)
> and then truncate it to very few elts (zero in this case)?
> So, perhaps doing instead:
> if (reuse)
> {
> vec<constructor_elt, va_gc> *v = NULL;
> if (nelts)
> vec_alloc (v, nelts);
> for (unsigned int i = 0; i < nelts; i++)
> {
> constructor_elt elt = CONSTRUCTOR_ELT (new_init, i);
> if (TREE_CODE (elt.value) == CONSTRUCTOR)
> elt.value = unshare_constructor (elt.value);
> v->quick_push (elt);
> }
> new_init = build_constructor (TREE_TYPE (new_init), v);
> }
> else
> vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
> ?
Large constructors aren't expected here. From bootstrap + testsuite
measurements I can see that we have 8035 actual truncations, and 7884
of those are of constructors shorter than 10 elements. There's one case
where we have a ctor of 567 elements and truncate it to 559 elements but
that seems pathological. Is it worth it (genuine quiestion)?
Note there are 37077 attempts to truncate, of which 29042 truncate to
the same length. Our ->truncate merely sets m_num, so it's cheap, but
I'm still a bit surprised we don't check
if (CONSTRUCTOR_NELTS (new_init) != nelts)
before calling vec_safe_truncate.
Marek
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-11 15:44 ` Marek Polacek
@ 2020-03-11 20:02 ` Jason Merrill
2020-03-11 22:28 ` Jakub Jelinek
0 siblings, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2020-03-11 20:02 UTC (permalink / raw)
To: Marek Polacek, Jakub Jelinek; +Cc: GCC Patches
On 3/11/20 11:44 AM, Marek Polacek wrote:
> On Wed, Mar 11, 2020 at 07:52:15AM +0100, Jakub Jelinek via Gcc-patches wrote:
>> On Tue, Mar 10, 2020 at 07:38:17PM -0400, Marek Polacek via Gcc-patches wrote:
>>> --- a/gcc/cp/decl.c
>>> +++ b/gcc/cp/decl.c
>>> @@ -6062,6 +6062,13 @@ reshape_init_array_1 (tree elt_type, tree max_index, reshape_iter *d,
>>> else if (last_nonzero < nelts - 1)
>>> nelts = last_nonzero + 1;
>>>
>>> + /* Sharing a stripped constructor can get in the way of
>>> + overload resolution. E.g., initializing a class from
>>> + {{0}} might be invalid while initializing the same class
>>> + from {{}} might be valid. */
>>> + if (reuse)
>>> + new_init = unshare_constructor (new_init);
>>> +
>>> vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
>>
>> Isn't it wasteful to first copy perhaps a large constructor (recursively)
>> and then truncate it to very few elts (zero in this case)?
>> So, perhaps doing instead:
>> if (reuse)
>> {
>> vec<constructor_elt, va_gc> *v = NULL;
>> if (nelts)
>> vec_alloc (v, nelts);
>> for (unsigned int i = 0; i < nelts; i++)
>> {
>> constructor_elt elt = CONSTRUCTOR_ELT (new_init, i);
>> if (TREE_CODE (elt.value) == CONSTRUCTOR)
>> elt.value = unshare_constructor (elt.value);
>> v->quick_push (elt);
>> }
>> new_init = build_constructor (TREE_TYPE (new_init), v);
>> }
>> else
>> vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
>> ?
>
> Large constructors aren't expected here. From bootstrap + testsuite
> measurements I can see that we have 8035 actual truncations, and 7884
> of those are of constructors shorter than 10 elements. There's one case
> where we have a ctor of 567 elements and truncate it to 559 elements but
> that seems pathological. Is it worth it (genuine quiestion)?
>
> Note there are 37077 attempts to truncate, of which 29042 truncate to
> the same length. Our ->truncate merely sets m_num, so it's cheap, but
> I'm still a bit surprised we don't check
>
> if (CONSTRUCTOR_NELTS (new_init) != nelts)
>
> before calling vec_safe_truncate.
We should certainly avoid copying if they're the same. The code above
for only copying the bits that aren't going to be thrown away seems
pretty straightforward, might as well use it even if the savings aren't
likely to be large.
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-11 20:02 ` Jason Merrill
@ 2020-03-11 22:28 ` Jakub Jelinek
2020-03-11 23:16 ` Marek Polacek
2020-03-12 5:58 ` Jason Merrill
0 siblings, 2 replies; 9+ messages in thread
From: Jakub Jelinek @ 2020-03-11 22:28 UTC (permalink / raw)
To: Jason Merrill; +Cc: Marek Polacek, GCC Patches
On Wed, Mar 11, 2020 at 04:02:51PM -0400, Jason Merrill via Gcc-patches wrote:
> We should certainly avoid copying if they're the same. The code above for
> only copying the bits that aren't going to be thrown away seems pretty
> straightforward, might as well use it even if the savings aren't likely to
> be large.
So like this if it passes bootstrap/regtest?
Calling vec_safe_truncate with the same number of elts the vector already
has is a nop, so IMHO we just should make sure we only unshare if it
changed.
2020-03-11 Jakub Jelinek <jakub@redhat.com>
PR c++/94124
* decl.c (reshape_init_array_1): Don't unshare constructor if there
aren't any trailing zero elts, otherwise only unshare the first
nelts.
--- gcc/cp/decl.c.jj 2020-03-11 09:28:53.966213943 +0100
+++ gcc/cp/decl.c 2020-03-11 23:19:08.832780798 +0100
@@ -6066,10 +6066,22 @@ reshape_init_array_1 (tree elt_type, tre
overload resolution. E.g., initializing a class from
{{0}} might be invalid while initializing the same class
from {{}} might be valid. */
- if (reuse)
- new_init = unshare_constructor (new_init);
-
- vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
+ if (reuse && nelts < CONSTRUCTOR_NELTS (new_init))
+ {
+ vec<constructor_elt, va_gc> *v = NULL;
+ if (nelts)
+ vec_alloc (v, nelts);
+ for (unsigned int i = 0; i < nelts; i++)
+ {
+ constructor_elt elt = *CONSTRUCTOR_ELT (new_init, i);
+ if (TREE_CODE (elt.value) == CONSTRUCTOR)
+ elt.value = unshare_constructor (elt.value);
+ v->quick_push (elt);
+ }
+ new_init = build_constructor (TREE_TYPE (new_init), v);
+ }
+ else
+ vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
}
return new_init;
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-11 22:28 ` Jakub Jelinek
@ 2020-03-11 23:16 ` Marek Polacek
2020-03-12 5:58 ` Jason Merrill
1 sibling, 0 replies; 9+ messages in thread
From: Marek Polacek @ 2020-03-11 23:16 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Jason Merrill, GCC Patches
On Wed, Mar 11, 2020 at 11:28:01PM +0100, Jakub Jelinek via Gcc-patches wrote:
> On Wed, Mar 11, 2020 at 04:02:51PM -0400, Jason Merrill via Gcc-patches wrote:
> > We should certainly avoid copying if they're the same. The code above for
> > only copying the bits that aren't going to be thrown away seems pretty
> > straightforward, might as well use it even if the savings aren't likely to
> > be large.
>
> So like this if it passes bootstrap/regtest?
> Calling vec_safe_truncate with the same number of elts the vector already
> has is a nop, so IMHO we just should make sure we only unshare if it
> changed.
>
> 2020-03-11 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/94124
> * decl.c (reshape_init_array_1): Don't unshare constructor if there
> aren't any trailing zero elts, otherwise only unshare the first
> nelts.
>
> --- gcc/cp/decl.c.jj 2020-03-11 09:28:53.966213943 +0100
> +++ gcc/cp/decl.c 2020-03-11 23:19:08.832780798 +0100
> @@ -6066,10 +6066,22 @@ reshape_init_array_1 (tree elt_type, tre
> overload resolution. E.g., initializing a class from
> {{0}} might be invalid while initializing the same class
> from {{}} might be valid. */
> - if (reuse)
> - new_init = unshare_constructor (new_init);
> -
> - vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
> + if (reuse && nelts < CONSTRUCTOR_NELTS (new_init))
> + {
> + vec<constructor_elt, va_gc> *v = NULL;
> + if (nelts)
> + vec_alloc (v, nelts);
> + for (unsigned int i = 0; i < nelts; i++)
> + {
> + constructor_elt elt = *CONSTRUCTOR_ELT (new_init, i);
> + if (TREE_CODE (elt.value) == CONSTRUCTOR)
> + elt.value = unshare_constructor (elt.value);
> + v->quick_push (elt);
> + }
> + new_init = build_constructor (TREE_TYPE (new_init), v);
> + }
> + else
> + vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
> }
>
> return new_init;
LGTM.
Marek
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-11 22:28 ` Jakub Jelinek
2020-03-11 23:16 ` Marek Polacek
@ 2020-03-12 5:58 ` Jason Merrill
2020-03-12 7:32 ` Jakub Jelinek
1 sibling, 1 reply; 9+ messages in thread
From: Jason Merrill @ 2020-03-12 5:58 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Marek Polacek, GCC Patches
On 3/11/20 6:28 PM, Jakub Jelinek wrote:
> On Wed, Mar 11, 2020 at 04:02:51PM -0400, Jason Merrill via Gcc-patches wrote:
>> We should certainly avoid copying if they're the same. The code above for
>> only copying the bits that aren't going to be thrown away seems pretty
>> straightforward, might as well use it even if the savings aren't likely to
>> be large.
>
> So like this if it passes bootstrap/regtest?
> Calling vec_safe_truncate with the same number of elts the vector already
> has is a nop, so IMHO we just should make sure we only unshare if it
> changed.
>
> 2020-03-11 Jakub Jelinek <jakub@redhat.com>
>
> PR c++/94124
> * decl.c (reshape_init_array_1): Don't unshare constructor if there
> aren't any trailing zero elts, otherwise only unshare the first
> nelts.
>
> --- gcc/cp/decl.c.jj 2020-03-11 09:28:53.966213943 +0100
> +++ gcc/cp/decl.c 2020-03-11 23:19:08.832780798 +0100
> @@ -6066,10 +6066,22 @@ reshape_init_array_1 (tree elt_type, tre
> overload resolution. E.g., initializing a class from
> {{0}} might be invalid while initializing the same class
> from {{}} might be valid. */
> - if (reuse)
> - new_init = unshare_constructor (new_init);
> -
> - vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
> + if (reuse && nelts < CONSTRUCTOR_NELTS (new_init))
> + {
> + vec<constructor_elt, va_gc> *v = NULL;
> + if (nelts)
vec_alloc does nothing if nelts is 0, so this test seems unnecessary.
OK either way.
> + vec_alloc (v, nelts);
> + for (unsigned int i = 0; i < nelts; i++)
> + {
> + constructor_elt elt = *CONSTRUCTOR_ELT (new_init, i);
> + if (TREE_CODE (elt.value) == CONSTRUCTOR)
> + elt.value = unshare_constructor (elt.value);
> + v->quick_push (elt);
> + }
> + new_init = build_constructor (TREE_TYPE (new_init), v);
> + }
> + else
> + vec_safe_truncate (CONSTRUCTOR_ELTS (new_init), nelts);
> }
>
> return new_init;
>
>
> Jakub
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124]
2020-03-12 5:58 ` Jason Merrill
@ 2020-03-12 7:32 ` Jakub Jelinek
0 siblings, 0 replies; 9+ messages in thread
From: Jakub Jelinek @ 2020-03-12 7:32 UTC (permalink / raw)
To: Jason Merrill; +Cc: Marek Polacek, GCC Patches
On Thu, Mar 12, 2020 at 01:58:20AM -0400, Jason Merrill wrote:
> > + if (reuse && nelts < CONSTRUCTOR_NELTS (new_init))
> > + {
> > + vec<constructor_elt, va_gc> *v = NULL;
> > + if (nelts)
>
> vec_alloc does nothing if nelts is 0, so this test seems unnecessary. OK
> either way.
I wasn't sure, but now I've verified it doesn't do anything but v = NULL,
which is exactly what we want. Dropped the " = NULL" and if (nelts)
and committed, thanks.
>
> > + vec_alloc (v, nelts);
Jakub
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-03-12 7:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-10 23:38 [PATCH] c++: Fix wrong conversion error with non-viable overload [PR94124] Marek Polacek
2020-03-11 2:04 ` Jason Merrill
2020-03-11 6:52 ` Jakub Jelinek
2020-03-11 15:44 ` Marek Polacek
2020-03-11 20:02 ` Jason Merrill
2020-03-11 22:28 ` Jakub Jelinek
2020-03-11 23:16 ` Marek Polacek
2020-03-12 5:58 ` Jason Merrill
2020-03-12 7:32 ` Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).