[PATCH 0/5] build fixes on FreeBSD

[PATCH 0/5] build fixes on FreeBSD

[Bruce Richardson]

This set contains a number of fixes, most of which are for
build issues discovered when compiling on FreeBSD with clang.

Bruce Richardson (5):
  eal/freebsd: add missing C++ include guards
  compressdev: separate out driver-only headers
  compressdev: fix missing space in header
  cryptodev: fix compilation with clang C++ builds
  eventdev:  fix compilation with clang C++ builds

 lib/compressdev/meson.build                |  6 ++++--
 lib/compressdev/rte_compressdev_internal.h |  2 +-
 lib/cryptodev/rte_crypto.h                 |  9 +++++++++
 lib/eal/freebsd/include/rte_os.h           |  8 ++++++++
 lib/eventdev/rte_eventdev.h                | 12 +++++++++++-
 5 files changed, 33 insertions(+), 4 deletions(-)

--
2.34.1

[PATCH 1/5] eal/freebsd: add missing C++ include guards

[Bruce Richardson]

Add mising 'extern "C"' to file.

Fixes: 428eb983f5f7 ("eal: add OS specific header file")

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/eal/freebsd/include/rte_os.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/eal/freebsd/include/rte_os.h b/lib/eal/freebsd/include/rte_os.h
index 9d8a69008c..b4afd45adc 100644
--- a/lib/eal/freebsd/include/rte_os.h
+++ b/lib/eal/freebsd/include/rte_os.h
@@ -5,6 +5,10 @@
 #ifndef _RTE_OS_H_
 #define _RTE_OS_H_
 
