On 11 Apr 2022 at 06:01, Dave Chinner wrote:
> 5.18 w/ std=gnu11 compiled with gcc-5 wants flags stored in unsigned
> fields to be unsigned. This manifests as a compiler error such as:
>
> /kisskb/src/fs/xfs/./xfs_trace.h:432:2: note: in expansion of macro 'TP_printk'
> TP_printk("dev %d:%d daddr 0x%llx bbcount 0x%x hold %d pincount %d "
> ^
> /kisskb/src/fs/xfs/./xfs_trace.h:440:5: note: in expansion of macro '__print_flags'
> __print_flags(__entry->flags, "|", XFS_BUF_FLAGS),
> ^
> /kisskb/src/fs/xfs/xfs_buf.h:67:4: note: in expansion of macro 'XBF_UNMAPPED'
> { XBF_UNMAPPED, "UNMAPPED" }
> ^
> /kisskb/src/fs/xfs/./xfs_trace.h:440:40: note: in expansion of macro 'XFS_BUF_FLAGS'
> __print_flags(__entry->flags, "|", XFS_BUF_FLAGS),
> ^
> /kisskb/src/fs/xfs/./xfs_trace.h: In function 'trace_raw_output_xfs_buf_flags_class':
> /kisskb/src/fs/xfs/xfs_buf.h:46:23: error: initializer element is not constant
> #define XBF_UNMAPPED (1 << 31)/* do not map the buffer */
>
> as __print_flags assigns XFS_BUF_FLAGS to a structure that uses an
> unsigned long for the flag. Since this results in the value of
> XBF_UNMAPPED causing a signed integer overflow, the result is
> technically undefined behavior, which gcc-5 does not accept as an
> integer constant.
>
> This is based on a patch from Arnd Bergman <arnd@xxxxxxxx>.
>
Although the underlying data type is the same, may be the type of the fifth
argument to xfs_trans_get_buf() should be changed to xfs_buf_flags_t as well.
With that fixed,
Reviewed-by: Chandan Babu R <chandan.babu@xxxxxxxxxx>
> Reported-by: Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_buf.c | 6 +++---
> fs/xfs/xfs_buf.h | 42 +++++++++++++++++++++---------------------
> 2 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index e1afb9e503e1..bf4e60871068 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -406,7 +406,7 @@ xfs_buf_alloc_pages(
> STATIC int
> _xfs_buf_map_pages(
> struct xfs_buf *bp,
> - uint flags)
> + xfs_buf_flags_t flags)
> {
> ASSERT(bp->b_flags & _XBF_PAGES);
> if (bp->b_page_count == 1) {
> @@ -868,7 +868,7 @@ xfs_buf_read_uncached(
> struct xfs_buftarg *target,
> xfs_daddr_t daddr,
> size_t numblks,
> - int flags,
> + xfs_buf_flags_t flags,
> struct xfs_buf **bpp,
> const struct xfs_buf_ops *ops)
> {
> @@ -903,7 +903,7 @@ int
> xfs_buf_get_uncached(
> struct xfs_buftarg *target,
> size_t numblks,
> - int flags,
> + xfs_buf_flags_t flags,
> struct xfs_buf **bpp)
> {
> int error;
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index edcb6254fa6a..1ee3056ff9cf 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -22,28 +22,28 @@ struct xfs_buf;
>
> #define XFS_BUF_DADDR_NULL ((xfs_daddr_t) (-1LL))
>
> -#define XBF_READ (1 << 0) /* buffer intended for reading from device */
> -#define XBF_WRITE (1 << 1) /* buffer intended for writing to device */
> -#define XBF_READ_AHEAD (1 << 2) /* asynchronous read-ahead */
> -#define XBF_NO_IOACCT (1 << 3) /* bypass I/O accounting (non-LRU bufs) */
> -#define XBF_ASYNC (1 << 4) /* initiator will not wait for completion */
> -#define XBF_DONE (1 << 5) /* all pages in the buffer uptodate */
> -#define XBF_STALE (1 << 6) /* buffer has been staled, do not find it */
> -#define XBF_WRITE_FAIL (1 << 7) /* async writes have failed on this buffer */
> +#define XBF_READ (1u << 0) /* buffer intended for reading from device */
> +#define XBF_WRITE (1u << 1) /* buffer intended for writing to device */
> +#define XBF_READ_AHEAD (1u << 2) /* asynchronous read-ahead */
> +#define XBF_NO_IOACCT (1u << 3) /* bypass I/O accounting (non-LRU bufs) */
> +#define XBF_ASYNC (1u << 4) /* initiator will not wait for completion */
> +#define XBF_DONE (1u << 5) /* all pages in the buffer uptodate */
> +#define XBF_STALE (1u << 6) /* buffer has been staled, do not find it */
> +#define XBF_WRITE_FAIL (1u << 7) /* async writes have failed on this buffer */
>
> /* buffer type flags for write callbacks */
> -#define _XBF_INODES (1 << 16)/* inode buffer */
> -#define _XBF_DQUOTS (1 << 17)/* dquot buffer */
> -#define _XBF_LOGRECOVERY (1 << 18)/* log recovery buffer */
> +#define _XBF_INODES (1u << 16)/* inode buffer */
> +#define _XBF_DQUOTS (1u << 17)/* dquot buffer */
> +#define _XBF_LOGRECOVERY (1u << 18)/* log recovery buffer */
>
> /* flags used only internally */
> -#define _XBF_PAGES (1 << 20)/* backed by refcounted pages */
> -#define _XBF_KMEM (1 << 21)/* backed by heap memory */
> -#define _XBF_DELWRI_Q (1 << 22)/* buffer on a delwri queue */
> +#define _XBF_PAGES (1u << 20)/* backed by refcounted pages */
> +#define _XBF_KMEM (1u << 21)/* backed by heap memory */
> +#define _XBF_DELWRI_Q (1u << 22)/* buffer on a delwri queue */
>
> /* flags used only as arguments to access routines */
> -#define XBF_TRYLOCK (1 << 30)/* lock requested, but do not wait */
> -#define XBF_UNMAPPED (1 << 31)/* do not map the buffer */
> +#define XBF_TRYLOCK (1u << 30)/* lock requested, but do not wait */
> +#define XBF_UNMAPPED (1u << 31)/* do not map the buffer */
>
> typedef unsigned int xfs_buf_flags_t;
>
> @@ -58,7 +58,7 @@ typedef unsigned int xfs_buf_flags_t;
> { XBF_WRITE_FAIL, "WRITE_FAIL" }, \
> { _XBF_INODES, "INODES" }, \
> { _XBF_DQUOTS, "DQUOTS" }, \
> - { _XBF_LOGRECOVERY, "LOG_RECOVERY" }, \
> + { _XBF_LOGRECOVERY, "LOG_RECOVERY" }, \
> { _XBF_PAGES, "PAGES" }, \
> { _XBF_KMEM, "KMEM" }, \
> { _XBF_DELWRI_Q, "DELWRI_Q" }, \
> @@ -247,11 +247,11 @@ xfs_buf_readahead(
> return xfs_buf_readahead_map(target, &map, 1, ops);
> }
>
> -int xfs_buf_get_uncached(struct xfs_buftarg *target, size_t numblks, int flags,
> - struct xfs_buf **bpp);
> +int xfs_buf_get_uncached(struct xfs_buftarg *target, size_t numblks,
> + xfs_buf_flags_t flags, struct xfs_buf **bpp);
> int xfs_buf_read_uncached(struct xfs_buftarg *target, xfs_daddr_t daddr,
> - size_t numblks, int flags, struct xfs_buf **bpp,
> - const struct xfs_buf_ops *ops);
> + size_t numblks, xfs_buf_flags_t flags, struct xfs_buf **bpp,
> + const struct xfs_buf_ops *ops);
> int _xfs_buf_read(struct xfs_buf *bp, xfs_buf_flags_t flags);
> void xfs_buf_hold(struct xfs_buf *bp);
--
chandan
