* Re: [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
2024-08-27 21:07 [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations bugzilla-daemon
@ 2024-08-27 21:40 ` Dave Chinner
2024-08-27 22:30 ` Darrick J. Wong
2024-08-27 21:40 ` [Bug 219203] " bugzilla-daemon
` (3 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Dave Chinner @ 2024-08-27 21:40 UTC (permalink / raw)
To: bugzilla-daemon; +Cc: linux-xfs
On Tue, Aug 27, 2024 at 09:07:03PM +0000, bugzilla-daemon@kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=219203
>
> Bug ID: 219203
> Summary: xfsprogs-6.10.0: missing cast in
> /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec)
> causes error in C++ compilations
> Product: File System
> Version: 2.5
> Hardware: All
> OS: Linux
> Status: NEW
> Severity: normal
> Priority: P3
> Component: XFS
> Assignee: filesystem_xfs@kernel-bugs.kernel.org
> Reporter: kernel@mattwhitlock.name
> Regression: No
>
> C allows implicit casts from void* to any pointer type, but C++ does not. Thus,
> when including <xfs/xfs_fs.h> in a C++ compilation unit, the compiler raises
> this error:
>
> /usr/include/xfs/xfs_fs.h: In function 'xfs_getparents_rec*
> xfs_getparents_next_rec(xfs_getparents*, xfs_getparents_rec*)':
> /usr/include/xfs/xfs_fs.h:915:16: error: invalid conversion from 'void*' to
> 'xfs_getparents_rec*' [-fpermissive]
> 915 | return next;
> | ^~~~
> | |
> | void*
>
>
> The return statement in xfs_getparents_next_rec() should have used an explicit
> cast, as the return statement in xfs_getparents_first_rec() does.
>
> --- /usr/include/xfs/xfs_fs.h
> +++ /usr/include/xfs/xfs_fs.h
> @@ -912,7 +912,7 @@
> if (next >= end)
> return NULL;
>
> - return next;
> + return (struct xfs_getparents_rec *)next;
> }
We shouldn't be putting static inline code in xfs_fs.h. That header
file is purely for kernel API definitions. Iterator helper functions
aren't part of the kernel API definition - they should be in some
other exported header file if they are needed at all. The helpers
could be defined in the getparents man page in the example code that
uses them rather than exposing the C code to the world...
I note that we've recently added a static inline function type
checking function to xfs_types.h rather than it being an external
function declaration, so there's more than one header file that
needs cleanup....
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
2024-08-27 21:40 ` Dave Chinner
@ 2024-08-27 22:30 ` Darrick J. Wong
2024-08-28 0:10 ` Dave Chinner
0 siblings, 1 reply; 8+ messages in thread
From: Darrick J. Wong @ 2024-08-27 22:30 UTC (permalink / raw)
To: Dave Chinner; +Cc: bugzilla-daemon, linux-xfs
On Wed, Aug 28, 2024 at 07:40:15AM +1000, Dave Chinner wrote:
> On Tue, Aug 27, 2024 at 09:07:03PM +0000, bugzilla-daemon@kernel.org wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=219203
> >
> > Bug ID: 219203
> > Summary: xfsprogs-6.10.0: missing cast in
> > /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec)
> > causes error in C++ compilations
> > Product: File System
> > Version: 2.5
> > Hardware: All
> > OS: Linux
> > Status: NEW
> > Severity: normal
> > Priority: P3
> > Component: XFS
> > Assignee: filesystem_xfs@kernel-bugs.kernel.org
> > Reporter: kernel@mattwhitlock.name
> > Regression: No
> >
> > C allows implicit casts from void* to any pointer type, but C++ does not. Thus,
> > when including <xfs/xfs_fs.h> in a C++ compilation unit, the compiler raises
> > this error:
> >
> > /usr/include/xfs/xfs_fs.h: In function 'xfs_getparents_rec*
> > xfs_getparents_next_rec(xfs_getparents*, xfs_getparents_rec*)':
> > /usr/include/xfs/xfs_fs.h:915:16: error: invalid conversion from 'void*' to
> > 'xfs_getparents_rec*' [-fpermissive]
> > 915 | return next;
> > | ^~~~
> > | |
> > | void*
> >
> >
> > The return statement in xfs_getparents_next_rec() should have used an explicit
> > cast, as the return statement in xfs_getparents_first_rec() does.
> >
> > --- /usr/include/xfs/xfs_fs.h
> > +++ /usr/include/xfs/xfs_fs.h
> > @@ -912,7 +912,7 @@
> > if (next >= end)
> > return NULL;
> >
> > - return next;
> > + return (struct xfs_getparents_rec *)next;
> > }
>
> We shouldn't be putting static inline code in xfs_fs.h. That header
> file is purely for kernel API definitions. Iterator helper functions
> aren't part of the kernel API definition - they should be in some
> other exported header file if they are needed at all. The helpers
> could be defined in the getparents man page in the example code that
> uses them rather than exposing the C code to the world...
>
> I note that we've recently added a static inline function type
> checking function to xfs_types.h rather than it being an external
> function declaration, so there's more than one header file that
> needs cleanup....
XFS has been exporting tons of static inline functions via xfslibs-dev
for ages:
$ grep static.*inline /usr/include/xfs/ | wc -l
103
And the kernel itself has been doing that for years:
$ grep static.*inline /usr/include/linux/ | wc -l
348
...most of which don't trip g++ errors. This was the first thing that
broke a build somewhere, because neither the kernel nor xfsprogs use
the C++ compiler.
Shouldn't code review have caught these kinds of problems? Why wasn't
there any automated testing to identify these issues before they got
merged? How many more guardrails do I get to build??
Or: should we add a dummy cpp source file to xfsprogs that includes
xfs.h so that developers have some chance of finding potential C++
errors sooner than 6 weeks after the kernel release?
So far as I can tell, this fixes the c++ compilation errors, at least
with gcc 12:
diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
index 0613239cad13..8fc305cce06b 100644
--- a/libxfs/xfs_fs.h
+++ b/libxfs/xfs_fs.h
@@ -971,13 +971,13 @@ static inline struct xfs_getparents_rec *
xfs_getparents_next_rec(struct xfs_getparents *gp,
struct xfs_getparents_rec *gpr)
{
- void *next = ((void *)gpr + gpr->gpr_reclen);
+ void *next = ((char *)gpr + gpr->gpr_reclen);
void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
if (next >= end)
return NULL;
- return next;
+ return (struct xfs_getparents_rec *)next;
}
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
2024-08-27 22:30 ` Darrick J. Wong
@ 2024-08-28 0:10 ` Dave Chinner
0 siblings, 0 replies; 8+ messages in thread
From: Dave Chinner @ 2024-08-28 0:10 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: bugzilla-daemon, linux-xfs
On Tue, Aug 27, 2024 at 03:30:52PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 28, 2024 at 07:40:15AM +1000, Dave Chinner wrote:
> > On Tue, Aug 27, 2024 at 09:07:03PM +0000, bugzilla-daemon@kernel.org wrote:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=219203
> > >
> > > Bug ID: 219203
> > > Summary: xfsprogs-6.10.0: missing cast in
> > > /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec)
> > > causes error in C++ compilations
> > > Product: File System
> > > Version: 2.5
> > > Hardware: All
> > > OS: Linux
> > > Status: NEW
> > > Severity: normal
> > > Priority: P3
> > > Component: XFS
> > > Assignee: filesystem_xfs@kernel-bugs.kernel.org
> > > Reporter: kernel@mattwhitlock.name
> > > Regression: No
> > >
> > > C allows implicit casts from void* to any pointer type, but C++ does not. Thus,
> > > when including <xfs/xfs_fs.h> in a C++ compilation unit, the compiler raises
> > > this error:
> > >
> > > /usr/include/xfs/xfs_fs.h: In function 'xfs_getparents_rec*
> > > xfs_getparents_next_rec(xfs_getparents*, xfs_getparents_rec*)':
> > > /usr/include/xfs/xfs_fs.h:915:16: error: invalid conversion from 'void*' to
> > > 'xfs_getparents_rec*' [-fpermissive]
> > > 915 | return next;
> > > | ^~~~
> > > | |
> > > | void*
> > >
> > >
> > > The return statement in xfs_getparents_next_rec() should have used an explicit
> > > cast, as the return statement in xfs_getparents_first_rec() does.
> > >
> > > --- /usr/include/xfs/xfs_fs.h
> > > +++ /usr/include/xfs/xfs_fs.h
> > > @@ -912,7 +912,7 @@
> > > if (next >= end)
> > > return NULL;
> > >
> > > - return next;
> > > + return (struct xfs_getparents_rec *)next;
> > > }
> >
> > We shouldn't be putting static inline code in xfs_fs.h. That header
> > file is purely for kernel API definitions. Iterator helper functions
> > aren't part of the kernel API definition - they should be in some
> > other exported header file if they are needed at all. The helpers
> > could be defined in the getparents man page in the example code that
> > uses them rather than exposing the C code to the world...
> >
> > I note that we've recently added a static inline function type
> > checking function to xfs_types.h rather than it being an external
> > function declaration, so there's more than one header file that
> > needs cleanup....
>
> XFS has been exporting tons of static inline functions via xfslibs-dev
> for ages:
>
> $ grep static.*inline /usr/include/xfs/ | wc -l
> 103
Yes, but have you looked at what they are?
$ grep -Rl static.*inline /usr/include/xfs/
/usr/include/xfs/linux.h
/usr/include/xfs/xfs_arch.h
/usr/include/xfs/xfs_da_format.h
/usr/include/xfs/xfs_format.h
/usr/include/xfs/xfs_fs.h
/usr/include/xfs/xfs_log_format.h
/usr/include/xfs/xfs_types.h
$
The platform functions (linux.h) are explicitly
shipped as static inlines to avoid needing a compiled library to
use.
The byte swapping (xfs_arch.h) is a mess of macros and static
inlines that have no external dependencies. They are simply byte
swapping functions.
The on-disk format headers contain some helper functions. We try to
keep them out of htose headers because of the wide includes they get
in kernel and that creates header dependency hell. Hence the only
static inline code in them allowed is code that only uses structures
already specifically defined in the on-disk format header functions.
This avoids the dependency hell that these foundational header files
might otherwise create.
Further, they have nothing to do with the actual kernel API or
fundamental XFS type definitions, so very few applications are
actually using these header files. Hence it's not critical to keep
code out of them, and we -never- care if they fail to compile with
c++ compilers because they are aren't for general application
usage....
That leaves xfs_fs.h and xfs_types.h:
$ grep static.*inline libxfs/xfs_fs.h libxfs/xfs_types.h
libxfs/xfs_fs.h:static inline uint32_t
libxfs/xfs_fs.h:static inline struct xfs_getparents_rec *
libxfs/xfs_fs.h:static inline struct xfs_getparents_rec *
libxfs/xfs_types.h:static inline bool
$
There are -four- functions in 6.10, up from 2 in 6.9. These 2 new
helper functions are the source of the problem. IOWs, our general
rule to keep code out of these two files has largely worked over the
years...
> And the kernel itself has been doing that for years:
>
> $ grep static.*inline /usr/include/linux/ | wc -l
> 348
Direct kernel uapi header includes are a different issue and the
policy for them is not in our control at all.
> ...most of which don't trip g++ errors. This was the first thing that
> broke a build somewhere, because neither the kernel nor xfsprogs use
> the C++ compiler.
>
> Shouldn't code review have caught these kinds of problems? Why wasn't
> there any automated testing to identify these issues before they got
> merged? How many more guardrails do I get to build??
We've used the "no code in xfs_fs.h and xfs_types.h" rule to avoid
these problems in the past. The whole point of xfs_types.c existing
is to provide a location for type verifier code that isn't
xfs_types.h. The verification code has dependencies on xfs_mount,
xfs_perag, etc, and we simply cannot put such code in xfs_types.h
because it creates circular header file dependencies.
IOWs, xfs_fs.h and xfs_types.h are foundational definitions for both
kernel and userspace code. Adding static inline code into them leads
to external header dependencies, and that is something we have
always tried our best to avoid. Hence the general rule is no static
inline code in these header files.
Clearly this was missed in review - it's a tiny chunk of code in the
massive merges that have recently occurred, and so it's no surprise
that little things like this get missed when reviewing tens of
thousands of lines of new code.
> Or: should we add a dummy cpp source file to xfsprogs that includes
> xfs.h so that developers have some chance of finding potential C++
> errors sooner than 6 weeks after the kernel release?
Put whatever thing you want in place to catch header compilation
issues in userspace, but the basic, fundamental principle is that no
static inline code should be placed in xfs_types.h and xfs_fs.h.
> So far as I can tell, this fixes the c++ compilation errors, at least
> with gcc 12:
>
> diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
> index 0613239cad13..8fc305cce06b 100644
> --- a/libxfs/xfs_fs.h
> +++ b/libxfs/xfs_fs.h
> @@ -971,13 +971,13 @@ static inline struct xfs_getparents_rec *
> xfs_getparents_next_rec(struct xfs_getparents *gp,
> struct xfs_getparents_rec *gpr)
> {
> - void *next = ((void *)gpr + gpr->gpr_reclen);
> + void *next = ((char *)gpr + gpr->gpr_reclen);
> void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
>
> if (next >= end)
> return NULL;
>
> - return next;
> + return (struct xfs_getparents_rec *)next;
> }
Sure, that fixes the compilation problem, but that's not the bug
that needs to be fixed. This code should not have been added to this
file in the first place.
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Bug 219203] xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
2024-08-27 21:07 [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations bugzilla-daemon
2024-08-27 21:40 ` Dave Chinner
@ 2024-08-27 21:40 ` bugzilla-daemon
2024-08-27 22:30 ` bugzilla-daemon
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: bugzilla-daemon @ 2024-08-27 21:40 UTC (permalink / raw)
To: linux-xfs
https://bugzilla.kernel.org/show_bug.cgi?id=219203
--- Comment #1 from Dave Chinner (david@fromorbit.com) ---
On Tue, Aug 27, 2024 at 09:07:03PM +0000, bugzilla-daemon@kernel.org wrote:
> https://bugzilla.kernel.org/show_bug.cgi?id=219203
>
> Bug ID: 219203
> Summary: xfsprogs-6.10.0: missing cast in
> /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec)
> causes error in C++ compilations
> Product: File System
> Version: 2.5
> Hardware: All
> OS: Linux
> Status: NEW
> Severity: normal
> Priority: P3
> Component: XFS
> Assignee: filesystem_xfs@kernel-bugs.kernel.org
> Reporter: kernel@mattwhitlock.name
> Regression: No
>
> C allows implicit casts from void* to any pointer type, but C++ does not.
> Thus,
> when including <xfs/xfs_fs.h> in a C++ compilation unit, the compiler raises
> this error:
>
> /usr/include/xfs/xfs_fs.h: In function 'xfs_getparents_rec*
> xfs_getparents_next_rec(xfs_getparents*, xfs_getparents_rec*)':
> /usr/include/xfs/xfs_fs.h:915:16: error: invalid conversion from 'void*' to
> 'xfs_getparents_rec*' [-fpermissive]
> 915 | return next;
> | ^~~~
> | |
> | void*
>
>
> The return statement in xfs_getparents_next_rec() should have used an
> explicit
> cast, as the return statement in xfs_getparents_first_rec() does.
>
> --- /usr/include/xfs/xfs_fs.h
> +++ /usr/include/xfs/xfs_fs.h
> @@ -912,7 +912,7 @@
> if (next >= end)
> return NULL;
>
> - return next;
> + return (struct xfs_getparents_rec *)next;
> }
We shouldn't be putting static inline code in xfs_fs.h. That header
file is purely for kernel API definitions. Iterator helper functions
aren't part of the kernel API definition - they should be in some
other exported header file if they are needed at all. The helpers
could be defined in the getparents man page in the example code that
uses them rather than exposing the C code to the world...
I note that we've recently added a static inline function type
checking function to xfs_types.h rather than it being an external
function declaration, so there's more than one header file that
needs cleanup....
-Dave.
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply [flat|nested] 8+ messages in thread* [Bug 219203] xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
2024-08-27 21:07 [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations bugzilla-daemon
2024-08-27 21:40 ` Dave Chinner
2024-08-27 21:40 ` [Bug 219203] " bugzilla-daemon
@ 2024-08-27 22:30 ` bugzilla-daemon
2024-08-27 23:12 ` bugzilla-daemon
2024-08-28 0:10 ` bugzilla-daemon
4 siblings, 0 replies; 8+ messages in thread
From: bugzilla-daemon @ 2024-08-27 22:30 UTC (permalink / raw)
To: linux-xfs
https://bugzilla.kernel.org/show_bug.cgi?id=219203
--- Comment #2 from Darrick J. Wong (djwong@kernel.org) ---
On Wed, Aug 28, 2024 at 07:40:15AM +1000, Dave Chinner wrote:
> On Tue, Aug 27, 2024 at 09:07:03PM +0000, bugzilla-daemon@kernel.org wrote:
> > https://bugzilla.kernel.org/show_bug.cgi?id=219203
> >
> > Bug ID: 219203
> > Summary: xfsprogs-6.10.0: missing cast in
> > /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec)
> > causes error in C++ compilations
> > Product: File System
> > Version: 2.5
> > Hardware: All
> > OS: Linux
> > Status: NEW
> > Severity: normal
> > Priority: P3
> > Component: XFS
> > Assignee: filesystem_xfs@kernel-bugs.kernel.org
> > Reporter: kernel@mattwhitlock.name
> > Regression: No
> >
> > C allows implicit casts from void* to any pointer type, but C++ does not.
> Thus,
> > when including <xfs/xfs_fs.h> in a C++ compilation unit, the compiler
> raises
> > this error:
> >
> > /usr/include/xfs/xfs_fs.h: In function 'xfs_getparents_rec*
> > xfs_getparents_next_rec(xfs_getparents*, xfs_getparents_rec*)':
> > /usr/include/xfs/xfs_fs.h:915:16: error: invalid conversion from 'void*' to
> > 'xfs_getparents_rec*' [-fpermissive]
> > 915 | return next;
> > | ^~~~
> > | |
> > | void*
> >
> >
> > The return statement in xfs_getparents_next_rec() should have used an
> explicit
> > cast, as the return statement in xfs_getparents_first_rec() does.
> >
> > --- /usr/include/xfs/xfs_fs.h
> > +++ /usr/include/xfs/xfs_fs.h
> > @@ -912,7 +912,7 @@
> > if (next >= end)
> > return NULL;
> >
> > - return next;
> > + return (struct xfs_getparents_rec *)next;
> > }
>
> We shouldn't be putting static inline code in xfs_fs.h. That header
> file is purely for kernel API definitions. Iterator helper functions
> aren't part of the kernel API definition - they should be in some
> other exported header file if they are needed at all. The helpers
> could be defined in the getparents man page in the example code that
> uses them rather than exposing the C code to the world...
>
> I note that we've recently added a static inline function type
> checking function to xfs_types.h rather than it being an external
> function declaration, so there's more than one header file that
> needs cleanup....
XFS has been exporting tons of static inline functions via xfslibs-dev
for ages:
$ grep static.*inline /usr/include/xfs/ | wc -l
103
And the kernel itself has been doing that for years:
$ grep static.*inline /usr/include/linux/ | wc -l
348
...most of which don't trip g++ errors. This was the first thing that
broke a build somewhere, because neither the kernel nor xfsprogs use
the C++ compiler.
Shouldn't code review have caught these kinds of problems? Why wasn't
there any automated testing to identify these issues before they got
merged? How many more guardrails do I get to build??
Or: should we add a dummy cpp source file to xfsprogs that includes
xfs.h so that developers have some chance of finding potential C++
errors sooner than 6 weeks after the kernel release?
So far as I can tell, this fixes the c++ compilation errors, at least
with gcc 12:
diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
index 0613239cad13..8fc305cce06b 100644
--- a/libxfs/xfs_fs.h
+++ b/libxfs/xfs_fs.h
@@ -971,13 +971,13 @@ static inline struct xfs_getparents_rec *
xfs_getparents_next_rec(struct xfs_getparents *gp,
struct xfs_getparents_rec *gpr)
{
- void *next = ((void *)gpr + gpr->gpr_reclen);
+ void *next = ((char *)gpr + gpr->gpr_reclen);
void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
if (next >= end)
return NULL;
- return next;
+ return (struct xfs_getparents_rec *)next;
}
--D
> -Dave.
> --
> Dave Chinner
> david@fromorbit.com
>
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply related [flat|nested] 8+ messages in thread* [Bug 219203] xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
2024-08-27 21:07 [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations bugzilla-daemon
` (2 preceding siblings ...)
2024-08-27 22:30 ` bugzilla-daemon
@ 2024-08-27 23:12 ` bugzilla-daemon
2024-08-28 0:10 ` bugzilla-daemon
4 siblings, 0 replies; 8+ messages in thread
From: bugzilla-daemon @ 2024-08-27 23:12 UTC (permalink / raw)
To: linux-xfs
https://bugzilla.kernel.org/show_bug.cgi?id=219203
--- Comment #3 from Matt Whitlock (kernel@mattwhitlock.name) ---
(In reply to Darrick J. Wong from comment #2)
> - void *next = ((void *)gpr + gpr->gpr_reclen);
> + void *next = ((char *)gpr + gpr->gpr_reclen);
G++ 14 apparently didn't care about the arithmetic on a void*. Only the cast in
the return statement was necessary for libktorrent to include <xfs/xfs_fs.h>
without error. (I did not check whether the compiler emitted a warning.)
Certainly it is preferable to do arithmetic on a pointer to a type of known
size, so thanks for suggesting this other fix as well.
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply [flat|nested] 8+ messages in thread* [Bug 219203] xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
2024-08-27 21:07 [Bug 219203] New: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations bugzilla-daemon
` (3 preceding siblings ...)
2024-08-27 23:12 ` bugzilla-daemon
@ 2024-08-28 0:10 ` bugzilla-daemon
4 siblings, 0 replies; 8+ messages in thread
From: bugzilla-daemon @ 2024-08-28 0:10 UTC (permalink / raw)
To: linux-xfs
https://bugzilla.kernel.org/show_bug.cgi?id=219203
--- Comment #4 from Dave Chinner (david@fromorbit.com) ---
On Tue, Aug 27, 2024 at 03:30:52PM -0700, Darrick J. Wong wrote:
> On Wed, Aug 28, 2024 at 07:40:15AM +1000, Dave Chinner wrote:
> > On Tue, Aug 27, 2024 at 09:07:03PM +0000, bugzilla-daemon@kernel.org wrote:
> > > https://bugzilla.kernel.org/show_bug.cgi?id=219203
> > >
> > > Bug ID: 219203
> > > Summary: xfsprogs-6.10.0: missing cast in
> > > /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec)
> > > causes error in C++ compilations
> > > Product: File System
> > > Version: 2.5
> > > Hardware: All
> > > OS: Linux
> > > Status: NEW
> > > Severity: normal
> > > Priority: P3
> > > Component: XFS
> > > Assignee: filesystem_xfs@kernel-bugs.kernel.org
> > > Reporter: kernel@mattwhitlock.name
> > > Regression: No
> > >
> > > C allows implicit casts from void* to any pointer type, but C++ does not.
> Thus,
> > > when including <xfs/xfs_fs.h> in a C++ compilation unit, the compiler
> raises
> > > this error:
> > >
> > > /usr/include/xfs/xfs_fs.h: In function 'xfs_getparents_rec*
> > > xfs_getparents_next_rec(xfs_getparents*, xfs_getparents_rec*)':
> > > /usr/include/xfs/xfs_fs.h:915:16: error: invalid conversion from 'void*'
> to
> > > 'xfs_getparents_rec*' [-fpermissive]
> > > 915 | return next;
> > > | ^~~~
> > > | |
> > > | void*
> > >
> > >
> > > The return statement in xfs_getparents_next_rec() should have used an
> explicit
> > > cast, as the return statement in xfs_getparents_first_rec() does.
> > >
> > > --- /usr/include/xfs/xfs_fs.h
> > > +++ /usr/include/xfs/xfs_fs.h
> > > @@ -912,7 +912,7 @@
> > > if (next >= end)
> > > return NULL;
> > >
> > > - return next;
> > > + return (struct xfs_getparents_rec *)next;
> > > }
> >
> > We shouldn't be putting static inline code in xfs_fs.h. That header
> > file is purely for kernel API definitions. Iterator helper functions
> > aren't part of the kernel API definition - they should be in some
> > other exported header file if they are needed at all. The helpers
> > could be defined in the getparents man page in the example code that
> > uses them rather than exposing the C code to the world...
> >
> > I note that we've recently added a static inline function type
> > checking function to xfs_types.h rather than it being an external
> > function declaration, so there's more than one header file that
> > needs cleanup....
>
> XFS has been exporting tons of static inline functions via xfslibs-dev
> for ages:
>
> $ grep static.*inline /usr/include/xfs/ | wc -l
> 103
Yes, but have you looked at what they are?
$ grep -Rl static.*inline /usr/include/xfs/
/usr/include/xfs/linux.h
/usr/include/xfs/xfs_arch.h
/usr/include/xfs/xfs_da_format.h
/usr/include/xfs/xfs_format.h
/usr/include/xfs/xfs_fs.h
/usr/include/xfs/xfs_log_format.h
/usr/include/xfs/xfs_types.h
$
The platform functions (linux.h) are explicitly
shipped as static inlines to avoid needing a compiled library to
use.
The byte swapping (xfs_arch.h) is a mess of macros and static
inlines that have no external dependencies. They are simply byte
swapping functions.
The on-disk format headers contain some helper functions. We try to
keep them out of htose headers because of the wide includes they get
in kernel and that creates header dependency hell. Hence the only
static inline code in them allowed is code that only uses structures
already specifically defined in the on-disk format header functions.
This avoids the dependency hell that these foundational header files
might otherwise create.
Further, they have nothing to do with the actual kernel API or
fundamental XFS type definitions, so very few applications are
actually using these header files. Hence it's not critical to keep
code out of them, and we -never- care if they fail to compile with
c++ compilers because they are aren't for general application
usage....
That leaves xfs_fs.h and xfs_types.h:
$ grep static.*inline libxfs/xfs_fs.h libxfs/xfs_types.h
libxfs/xfs_fs.h:static inline uint32_t
libxfs/xfs_fs.h:static inline struct xfs_getparents_rec *
libxfs/xfs_fs.h:static inline struct xfs_getparents_rec *
libxfs/xfs_types.h:static inline bool
$
There are -four- functions in 6.10, up from 2 in 6.9. These 2 new
helper functions are the source of the problem. IOWs, our general
rule to keep code out of these two files has largely worked over the
years...
> And the kernel itself has been doing that for years:
>
> $ grep static.*inline /usr/include/linux/ | wc -l
> 348
Direct kernel uapi header includes are a different issue and the
policy for them is not in our control at all.
> ...most of which don't trip g++ errors. This was the first thing that
> broke a build somewhere, because neither the kernel nor xfsprogs use
> the C++ compiler.
>
> Shouldn't code review have caught these kinds of problems? Why wasn't
> there any automated testing to identify these issues before they got
> merged? How many more guardrails do I get to build??
We've used the "no code in xfs_fs.h and xfs_types.h" rule to avoid
these problems in the past. The whole point of xfs_types.c existing
is to provide a location for type verifier code that isn't
xfs_types.h. The verification code has dependencies on xfs_mount,
xfs_perag, etc, and we simply cannot put such code in xfs_types.h
because it creates circular header file dependencies.
IOWs, xfs_fs.h and xfs_types.h are foundational definitions for both
kernel and userspace code. Adding static inline code into them leads
to external header dependencies, and that is something we have
always tried our best to avoid. Hence the general rule is no static
inline code in these header files.
Clearly this was missed in review - it's a tiny chunk of code in the
massive merges that have recently occurred, and so it's no surprise
that little things like this get missed when reviewing tens of
thousands of lines of new code.
> Or: should we add a dummy cpp source file to xfsprogs that includes
> xfs.h so that developers have some chance of finding potential C++
> errors sooner than 6 weeks after the kernel release?
Put whatever thing you want in place to catch header compilation
issues in userspace, but the basic, fundamental principle is that no
static inline code should be placed in xfs_types.h and xfs_fs.h.
> So far as I can tell, this fixes the c++ compilation errors, at least
> with gcc 12:
>
> diff --git a/libxfs/xfs_fs.h b/libxfs/xfs_fs.h
> index 0613239cad13..8fc305cce06b 100644
> --- a/libxfs/xfs_fs.h
> +++ b/libxfs/xfs_fs.h
> @@ -971,13 +971,13 @@ static inline struct xfs_getparents_rec *
> xfs_getparents_next_rec(struct xfs_getparents *gp,
> struct xfs_getparents_rec *gpr)
> {
> - void *next = ((void *)gpr + gpr->gpr_reclen);
> + void *next = ((char *)gpr + gpr->gpr_reclen);
> void *end = (void *)(uintptr_t)(gp->gp_buffer + gp->gp_bufsize);
>
> if (next >= end)
> return NULL;
>
> - return next;
> + return (struct xfs_getparents_rec *)next;
> }
Sure, that fixes the compilation problem, but that's not the bug
that needs to be fixed. This code should not have been added to this
file in the first place.
-Dave.
--
You may reply to this email to add a comment.
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply [flat|nested] 8+ messages in thread