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.
> 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.
>
> switch (extract32(x, 8, 4)) {
> case 0: /* XY */
thanks
-- PMM
