Bug 219203 - xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_next_rec) causes error in C++ compilations
Summary: xfsprogs-6.10.0: missing cast in /usr/include/xfs/xfs_fs.h(xfs_getparents_nex...
Status: NEW
Alias: None
Product: File System
Classification: Unclassified
Component: XFS (show other bugs)
Hardware: All Linux
: P3 normal
Assignee: FileSystem/XFS Default Virtual Assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-08-27 21:07 UTC by Matt Whitlock
Modified: 2024-08-28 00:10 UTC (History)
1 user (show)

See Also:
Kernel Version:
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Matt Whitlock 2024-08-27 21:07:03 UTC
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;
 }
 
 /* Iterate through this file handle's directory parent pointers. */
Comment 1 Dave Chinner 2024-08-27 21:40:21 UTC
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.
Comment 2 Darrick J. Wong 2024-08-27 22:30:54 UTC
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
>
Comment 3 Matt Whitlock 2024-08-27 23:12:50 UTC
(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.
Comment 4 Dave Chinner 2024-08-28 00:10:32 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.