Re: [PATCH] bswap.h: Fix const_le64() macro

[Thomas Huth <thuth@redhat.com>]

On 22/01/2024 18.37, Peter Maydell wrote:
> The const_le64() macro introduced in commit 845d80a8c7b187 turns out
> to have a bug which means that on big-endian systems the compiler
> complains if the argument isn't already a 64-bit type. This hasn't
> caused a problem yet, because there are no in-tree uses, but it
> means it's not possible for anybody to add one without it failing CI.
> 
> This example is from an attempted use of it with the argument '0',
> from the s390 CI runner's gcc:
> 
> ../block/blklogwrites.c: In function ‘blk_log_writes_co_do_log’:
> ../include/qemu/bswap.h:148:36: error: left shift count >= width of
> type [-Werror=shift-count-overflow]
> 148 | ((((_x) & 0x00000000000000ffU) << 56) | \
> | ^~
> ../block/blklogwrites.c:409:27: note: in expansion of macro ‘const_le64’
> 409 | .nr_entries = const_le64(0),
> | ^~~~~~~~~~
> ../include/qemu/bswap.h:149:36: error: left shift count >= width of
> type [-Werror=shift-count-overflow]
> 149 | (((_x) & 0x000000000000ff00U) << 40) | \
> | ^~
> ../block/blklogwrites.c:409:27: note: in expansion of macro ‘const_le64’
> 409 | .nr_entries = const_le64(0),
> | ^~~~~~~~~~
> cc1: all warnings being treated as errors
> 
> Fix this by making all the constants in the macro have the ULL
> suffix.  This will cause them all to be 64-bit integers, which means
> the result of the logical & will also be an unsigned 64-bit type,
> even if the input to the macro is a smaller type, and so the shifts
> will be in range.
> 
> Fixes: 845d80a8c7b187 ("qemu/bswap: Add const_le64()")
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Tested 'by hand' on the s390 box that was complaining about
> Kevin's pullreq.
> ---
>   include/qemu/bswap.h | 16 ++++++++--------
>   1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h
> index 933a66ee87e..bd67468e5e4 100644
> --- a/include/qemu/bswap.h
> +++ b/include/qemu/bswap.h
> @@ -145,14 +145,14 @@ CPU_CONVERT(le, 64, uint64_t)
>    */
>   #if HOST_BIG_ENDIAN
>   # define const_le64(_x)                          \
> -    ((((_x) & 0x00000000000000ffU) << 56) |      \
> -     (((_x) & 0x000000000000ff00U) << 40) |      \
> -     (((_x) & 0x0000000000ff0000U) << 24) |      \
> -     (((_x) & 0x00000000ff000000U) <<  8) |      \
> -     (((_x) & 0x000000ff00000000U) >>  8) |      \
> -     (((_x) & 0x0000ff0000000000U) >> 24) |      \
> -     (((_x) & 0x00ff000000000000U) >> 40) |      \
> -     (((_x) & 0xff00000000000000U) >> 56))
> +    ((((_x) & 0x00000000000000ffULL) << 56) |    \
> +     (((_x) & 0x000000000000ff00ULL) << 40) |    \
> +     (((_x) & 0x0000000000ff0000ULL) << 24) |    \
> +     (((_x) & 0x00000000ff000000ULL) <<  8) |    \
> +     (((_x) & 0x000000ff00000000ULL) >>  8) |    \
> +     (((_x) & 0x0000ff0000000000ULL) >> 24) |    \
> +     (((_x) & 0x00ff000000000000ULL) >> 40) |    \
> +     (((_x) & 0xff00000000000000ULL) >> 56))
>   # define const_le32(_x)                          \
>       ((((_x) & 0x000000ffU) << 24) |              \
>        (((_x) & 0x0000ff00U) <<  8) |              \

Reviewed-by: Thomas Huth <thuth@redhat.com>