| Message ID | 20121030235636.GB32197@hall.aurel32.net |
|---|---|
| State | New |
| Headers | show |
On Wed, Oct 31, 2012 at 12:56:36AM +0100, Aurelien Jarno wrote: > On Tue, Oct 30, 2012 at 11:24:34PM +0100, Stefan Weil wrote: > > Am 30.10.2012 09:15, schrieb Paolo Bonzini: > > >Il 29/10/2012 19:29, Aurelien Jarno ha scritto: > > >>On Mon, Oct 29, 2012 at 06:53:14PM +0100, Paolo Bonzini wrote: > > >>>>Known-good commit: 8473f377393219390ea6f2d8d450a2b054bb823e > > >>>>Known-bad commit: d262cb02861dd33375c08fc798930653b14769e9 > > >>>> > > >>>>i386-softmmu seems to work. I may try to bisect it tomorrow, but I'd be > > >>>>glad if somebody else beats me. It can be reproduced with Wine and > > >>>>"x86_64-softmmu/qemu-system-x86_64.exe -L ../pc-bios"; it hangs at iPXE. > > >>Oops, sorry about that. Is it win32 or win64? I'll try to fix it asap, > > >>but right now I don't have a good network connection enough to either > > >>setup a mingw build environment or to connect to a remote machine with > > >>such an environment. > > > > > >It's win32, and the first bad commit is 9c43b68 (tcg: rework liveness > > >analysis, 2012-10-09). But it looks like 64-on-32 emulation is more > > >generally broken. I now tried x86_64-linux-user compiled for 32-bit, > > >and it segfaults on startup. Even the previous commit cannot run > > >qemu-x86_64 /bin/ls correctly: > > > > > > > I just tested with latest qemu-system-x86_64 on 32 bit Linux. > > > > It also hangs during boot (BIOS), so it looks like this > > is not a MinGW only problem. > > > > Your test with x86_64-linux-user indicates that, too. > > > > I also get the problem with TCI. Therefore I expect that any > > 32 bit TCG target will show it. > > > > It ended up to be a merge issue. The newly added special cases > for half-dead operations also need to be changed with the liveness > analysis rework. > > The attached patch fixes the issue on a 32-bit linux host. I haven't > tried win32 yet, maybe someone will beat me. > I have just been able to try, and I confirm it fixes the problem on win32.
Il 31/10/2012 13:40, Aurelien Jarno ha scritto: >>> I just tested with latest qemu-system-x86_64 on 32 bit Linux. >>> >>> It also hangs during boot (BIOS), so it looks like this >>> is not a MinGW only problem. >>> >>> Your test with x86_64-linux-user indicates that, too. >>> >>> I also get the problem with TCI. Therefore I expect that any >>> 32 bit TCG target will show it. >>> >> >> It ended up to be a merge issue. The newly added special cases >> for half-dead operations also need to be changed with the liveness >> analysis rework. >> >> The attached patch fixes the issue on a 32-bit linux host. I haven't >> tried win32 yet, maybe someone will beat me. >> > > I have just been able to try, and I confirm it fixes the problem on > win32. Thanks! In general, do not rebase a branch unless you were able to test the rebase fully. Use "git merge" instead. This does not apply to people without commit access (unless they use pull requests---perhaps we should use them more), but it is easy for you. If you hadn't rebased the series, "git bisect" would have pointed out that the original series worked, and that the merge was the problem. (Yes, this is less accurate than knowing *which* patch made invalid assumptions after the rebase. However, you can always do a temporary rebase to find that. A linearized history doesn't say what was the latest "master" commit on top of which you tested). Paolo
On 31 October 2012 15:01, Paolo Bonzini <pbonzini@redhat.com> wrote: > Thanks! In general, do not rebase a branch unless you were able to test > the rebase fully. Use "git merge" instead. This does not apply to > people without commit access (unless they use pull requests---perhaps we > should use them more), but it is easy for you. > > If you hadn't rebased the series, "git bisect" would have pointed out > that the original series worked, and that the merge was the problem. I don't think this actually gains us anything, because we've still checked broken code into master, whether it was via a rebase or a merge. The correct answer is "test your commits before sending them", surely? -- PMM
Il 31/10/2012 15:05, Peter Maydell ha scritto: >> > Thanks! In general, do not rebase a branch unless you were able to test >> > the rebase fully. Use "git merge" instead. This does not apply to >> > people without commit access (unless they use pull requests---perhaps we >> > should use them more), but it is easy for you. >> > >> > If you hadn't rebased the series, "git bisect" would have pointed out >> > that the original series worked, and that the merge was the problem. > I don't think this actually gains us anything, because we've still > checked broken code into master, whether it was via a rebase or a > merge. The correct answer is "test your commits before sending them", > surely? I think Aurelien did test, but only on a 64-bit host. So the idea still holds: if you have tested more than that earlier, do not rebase. Paolo
Le 31/10/2012 15:08, Paolo Bonzini a écrit : > Il 31/10/2012 15:05, Peter Maydell ha scritto: >>>> Thanks! In general, do not rebase a branch unless you were able to test >>>> the rebase fully. Use "git merge" instead. This does not apply to >>>> people without commit access (unless they use pull requests---perhaps we >>>> should use them more), but it is easy for you. >>>> >>>> If you hadn't rebased the series, "git bisect" would have pointed out >>>> that the original series worked, and that the merge was the problem. >> I don't think this actually gains us anything, because we've still >> checked broken code into master, whether it was via a rebase or a >> merge. The correct answer is "test your commits before sending them", >> surely? > > I think Aurelien did test, but only on a 64-bit host. So the idea still > holds: if you have tested more than that earlier, do not rebase. When I said a merge issue, it's not in the sense "git merge issue". Both series are touching different part of the code, but the resulting behaviour was wrong. If we want to do it correctly here, it means that we have to rebase and test all series each time a single patch is committed. Not really doable.
Am 31.10.2012 00:56, schrieb Aurelien Jarno: > On Tue, Oct 30, 2012 at 11:24:34PM +0100, Stefan Weil wrote: >> Am 30.10.2012 09:15, schrieb Paolo Bonzini: >>> Il 29/10/2012 19:29, Aurelien Jarno ha scritto: >>>> On Mon, Oct 29, 2012 at 06:53:14PM +0100, Paolo Bonzini wrote: >>>>>> Known-good commit: 8473f377393219390ea6f2d8d450a2b054bb823e >>>>>> Known-bad commit: d262cb02861dd33375c08fc798930653b14769e9 >>>>>> >>>>>> i386-softmmu seems to work. I may try to bisect it tomorrow, but I'd be >>>>>> glad if somebody else beats me. It can be reproduced with Wine and >>>>>> "x86_64-softmmu/qemu-system-x86_64.exe -L ../pc-bios"; it hangs at iPXE. >>>> Oops, sorry about that. Is it win32 or win64? I'll try to fix it asap, >>>> but right now I don't have a good network connection enough to either >>>> setup a mingw build environment or to connect to a remote machine with >>>> such an environment. >>> It's win32, and the first bad commit is 9c43b68 (tcg: rework liveness >>> analysis, 2012-10-09). But it looks like 64-on-32 emulation is more >>> generally broken. I now tried x86_64-linux-user compiled for 32-bit, >>> and it segfaults on startup. Even the previous commit cannot run >>> qemu-x86_64 /bin/ls correctly: >>> >> I just tested with latest qemu-system-x86_64 on 32 bit Linux. >> >> It also hangs during boot (BIOS), so it looks like this >> is not a MinGW only problem. >> >> Your test with x86_64-linux-user indicates that, too. >> >> I also get the problem with TCI. Therefore I expect that any >> 32 bit TCG target will show it. >> > It ended up to be a merge issue. The newly added special cases > for half-dead operations also need to be changed with the liveness > analysis rework. > > The attached patch fixes the issue on a 32-bit linux host. I haven't > tried win32 yet, maybe someone will beat me. Tested-by: Stefan Weil <sw@weilnetz.de> Your attached patch fixes the problem seen with qemu-system-x86_64 on 32 bit Linux and with 32 bit Windows. I think you can commit it to git master. Merci, Stefan
On Wed, Oct 31, 2012 at 06:05:57PM +0100, Stefan Weil wrote: > Am 31.10.2012 00:56, schrieb Aurelien Jarno: > >On Tue, Oct 30, 2012 at 11:24:34PM +0100, Stefan Weil wrote: > >>Am 30.10.2012 09:15, schrieb Paolo Bonzini: > >>>Il 29/10/2012 19:29, Aurelien Jarno ha scritto: > >>>>On Mon, Oct 29, 2012 at 06:53:14PM +0100, Paolo Bonzini wrote: > >>>>>>Known-good commit: 8473f377393219390ea6f2d8d450a2b054bb823e > >>>>>>Known-bad commit: d262cb02861dd33375c08fc798930653b14769e9 > >>>>>> > >>>>>>i386-softmmu seems to work. I may try to bisect it tomorrow, but I'd be > >>>>>>glad if somebody else beats me. It can be reproduced with Wine and > >>>>>>"x86_64-softmmu/qemu-system-x86_64.exe -L ../pc-bios"; it hangs at iPXE. > >>>>Oops, sorry about that. Is it win32 or win64? I'll try to fix it asap, > >>>>but right now I don't have a good network connection enough to either > >>>>setup a mingw build environment or to connect to a remote machine with > >>>>such an environment. > >>>It's win32, and the first bad commit is 9c43b68 (tcg: rework liveness > >>>analysis, 2012-10-09). But it looks like 64-on-32 emulation is more > >>>generally broken. I now tried x86_64-linux-user compiled for 32-bit, > >>>and it segfaults on startup. Even the previous commit cannot run > >>>qemu-x86_64 /bin/ls correctly: > >>> > >>I just tested with latest qemu-system-x86_64 on 32 bit Linux. > >> > >>It also hangs during boot (BIOS), so it looks like this > >>is not a MinGW only problem. > >> > >>Your test with x86_64-linux-user indicates that, too. > >> > >>I also get the problem with TCI. Therefore I expect that any > >>32 bit TCG target will show it. > >> > >It ended up to be a merge issue. The newly added special cases > >for half-dead operations also need to be changed with the liveness > >analysis rework. > > > >The attached patch fixes the issue on a 32-bit linux host. I haven't > >tried win32 yet, maybe someone will beat me. > > > Tested-by: Stefan Weil <sw@weilnetz.de> > > Your attached patch fixes the problem seen with qemu-system-x86_64 > on 32 bit Linux and with 32 bit Windows. > > I think you can commit it to git master. > Done. Thanks for the test.
From 8a99a0e875f2de8bf47e6fd27523723176251333 Mon Sep 17 00:00:00 2001 From: Aurelien Jarno <aurelien@aurel32.net> Date: Wed, 31 Oct 2012 00:50:15 +0100 Subject: [PATCH] tcg: don't remove op if output needs to be synced to memory Commit 9c43b68de628a1e2cba556adfb71c17028eb802e do not correctly check for dead outputs when they need to be synced to memory in case of half-dead operations. Fix that by applying the same pattern than for the default case. Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- tcg/tcg.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tcg/tcg.c b/tcg/tcg.c index c3a7f19..1133438 100644 --- a/tcg/tcg.c +++ b/tcg/tcg.c @@ -1329,8 +1329,8 @@ static void tcg_liveness_analysis(TCGContext *s) the low part. The result can be optimized to a simple add or sub. This happens often for x86_64 guest when the cpu mode is set to 32 bit. */ - if (dead_temps[args[1]]) { - if (dead_temps[args[0]]) { + if (dead_temps[args[1]] && !mem_temps[1]) { + if (dead_temps[args[0]] && !mem_temps[0]) { goto do_remove; } /* Create the single operation plus nop. */ @@ -1355,8 +1355,8 @@ static void tcg_liveness_analysis(TCGContext *s) nb_iargs = 2; nb_oargs = 2; /* Likewise, test for the high part of the operation dead. */ - if (dead_temps[args[1]]) { - if (dead_temps[args[0]]) { + if (dead_temps[args[1]] && !mem_temps[1]) { + if (dead_temps[args[0]] && !mem_temps[0]) { goto do_remove; } gen_opc_buf[op_index] = op = INDEX_op_mul_i32; -- 1.7.2.5