> -----Original Message----- > From: Brian Cain > Sent: Tuesday, November 21, 2023 9:52 AM > To: Peter Maydell <peter.mayd...@linaro.org> > Cc: qemu-devel@nongnu.org; richard.hender...@linaro.org; f4...@amsat.org > Subject: RE: [PULL v2 25/30] Hexagon HVX (target/hexagon) instruction > decoding > > > > > -----Original Message----- > > From: qemu-devel-bounces+bcain=quicinc....@nongnu.org <qemu-devel- > > bounces+bcain=quicinc....@nongnu.org> On Behalf Of Peter Maydell > > Sent: Tuesday, November 21, 2023 8:33 AM > > To: Taylor Simpson <tsimp...@quicinc.com> > > Cc: qemu-devel@nongnu.org; richard.hender...@linaro.org; > f4...@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 <tsimp...@quicinc.com> > wrote: > > > > > > Add new file to target/hexagon/meson.build > > > > > > Acked-by: Richard Henderson <richard.hender...@linaro.org> > > > Signed-off-by: Taylor Simpson <tsimp...@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.
Yes, I believe that the declaration should move inside and remove the initialization. I'll prepare a patch to address this. -Brian