+#ifdef __cplusplus
+extern "C" {
+#endif
+
 /**
  * This header should contain any definition
  * which is not supported natively or named differently in FreeBSD.
@@ -59,4 +63,8 @@ typedef cpuset_t rte_cpuset_t;
 } while (0)
 #endif
 
+#ifdef __cplusplus
+}
+#endif
+
 #endif /* _RTE_OS_H_ */
-- 
2.34.1

[PATCH 2/5] compressdev: separate out driver-only headers

[Bruce Richardson]

The headers rte_compressdev_pmd.h and rte_compressdev_internal.h are,
as the filenames suggest, headers for building drivers using the
compressdev APIs. As such they should be marked as
"driver_sdk_headers" rather than just "headers" in the meson.build
file.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/compressdev/meson.build | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/compressdev/meson.build b/lib/compressdev/meson.build
index 75ea666a9b..c80295dc0d 100644
--- a/lib/compressdev/meson.build
+++ b/lib/compressdev/meson.build
@@ -11,7 +11,9 @@ sources = files('rte_compressdev.c',
     'rte_compressdev_pmd.c',
     'rte_comp.c')
 headers = files('rte_compressdev.h',
-    'rte_compressdev_pmd.h',
-    'rte_compressdev_internal.h',
     'rte_comp.h')
+driver_sdk_headers = files(
+        'rte_compressdev_pmd.h',
+        'rte_compressdev_internal.h',
+    )
 deps += ['kvargs', 'mbuf']
-- 
2.34.1

[PATCH 3/5] compressdev: fix missing space in header

[Bruce Richardson]

Building with clang on FreeBSD with chkincs enabled, we get the
following error about a missing space:

../lib/compressdev/rte_compressdev_internal.h:25:58: error: invalid suffix on literal; C++11 requires a space between literal and identifier [-Wreserved-user-defined-literal]
        rte_log(RTE_LOG_ ## level, compressdev_logtype, "%s(): "fmt "\n", \

Adding in a space between the '"' and 'fmt' removes the error.

Fixes: ed7dd94f7f66 ("compressdev: add basic device management")
Cc: fiona.trahe@intel.com

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/compressdev/rte_compressdev_internal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/compressdev/rte_compressdev_internal.h b/lib/compressdev/rte_compressdev_internal.h
index 25d8afbfb9..b3b193e3ee 100644
--- a/lib/compressdev/rte_compressdev_internal.h
+++ b/lib/compressdev/rte_compressdev_internal.h
@@ -22,7 +22,7 @@ extern "C" {
 /* Logging Macros */
 extern int compressdev_logtype;
 #define COMPRESSDEV_LOG(level, fmt, args...) \
-	rte_log(RTE_LOG_ ## level, compressdev_logtype, "%s(): "fmt "\n", \
+	rte_log(RTE_LOG_ ## level, compressdev_logtype, "%s(): " fmt "\n", \
 			__func__, ##args)
 
 /**
-- 
2.34.1

[PATCH 4/5] cryptodev: fix compilation with clang C++ builds

[Bruce Richardson]

When compiling on FreeBSD with clang and include checking enabled,
errors are emitted due to differences in how empty structs/unions are
handled in C and C++, as C++ structs cannot have zero size.

../lib/cryptodev/rte_crypto.h:127:2: error: union has size 0 in C, non-zero size in C++

Since the contents of the union are all themselves of zero size,
the actual union wrapper is unnecessary. We therefore remove it for C++
builds - though keep it for C builds for safety and clarity of
understanding the code.

Fixes: c0f87eb5252b ("cryptodev: change burst API to be crypto op oriented")
Fixes: d2a4223c4c6d ("cryptodev: do not store pointer to op specific params")
Cc: pablo.de.lara.guarch@intel.com
Cc: declan.doherty@intel.com

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/cryptodev/rte_crypto.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/lib/cryptodev/rte_crypto.h b/lib/cryptodev/rte_crypto.h
index a864f5036f..aeb3bf6e38 100644
--- a/lib/cryptodev/rte_crypto.h
+++ b/lib/cryptodev/rte_crypto.h
@@ -123,15 +123,24 @@ struct rte_crypto_op {
 	rte_iova_t phys_addr;
 	/**< physical address of crypto operation */
 
+/* empty structures do not have zero size in C++ leading to compilation errors
+ * with clang about structure/union having different sizes in C and C++.
+ * While things are clearer with an explicit union, since each field is
+ * zero-sized it's not actually needed, so omit it for C++
+ */
+#ifndef __cplusplus
 	__extension__
 	union {
+#endif
 		struct rte_crypto_sym_op sym[0];
 		/**< Symmetric operation parameters */
 
 		struct rte_crypto_asym_op asym[0];
 		/**< Asymmetric operation parameters */
 
+#ifndef __cplusplus
 	}; /**< operation specific parameters */
+#endif
 };
 
 /**
-- 
2.34.1

[PATCH 5/5] eventdev: fix compilation with clang C++ builds

[Bruce Richardson]

When compiling on FreeBSD with clang and include checking enabled,
errors are emitted due to differences in how empty structs/unions are
handled in C and C++, as C++ structs cannot have zero size.

../lib/eventdev/rte_eventdev.h:992:2: error: union has size 0 in C, non-zero size in C++

Since the contents of the union are all themselves of zero size,
the actual union wrapper is unnecessary. We therefore remove it for C++
builds - though keep it for C builds for safety and clarity of
understanding the code. The alignment constraint on the union is
unnecessary in the case where the whole struct is aligned on a 16-byte
boundary, so we add that constraint to the overall structure to ensure
it applies for C++ code as well as C.

Fixes: 1cc44d409271 ("eventdev: introduce event vector capability")
Cc: pbhagavatula@marvell.com

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/eventdev/rte_eventdev.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
index 67c4a5e036..42a5660169 100644
--- a/lib/eventdev/rte_eventdev.h
+++ b/lib/eventdev/rte_eventdev.h
@@ -984,21 +984,31 @@ struct rte_event_vector {
 	};
 	/**< Union to hold common attributes of the vector array. */
 	uint64_t impl_opaque;
+
+/* empty structures do not have zero size in C++ leading to compilation errors
+ * with clang about structure having different sizes in C and C++.
+ * Since these are all zero-sized arrays, we can omit the "union" wrapper for
+ * C++ builds, removing the warning.
+ */
+#ifndef __cplusplus
 	/**< Implementation specific opaque value.
 	 * An implementation may use this field to hold implementation specific
 	 * value to share between dequeue and enqueue operation.
 	 * The application should not modify this field.
 	 */
 	union {
+#endif
 		struct rte_mbuf *mbufs[0];
 		void *ptrs[0];
 		uint64_t *u64s[0];
+#ifndef __cplusplus
 	} __rte_aligned(16);
+#endif
 	/**< Start of the vector array union. Depending upon the event type the
 	 * vector array can be an array of mbufs or pointers or opaque u64
 	 * values.
 	 */
-};
+} __rte_aligned(16);
 
 /* Scheduler type definitions */
 #define RTE_SCHED_TYPE_ORDERED          0
-- 
2.34.1

Re: [PATCH 5/5] eventdev: fix compilation with clang C++ builds

[Stephen Hemminger]

On Fri, 11 Mar 2022 20:05:23 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> When compiling on FreeBSD with clang and include checking enabled,
> errors are emitted due to differences in how empty structs/unions are
> handled in C and C++, as C++ structs cannot have zero size.
> 
> ../lib/eventdev/rte_eventdev.h:992:2: error: union has size 0 in C, non-zero size in C++
> 
> Since the contents of the union are all themselves of zero size,
> the actual union wrapper is unnecessary. We therefore remove it for C++
> builds - though keep it for C builds for safety and clarity of
> understanding the code. The alignment constraint on the union is
> unnecessary in the case where the whole struct is aligned on a 16-byte
> boundary, so we add that constraint to the overall structure to ensure
> it applies for C++ code as well as C.
> 
> Fixes: 1cc44d409271 ("eventdev: introduce event vector capability")
> Cc: pbhagavatula@marvell.com
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/eventdev/rte_eventdev.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> index 67c4a5e036..42a5660169 100644
> --- a/lib/eventdev/rte_eventdev.h
> +++ b/lib/eventdev/rte_eventdev.h
> @@ -984,21 +984,31 @@ struct rte_event_vector {
>  	};
>  	/**< Union to hold common attributes of the vector array. */
>  	uint64_t impl_opaque;
> +
> +/* empty structures do not have zero size in C++ leading to compilation errors
> + * with clang about structure having different sizes in C and C++.
> + * Since these are all zero-sized arrays, we can omit the "union" wrapper for
> + * C++ builds, removing the warning.
> + */
> +#ifndef __cplusplus
>  	/**< Implementation specific opaque value.
>  	 * An implementation may use this field to hold implementation specific
>  	 * value to share between dequeue and enqueue operation.
>  	 * The application should not modify this field.
>  	 */
>  	union {
> +#endif
>  		struct rte_mbuf *mbufs[0];
>  		void *ptrs[0];
>  		uint64_t *u64s[0];
> +#ifndef __cplusplus
>  	} __rte_aligned(16);
> +#endif
>  	/**< Start of the vector array union. Depending upon the event type the
>  	 * vector array can be an array of mbufs or pointers or opaque u64
>  	 * values.
>  	 */
> -};
> +} __rte_aligned(16);
>  
>  /* Scheduler type definitions */
>  #define RTE_SCHED_TYPE_ORDERED          0

