Patchwork Fix twolf -funroll-loops -O3 miscompilation (a semi-latent web.c bug)

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 15, 2012, 1:21 p.m.
Message ID <507C0DCE.4070208@gnu.org>
Download mbox | patch
Permalink /patch/191561/
State New
Headers show

Comments

Paolo Bonzini - Oct. 15, 2012, 1:21 p.m.
Il 15/10/2012 14:53, Steven Bosscher ha scritto:
> I think I've shown above that we're all looking at the wrong pass...

I think you have... so we want a patch like this?


Paolo
Steven Bosscher - Oct. 15, 2012, 1:26 p.m.
On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
> Il 15/10/2012 14:53, Steven Bosscher ha scritto:
>> I think I've shown above that we're all looking at the wrong pass...
>
> I think you have... so we want a patch like this?

I don't think so. df_kill_notes is already supposed to take care of this.

Ciao!
Steven
Steven Bosscher - Oct. 16, 2012, 10:15 a.m.
On Mon, Oct 15, 2012 at 3:26 PM, Steven Bosscher wrote:
> On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini wrote:
>> Il 15/10/2012 14:53, Steven Bosscher ha scritto:
>>> I think I've shown above that we're all looking at the wrong pass...
>>
>> I think you have... so we want a patch like this?
>
> I don't think so. df_kill_notes is already supposed to take care of this.

But it doesn't because if the SET_DEST of an insn is the same as the
register dieing in the insn's REG_EQUAL note, the reg is live at the
end of the insn and so the note stays:

Breakpoint 2, df_kill_notes (insn=0x7ffff5e3e7e0, live=0x7fffffffda90)
at ../../trunk/gcc/df-problems.c:2833
2833      rtx *pprev = &REG_NOTES (insn);
1: debug_rtx(insn) = (insn 79 50 52 8 (set (reg:DI 72 [ ivtmp.17D.1758 ])
        (reg:DI 103 [ ivtmp.17D.1758 ])) -1
     (expr_list:REG_DEAD (reg:DI 103 [ ivtmp.17D.1758 ])
        (expr_list:REG_EQUAL (plus:DI (reg:DI 72 [ ivtmp.17D.1758 ])
                (const_int 24 [0x18]))
            (nil))))
void
(gdb) undisp 1
(gdb) p debug_bitmap(live)

first = 0x1627200 current = 0x1627200 indx = 0
        0x1627200 next = (nil) prev = (nil) indx = 0
                bits = { 6 7 16 20 72 82 85 87 }
$2 = void
(gdb)


So GCC should be looking at whether the reg in the REG_EQUAL note is
dead *before* the insn.

Bottom line: This is a bug in df_kill_notes.

Ciao!
Steven
Steven Bosscher - Oct. 16, 2012, 10:35 a.m.
On Tue, Oct 16, 2012 at 12:15 PM, Steven Bosscher wrote:
> On Mon, Oct 15, 2012 at 3:26 PM, Steven Bosscher wrote:
>> On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini wrote:
>>> Il 15/10/2012 14:53, Steven Bosscher ha scritto:
>>>> I think I've shown above that we're all looking at the wrong pass...
>>>
>>> I think you have... so we want a patch like this?
>>
>> I don't think so. df_kill_notes is already supposed to take care of this.
>
> But it doesn't because if the SET_DEST of an insn is the same as the
> register dieing in the insn's REG_EQUAL note, the reg is live at the
> end of the insn and so the note stays:
>
> Breakpoint 2, df_kill_notes (insn=0x7ffff5e3e7e0, live=0x7fffffffda90)
> at ../../trunk/gcc/df-problems.c:2833
> 2833      rtx *pprev = &REG_NOTES (insn);
> 1: debug_rtx(insn) = (insn 79 50 52 8 (set (reg:DI 72 [ ivtmp.17D.1758 ])
>         (reg:DI 103 [ ivtmp.17D.1758 ])) -1
>      (expr_list:REG_DEAD (reg:DI 103 [ ivtmp.17D.1758 ])
>         (expr_list:REG_EQUAL (plus:DI (reg:DI 72 [ ivtmp.17D.1758 ])
>                 (const_int 24 [0x18]))
>             (nil))))
> void
> (gdb) undisp 1
> (gdb) p debug_bitmap(live)
>
> first = 0x1627200 current = 0x1627200 indx = 0
>         0x1627200 next = (nil) prev = (nil) indx = 0
>                 bits = { 6 7 16 20 72 82 85 87 }
> $2 = void
> (gdb)
>
>
> So GCC should be looking at whether the reg in the REG_EQUAL note is
> dead *before* the insn.
>
> Bottom line: This is a bug in df_kill_notes.

I think this should fix it. Can't test it right now, so help
appreciated (Honza: hint hint! ;-)

