Re: [Qemu-devel] [RFC][PATCH v2 3/4] tcg: add optimized TCG qemu_ld/st generation

[Peter Maydell]

On 6 July 2012 12:20, Yeongkyoon Lee <address@hidden> wrote:
>
>>> +#ifdef CONFIG_QEMU_LDST_OPTIMIZATION
>>> +    /* jne slow_path */
>>> +    tcg_out_opc(s, OPC_JCC_long + JCC_JNE, 0, 0, 0);
>>> +    if (!label_ptr) {
>>> +        tcg_abort();
>>> +    }
>>
>> There's no point in this check and abort -- label_ptr will always be
>> non-NULL (it would be an internal error if it wasn't), and if it is by some
>> future bug NULL, we'll just crash on the next line, which is just as good.
>> The existing code didn't feel the need to make this check, we don't need to
>> do it in the new code.
>
> It cannot be happened now as you said. It is just for a possible future bug.
> But I cannot understand "we'll just crash on the next line" you mentioned
> above.

If the check was not present and label_ptr was somehow NULL, then
attempting to execute "label_ptr[0] = s->code_ptr;" will crash.
This is just as helpful for debugging purposes as an abort.
It's sometimes worth having sanity-checking assertions when the
code would otherwise proceed for a long time doing something wrong
but not crashing, because the assert means that you get an early
indication of failure near the point of failure. However the check
you have here is delaying the failure by exactly one line, which is
not useful.

>>> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION)
>>> +/* Macros and structures for qemu_ld/st IR code optimization:
>>> +   It looks good for TCG_MAX_HELPER_LABELS to be half of OPC_BUF_SIZE in
>>> exec-all.h. */
>>> +#define TCG_MAX_QEMU_LDST       320
>>
>> Is that true even if you have a huge block with nothing but simple
>> guest load instructions in it?
>
> I agree. It needs to be set as same size with OPC_BUF_SIZE for covering
> extreme cases.

The point here, incidentally, is that guest code should never be able
to make qemu crash or abort, so any fixed sized buffer has to be able
to handle the worst case.

-- PMM