| Message ID | 20240612175637.2714077-1-ppalka@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: ICE w/ ambig and non-strictly-viable cands [PR115239] | expand |
On Wed, 12 Jun 2024, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk/14? > > -- >8 -- > > Here during overload resolution we have two strictly viable ambiguous > candidates #1 and #2, and two non-strictly viable candidates #3 and #4 > which we hold on to ever since r14-6522. These latter candidates have > an empty third arg conversion since the second arg conversion was deemed > bad. This ends up causing an ICE during joust for #3 and #4 due to this > empty arg conversion. > > We can fix this by making joust robust to empty arg conversions, but in > this situation we shouldn't need to compare #3 and #4 at all given that > we have a strictly viable candidate. To that end, this patch makes > tourney shortcut considering non-strictly viable candidates upon > encountering ambiguity between two strictly viable candidates, taking > advantage of the fact that the candidates list is sorted according to > viability via splice_viable. > > PR c++/115239 > > gcc/cp/ChangeLog: > > * call.cc (tourney): Don't consider a non-strictly viable > candidate as the champ if there was ambiguity between two > strictly viable candidates. > > gcc/testsuite/ChangeLog: > > * g++.dg/overload/error7.C: New test. > --- > gcc/cp/call.cc | 4 +++- > gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/overload/error7.C > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > index ed68eb3c568..82c70f5c39f 100644 > --- a/gcc/cp/call.cc > +++ b/gcc/cp/call.cc > @@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain) > } > else > { > + z_candidate *prev_champ = *champ; > previous_worse_champ = nullptr; > champ = &(*challenger)->next; > - if (!*champ || !(*champ)->viable) > + if (!*champ || !(*champ)->viable > + || (prev_champ->viable == 1 && (*champ)->viable == -1)) > { > champ = nullptr; > break; > diff --git a/gcc/testsuite/g++.dg/overload/error7.C b/gcc/testsuite/g++.dg/overload/error7.C > new file mode 100644 > index 00000000000..68aaa236de4 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/overload/error7.C > @@ -0,0 +1,10 @@ > +// PR c++/115239 > + > +bool foo(const char *, char *, long); // #1, strictly viable, ambig with #2 > +bool foo(const char *, char *, unsigned); // #2, strictly viable, ambig with #1 > +bool foo(char, char, long); // #3, non-strictly viable > +bool foo(char, char, unsigned); // #4, non-strictly viable > + > +int main() { > + foo((char *)0, (char *)0, 0); // { dg-error "ambiguous" } > +} FWIW I just realized this testcase can be simplified to: // PR c++/115239 bool foo(char *, long); // #1, strictly viable, ambig with #2 bool foo(char *, unsigned); // #2, strictly viable, ambig with #1 bool foo(char, long); // #3, non-strictly viable bool foo(char, unsigned); // #4, non-strictly viable int main() { foo((char *)0, 0); // { dg-error "ambiguous" } }
On 6/12/24 13:56, Patrick Palka wrote: > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > trunk/14? > > -- >8 -- > > Here during overload resolution we have two strictly viable ambiguous > candidates #1 and #2, and two non-strictly viable candidates #3 and #4 > which we hold on to ever since r14-6522. These latter candidates have > an empty third arg conversion since the second arg conversion was deemed > bad. This ends up causing an ICE during joust for #3 and #4 due to this > empty arg conversion. > > We can fix this by making joust robust to empty arg conversions, but in > this situation we shouldn't need to compare #3 and #4 at all given that > we have a strictly viable candidate. To that end, this patch makes > tourney shortcut considering non-strictly viable candidates upon > encountering ambiguity between two strictly viable candidates, taking > advantage of the fact that the candidates list is sorted according to > viability via splice_viable. > > PR c++/115239 > > gcc/cp/ChangeLog: > > * call.cc (tourney): Don't consider a non-strictly viable > candidate as the champ if there was ambiguity between two > strictly viable candidates. > > gcc/testsuite/ChangeLog: > > * g++.dg/overload/error7.C: New test. > --- > gcc/cp/call.cc | 4 +++- > gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/g++.dg/overload/error7.C > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > index ed68eb3c568..82c70f5c39f 100644 > --- a/gcc/cp/call.cc > +++ b/gcc/cp/call.cc > @@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain) > } > else > { > + z_candidate *prev_champ = *champ; > previous_worse_champ = nullptr; > champ = &(*challenger)->next; > - if (!*champ || !(*champ)->viable) > + if (!*champ || !(*champ)->viable > + || (prev_champ->viable == 1 && (*champ)->viable == -1)) Maybe (!*champ || (*champ)->viable < prev_champ->viable) ? OK with that change. Jason
On Wed, 12 Jun 2024, Jason Merrill wrote: > On 6/12/24 13:56, Patrick Palka wrote: > > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for > > trunk/14? > > > > -- >8 -- > > > > Here during overload resolution we have two strictly viable ambiguous > > candidates #1 and #2, and two non-strictly viable candidates #3 and #4 > > which we hold on to ever since r14-6522. These latter candidates have > > an empty third arg conversion since the second arg conversion was deemed > > bad. This ends up causing an ICE during joust for #3 and #4 due to this > > empty arg conversion. > > > > We can fix this by making joust robust to empty arg conversions, but in > > this situation we shouldn't need to compare #3 and #4 at all given that > > we have a strictly viable candidate. To that end, this patch makes > > tourney shortcut considering non-strictly viable candidates upon > > encountering ambiguity between two strictly viable candidates, taking > > advantage of the fact that the candidates list is sorted according to > > viability via splice_viable. > > > > PR c++/115239 > > > > gcc/cp/ChangeLog: > > > > * call.cc (tourney): Don't consider a non-strictly viable > > candidate as the champ if there was ambiguity between two > > strictly viable candidates. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/overload/error7.C: New test. > > --- > > gcc/cp/call.cc | 4 +++- > > gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++ > > 2 files changed, 13 insertions(+), 1 deletion(-) > > create mode 100644 gcc/testsuite/g++.dg/overload/error7.C > > > > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc > > index ed68eb3c568..82c70f5c39f 100644 > > --- a/gcc/cp/call.cc > > +++ b/gcc/cp/call.cc > > @@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates, > > tsubst_flags_t complain) > > } > > else > > { > > + z_candidate *prev_champ = *champ; > > previous_worse_champ = nullptr; > > champ = &(*challenger)->next; > > - if (!*champ || !(*champ)->viable) > > + if (!*champ || !(*champ)->viable > > + || (prev_champ->viable == 1 && (*champ)->viable == -1)) > > Maybe > > (!*champ || (*champ)->viable < prev_champ->viable) ? > > OK with that change. Nice, done. In passing it occurred to me that we can just consider challenger->viable instead of prev_champ->viable (they must be the same since there was ambiguity), so we don't need to introduce prev_champ. This is what I ended up pushing: -- >8 -- Subject: [PATCH] c++: ICE w/ ambig and non-strictly-viable cands [PR115239] PR c++/115239 gcc/cp/ChangeLog: * call.cc (tourney): Don't consider a non-strictly viable candidate as the champ if there was ambiguity between two strictly viable candidates. gcc/testsuite/ChangeLog: * g++.dg/overload/error7.C: New test. Reviewed-by: Jason Merrill <jason@redhat.com> --- gcc/cp/call.cc | 3 ++- gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/g++.dg/overload/error7.C diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc index 85536fc25ff..7bbc1fb0c78 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -13490,7 +13490,8 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain) { previous_worse_champ = nullptr; champ = &(*challenger)->next; - if (!*champ || !(*champ)->viable) + if (!*champ || !(*champ)->viable + || (*champ)->viable < (*challenger)->viable) { champ = nullptr; break; diff --git a/gcc/testsuite/g++.dg/overload/error7.C b/gcc/testsuite/g++.dg/overload/error7.C new file mode 100644 index 00000000000..de50ce5f66e --- /dev/null +++ b/gcc/testsuite/g++.dg/overload/error7.C @@ -0,0 +1,10 @@ +// PR c++/115239 + +bool foo(char *, long); // #1, strictly viable, ambig with #2 +bool foo(char *, unsigned); // #2, strictly viable, ambig with #1 +bool foo(char, long); // #3, non-strictly viable +bool foo(char, unsigned); // #4, non-strictly viable + +int main() { + foo((char *)0, 0); // { dg-error "ambiguous" } +}
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc index ed68eb3c568..82c70f5c39f 100644 --- a/gcc/cp/call.cc +++ b/gcc/cp/call.cc @@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain) } else { + z_candidate *prev_champ = *champ; previous_worse_champ = nullptr; champ = &(*challenger)->next; - if (!*champ || !(*champ)->viable) + if (!*champ || !(*champ)->viable + || (prev_champ->viable == 1 && (*champ)->viable == -1)) { champ = nullptr; break; diff --git a/gcc/testsuite/g++.dg/overload/error7.C b/gcc/testsuite/g++.dg/overload/error7.C new file mode 100644 index 00000000000..68aaa236de4 --- /dev/null +++ b/gcc/testsuite/g++.dg/overload/error7.C @@ -0,0 +1,10 @@ +// PR c++/115239 + +bool foo(const char *, char *, long); // #1, strictly viable, ambig with #2 +bool foo(const char *, char *, unsigned); // #2, strictly viable, ambig with #1 +bool foo(char, char, long); // #3, non-strictly viable +bool foo(char, char, unsigned); // #4, non-strictly viable + +int main() { + foo((char *)0, (char *)0, 0); // { dg-error "ambiguous" } +}