Re: [Qemu-devel] [PATCH v2 1/8] target-arm: A64: add support for ld/st pair

[C Fontana]

On 12 December 2013 12:45, Peter Maydell <address@hidden> wrote:
> On 12 December 2013 11:43, Alex Bennée <address@hidden> wrote:
>>
>> address@hidden writes:
>>
>>> Hi,
>>>
>>> I saw a missing return below:
>> <snip>
>>>> +    default: /* Failed decoder tree? */
>>>> +        unallocated_encoding(s);
>>>> +        break;
>>>> +    }
>>>
>>> This doesn't seem right (break instead of return):
>>>
>>> default:
>>>     unallocated_encoding(s);
>>>     return;
>>> }
>>
>> Good catch. I suspect it never hits though (or risu doesn't generate
>> enough unallocated versions).
>
> Oh, I saw that and then forgot to mention it.
> Either the comment or the code is wrong -- if
> the decoder tree prevents us getting to this point
> with that value of index then we should be asserting,
> not calling unallocated_encoding(). If the encoding
> is really supposed to be unallocated then it's not
> a failure of the decoder tree that we got here,
> it's just another case we need to check. Which is it?
>
> thanks
> -- PMM

I think that there is more than the missing return:

we need to handle the case 0 as well, as it's a perfectly valid form
of a load/store pair:
it's the Load/Store no-allocate pair (offset) (LDNP, STNP).

So in my view we need to add a case 0 where we handle  the load/store
no-allocate pair,
and no default case, or a default case where we assert(0) for unreachable code,
since all possible index values (2 bits) should be handled by the
switch statement.

Ref: C3.3 Loads and stores Table C3-3

Claudio