Zero size arrays should be replaced by flexible arrays.
Linux has this coccinelle script for that:

// SPDX-License-Identifier: GPL-2.0-only
///
/// Zero-length and one-element arrays are deprecated, see
/// Documentation/process/deprecated.rst
/// Flexible-array members should be used instead.
///
//
// Confidence: High
// Copyright: (C) 2020 Denis Efremov ISPRAS.
// Comments:
// Options: --no-includes --include-headers

virtual context
virtual report
virtual org
virtual patch

@initialize:python@
@@
def relevant(positions):
    for p in positions:
        if "uapi" in p.file:
             return False
    return True

@r depends on !patch@
identifier name, array;
type T;
position p : script:python() { relevant(p) };
@@

(
  struct name {
    ...
*   T array@p[\(0\|1\)];
  };
|
  struct {
    ...
*   T array@p[\(0\|1\)];
  };
|
  union name {
    ...
*   T array@p[\(0\|1\)];
  };
|
  union {
    ...
*   T array@p[\(0\|1\)];
  };
)

@only_field depends on patch@
identifier name, array;
type T;
position q;
@@

(
  struct name {@q
    T array[0];
  };
|
  struct {@q
    T array[0];
  };
)

@depends on patch@
identifier name, array;
type T;
position p : script:python() { relevant(p) };
// position @q with rule "only_field" simplifies
// handling of bitfields, arrays, etc.
position q != only_field.q;
@@

(
  struct name {@q
    ...
    T array@p[
-       0
    ];
  };
|
  struct {@q
    ...
    T array@p[
-       0
    ];
  };
)

