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

[Peter Maydell <peter.maydell@linaro.org>]

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