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

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

[Taylor Simpson]

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.

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;
 
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

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

[Peter Maydell]

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?

> 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;
>
> 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

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

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

[Paolo Bonzini]

On 4/28/23 22:44, Taylor Simpson 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.
> 
> Co-authored-by: Taylor Simpson<tsimpson@quicinc.com>

Thanks for the fix Taylor, I'll let you submit this as part of your pull 
requests.

Paolo

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

[Taylor Simpson]

> -----Original Message-----
> From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: Saturday, April 29, 2023 7:24 AM
> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org
> Cc: 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 4/28/23 22:44, Taylor Simpson 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.
> >
> > Co-authored-by: Taylor Simpson<tsimpson@quicinc.com>
> 
> Thanks for the fix Taylor, I'll let you submit this as part of your pull requests.
> 
> Paolo

Will do.

Thanks,
Taylor

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

[Anton Johansson via]

On 4/28/23 22:44, Taylor Simpson 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.
>
> 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(-)
>

Reviewed-by: Anton Johansson <anjo@rev.ng>
Tested-by: Anton Johansson <anjo@rev.ng>