On 2/4/21 6:29 AM, David Hildenbrand wrote: > On 04.02.21 17:04, Philippe Mathieu-Daudé wrote: >> On 2/4/21 10:37 AM, David Hildenbrand wrote: >>> On 04.02.21 10:29, Richard W.M. Jones wrote: >>>>>> commit 8f17a975e60b773d7c366a81c0d9bbe304f30859 >>>>>> Author: Richard Henderson <richard.hender...@linaro.org> >>>>>> Date: Mon Mar 30 19:52:02 2020 -0700 >>>>>> >>>>>> tcg/optimize: Adjust TempOptInfo allocation >>>>>> >>>>>> The image boots just fine on s390x/TCG as well. >>>>> >>>>> Let me try this in a minute on my original test machine. >>>> >>>> I got the wrong end of the stick as David pointed out in the other email. >>>> >>>> However I did test things again this morning (all on s390 host), and >>>> current head (1ed9228f63ea4b) fails same as before ("mount" command >>>> fails). >>>> >>>> Also I downloaded: >>>> >>>> >>>> https://dl.fedoraproject.org/pub/fedora-secondary/releases/33/Cloud/s390x/images/Fedora-Cloud-Base-33-1.2.s390x.qcow2 >>>> >>>> >>>> >>>> and booted it on 1ed9228f63ea4b using this command: >>>> >>>> $ ~/d/qemu/build/s390x-softmmu/qemu-system-s390x -machine accel=tcg >>>> -m 2048 -drive >>>> file=Fedora-Cloud-Base-33-1.2.s390x.qcow2,format=qcow2,if=virtio >>>> -serial stdio >>>> >>>> Lots of core dumps inside the guest, same as David saw. >>>> >>>> I then reset qemu back to 8f17a975e60b773d ("tcg/optimize: Adjust >>>> TempOptInfo allocation"), rebuilt qemu, tested the same command and >>>> cloud image, and that booted up much happier with no failures or core >>>> dumps. >>>> >>>> Isn't it kind of weird that this would only affect an s390 host? I >>>> don't understand why the host would make a difference if we're doing >>>> TCG. >>> >>> I assume an existing BUG in the s390x TCG backend ... which makes it >>> harder to debug :) >> >> This seems to fix it: >> >> -- >8 -- >> diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc >> index 8517e552323..32d03dbfbaf 100644 >> --- a/tcg/s390/tcg-target.c.inc >> +++ b/tcg/s390/tcg-target.c.inc >> @@ -1094,10 +1094,16 @@ static int tgen_cmp(TCGContext *s, TCGType type, >> TCGCond c, TCGReg r1, >> op = (is_unsigned ? RIL_CLFI : RIL_CFI); >> tcg_out_insn_RIL(s, op, r1, c2); >> goto exit; >> - } else if (c2 == (is_unsigned ? (uint32_t)c2 : (int32_t)c2)) { >> - op = (is_unsigned ? RIL_CLGFI : RIL_CGFI); >> - tcg_out_insn_RIL(s, op, r1, c2); >> - goto exit; >> + } else if (is_unsigned) { >> + if (c2 == (uint32_t)c2) { >> + tcg_out_insn_RIL(s, RIL_CLGFI, r1, c2); >> + goto exit; >> + } >> + } else { >> + if (c2 == (int32_t)c2) { >> + tcg_out_insn_RIL(s, RIL_CGFI, r1, c2); >> + goto exit; >> + } >> } >> } >> --- >> > > This works as well I think. Broken casting. > > diff --git a/tcg/s390/tcg-target.c.inc b/tcg/s390/tcg-target.c.inc > index 8517e55232..f41ca02492 100644 > --- a/tcg/s390/tcg-target.c.inc > +++ b/tcg/s390/tcg-target.c.inc > @@ -1094,7 +1094,7 @@ static int tgen_cmp(TCGContext *s, TCGType type, TCGCond > c, TCGReg r1, > op = (is_unsigned ? RIL_CLFI : RIL_CFI); > tcg_out_insn_RIL(s, op, r1, c2); > goto exit; > - } else if (c2 == (is_unsigned ? (uint32_t)c2 : (int32_t)c2)) { > + } else if (c2 == (is_unsigned ? (TCGArg)(uint32_t)c2 : > (TCGArg)(int32_t)c2)) { > op = (is_unsigned ? RIL_CLGFI : RIL_CGFI); > > (int32_t)c2 will be converted to (uint32_t) first, then to (TCGArg). > Which is different than casting directly from (int32_t)c2 to (TCGArg). > > Nasty.
Oh excellent. Thanks for the find and analysis, guys. I totally agree that this is a bug. And it makes sense that extra constant propagation from the optimizer from the patch in question might tickle this problem. r~