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
