| Message ID | 20240723151454.1396826-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | target/xtensa: Make use of 'segment' in pptlb helper less confusing | expand |
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 >
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
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
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; } }
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(-)