| Message ID | 515E7538.4070408@ispras.ru |
|---|---|
| State | New |
| Headers | show |
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
> 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).
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
> 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.
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 :)
(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>
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
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 ?
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
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
> Andrey, could you please take care of this ?
I've reverted the patch after bootstrapping/regtesting on x86-64/Linux.
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.
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)