RE: [PATCH v2] target/hexagon: fix = vs. == mishap

[Taylor Simpson]

> -----Original Message-----
> From: Peter Maydell <peter.maydell@linaro.org>
> Sent: Saturday, April 29, 2023 4:27 AM
> To: Taylor Simpson <tsimpson@quicinc.com>
> Cc: qemu-devel@nongnu.org; pbonzini@redhat.com;
> richard.henderson@linaro.org; philmd@linaro.org; ale@rev.ng;
> anjo@rev.ng; Brian Cain <bcain@quicinc.com>; Matheus Bernardino (QUIC)
> <quic_mathbern@quicinc.com>
> Subject: Re: [PATCH v2] target/hexagon: fix = vs. == mishap
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> On Fri, 28 Apr 2023 at 21:45, Taylor Simpson <tsimpson@quicinc.com> wrote:
> >
> > From: Paolo Bonzini <pbonzini@redhat.com>
> >
> > **** Changes in v2 ****
> > Fix yyassert's for sign and zero extends
> >
> > Coverity reports a parameter that is "set but never used".  This is
> > caused by an assignment operator being used instead of equality.
> 
> The commit message says it's fixing yyasserts, but the new changed code
> doesn't seem to be fixing yyasserts?

Hi Peter,

See below ...

Taylor

> 
> > Co-authored-by: Taylor Simpson <tsimpson@quicinc.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com>
> > ---
> >  target/hexagon/idef-parser/parser-helpers.c | 2 +-
> >  target/hexagon/idef-parser/idef-parser.y    | 4 ++--
> >  2 files changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/hexagon/idef-parser/parser-helpers.c
> > b/target/hexagon/idef-parser/parser-helpers.c
> > index 86511efb62..0a01ec39b7 100644
> > --- a/target/hexagon/idef-parser/parser-helpers.c
> > +++ b/target/hexagon/idef-parser/parser-helpers.c
> > @@ -1123,7 +1123,7 @@ HexValue gen_extend_op(Context *c,
> >                         HexValue *value,
> >                         HexSignedness signedness)  {
> > -    unsigned bit_width = (dst_width = 64) ? 64 : 32;
> > +    unsigned bit_width = (dst_width == 64) ? 64 : 32;
> >      HexValue value_m = *value;
> >      HexValue src_width_m = *src_width;
> >

Not that before Paolo's change, bit_width would always be set to 64.  After the 
change, it will be either 64 or 32.

The yyassert in question doesn't show up in the diff.  It's just below the code 
above
    yyassert(c, locp, value_m.bit_width <= bit_width &&
                      src_width_m.bit_width <= bit_width,
                      "Extending to a size smaller than the current size"
                      " makes no sense");
There are some cases where the semantics code being parsed passes 33 as the 
dst_width and a 64-bit value argument.  So, the assert fails.

To fix the problem, we pass dst_width as 64 for the SXT (sign extend) and ZXT 
(zero extend) productions below.

After this change, all call sites pass 64 as the value of that argument.  So, 
we could go even further and remove that parameter and always set bit_width to 
64.  Alessandro and Anton are more familiar with this code than I am, so I'll 
wait for them to weigh in.



> > diff --git a/target/hexagon/idef-parser/idef-parser.y
> > b/target/hexagon/idef-parser/idef-parser.y
> > index 5444fd4749..2561f0ebb0 100644
> > --- a/target/hexagon/idef-parser/idef-parser.y
> > +++ b/target/hexagon/idef-parser/idef-parser.y
> > @@ -685,7 +685,7 @@ rvalue : FAIL
> >               yyassert(c, &@1, $5.type == IMMEDIATE &&
> >                        $5.imm.type == VALUE,
> >                        "SXT expects immediate values\n");
> > -             $$ = gen_extend_op(c, &@1, &$3, $5.imm.value, &$7, SIGNED);
> > +             $$ = gen_extend_op(c, &@1, &$3, 64, &$7, SIGNED);
> >           }
> >         | ZXT '(' rvalue ',' IMM ',' rvalue ')'
> >           {
> > @@ -693,7 +693,7 @@ rvalue : FAIL
> >               yyassert(c, &@1, $5.type == IMMEDIATE &&
> >                        $5.imm.type == VALUE,
> >                        "ZXT expects immediate values\n");
> > -             $$ = gen_extend_op(c, &@1, &$3, $5.imm.value, &$7, UNSIGNED);
> > +             $$ = gen_extend_op(c, &@1, &$3, 64, &$7, UNSIGNED);
> >           }
> >         | '(' rvalue ')'
> >           {
> > --
> > 2.25.1
> 
> thanks
> -- PMM