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

[Peter Maydell]

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?

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.

thanks
-- PMM