target/xtensa: Make use of 'segment' in pptlb helper less confusing

[PATCH] target/xtensa: Make use of 'segment' in pptlb helper less confusing

Posted by Peter Maydell 1 month, 2 weeks ago

Coverity gets confused about the use of the 'segment' variable in the
pptlb helper function: it thinks that we can take a code path where
we first initialize it:
  unsigned segment = XTENSA_MPU_PROBE_B;  // 0x40000000
and then use that value as a shift count:
  } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) {

In fact this isn't possible, beacuse xtensa_mpu_lookup() is passed
'&segment', and it uses that as an output value, which it will always
set if it returns nonzero.  But the way the code is currently written
is confusing to a human reader as well as to Coverity.

Instead of initializing 'segment' at the top of the function with a
value that's only used in the "nhits == 0" code path, use the
constant value directly in that code path, and don't initialize
segment.  This matches the way we use xtensa_mpu_lookup() in its
other callsites in get_physical_addr_mpu().

Resolves: Coverity CID 1547589

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/xtensa/mmu_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
index 997b21d3890..29b84d5dbf6 100644
--- a/target/xtensa/mmu_helper.c
+++ b/target/xtensa/mmu_helper.c
@@ -991,7 +991,7 @@ uint32_t HELPER(rptlb1)(CPUXtensaState *env, uint32_t s)
 uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
 {
     unsigned nhits;
-    unsigned segment = XTENSA_MPU_PROBE_B;
+    unsigned segment;
     unsigned bg_segment;
 
     nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments,
@@ -1005,7 +1005,7 @@ uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
         xtensa_mpu_lookup(env->config->mpu_bg,
                           env->config->n_mpu_bg_segments,
                           v, &bg_segment);
-        return env->config->mpu_bg[bg_segment].attr | segment;
+        return env->config->mpu_bg[bg_segment].attr | XTENSA_MPU_PROBE_B;
     }
 }
 
-- 
2.34.1

Re: [PATCH] target/xtensa: Make use of 'segment' in pptlb helper less confusing

Posted by Peter Maydell 1 month, 1 week ago

