diff mbox

[C++] Fix GC related issues in C++ FE (PR c++/58627)

Message ID 20131212133611.GN892@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Dec. 12, 2013, 1:36 p.m. UTC
On Wed, Dec 11, 2013 at 11:51:55AM -0500, Jason Merrill wrote:
> It's only safe to free the targs if they weren't used to instantiate
> any templates, so I lean toward option #1.  Did you test this with
> strict gc?

Ok, after IRC discussion and another bootstrap/regtest I've installed
this variant instead:

2013-12-12  Jakub Jelinek  <jakub@redhat.com>

	PR c++/58627
	* call.c (add_template_candidate_real): Don't call ggc_free on targs.



	Jakub

Comments

H.J. Lu Dec. 13, 2013, 2:19 p.m. UTC | #1
On Thu, Dec 12, 2013 at 5:36 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 11, 2013 at 11:51:55AM -0500, Jason Merrill wrote:
>> It's only safe to free the targs if they weren't used to instantiate
>> any templates, so I lean toward option #1.  Did you test this with
>> strict gc?
>
> Ok, after IRC discussion and another bootstrap/regtest I've installed
> this variant instead:
>
> 2013-12-12  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/58627
>         * call.c (add_template_candidate_real): Don't call ggc_free on targs.
>
> --- gcc/cp/class.c.jj   2013-11-28 08:18:58.000000000 +0100
> +++ gcc/cp/class.c      2013-12-11 20:57:40.155059669 +0100
> @@ -7475,8 +7475,6 @@ resolve_address_of_overloaded_function (
>           /* See if there's a match.  */
>           if (same_type_p (target_fn_type, static_fn_type (instantiation)))
>             matches = tree_cons (instantiation, fn, matches);
> -
> -         ggc_free (targs);
>         }
>
>        /* Now, remove all but the most specialized of the matches.  */
>

Has this been checked in?  I still see random:

FAIL: 17_intro/headers/c++200x/stdc++.cc (test for excess errors)
FAIL: 17_intro/headers/c++200x/stdc++_multiple_inclusion.cc (test for
excess errors)

http://gcc.gnu.org/ml/gcc-testresults/2013-12/msg01230.html
H.J. Lu Dec. 16, 2013, 5:51 p.m. UTC | #2
On Thu, Dec 12, 2013 at 5:36 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Dec 11, 2013 at 11:51:55AM -0500, Jason Merrill wrote:
>> It's only safe to free the targs if they weren't used to instantiate
>> any templates, so I lean toward option #1.  Did you test this with
>> strict gc?
>
> Ok, after IRC discussion and another bootstrap/regtest I've installed
> this variant instead:
>
> 2013-12-12  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/58627
>         * call.c (add_template_candidate_real): Don't call ggc_free on targs.
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Hi Jakub,

This checkin doesn't match its ChangeLog entry and it doesn't fix

FAIL: 17_intro/headers/c++200x/stdc++.cc (test for excess errors)
FAIL: 17_intro/headers/c++200x/stdc++_multiple_inclusion.cc (test for
excess errors)

Is this an oversight?

>
> --- gcc/cp/class.c.jj   2013-11-28 08:18:58.000000000 +0100
> +++ gcc/cp/class.c      2013-12-11 20:57:40.155059669 +0100
> @@ -7475,8 +7475,6 @@ resolve_address_of_overloaded_function (
>           /* See if there's a match.  */
>           if (same_type_p (target_fn_type, static_fn_type (instantiation)))
>             matches = tree_cons (instantiation, fn, matches);
> -
> -         ggc_free (targs);
>         }
>
>        /* Now, remove all but the most specialized of the matches.  */
>
>
>         Jakub
Jakub Jelinek Dec. 16, 2013, 5:56 p.m. UTC | #3
On Mon, Dec 16, 2013 at 09:51:38AM -0800, H.J. Lu wrote:
> On Thu, Dec 12, 2013 at 5:36 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Wed, Dec 11, 2013 at 11:51:55AM -0500, Jason Merrill wrote:
> >> It's only safe to free the targs if they weren't used to instantiate
> >> any templates, so I lean toward option #1.  Did you test this with
> >> strict gc?
> >
> > Ok, after IRC discussion and another bootstrap/regtest I've installed
> > this variant instead:
> >
> > 2013-12-12  Jakub Jelinek  <jakub@redhat.com>
> >
> >         PR c++/58627
> >         * call.c (add_template_candidate_real): Don't call ggc_free on targs.
>              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This checkin doesn't match its ChangeLog entry and it doesn't fix

I have fixed up the function name in the ChangeLog entry.

As for the stdc++.cc failure, I've commented in the PR, it is PCH related,
the relation to PR58627 was just a guess from me and proved to be wrong
guess.

> FAIL: 17_intro/headers/c++200x/stdc++.cc (test for excess errors)
> FAIL: 17_intro/headers/c++200x/stdc++_multiple_inclusion.cc (test for
> excess errors)

	Jakub
H.J. Lu Dec. 16, 2013, 5:58 p.m. UTC | #4
On Mon, Dec 16, 2013 at 9:56 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Dec 16, 2013 at 09:51:38AM -0800, H.J. Lu wrote:
>> On Thu, Dec 12, 2013 at 5:36 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> > On Wed, Dec 11, 2013 at 11:51:55AM -0500, Jason Merrill wrote:
>> >> It's only safe to free the targs if they weren't used to instantiate
>> >> any templates, so I lean toward option #1.  Did you test this with
>> >> strict gc?
>> >
>> > Ok, after IRC discussion and another bootstrap/regtest I've installed
>> > this variant instead:
>> >
>> > 2013-12-12  Jakub Jelinek  <jakub@redhat.com>
>> >
>> >         PR c++/58627
>> >         * call.c (add_template_candidate_real): Don't call ggc_free on targs.
>>              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> This checkin doesn't match its ChangeLog entry and it doesn't fix
>
> I have fixed up the function name in the ChangeLog entry.
>
> As for the stdc++.cc failure, I've commented in the PR, it is PCH related,

The filename is class.c, not call.c.
diff mbox

Patch

--- gcc/cp/class.c.jj	2013-11-28 08:18:58.000000000 +0100
+++ gcc/cp/class.c	2013-12-11 20:57:40.155059669 +0100
@@ -7475,8 +7475,6 @@  resolve_address_of_overloaded_function (
 	  /* See if there's a match.  */
 	  if (same_type_p (target_fn_type, static_fn_type (instantiation)))
 	    matches = tree_cons (instantiation, fn, matches);
-
-	  ggc_free (targs);
 	}
 
       /* Now, remove all but the most specialized of the matches.  */