On 3/18/25 1:42 PM, Peter Maydell wrote:
On Fri, 7 Mar 2025 at 12:46, Daniel Henrique Barboza <dbarb...@ventanamicro.com> wrote:Coverity found the following issue: >>> CID 1593156: Integer handling issues (OVERFLOW_BEFORE_WIDEN) >>> Potentially overflowing expression "0x10 << depth" with type "int" (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type "uint64_t" (64 bits, unsigned). 4299 depth = 16 << depth; Fix it by forcing the expression to be 64 bits wide by using '16ULL'. Resolves: Coverity CID 1593156 Fixes: c48bd18eae ("target/riscv: Add support for Control Transfer Records extension CSRs.") Signed-off-by: Daniel Henrique Barboza <dbarb...@ventanamicro.com> --- target/riscv/csr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/target/riscv/csr.c b/target/riscv/csr.c index 0ebcca4597..e832ff3ca9 100644 --- a/target/riscv/csr.c +++ b/target/riscv/csr.c @@ -4296,7 +4296,7 @@ static RISCVException rmw_sctrdepth(CPURISCVState *env, int csrno, } /* Update sctrstatus.WRPTR with a legal value */ - depth = 16 << depth; + depth = 16ULL << depth; env->sctrstatus = env->sctrstatus & (~SCTRSTATUS_WRPTR_MASK | (depth - 1)); }This is a clear false-positive from Coverity, by the way: we just checked and enforced that depth is at most SCTRDEPTH_MAX, i.e. 4, and 16 << 4 cannot possibly overflow anything.
True. I wonder if we should keep this patch anyway due to the better code pattern in using ULL when left shifting into a 64 bit var, regardless of not fixing any overflows. There's a chance that we might copy/paste the existing pattern into another situation where an overflow might actually happen. I'll leave to Alistair to decide whether to keep to drop this patch. Either way works for me. Thanks, Daniel
-- PMM
