Patchwork Fix PR 56077

login
register
mail settings
Submitter Andrey Belevantsev
Date April 5, 2013, 6:54 a.m.
Message ID <515E7538.4070408@ispras.ru>
Download mbox | patch
Permalink /patch/234055/
State New
Headers show

Comments

Andrey Belevantsev - April 5, 2013, 6:54 a.m.
On 01.04.2013 12:38, Andrey Belevantsev wrote:
> On 22.02.2013 17:30, Andrey Belevantsev wrote:
>> Hello,
>>
>> As found by Jakub and explained in the PR audit trail by Alexander, this
>> patch fixes the selective scheduler merge glitch of 2008 that added the
>> unnecessary JUMP_P check to the flush_pending_lists call.  I have removed
>> the check and expanded the binary negation for clarity.
>>
>> The patch was tested on x86-64, ia64, and ppc64 to be safe.  The patch
>> should be conservatively safe at this stage as it adds more flushes and
>> thus more dependencies to the scheduler.  The original test is fixed, but I
>> don't know how to add the test checking assembly insns order to the
>> testsuite.
>>
>> OK for trunk?
>>
>> Andrey
>>
>> 2012-02-22  Alexander Monakov  <amonakov@ispras.ru>
>>          Andrey Belevantsev  <abel@ispras.ru>
>>
>>      PR middle-end/56077
>>      * sched-deps.c (sched_analyze_insn): When reg_pending_barrier,
>>      flush pending lists also on non-jumps.
>
> Now backported to 4.7 and 4.6.

Leonid has reported that this patch breaks the kernel build on 
mipsel-linux.  I have quickly analyzed the issue.  The patch removes the 
JUMP_P check when flushing pending lists at the reg_pending_barrier, 
restoring the 2008 year behavior.  But at that time we didn't have debug 
insns.  Now with the removed check we flush pending lists also on debug 
insns, which results in freeing pending_read_mems but not in freeing 
pending_read_insns, as in add_dependence_list_and_free we explicitly forbid 
freeing the dependency list for debug insns (that was for fixing PR 44013). 
  This is inconsistent, and when proceeding with analysis we immediately hit

