On Wed, Feb 19, 2025 at 7:26 AM Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Tue, 18 Feb 2025 at 22:22, Stephen Longfield <slongfi...@google.com> > wrote: > > > > The problem is internal to t32_expandimm_imm, the imm intermediate > > immediate value. This value is sourced from x, which always comes from > > the return of a deposit32 call, which returns uint32_t already. > > > > It's extracted via: int imm = extract32(x, 0, 8);, so the value will be > > between 0-255 > > > > It is then multiplied by one of 1, 0x00010001, 0x01000100, 0x01010101, > > or 0x80. > > > > Values between 128-255 multiplied by 0x01000100 or 0x01010101 will cause > > the upper bit to get set, which is a signed integer overflow. From > > Chapter 6.5, paragraph 5 of the C11 spec: > > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf this is > > undefined behavior. > > QEMU always compiles with -fwrapv. This means that this integer > overflow is not undefined behaviour in our dialect of C. > > > Though this is a minor undefined behavior, I'd like to see this fixed, > > since the error is showing up when I enable clang's sanitizers while > > looking for other issues. > > If clang's sanitizer reports the overflow as UB when built with > -fwrapv, that is a bug in the sanitizer and you should get > it fixed in clang. > We use and rely on 2s complement handling of signed integers > in a lot of places, so if you try to find and fix them > all you're going to be playing a pointless game of whackamole. > Yeah, I was running with `-ftrapv` instead of `-fwrapv` looking for errors in other code. This was the only place that got flagged, but sounds like that's likely just an artifact of the test I was running. (Though, there is a vanishingly small amount of math done on `int`s in target/arm/tcg/translate.c, which also probably helps.) > > Signed-off-by: Stephen Longfield <slongfi...@google.com> > > Signed-off-by: Roque Arcudia Hernandez <roq...@google.com> > > --- > > target/arm/tcg/translate.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c > > index 68ac393415..8770f0ce1c 100644 > > --- a/target/arm/tcg/translate.c > > +++ b/target/arm/tcg/translate.c > > @@ -3508,9 +3508,9 @@ static int t32_expandimm_rot(DisasContext *s, int > x) > > } > > > > /* Return the unrotated immediate from T32ExpandImm. */ > > -static int t32_expandimm_imm(DisasContext *s, int x) > > +static uint32_t t32_expandimm_imm(DisasContext *s, uint32_t x) > > This function is following the API for decodetree !function > filters, which return 'int', not 'uint32_t'. > > > { > > - int imm = extract32(x, 0, 8); > > + uint32_t imm = extract32(x, 0, 8); > > Given what we're doing in the function, it is reasonable > to make this a uint32_t, though. > Changing this to uint32_t is sufficient for me. I'll send out a v2 of the patch. > > > > switch (extract32(x, 8, 4)) { > > case 0: /* XY */ > > thanks > -- PMM > Thank you for your comprehensive and quick feedback! --Stephen