On Tue, 23 Jul 2024 at 16:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Coverity gets confused about the use of the 'segment' variable in the
> pptlb helper function: it thinks that we can take a code path where
> we first initialize it:
>   unsigned segment = XTENSA_MPU_PROBE_B;  // 0x40000000
> and then use that value as a shift count:
>   } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) {
>
> In fact this isn't possible, beacuse xtensa_mpu_lookup() is passed
> '&segment', and it uses that as an output value, which it will always
> set if it returns nonzero.  But the way the code is currently written
> is confusing to a human reader as well as to Coverity.
>
> Instead of initializing 'segment' at the top of the function with a
> value that's only used in the "nhits == 0" code path, use the
> constant value directly in that code path, and don't initialize
> segment.  This matches the way we use xtensa_mpu_lookup() in its
> other callsites in get_physical_addr_mpu().
>
> Resolves: Coverity CID 1547589
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> --


I'll take this via target-arm.next since I'm doing a pullreq
anyway.

thanks
-- PMM

Re: [PATCH] target/xtensa: Make use of 'segment' in pptlb helper less confusing

Posted by Max Filippov 1 month, 2 weeks ago

On Tue, Jul 23, 2024 at 8:14 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Coverity gets confused about the use of the 'segment' variable in the
> pptlb helper function: it thinks that we can take a code path where
> we first initialize it:
>   unsigned segment = XTENSA_MPU_PROBE_B;  // 0x40000000
> and then use that value as a shift count:
>   } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) {
>
> In fact this isn't possible, beacuse xtensa_mpu_lookup() is passed
> '&segment', and it uses that as an output value, which it will always
> set if it returns nonzero.  But the way the code is currently written
> is confusing to a human reader as well as to Coverity.
>
> Instead of initializing 'segment' at the top of the function with a
> value that's only used in the "nhits == 0" code path, use the
> constant value directly in that code path, and don't initialize
> segment.  This matches the way we use xtensa_mpu_lookup() in its
> other callsites in get_physical_addr_mpu().
>
> Resolves: Coverity CID 1547589
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/xtensa/mmu_helper.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
> index 997b21d3890..29b84d5dbf6 100644
> --- a/target/xtensa/mmu_helper.c
> +++ b/target/xtensa/mmu_helper.c
> @@ -991,7 +991,7 @@ uint32_t HELPER(rptlb1)(CPUXtensaState *env, uint32_t s)
>  uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
>  {
>      unsigned nhits;
> -    unsigned segment = XTENSA_MPU_PROBE_B;
> +    unsigned segment;

The change suggests that coverity is ok with potentially using
uninitialized value in the shift, but not with the value that would
definitely make the shift illegal, which is a bit odd.

Acked-by: Max Filippov <jcmvbkbc@gmail.com>

>      unsigned bg_segment;
>
>      nhits = xtensa_mpu_lookup(env->mpu_fg, env->config->n_mpu_fg_segments,
> @@ -1005,7 +1005,7 @@ uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
>          xtensa_mpu_lookup(env->config->mpu_bg,
>                            env->config->n_mpu_bg_segments,
>                            v, &bg_segment);
> -        return env->config->mpu_bg[bg_segment].attr | segment;
> +        return env->config->mpu_bg[bg_segment].attr | XTENSA_MPU_PROBE_B;
>      }
>  }
>
> --
> 2.34.1
>


-- 
Thanks.
-- Max

Re: [PATCH] target/xtensa: Make use of 'segment' in pptlb helper less confusing

Posted by Peter Maydell 1 month, 2 weeks ago

On Tue, 23 Jul 2024 at 18:09, Max Filippov <jcmvbkbc@gmail.com> wrote:
>
> On Tue, Jul 23, 2024 at 8:14 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> > Coverity gets confused about the use of the 'segment' variable in the
> > pptlb helper function: it thinks that we can take a code path where
> > we first initialize it:
> >   unsigned segment = XTENSA_MPU_PROBE_B;  // 0x40000000
> > and then use that value as a shift count:
> >   } else if (nhits == 1 && (env->sregs[MPUENB] & (1u << segment))) {
> >
> > In fact this isn't possible, beacuse xtensa_mpu_lookup() is passed
> > '&segment', and it uses that as an output value, which it will always
> > set if it returns nonzero.  But the way the code is currently written
> > is confusing to a human reader as well as to Coverity.
> >
> > Instead of initializing 'segment' at the top of the function with a
> > value that's only used in the "nhits == 0" code path, use the
> > constant value directly in that code path, and don't initialize
> > segment.  This matches the way we use xtensa_mpu_lookup() in its
> > other callsites in get_physical_addr_mpu().
> >
> > Resolves: Coverity CID 1547589
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >  target/xtensa/mmu_helper.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/xtensa/mmu_helper.c b/target/xtensa/mmu_helper.c
> > index 997b21d3890..29b84d5dbf6 100644
> > --- a/target/xtensa/mmu_helper.c
> > +++ b/target/xtensa/mmu_helper.c
> > @@ -991,7 +991,7 @@ uint32_t HELPER(rptlb1)(CPUXtensaState *env, uint32_t s)
> >  uint32_t HELPER(pptlb)(CPUXtensaState *env, uint32_t v)
> >  {
> >      unsigned nhits;
> > -    unsigned segment = XTENSA_MPU_PROBE_B;
> > +    unsigned segment;
>
> The change suggests that coverity is ok with potentially using
> uninitialized value in the shift, but not with the value that would
> definitely make the shift illegal, which is a bit odd.

Yeah, the new Coverity check that has resulted in it detecting
hundreds of new "issues" relating to overflow is a bit broken
and has produced a lot of "it ought to be able to realise that
what it's suggesting is impossible" issues.

For instance there is a whole category of issues which look like:

     int x = foo(); /* returns -1 on error */
     if (x < 0) {
         return;
     }
     some arithmetic operation involving x;

where it complains that the arithmetic operation might overflow
because x might be negative because foo() can return a negative
value. It seems like it traces a line from "x could be a
specific constant value here" through to "this is a place where
if we use that constant value then it would go wrong", without
tying it into its own dataflow knowledge that would tell it that
the code-execution-path it claims is problematic can't happen
with that value of the constant.

I resolved at least a hundred of these new errors as false-positives;
this is one of the cases where I felt that even though it wasn't
actually correct about the possible error it was somewhere where
we could make our code more readable to humans (it took me two
tries reading the code before I figured out what was going on
and that this wasn't a "confusion between whether variable is
a bit value or a mask" bug).

(Also it's possible Coverity will also complain about the
possible-use-uninitialized; we can't tell until the change is
in the tree and it gets re-scanned. But if it does I'll mark
that as a false-positive.)

thanks
-- PMM