Le 21/12/2017 à 15:14, Paolo Bonzini a écrit : > On 21/12/2017 15:13, Laurent Vivier wrote: >> Le 21/12/2017 à 15:10, Paolo Bonzini a écrit : >>> On 21/12/2017 14:32, Laurent Vivier wrote: >>>> Le 21/12/2017 à 14:07, Laurent Vivier a écrit : >>>>> Le 21/12/2017 à 13:49, Thomas Huth a écrit : >>>>>> On 20.12.2017 22:56, Paolo Bonzini wrote: >>>>>>> On 20/12/2017 20:20, Peter Maydell wrote: >>>>>>>> On the x86/sanitizer build, new runtime errors: >>>>>>>> GTESTER check-qtest-m68k >>>>>>>> /home/petmay01/linaro/qemu-for-merges/target/m68k/translate.c:230:12: >>>>>>>> runtime error: index -1 out of bounds for type 'const uint8_t [11]' >>>>>>>> >>>>>>>> ...and similar fails on one or two boards on most of the other >>>>>>>> guest architectures. >>>>>>> >>>>>>> These are preexisting bugs, now exposed by the boot-serial-test. >>>>>>> Thomas, can you identify the architectures that have a problem and >>>>>>> notify the maintainers? In the meanwhile I'll keep the boot-serial-test >>>>>>> enhancements queued locally, and remove them from the pull request. >>>>>> >>>>>> Laurent, Richard, >>>>>> >>>>>> looks like old_op is -1 when set_cc_op() is called here for the first >>>>>> time. The problem can be reproduced by running the mini-kernel directly. >>>>>> Just get http://people.redhat.com/~thuth/m68k-uart.bin and run QEMU like >>>>>> this: >>>>>> >>>>>> qemu-system-m68k -nographic -kernel ~/tmp/m68k-uart.bin -serial none >>>>>> >>>>>> That kernel only contains these few instructions: >>>>>> >>>>>> 0x41, 0xf9, 0xfc, 0x06, 0x00, 0x00, /* lea 0xfc060000,%a0 */ >>>>>> 0x10, 0x3c, 0x00, 0x54, /* move.b #'T',%d0 */ >>>>>> 0x11, 0x7c, 0x00, 0x04, 0x00, 0x08, /* move.b #4,8(%a0) */ >>>>>> 0x11, 0x40, 0x00, 0x0c, /* move.b %d0,12(%a0) */ >>>>>> 0x60, 0xfa /* bra.s loop */ >>>>>> >>>>>> The problem occurs during the second instruction (i.e. the first move.b). >>>>>> >>>>>> Do you have any ideas where this -1 in s->cc_op could come from? >>>>> >>>>> I think it comes from CCOp: it's the value of CC_OP_DYNAMIC. >>>>> >>>>> We should not use it to access cc_op_live[]. >>>>> >>>>> I try to find a fix, but I think Richard knows this better than me. >>>> >>>> This should fix the problem, but I'd like Richard checks it... >>>> >>>> diff --git a/target/m68k/translate.c b/target/m68k/translate.c >>>> index b60909222c..721b5801da 100644 >>>> --- a/target/m68k/translate.c >>>> +++ b/target/m68k/translate.c >>>> @@ -225,6 +225,11 @@ static void set_cc_op(DisasContext *s, CCOp op) >>>> s->cc_op = op; >>>> s->cc_op_synced = 0; >>>> >>>> + if (old_op == CC_OP_DYNAMIC) { >>>> + tcg_gen_discard_i32(QREG_CC_OP); >>>> + return; >>>> + } >>> >>> This tcg_gen_discard_i32 is correct, but all flags were potentially live >>> and can be discarded if the new op uses it(*). So I'd replace "return" >>> with "old_op = CC_OP_FLAGS". >> >> Yes, I agree, we can also have: >> >> iff --git a/target/m68k/cpu.h b/target/m68k/cpu.h >> index afae5f68ac..5d03764eab 100644 >> --- a/target/m68k/cpu.h >> +++ b/target/m68k/cpu.h >> @@ -182,7 +182,7 @@ void cpu_m68k_set_fpcr(CPUM68KState *env, uint32_t val); >> */ >> typedef enum { >> /* Translator only -- use env->cc_op. */ >> - CC_OP_DYNAMIC = -1, >> + CC_OP_DYNAMIC, >> >> /* Each flag bit computed into cc_[xcnvz]. */ >> CC_OP_FLAGS, >> diff --git a/target/m68k/translate.c b/target/m68k/translate.c >> index b60909222c..61ac1a8e83 100644 >> --- a/target/m68k/translate.c >> +++ b/target/m68k/translate.c >> @@ -207,6 +207,7 @@ typedef void (*disas_proc)(CPUM68KState *env, >> DisasContext *s, uint16_t insn); >> #endif >> >> static const uint8_t cc_op_live[CC_OP_NB] = { >> + [CC_OP_DYNAMIC] = CCF_C | CCF_V | CCF_Z | CCF_N | CCF_X, >> [CC_OP_FLAGS] = CCF_C | CCF_V | CCF_Z | CCF_N | CCF_X, >> [CC_OP_ADDB ... CC_OP_ADDL] = CCF_X | CCF_N | CCF_V, >> [CC_OP_SUBB ... CC_OP_SUBL] = CCF_X | CCF_N | CCF_V, >> @@ -237,6 +238,11 @@ static void set_cc_op(DisasContext *s, CCOp op) >> if (dead & CCF_V) { >> tcg_gen_discard_i32(QREG_CC_V); >> } >> + >> + /* Discard any computed CC_OP value */ >> + if (old_op == CC_OP_DYNAMIC) { >> + tcg_gen_discard_i32(QREG_CC_OP); >> + } >> } >> >> /* Update the CPU env CC_OP state. */ >> >> > > Yes, this is good too. After my pull request is in, feel free to take > Thomas's m68k boot-serial-test patch in your tree.
I will. I plan a PULL request before the end of the week. Thanks, Laurent