2451             t = canon_rtx (t);
2452             pending = deps->pending_read_insns;
2453             pending_mem = deps->pending_read_mems;
2454             while (pending)
2455               {
2456                 if (read_dependence (XEXP (pending_mem, 0), t)

and the line 2456 segfaults because pending is not NULL, but pending mem is 
NULL.

I am testing the revert of this backport for 4.6 and will commit it in 
about an hour or so.  However, I am surprised we don't hit this either on 
4.7, 4.8 or trunk.  Some flush_pending_lists calls are protected from debug 
insns as they check CALL_P or JUMP_P, but not all of them.  It looks like 
flush_pending_lists should not be called on debug insns at all.  And 
indeed, the attached patch fixes Leonid's test case.

Jakub, you don't happen to remember any changes in this area that could 
hide the problem for 4.7 and later?

Andrey
Jakub Jelinek - April 5, 2013, 7:16 a.m.
On Fri, Apr 05, 2013 at 10:54:48AM +0400, Andrey Belevantsev wrote:
> I am testing the revert of this backport for 4.6 and will commit it
> in about an hour or so.  However, I am surprised we don't hit this

Ok, thanks.

> either on 4.7, 4.8 or trunk.  Some flush_pending_lists calls are
> protected from debug insns as they check CALL_P or JUMP_P, but not
> all of them.  It looks like flush_pending_lists should not be called
> on debug insns at all.  And indeed, the attached patch fixes
> Leonid's test case.
> 
> Jakub, you don't happen to remember any changes in this area that
> could hide the problem for 4.7 and later?

No, but Alex or Vlad could know better.  In any case, perhaps it could be
bisected (I only have x86_64 compilers around for bisecting seed though,
I'm afraid the testcase is mips only and hasn't been even posted).

> *** gcc/sched-deps.c	(revision 197492)
> --- gcc/sched-deps.c	(working copy)
> *************** sched_analyze_insn (struct deps_desc *de
> *** 3044,3050 ****
>   
>         /* Don't flush pending lists on speculative checks for
>   	 selective scheduling.  */
> !       if (!sel_sched_p () || !sel_insn_is_speculation_check (insn))
>   	flush_pending_lists (deps, insn, true, true);
>   
>         if (!deps->readonly)
> --- 3044,3050 ----
>   
>         /* Don't flush pending lists on speculative checks for
>   	 selective scheduling.  */
> !       if (NONDEBUG_INSN_P (insn) && (!sel_sched_p () || !sel_insn_is_speculation_check (insn)))

Too long line.  Start && below NONDEBUG.

>   	flush_pending_lists (deps, insn, true, true);
>   
>         if (!deps->readonly)


	Jakub
Eric Botcazou - April 5, 2013, 8:13 a.m.
> Jakub, you don't happen to remember any changes in this area that could
> hide the problem for 4.7 and later?

We do have regressions on the 4.7 branch in the scheduler (CCed Olivier who 
has more information).
Andrey Belevantsev - April 5, 2013, 8:17 a.m.
On 05.04.2013 11:16, Jakub Jelinek wrote:
> On Fri, Apr 05, 2013 at 10:54:48AM +0400, Andrey Belevantsev wrote:
>> I am testing the revert of this backport for 4.6 and will commit it
>> in about an hour or so.  However, I am surprised we don't hit this
>
> Ok, thanks.

Done, r197509.

>> either on 4.7, 4.8 or trunk.  Some flush_pending_lists calls are
>> protected from debug insns as they check CALL_P or JUMP_P, but not
>> all of them.  It looks like flush_pending_lists should not be called
>> on debug insns at all.  And indeed, the attached patch fixes
>> Leonid's test case.
>>
>> Jakub, you don't happen to remember any changes in this area that
>> could hide the problem for 4.7 and later?
>
> No, but Alex or Vlad could know better.  In any case, perhaps it could be
> bisected (I only have x86_64 compilers around for bisecting seed though,
> I'm afraid the testcase is mips only and hasn't been even posted).

I'm attaching it here from Leonid's mail for reference (I didn't reduce 
it).  I configured the branch with (somewhat shortened) Leonid's data as

--target=mipsel-linux-uclibc --with-gnu-ld --enable-target-optspace 
--disable-nls --disable-__cxa_atexit --enable-languages=c --without-headers 
--disable-libssp --disable-shared --disable-threads,

and then the file was built as

./gcc/cc1 -fno-strict-aliasing -fno-common -fno-delete-null-pointer-checks 
-mno-check-zero-division -mabi=32 -G 0 -mno-abicalls -fno-pic -msoft-float 
-ggdb -ffreestanding -march=mips32r2 -mno-branch-likely -O2 traps.i

>
>> *** gcc/sched-deps.c	(revision 197492)
>> --- gcc/sched-deps.c	(working copy)
>> *************** sched_analyze_insn (struct deps_desc *de
>> *** 3044,3050 ****
>>
>>          /* Don't flush pending lists on speculative checks for
>>    	 selective scheduling.  */
>> !       if (!sel_sched_p () || !sel_insn_is_speculation_check (insn))
>>    	flush_pending_lists (deps, insn, true, true);
>>
>>          if (!deps->readonly)
>> --- 3044,3050 ----
>>
>>          /* Don't flush pending lists on speculative checks for
>>    	 selective scheduling.  */
>> !       if (NONDEBUG_INSN_P (insn) && (!sel_sched_p () || !sel_insn_is_speculation_check (insn)))
>
> Too long line.  Start && below NONDEBUG.

Sure, the patch is just a reference that this particular issue is fixed 
when not calling flush_pending_lists for debug insns.

Andrey
Eric Botcazou - April 5, 2013, 10:21 a.m.
>  I don't know whether backporting this would be better than reverting
>  the offending change as just done on 4.7.

I presume that you meant on the 4.6 branch.
Olivier Hainque - April 5, 2013, 10:22 a.m.
On Apr 5, 2013, at 12:21 , Eric Botcazou <ebotcazou@adacore.com> wrote:

>> I don't know whether backporting this would be better than reverting
>> the offending change as just done on 4.7.
> 
> I presume that you meant on the 4.6 branch.

 Arf, indeed, thanks for correcting :)
Olivier Hainque - April 5, 2013, 10:53 a.m.
(re-sending with the attachement compressed so it gets through
 to the list. sorry for the duplicate and the big original attachement
 to individual addressees)

And as Eric noticed, I meant 4.6 instead of 4.7 when referring
to the recent revert.
> On Apr 5, 2013, at 10:13 , Eric Botcazou <ebotcazou@adacore.com> wrote:
>> We do have regressions on the 4.7 branch in the scheduler (CCed Olivier who 
>> has more information).
> 
> Right: we do see a SEGV while compiling the attached monitor.i (preprocessed
> output from a qemu tree) with -O2 -g.
> 
>  ./cc1 -m32 -O2 -g -quiet monitor.i
> 
> .../monitor.c: In function ‘memory_dump’:
> .../monitor.c:1109:1: internal compiler error: Segmentation fault
> 
> As already mentioned upthread, this is triggered by a call to
> flush_pending_lists with a DEBUG_INSN. We get into:
> 
> if (for_write)
>   {
>     add_dependence_list_and_free (deps, insn, &deps->pending_read_insns,
>                                   1, REG_DEP_ANTI);
>     if (!deps->readonly)
>       {
>         free_EXPR_LIST_list (&deps->pending_read_mems);
>         deps->pending_read_list_length = 0;
>       }
>   }
> 
> add_dependence_list_and_free doesn't free *LISTP when
> operating on DEBUG_INSNs, so we end up with pending_read_mems freed together
> with pending_read_insns not freed.
> 
> This was cured on mainline by:
> 
>   Author: mkuvyrkov
>   Date:   Mon Aug 27 22:11:48 2012 +0000
> 
>       * sched-deps.c (add_dependence_list_and_free): Simplify.
>       (flush_pending_list_and_free): Fix a hack that was fixing a hack.  Free
>       lists when add_dependence_list_and_free doesn't free them.
> 
>   (svn+ssh://gcc.gnu.org/svn/gcc/trunk@190733)
> 
> http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01625.html
> 
> I don't know whether backporting this would be better than reverting
> the offending change as just done on 4.7.
> 
> Olivier
> 
> <monitor.i>
Andrey Belevantsev - April 5, 2013, 11:22 a.m.
On 05.04.2013 14:10, Olivier Hainque wrote:
> On Apr 5, 2013, at 10:13 , Eric Botcazou <ebotcazou@adacore.com> wrote:
>> We do have regressions on the 4.7 branch in the scheduler (CCed Olivier who
>> has more information).
>
>   Right: we do see a SEGV while compiling the attached monitor.i (preprocessed
>   output from a qemu tree) with -O2 -g.
>
>    ./cc1 -m32 -O2 -g -quiet monitor.i
>
>   .../monitor.c: In function ‘memory_dump’:
>   .../monitor.c:1109:1: internal compiler error: Segmentation fault
>
>   As already mentioned upthread, this is triggered by a call to
>   flush_pending_lists with a DEBUG_INSN. We get into:
>
>   if (for_write)
>     {
>       add_dependence_list_and_free (deps, insn, &deps->pending_read_insns,
>                                     1, REG_DEP_ANTI);
>       if (!deps->readonly)
>         {
>           free_EXPR_LIST_list (&deps->pending_read_mems);
>           deps->pending_read_list_length = 0;
>         }
>     }
>
>   add_dependence_list_and_free doesn't free *LISTP when
>   operating on DEBUG_INSNs, so we end up with pending_read_mems freed together
>   with pending_read_insns not freed.
>
>   This was cured on mainline by:
>
>     Author: mkuvyrkov
>     Date:   Mon Aug 27 22:11:48 2012 +0000
>
>         * sched-deps.c (add_dependence_list_and_free): Simplify.
>         (flush_pending_list_and_free): Fix a hack that was fixing a hack.  Free
>         lists when add_dependence_list_and_free doesn't free them.
>
>     (svn+ssh://gcc.gnu.org/svn/gcc/trunk@190733)
>
>   http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01625.html
>
>   I don't know whether backporting this would be better than reverting
>   the offending change as just done on 4.7.

I'd say for 4.6 the best way is to revert.  PR 56077 is not that important, 
and this 4.6 release will be the last one.  For 4.7, we can additionally 
backport Maxim's patch or revert this one.  I'm fine with both options, but 
I'll test 4.7 backport too just to be ready for that.

Andrey
Olivier Hainque - April 5, 2013, 1:28 p.m.
On Apr 5, 2013, at 13:22 , Andrey Belevantsev <abel@ispras.ru> wrote:
>>  http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01625.html
>> 
>>  I don't know whether backporting this would be better than reverting
>>  the offending change as just done on 4.7.
> 
> I'd say for 4.6 the best way is to revert.  PR 56077 is not that important, and this 4.6 release will be the last one.  For 4.7, we can additionally backport Maxim's patch or revert this one.  I'm fine with both options, but I'll test 4.7 backport too just to be ready for that.

 Understood, thanks. Who's decision is it to pick one track or the other for 4.7 ?

 RMs in addition to the maintainers of this particular area ?
Jakub Jelinek - April 5, 2013, 1:40 p.m.
On Fri, Apr 05, 2013 at 03:28:11PM +0200, Olivier Hainque wrote:
> 
> On Apr 5, 2013, at 13:22 , Andrey Belevantsev <abel@ispras.ru> wrote:
> >>  http://gcc.gnu.org/ml/gcc-patches/2012-08/msg01625.html
> >> 
> >>  I don't know whether backporting this would be better than reverting
> >>  the offending change as just done on 4.7.
> > 
> > I'd say for 4.6 the best way is to revert.  PR 56077 is not that important, and this 4.6 release will be the last one.  For 4.7, we can additionally backport Maxim's patch or revert this one.  I'm fine with both options, but I'll test 4.7 backport too just to be ready for that.
> 
>  Understood, thanks. Who's decision is it to pick one track or the other for 4.7 ?
> 
>  RMs in addition to the maintainers of this particular area ?

As written in PR56848, the patch should be reverted for 4.7.3
and reapplied together with the additional fix after 4.7.3 is released
(before 4.7.3 release there is just too short time to do anything else,
while before 4.7.4 there will be plenty of time to test it sufficiently).

	Jakub
Olivier Hainque - April 5, 2013, 2:01 p.m.
On Apr 5, 2013, at 15:40 , Jakub Jelinek <jakub@redhat.com> wrote:
> As written in PR56848, the patch should be reverted for 4.7.3
> and reapplied together with the additional fix after 4.7.3 is released
> (before 4.7.3 release there is just too short time to do anything else,
> while before 4.7.4 there will be plenty of time to test it sufficiently).

 OK, thanks for the guidance Jakub.

 Andrey, could you please take care of this ?

 Many thanks in advance,

 Olivier
Eric Botcazou - April 5, 2013, 9:18 p.m.
>  Andrey, could you please take care of this ?

I've reverted the patch after bootstrapping/regtesting on x86-64/Linux.
Olivier Hainque - April 6, 2013, 5:59 a.m.
On Apr 5, 2013, at 11:18 PM, Eric Botcazou wrote:

>> Andrey, could you please take care of this ?
> 
> I've reverted the patch after bootstrapping/regtesting on x86-64/Linux.

 Thanks Eric.

Patch

Index: gcc/sched-deps.c
===================================================================
*** gcc/sched-deps.c	(revision 197492)
--- gcc/sched-deps.c	(working copy)
*************** sched_analyze_insn (struct deps_desc *de
*** 3044,3050 ****
  
        /* Don't flush pending lists on speculative checks for
  	 selective scheduling.  */
!       if (!sel_sched_p () || !sel_insn_is_speculation_check (insn))
  	flush_pending_lists (deps, insn, true, true);
  
        if (!deps->readonly)
--- 3044,3050 ----
  
        /* Don't flush pending lists on speculative checks for
  	 selective scheduling.  */
!       if (NONDEBUG_INSN_P (insn) && (!sel_sched_p () || !sel_insn_is_speculation_check (insn)))
  	flush_pending_lists (deps, insn, true, true);
  
        if (!deps->readonly)