On Mon, Apr 04, 2022 at 01:45:05PM +0200, Arnd Bergmann wrote:
> On Mon, Apr 4, 2022 at 12:19 PM Geert Uytterhoeven <[email protected]> wrote:
>
> > >
> > > /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 */
> > >
> > > This doesn't make a whole lotta sense to me. It's blown up in a
> > > tracepoint macro in XFS that was not changed at all in 5.18-rc1, nor
> > > was any of the surrounding XFS code or contexts. Perhaps something
> > > outside XFS changed to cause this on these platforms?
> >
> > Upon closer look, all builds showing this issue are using gcc-5...
> >
> > > Can you bisect this, please?
> >
> > Fortunately I still have gcc-5 installed on an older machine,
> > and I could reproduce the issue on amd64 with
> > "make allmodconfig fs/xfs/xfs_trace.o".
> >
> > Bisection points to commit e8c07082a810fbb9 ("Kbuild: move to
> > -std=gnu11").
> >
> > [1] gcc version 5.5.0 20171010 (Ubuntu 5.5.0-12ubuntu1
>
> Thanks for the report. I've produced it and can see that the problem
> is assigning
> the value of "(1 << 31)" to an 'unsigned long' struct member. Since this is
> a signed integer overflow, the result is technically undefined behavior,
> which gcc-5 does not accept as an integer constant.
>
> The patch below fixes it for me, but I have not checked if there are any
> other instances. This could also be done using the 'BIT()' macro if the
> XFS maintainers prefer:
So XFS only uses these flags in unsigned int fields that are
typed via:
typedef unsigned int xfs_buf_flags_t;
So on the surface, declaring the flag values as ULONG and then writing
them into a UINT field is not a nice thing to be doing.
I really don't want to change the xfs_buf_flags_t type to an
unsigned long, because that changes the packing of the first
cacheline of the struct xfs_buf and the contents of that cacheline
are performance critical for the lookup fastpath....
Looking at __print_flags, the internal array type declaration is:
struct trace_print_flags {
unsigned long mask;
const char *name;
};
and that's the source of the problem. I notice __print_flags_u64()
exists, but __print_flags_u32() does not. Should it?
Cheers,
Dave.
--
Dave Chinner
[email protected]
Hi Dave,
On Tue, Apr 5, 2022 at 12:16 AM Dave Chinner <[email protected]> wrote:
> On Mon, Apr 04, 2022 at 01:45:05PM +0200, Arnd Bergmann wrote:
> > On Mon, Apr 4, 2022 at 12:19 PM Geert Uytterhoeven <[email protected]> wrote:
> > > > /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 */
> > > >
> > > > This doesn't make a whole lotta sense to me. It's blown up in a
> > > > tracepoint macro in XFS that was not changed at all in 5.18-rc1, nor
> > > > was any of the surrounding XFS code or contexts. Perhaps something
> > > > outside XFS changed to cause this on these platforms?
> > >
> > > Upon closer look, all builds showing this issue are using gcc-5...
> > >
> > > > Can you bisect this, please?
> > >
> > > Fortunately I still have gcc-5 installed on an older machine,
> > > and I could reproduce the issue on amd64 with
> > > "make allmodconfig fs/xfs/xfs_trace.o".
> > >
> > > Bisection points to commit e8c07082a810fbb9 ("Kbuild: move to
> > > -std=gnu11").
> > >
> > > [1] gcc version 5.5.0 20171010 (Ubuntu 5.5.0-12ubuntu1
> >
> > Thanks for the report. I've produced it and can see that the problem
> > is assigning
> > the value of "(1 << 31)" to an 'unsigned long' struct member. Since this is
> > a signed integer overflow, the result is technically undefined behavior,
> > which gcc-5 does not accept as an integer constant.
> >
> > The patch below fixes it for me, but I have not checked if there are any
> > other instances. This could also be done using the 'BIT()' macro if the
> > XFS maintainers prefer:
>
> So XFS only uses these flags in unsigned int fields that are
> typed via:
>
> typedef unsigned int xfs_buf_flags_t;
>
> So on the surface, declaring the flag values as ULONG and then writing
> them into a UINT field is not a nice thing to be doing.
>
> I really don't want to change the xfs_buf_flags_t type to an
> unsigned long, because that changes the packing of the first
> cacheline of the struct xfs_buf and the contents of that cacheline
> are performance critical for the lookup fastpath....
Hence just use "1u << n" instead of "1ul << n"?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
?On Tue, Apr 5, 2022 at 8:47 AM Geert Uytterhoeven <[email protected]> wrote:
> On Tue, Apr 5, 2022 at 12:16 AM Dave Chinner <[email protected]> wrote:
> > So XFS only uses these flags in unsigned int fields that are
> > typed via:
> >
> > typedef unsigned int xfs_buf_flags_t;
> >
> > So on the surface, declaring the flag values as ULONG and then writing
> > them into a UINT field is not a nice thing to be doing.
> >
> > I really don't want to change the xfs_buf_flags_t type to an
> > unsigned long, because that changes the packing of the first
> > cacheline of the struct xfs_buf and the contents of that cacheline
> > are performance critical for the lookup fastpath....
>
> Hence just use "1u << n" instead of "1ul << n"?
Right, that avoids the error as well. I picked '1ul' to match the type of
the variable it's assigned to, but as Dave said the intended type is
'u32', so '1u' is better here.
> > Looking at __print_flags, the internal array type declaration is:
> >
> > struct trace_print_flags {
> > unsigned long mask;
> > const char *name;
> > };
> >
> > and that's the source of the problem. I notice __print_flags_u64()
> > exists, but __print_flags_u32() does not. Should it?
It's not the source of the error, as there is no signed integer
overflow when assigning an unsigned int to an unsigned long.
It may be helpful to add a __print_flags_u32(), but it's unrelated
to the problem at hand.
Arnd
On Tue, Apr 05, 2022 at 09:08:13AM +0200, Arnd Bergmann wrote:
> ?On Tue, Apr 5, 2022 at 8:47 AM Geert Uytterhoeven <[email protected]> wrote:
> > On Tue, Apr 5, 2022 at 12:16 AM Dave Chinner <[email protected]> wrote:
> > > So XFS only uses these flags in unsigned int fields that are
> > > typed via:
> > >
> > > typedef unsigned int xfs_buf_flags_t;
> > >
> > > So on the surface, declaring the flag values as ULONG and then writing
> > > them into a UINT field is not a nice thing to be doing.
> > >
> > > I really don't want to change the xfs_buf_flags_t type to an
> > > unsigned long, because that changes the packing of the first
> > > cacheline of the struct xfs_buf and the contents of that cacheline
> > > are performance critical for the lookup fastpath....
> >
> > Hence just use "1u << n" instead of "1ul << n"?
>
> Right, that avoids the error as well. I picked '1ul' to match the type of
> the variable it's assigned to, but as Dave said the intended type is
> 'u32', so '1u' is better here.
Ok, I'll queue up a patch to make this modification. I'll also need
to check all the other trace flags fields we print as well, as they
likely have the same issue but there's no warnings from them because
they don't use the high bit in the 32 bit field yet...
> > > Looking at __print_flags, the internal array type declaration is:
> > >
> > > struct trace_print_flags {
> > > unsigned long mask;
> > > const char *name;
> > > };
> > >
> > > and that's the source of the problem. I notice __print_flags_u64()
> > > exists, but __print_flags_u32() does not. Should it?
>
> It's not the source of the error, as there is no signed integer
> overflow when assigning an unsigned int to an unsigned long.
Thanks for the clarification!
Cheers,
Dave.
--
Dave Chinner
[email protected]