RE: [PULL v2 25/30] Hexagon HVX (target/hexagon) instruction decoding

[Brian Cain]

> -----Original Message-----
> From: qemu-devel-bounces+bcain=quicinc.com@nongnu.org <qemu-devel-
> bounces+bcain=quicinc.com@nongnu.org> On Behalf Of Peter Maydell
> Sent: Tuesday, November 21, 2023 8:33 AM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: qemu-devel@nongnu.org; richard.henderson@linaro.org; f4bug@amsat.org
> Subject: Re: [PULL v2 25/30] Hexagon HVX (target/hexagon) instruction
> decoding
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
> 
> On Wed, 3 Nov 2021 at 21:17, Taylor Simpson <tsimpson@quicinc.com> wrote:
> >
> > Add new file to target/hexagon/meson.build
> >
> > Acked-by: Richard Henderson <richard.henderson@linaro.org>
> > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> 
> Hi; Coverity points out a variable written to and then
> overwritten before it's ever used in this function (CID 1527408):
> 
> 
> 
> 
> > +static void
> > +check_new_value(Packet *pkt)
> > +{
> > +    /* .new value for a MMVector store */
> > +    int i, j;
> > +    const char *reginfo;
> > +    const char *destletters;
> > +    const char *dststr = NULL;
> > +    uint16_t def_opcode;
> > +    char letter;
> > +    int def_regnum;
> 
> def_regnum has function level scope...
> 
> > +
> > +    for (i = 1; i < pkt->num_insns; i++) {
> > +        uint16_t use_opcode = pkt->insn[i].opcode;
> > +        if (GET_ATTRIB(use_opcode, A_DOTNEWVALUE) &&
> > +            GET_ATTRIB(use_opcode, A_CVI) &&
> > +            GET_ATTRIB(use_opcode, A_STORE)) {
> > +            int use_regidx = strchr(opcode_reginfo[use_opcode], 's') -
> > +                opcode_reginfo[use_opcode];
> > +            /*
> > +             * What's encoded at the N-field is the offset to who's 
> > producing
> > +             * the value.
> > +             * Shift off the LSB which indicates odd/even register.
> > +             */
> > +            int def_off = ((pkt->insn[i].regno[use_regidx]) >> 1);
> > +            int def_oreg = pkt->insn[i].regno[use_regidx] & 1;
> > +            int def_idx = -1;
> > +            for (j = i - 1; (j >= 0) && (def_off >= 0); j--) {
> > +                if (!GET_ATTRIB(pkt->insn[j].opcode, A_CVI)) {
> > +                    continue;
> > +                }
> > +                def_off--;
> > +                if (def_off == 0) {
> > +                    def_idx = j;
> > +                    break;
> > +                }
> > +            }
> > +            /*
> > +             * Check for a badly encoded N-field which points to an 
> > instruction
> > +             * out-of-range
> > +             */
> > +            g_assert(!((def_off != 0) || (def_idx < 0) ||
> > +                       (def_idx > (pkt->num_insns - 1))));
> > +
> > +            /* def_idx is the index of the producer */
> > +            def_opcode = pkt->insn[def_idx].opcode;
> > +            reginfo = opcode_reginfo[def_opcode];
> > +            destletters = "dexy";
> > +            for (j = 0; (letter = destletters[j]) != 0; j++) {
> > +                dststr = strchr(reginfo, letter);
> > +                if (dststr != NULL) {
> > +                    break;
> > +                }
> > +            }
> > +            if ((dststr == NULL)  && GET_ATTRIB(def_opcode, A_CVI_GATHER)) 
> > {
> > +                def_regnum = 0;
> 
> In this half of the if() we set it to 0...
> 
> > +                pkt->insn[i].regno[use_regidx] = def_oreg;
> > +                pkt->insn[i].new_value_producer_slot = 
> > pkt->insn[def_idx].slot;
> > +            } else {
> > +                if (dststr == NULL) {
> > +                    /* still not there, we have a bad packet */
> > +                    g_assert_not_reached();
> > +                }
> > +                def_regnum = pkt->insn[def_idx].regno[dststr - reginfo];
> > +                /* Now patch up the consumer with the register number */
> > +                pkt->insn[i].regno[use_regidx] = def_regnum ^ def_oreg;
> > +                /* special case for (Vx,Vy) */
> > +                dststr = strchr(reginfo, 'y');
> > +                if (def_oreg && strchr(reginfo, 'x') && dststr) {
> > +                    def_regnum = pkt->insn[def_idx].regno[dststr - 
> > reginfo];
> > +                    pkt->insn[i].regno[use_regidx] = def_regnum;
> > +                }
> 
> ...but the only place we read def_regnum is in this other half of the
> if(), and if we get here then we've set it to something out of pxt->insn.
> 
> Were we supposed to do something with def_regnum outside this if(),
> or could we instead drop the initialization in the first half of the if()
> and move its declaration inside this else {} block ?

Hmm -- we'll take a look at this and get back to you.

> > +                /*
> > +                 * We need to remember who produces this value to later
> > +                 * check if it was dynamically cancelled
> > +                 */
> > +                pkt->insn[i].new_value_producer_slot = 
> > pkt->insn[def_idx].slot;
> > +            }
> > +        }
> > +    }
> > +}
> 
> thanks
> -- PMM