bswap.h: Fix const_le64() macro

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

Posted by Peter Maydell 10 months, 1 week ago

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) |              \
-- 
2.34.1

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

Posted by Ira Weiny 10 months, 1 week ago

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>

Thanks!

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

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

Posted by Kevin Wolf 10 months, 1 week ago

Am 22.01.2024 um 18:37 hat Peter Maydell geschrieben:
> 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>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

Posted by Thomas Huth 10 months, 1 week ago

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>

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

Posted by Philippe Mathieu-Daudé 10 months, 1 week ago

On 22/1/24 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(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>