Re: [RFC PATCH] libxfs: compile with a C++ compiler

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Aug 28, 2024 at 01:01:31AM +0100, Sam James wrote:
> "Darrick J. Wong" <djwong@kernel.org> writes:
> 
> > From: Darrick J. Wong <djwong@kernel.org>
> >
> > Apparently C++ compilers don't like the implicit void* casts that go on
> > in the system headers.  Compile a dummy program with the C++ compiler to
> > make sure this works, so Darrick has /some/ chance of figuring these
> > things out before the users do.
> 
> Thanks, this is a good idea. Double thanks for the quick fix.
> 
> 1) yes, it finds the breakage:
> Tested-by: Sam James <sam@gentoo.org>
> 
> 2) with the fix below (CC -> CXX):
> Reviewed-by: Sam James <sam@gentoo.org>
> 
> 3) another thing to think about is:
> * -pedantic?

-pedantic won't build because C++ doesn't support flexarrays:

In file included from ../include/xfs.h:61:
../include/xfs/xfs_fs.h:523:33: error: ISO C++ forbids flexible array member ‘bulkstat’ [-Werror=pedantic]
  523 |         struct xfs_bulkstat     bulkstat[];
      |                                 ^~~~~~~~

even if you wrap it in extern "C" { ... };

> * maybe do one for a bunch of standards? (I think systemd does every
> possible value [1])

That might be overkill since xfsprogs' build system doesn't have a good
mechanism for detecting if a compiler supports a particular standard.
I'm not even sure there's a good "reference" C++ standard to pick here,
since the kernel doesn't require a C++ compiler.

> * doing the above for C as well

Hmm, that's a good idea.

I think the only relevant standard here is C11 (well really gnu11),
because that's what the kernel compiles with since 5.18.  xfsprogs
doesn't specify any particular version of C, but perhaps we should match
the kernel every time they bump that up?

IOWs, should we build xfsprogs with -std=gnu11?  The commit changing the
kernel to gnu11 (e8c07082a810) remarks that gcc 5.1 supports it just
fine.  IIRC RHEL 7 only has 4.8.5 but it's now in extended support so
... who cares?  The oldest supported Debian stable has gcc 8.

--D

> I know that sounds a bit like overkill, but systemd
> does it and it's cheap to do it versus the blowup if something goes
> wrong. I don't have a strong opinion on this or how far you want to go
> with it.
> 
> [1] https://github.com/systemd/systemd/blob/3317aedff0901e08a8efc8346ad76b184d5d40ea/src/systemd/meson.build#L60
> 
> >
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  configure.ac         |    1 +
> >  include/builddefs.in |    7 +++++++
> >  libxfs/Makefile      |    8 +++++++-
> >  libxfs/dummy.cpp     |   15 +++++++++++++++
> >  4 files changed, 30 insertions(+), 1 deletion(-)
> >  create mode 100644 libxfs/dummy.cpp
> >
> > diff --git a/configure.ac b/configure.ac
> > index 0ffe2e5dfc53..04544f85395b 100644
> > --- a/configure.ac
> > +++ b/configure.ac
> > @@ -9,6 +9,7 @@ AC_PROG_INSTALL
> >  LT_INIT
> >  
> >  AC_PROG_CC
> > +AC_PROG_CXX
> >  AC_ARG_VAR(BUILD_CC, [C compiler for build tools])
> >  if test "${BUILD_CC+set}" != "set"; then
> >    if test $cross_compiling = no; then
> > diff --git a/include/builddefs.in b/include/builddefs.in
> > index 44f95234d21b..0f312b8b88fe 100644
> > --- a/include/builddefs.in
> > +++ b/include/builddefs.in
> > @@ -14,6 +14,7 @@ MALLOCLIB = @malloc_lib@
> >  LOADERFLAGS = @LDFLAGS@
> >  LTLDFLAGS = @LDFLAGS@
> >  CFLAGS = @CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
> > +CXXFLAGS = @CXXFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64 -Wno-address-of-packed-member
> >  BUILD_CFLAGS = @BUILD_CFLAGS@ -D_FILE_OFFSET_BITS=64 -D_TIME_BITS=64
> >  
> >  # make sure we don't pick up whacky LDFLAGS from the make environment and
> > @@ -234,9 +235,15 @@ ifeq ($(ENABLE_GETTEXT),yes)
> >  GCFLAGS += -DENABLE_GETTEXT
> >  endif
> >  
> > +# Override these if C++ needs other options
> > +SANITIZER_CXXFLAGS = $(SANITIZER_CFLAGS)
> > +GCXXFLAGS = $(GCFLAGS)
> > +PCXXFLAGS = $(PCFLAGS)
> > +
> >  BUILD_CFLAGS += $(GCFLAGS) $(PCFLAGS)
> >  # First, Sanitizer, Global, Platform, Local CFLAGS
> >  CFLAGS += $(FCFLAGS) $(SANITIZER_CFLAGS) $(OPTIMIZER) $(GCFLAGS) $(PCFLAGS) $(LCFLAGS)
> > +CXXFLAGS += $(FCXXFLAGS) $(SANITIZER_CXXFLAGS) $(OPTIMIZER) $(GCXXFLAGS) $(PCXXFLAGS) $(LCXXFLAGS)
> >  
> >  include $(TOPDIR)/include/buildmacros
> >  
> > diff --git a/libxfs/Makefile b/libxfs/Makefile
> > index 1185a5e6cb26..bb851ab74204 100644
> > --- a/libxfs/Makefile
> > +++ b/libxfs/Makefile
> > @@ -125,6 +125,8 @@ CFILES = buf_mem.c \
> >  	xfs_trans_space.c \
> >  	xfs_types.c
> >  
> > +LDIRT += dummy.o
> > +
> >  #
> >  # Tracing flags:
> >  # -DMEM_DEBUG		all zone memory use
> > @@ -144,7 +146,11 @@ LTLIBS = $(LIBPTHREAD) $(LIBRT)
> >  # don't try linking xfs_repair with a debug libxfs.
> >  DEBUG = -DNDEBUG
> >  
> > -default: ltdepend $(LTLIBRARY)
> > +default: ltdepend $(LTLIBRARY) dummy.o
> > +
> > +dummy.o: dummy.cpp
> > +	@echo "    [CXX]    $@"
> > +	$(Q)$(CC) $(CXXFLAGS) -c $<
> 
> $(CXX) ;)
> 
> >  
> >  # set up include/xfs header directory
> >  include $(BUILDRULES)
> > diff --git a/libxfs/dummy.cpp b/libxfs/dummy.cpp
> > new file mode 100644
> > index 000000000000..a872c00ad84b
> > --- /dev/null
> > +++ b/libxfs/dummy.cpp
> > @@ -0,0 +1,15 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2024 Oracle.  All Rights Reserved.
> > + * Author: Darrick J. Wong <djwong@kernel.org>
> > + */
> > +#include "include/xfs.h"
> > +#include "include/handle.h"
> > +#include "include/jdm.h"
> > +
> > +/* Dummy program to test C++ compilation of user-exported xfs headers */
> > +
> > +int main(int argc, char *argv[])
> > +{
> > +	return 0;
> > +}
> 
> cheers,
> sam