Re: [PATCH] target/mips/mxu: Rewrite D16MIN / D16MAX opcodes

[Philippe Mathieu-Daudé]

Hi Peter,

On 3/16/21 1:03 PM, Philippe Mathieu-Daudé wrote:
> On 3/16/21 11:51 AM, Peter Maydell wrote:
>> On Mon, 15 Mar 2021 at 22:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> Coverity reported (CID 1450831) an array overrun in
>>> gen_mxu_D16MAX_D16MIN():
>>>
>>>   1103     } else if (unlikely((XRb == 0) || (XRa == 0))) {
>>>   ....
>>>   1112         if (opc == OPC_MXU_D16MAX) {
>>>   1113             tcg_gen_smax_i32(mxu_gpr[XRa - 1], t0, t1);
>>>   1114         } else {
>>>   1115             tcg_gen_smin_i32(mxu_gpr[XRa - 1], t0, t1);
>>>   1116         }
>>>
>>>>>> Overrunning array "mxu_gpr" of 15 8-byte elements at element
>>>     index 4294967295 (byte offset 34359738367) using index "XRa - 1U"
>>>     (which evaluates to 4294967295).
>>>
>>> Because we check if 'XRa == 0' then access 'XRa - 1' in array.
>>>
>>> I figured it could be easier to rewrite this function to something
>>> simpler rather than trying to understand it.
>>
>> This seems to drop half of the optimised cases the old
>> code had. Wouldn't it be simpler just to fix the bugs
>> in the conditions?

Besides your other simpler patch fixing Coverity, I've been
thinking at your comment during lunch. IMHO it would be
easier (or maintainance to keep TCG frontends simple
enough, and move the optimizations to common TCG, so all
backends could benefit of them.

MIPS frontend might not be the best example because of the
hard-wired $zero register, which is harder to defer to
common TCG for optimization.

>> That is:
>>
>>>      if (unlikely(pad != 0)) {
>>>          /* opcode padding incorrect -> do nothing */
>>> -    } else if (unlikely(XRc == 0)) {
>>> -        /* destination is zero register -> do nothing */
>>
>> This should be "XRa == 0"
>>
>>> -    } else if (unlikely((XRb == 0) && (XRa == 0))) {
>>> -        /* both operands zero registers -> just set destination to zero */
>>
>> This should be "XRb == 0 && XRc == 0"
>>
>>> -        tcg_gen_movi_i32(mxu_gpr[XRc - 1], 0);
>>
>> This should set mxu_gpr[XRa - 1]
>>
>>> -    } else if (unlikely((XRb == 0) || (XRa == 0))) {
>>
>> This should be "XRb == 0 || XRc == 0"
>>
>> And everything else in the function looks OK to me.
> 
> If you have the changes clear, do you mind sending a patch?
> 
> Thanks,
> 
> Phil.
>