On Mon, 16 Mar 2026 at 00:06, BALATON Zoltan <[email protected]> wrote: > > Coverity reports possible sign extension error (CID 1645615) when > reading DEFAULT_SC_BOTTOM_RIGHT register. It cannot happen because the > values are limited when writing the register but change the code to > match other similar registers and to avoid this warning. > > Signed-off-by: BALATON Zoltan <[email protected]> > --- > hw/display/ati.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/display/ati.c b/hw/display/ati.c > index 1a6a5ad4fd..d194f15002 100644 > --- a/hw/display/ati.c > +++ b/hw/display/ati.c > @@ -513,8 +513,8 @@ static uint64_t ati_mm_read(void *opaque, hwaddr addr, > unsigned int size) > val |= s->regs.default_tile << 16; > break; > case DEFAULT_SC_BOTTOM_RIGHT: > - val = (s->regs.default_sc_bottom << 16) | > - s->regs.default_sc_right; > + val = s->regs.default_sc_right; > + val |= s->regs.default_sc_bottom << 16;
I don't think this is going to make Coverity any happier, because we're still doing: * take a value in a 16-bit unsigned type * shift it without a cast, giving a 'signed int' (32-bit) * promote that to 64-bits to do the |=, because 'val' is 64 bits, causing a potential sign extension The rephrasing of the code doesn't avoid the sign-extension. Indeed there are other (previously dismissed as false positives years ago) issues in the coverity DB for the other cases in this switch where we put together values like this without a cast. But as you say this is a "can't happen" case for this register, so I've already marked CID 1645615 as a false positive. We should do whatever we think is reasonable and readable for this code. -- PMM
