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 ? > + /* > + * 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