RE: [PULL v2 25/30] Hexagon HVX (target/hexagon) instruction decoding
> -Original Message-
> From: Brian Cain
> Sent: Tuesday, November 21, 2023 9:52 AM
> To: Peter Maydell
> 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 > bounces+bcain=quicinc@nongnu.org> On Behalf Of Peter Maydell
> > Sent: Tuesday, November 21, 2023 8:33 AM
> > To: Taylor Simpson
> > 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
> wrote:
> > >
> > > Add new file to target/hexagon/meson.build
> > >
> > > Acked-by: Richard Henderson
> > > Signed-off-by: Taylor Simpson
> >
> > 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 */
> > > +
RE: [PULL v2 25/30] Hexagon HVX (target/hexagon) instruction decoding
> -Original Message-
> From: qemu-devel-bounces+bcain=quicinc@nongnu.org bounces+bcain=quicinc@nongnu.org> On Behalf Of Peter Maydell
> Sent: Tuesday, November 21, 2023 8:33 AM
> To: Taylor Simpson
> 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 wrote:
> >
> > Add new file to target/hexagon/meson.build
> >
> > Acked-by: Richard Henderson
> > Signed-off-by: Taylor Simpson
>
> 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
Re: [PULL v2 25/30] Hexagon HVX (target/hexagon) instruction decoding
On Wed, 3 Nov 2021 at 21:17, Taylor Simpson wrote:
>
> Add new file to target/hexagon/meson.build
>
> Acked-by: Richard Henderson
> Signed-off-by: Taylor Simpson
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