Ciao!
Steven
Paolo Bonzini - Oct. 16, 2012, 11:38 a.m.
Il 16/10/2012 12:35, Steven Bosscher ha scritto:
> On Tue, Oct 16, 2012 at 12:15 PM, Steven Bosscher wrote:
>> On Mon, Oct 15, 2012 at 3:26 PM, Steven Bosscher wrote:
>>> On Mon, Oct 15, 2012 at 3:21 PM, Paolo Bonzini wrote:
>>>> Il 15/10/2012 14:53, Steven Bosscher ha scritto:
>>>>> I think I've shown above that we're all looking at the wrong pass...
>>>>
>>>> I think you have... so we want a patch like this?
>>>
>>> I don't think so. df_kill_notes is already supposed to take care of this.
>>
>> But it doesn't because if the SET_DEST of an insn is the same as the
>> register dieing in the insn's REG_EQUAL note, the reg is live at the
>> end of the insn and so the note stays:
>>
>> Breakpoint 2, df_kill_notes (insn=0x7ffff5e3e7e0, live=0x7fffffffda90)
>> at ../../trunk/gcc/df-problems.c:2833
>> 2833      rtx *pprev = &REG_NOTES (insn);
>> 1: debug_rtx(insn) = (insn 79 50 52 8 (set (reg:DI 72 [ ivtmp.17D.1758 ])
>>         (reg:DI 103 [ ivtmp.17D.1758 ])) -1
>>      (expr_list:REG_DEAD (reg:DI 103 [ ivtmp.17D.1758 ])
>>         (expr_list:REG_EQUAL (plus:DI (reg:DI 72 [ ivtmp.17D.1758 ])
>>                 (const_int 24 [0x18]))
>>             (nil))))
>> void
>> (gdb) undisp 1
>> (gdb) p debug_bitmap(live)
>>
>> first = 0x1627200 current = 0x1627200 indx = 0
>>         0x1627200 next = (nil) prev = (nil) indx = 0
>>                 bits = { 6 7 16 20 72 82 85 87 }
>> $2 = void
>> (gdb)
>>
>>
>> So GCC should be looking at whether the reg in the REG_EQUAL note is
>> dead *before* the insn.
>>
>> Bottom line: This is a bug in df_kill_notes.

Yep, and it could cause wrong code generation, couldn't it?  Because the
new (reg:DI 72) is obviously not equal to (plus:DI (reg:DI 72)
(const_int 24)), but it could be propagated to subsequent insns.

So I think this patch should be backported to all release branches after
regstrapping.

> I think this should fix it. Can't test it right now, so help
> appreciated (Honza: hint hint! ;-)

Ok after bootstrap, regtest and checking that it actually fixes the bug.

Paolo
Steven Bosscher - Oct. 16, 2012, 10:54 p.m.
On Tue, Oct 16, 2012 at 1:38 PM, Paolo Bonzini wrote:
>>> Bottom line: This is a bug in df_kill_notes.
>
> Yep, and it could cause wrong code generation, couldn't it?  Because the
> new (reg:DI 72) is obviously not equal to (plus:DI (reg:DI 72)
> (const_int 24)), but it could be propagated to subsequent insns.

Right.


> So I think this patch should be backported to all release branches after
> regstrapping.

Agreed, but let's wait a few days at least to see what happens with
the patch on the trunk.


> Ok after bootstrap, regtest and checking that it actually fixes the bug.

I made sure of that before posting the patch ;-)

Bootstrap&regtest is running on x86_64-unknown-linux-gnu and
powerpc64-unknown-linux-gnu. I'll commit the patch tomorrow if there
are no test suite regressions.

Well done to us all for probing until we found the root cause of this bug!

Ciao!
Steven
Bin.Cheng - Oct. 19, 2012, 3:48 a.m.
On Wed, Oct 17, 2012 at 6:54 AM, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> On Tue, Oct 16, 2012 at 1:38 PM, Paolo Bonzini wrote:
>>>> Bottom line: This is a bug in df_kill_notes.
>>
>> Yep, and it could cause wrong code generation, couldn't it?  Because the
>> new (reg:DI 72) is obviously not equal to (plus:DI (reg:DI 72)
>> (const_int 24)), but it could be propagated to subsequent insns.
>
> Right.
>
>
>> So I think this patch should be backported to all release branches after
>> regstrapping.
>
> Agreed, but let's wait a few days at least to see what happens with
> the patch on the trunk.
>
>
>> Ok after bootstrap, regtest and checking that it actually fixes the bug.
>
> I made sure of that before posting the patch ;-)
>
> Bootstrap&regtest is running on x86_64-unknown-linux-gnu and
> powerpc64-unknown-linux-gnu. I'll commit the patch tomorrow if there
> are no test suite regressions.
>
> Well done to us all for probing until we found the root cause of this bug!
>

Hi Jan,

For the test case:

main()
{
  configure2();
}

Please make main return ZERO by default.

When cross testing with qemu/simulator, the wrap exit checks whether
the case by verifying the return value.
For ARM target, R0 is checked while it may be clobbered(thus non-ZERO)
if main does not return any value, which causes failure of test case.

Thanks very much.

Patch

Index: df-problems.c
===================================================================
--- df-problems.c	(revisione 183719)
+++ df-problems.c	(copia locale)
@@ -3480,6 +3485,18 @@  df_note_bb_compute (unsigned int bb_inde
 	    }
 	}
 
+      for (use_rec = DF_INSN_UID_EQ_USES (uid); *use_rec; use_rec++)
+	{
+	  df_ref use = *use_rec;
+	  unsigned int uregno = DF_REF_REGNO (use);
+
+	  if (!bitmap_bit_p (live, uregno))
+            {
+              remove_note (insn, find_reg_equal_equiv_note (insn));
+              break;
+            }
+	}
+
       if (debug_insn == -1)
 	{
 	  /* ??? We could probably do better here, replacing dead