On Tue, 10 Mar 2026, Philippe Mathieu-Daudé wrote:
On 10/3/26 13:47, BALATON Zoltan wrote:
On Tue, 10 Mar 2026, Philippe Mathieu-Daudé wrote:
On 9/3/26 23:59, BALATON Zoltan wrote:
On Mon, 9 Mar 2026, Philippe Mathieu-Daudé wrote:
On 9/3/26 22:41, BALATON Zoltan wrote:
On Mon, 9 Mar 2026, Chad Jablonski wrote:
Cast default_sc_bottom to uint32_t before shifting left to prevent
sign extension when assigning to uint64_t.
Fixes: CID 1645615
Signed-off-by: Chad Jablonski <[email protected]>
---
hw/display/ati.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/display/ati.c b/hw/display/ati.c
index 6cf243bcf9..f9773e1154 100644
--- a/hw/display/ati.c
+++ b/hw/display/ati.c
@@ -514,7 +514,7 @@ 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) |
+ val = ((uint32_t)s->regs.default_sc_bottom << 16) |
s->regs.default_sc_right;
What about using deposit64() which is uncontroversial?
val = deposit64(s->regs.default_sc_right, 16,
13, s->regs.default_sc_bottom);
(alternatively deposit32)
I can't make sense of that without going and looking what the arguments
of this function are so I'd rather keep it simple. As I said, the values
here can't be large enough to sign extend and there may be other similar
cases that are already marked in Coverity. But if we still want to fix
all of them then maybe we could try to make val uint32_t then do
something about it once at the return for all cases. That seems better
than trying to handle in each case inconsistently. The simplest would
still be to tell Converity it's wrong here.
Until the next register is implemented and Coverity complains again,
and another round-up on the mailing list until Peter manually marks it.
I was hoping for a way to avoid all that hassle.
Changing local val to uint32_t and cast it at the last return line should
also solve this if the issue is assigning to uint64_t without a cast. To me
that seems better than making changes to every case separately and would
allow me to avoid additional complexity. If you think it would help I can
make a patch but I can't test it.
No more time / energy for this topic, so do what you think is best for
the project, I don't mind much anymore. I shouldn't have intervened at
all first, but since I merged this patch, I felt a bit concerned.
Actually the simplest is you mark that issue yourself on Coverity;
anybody can request an account for free. I'm done with this thread.
I did not know I can get access to Coverity. Then I can take a look there.
I still think there's nothing to fix here because even if uint16_t
default_sc_bottom is promoted to int for the calculation we don't support
32 bit hosts any more so this can't get sign extension issue even in that
case. So I think Converity is just confused here and we should not make
code more complex for it.
Regards,
BALATON Zoltan