@script: python depends on report@
p << r.p;
@@

msg = "WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)"
coccilib.report.print_report(p[0], msg)

@script: python depends on org@
p << r.p;
@@

msg = "WARNING use flexible-array member instead (https://www.kernel.org/doc/html/latest/process/deprecated.html#zero-length-and-one-element-arrays)"
coccilib.org.print_todo(p[0], msg)

Re: [PATCH 5/5] eventdev: fix compilation with clang C++ builds

[Bruce Richardson]

On Fri, Mar 11, 2022 at 05:11:02PM -0800, Stephen Hemminger wrote:
> On Fri, 11 Mar 2022 20:05:23 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > When compiling on FreeBSD with clang and include checking enabled,
> > errors are emitted due to differences in how empty structs/unions are
> > handled in C and C++, as C++ structs cannot have zero size.
> > 
> > ../lib/eventdev/rte_eventdev.h:992:2: error: union has size 0 in C, non-zero size in C++
> > 
> > Since the contents of the union are all themselves of zero size,
> > the actual union wrapper is unnecessary. We therefore remove it for C++
> > builds - though keep it for C builds for safety and clarity of
> > understanding the code. The alignment constraint on the union is
> > unnecessary in the case where the whole struct is aligned on a 16-byte
> > boundary, so we add that constraint to the overall structure to ensure
> > it applies for C++ code as well as C.
> > 
> > Fixes: 1cc44d409271 ("eventdev: introduce event vector capability")
> > Cc: pbhagavatula@marvell.com
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/eventdev/rte_eventdev.h | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/eventdev/rte_eventdev.h b/lib/eventdev/rte_eventdev.h
> > index 67c4a5e036..42a5660169 100644
> > --- a/lib/eventdev/rte_eventdev.h
> > +++ b/lib/eventdev/rte_eventdev.h
> > @@ -984,21 +984,31 @@ struct rte_event_vector {
> >  	};
> >  	/**< Union to hold common attributes of the vector array. */
> >  	uint64_t impl_opaque;
> > +
> > +/* empty structures do not have zero size in C++ leading to compilation errors
> > + * with clang about structure having different sizes in C and C++.
> > + * Since these are all zero-sized arrays, we can omit the "union" wrapper for
> > + * C++ builds, removing the warning.
> > + */
> > +#ifndef __cplusplus
> >  	/**< Implementation specific opaque value.
> >  	 * An implementation may use this field to hold implementation specific
> >  	 * value to share between dequeue and enqueue operation.
> >  	 * The application should not modify this field.
> >  	 */
> >  	union {
> > +#endif
> >  		struct rte_mbuf *mbufs[0];
> >  		void *ptrs[0];
> >  		uint64_t *u64s[0];
> > +#ifndef __cplusplus
> >  	} __rte_aligned(16);
> > +#endif
> >  	/**< Start of the vector array union. Depending upon the event type the
> >  	 * vector array can be an array of mbufs or pointers or opaque u64
> >  	 * values.
> >  	 */
> > -};
> > +} __rte_aligned(16);
> >  
> >  /* Scheduler type definitions */
> >  #define RTE_SCHED_TYPE_ORDERED          0
> 
> Zero size arrays should be replaced by flexible arrays.
> Linux has this coccinelle script for that:
>
Yes, I agree. However, given how late in the release process I discovered
this, I wanted a fix with absolute minimum impact, which is why I chose the
above. Zero struct change for C code, and enough of a fix for C++ to get it
compiling.
If you feel that replacing with flexible arrays is safe enough to do at
this late stage, we can perhaps consider it, but overall I'd suggest doing
any such replacement in 22.07

/Bruce

Re: [PATCH 0/5] build fixes on FreeBSD

[Thomas Monjalon]

11/03/2022 21:05, Bruce Richardson:
> This set contains a number of fixes, most of which are for
> build issues discovered when compiling on FreeBSD with clang.
> 
> Bruce Richardson (5):
>   eal/freebsd: add missing C++ include guards
>   compressdev: separate out driver-only headers
>   compressdev: fix missing space in header
>   cryptodev: fix compilation with clang C++ builds
>   eventdev:  fix compilation with clang C++ builds

Applied, thanks.