[dpdk-dev,v1,5/6] fix missing includes in exported headers
Checks
Commit Message
Many exported headers rely on definitions found in rte_config.h without
including it, as shown by the following command:
grep -L '^#include <rte_config.h>' -- \
$(grep -Rl \
$(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
build/include/rte_config.h) \
-- build/include/)
We cannot assume external applications will include rte_config.h on their
own, neither directly nor through a -include parameter like DPDK does
internally.
This not only causes obvious compilation failures that can be reproduced
with check-includes.sh such as:
[...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
this scope
#define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
^
It also results in less visible issues, for instance rte_hash_crc.h relying
on RTE_ARCH_X86_64's presence to provide dedicated inline functions.
This patch partially reverts the commit below and adds missing include
lines to the remaining files.
Fixes: f1a7a5c5f404 ("remove include of generated config header")
Cc: Thomas Monjalon <thomas@monjalon.net>
Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
---
drivers/net/avp/rte_avp_common.h | 1 +
lib/librte_cmdline/cmdline_cirbuf.h | 2 ++
lib/librte_cryptodev/rte_cryptodev.h | 1 +
lib/librte_cryptodev/rte_cryptodev_pmd.h | 1 +
lib/librte_eal/common/include/arch/x86/rte_atomic.h | 1 +
lib/librte_eal/common/include/arch/x86/rte_byteorder.h | 1 +
lib/librte_eal/common/include/arch/x86/rte_cycles.h | 1 +
lib/librte_eal/common/include/arch/x86/rte_memcpy.h | 1 +
lib/librte_eal/common/include/arch/x86/rte_vect.h | 1 +
lib/librte_eal/common/include/generic/rte_byteorder.h | 1 +
lib/librte_eal/common/include/rte_bitmap.h | 1 +
lib/librte_eal/common/include/rte_common.h | 2 ++
lib/librte_eal/common/include/rte_dev.h | 1 +
lib/librte_eal/common/include/rte_eal.h | 1 +
lib/librte_eal/common/include/rte_eal_memconfig.h | 1 +
lib/librte_eal/common/include/rte_keepalive.h | 1 +
lib/librte_eal/common/include/rte_lcore.h | 1 +
lib/librte_eal/common/include/rte_log.h | 1 +
lib/librte_eal/common/include/rte_memory.h | 1 +
lib/librte_eal/common/include/rte_service.h | 1 +
lib/librte_eal/linuxapp/eal/include/exec-env/rte_kni_common.h | 1 +
lib/librte_ether/rte_ethdev.h | 1 +
lib/librte_ether/rte_ethdev_pci.h | 1 +
lib/librte_ether/rte_ethdev_vdev.h | 1 +
lib/librte_eventdev/rte_eventdev.h | 1 +
lib/librte_eventdev/rte_eventdev_pmd.h | 1 +
lib/librte_eventdev/rte_eventdev_pmd_pci.h | 1 +
lib/librte_eventdev/rte_eventdev_pmd_vdev.h | 1 +
lib/librte_hash/rte_fbk_hash.h | 1 +
lib/librte_hash/rte_hash_crc.h | 1 +
lib/librte_hash/rte_jhash.h | 1 +
lib/librte_hash/rte_thash.h | 1 +
lib/librte_ip_frag/rte_ip_frag.h | 1 +
lib/librte_lpm/rte_lpm.h | 1 +
lib/librte_mbuf/rte_mbuf.h | 1 +
lib/librte_member/rte_member.h | 1 +
lib/librte_mempool/rte_mempool.h | 1 +
lib/librte_ring/rte_ring.h | 1 +
lib/librte_table/rte_lru.h | 1 +
lib/librte_table/rte_lru_x86.h | 2 ++
lib/librte_timer/rte_timer.h | 1 +
41 files changed, 44 insertions(+)
Comments
On Thu, Dec 21, 2017 at 02:00:04PM +0100, Adrien Mazarguil wrote:
> Many exported headers rely on definitions found in rte_config.h without
> including it, as shown by the following command:
>
> grep -L '^#include <rte_config.h>' -- \
> $(grep -Rl \
> $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
> build/include/rte_config.h) \
> -- build/include/)
>
> We cannot assume external applications will include rte_config.h on their
> own, neither directly nor through a -include parameter like DPDK does
> internally.
>
> This not only causes obvious compilation failures that can be reproduced
> with check-includes.sh such as:
>
> [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
> this scope
> #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
> ^
>
> It also results in less visible issues, for instance rte_hash_crc.h relying
> on RTE_ARCH_X86_64's presence to provide dedicated inline functions.
>
> This patch partially reverts the commit below and adds missing include
> lines to the remaining files.
>
> Fixes: f1a7a5c5f404 ("remove include of generated config header")
> Cc: Thomas Monjalon <thomas@monjalon.net>
>
> Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> ---
Hi Adrien,
Just FYI, when we move to the new DPDK build system and pass the
necessary build meta-data to the application using pkg-config, this
should be a non-issue, as the pkg-config information will include the
"-include rte_config.h" parameter.
When investigating that, I also tried this approach of adding rte_config
to files explicitly but it did not work for me as expected, as there
were cases where the build was depending upon the rte_config.h always
being the first include in the file. Normally, the rte_* headers should
be last included, so putting it at the top just didn't seem right to me.
I don't remember the specifics, but it was something like using the RTE_
defines to determine which system header file to use e.g. BSD vs Linux.
However, this may be an internal DPDK-build restriction rather than one
that would affect user-apps our or examples.
So, with transitioning to meson and pkg-config, this issue becomes less
significant.
/Bruce
On Thu, Dec 21, 2017 at 02:12:57PM +0000, Bruce Richardson wrote:
> On Thu, Dec 21, 2017 at 02:00:04PM +0100, Adrien Mazarguil wrote:
> > Many exported headers rely on definitions found in rte_config.h without
> > including it, as shown by the following command:
> >
> > grep -L '^#include <rte_config.h>' -- \
> > $(grep -Rl \
> > $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
> > build/include/rte_config.h) \
> > -- build/include/)
> >
> > We cannot assume external applications will include rte_config.h on their
> > own, neither directly nor through a -include parameter like DPDK does
> > internally.
> >
> > This not only causes obvious compilation failures that can be reproduced
> > with check-includes.sh such as:
> >
> > [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
> > this scope
> > #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
> > ^
> >
> > It also results in less visible issues, for instance rte_hash_crc.h relying
> > on RTE_ARCH_X86_64's presence to provide dedicated inline functions.
> >
> > This patch partially reverts the commit below and adds missing include
> > lines to the remaining files.
> >
> > Fixes: f1a7a5c5f404 ("remove include of generated config header")
> > Cc: Thomas Monjalon <thomas@monjalon.net>
> >
> > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > ---
>
> Hi Adrien,
>
> Just FYI, when we move to the new DPDK build system and pass the
> necessary build meta-data to the application using pkg-config, this
> should be a non-issue, as the pkg-config information will include the
> "-include rte_config.h" parameter.
Right, actually -include rte_config.h is also provided to applications that
rely on the current DPDK build system, those are already safe.
The motivation behind this patch is other applications that casually
#include things without even depending on the build features of DPDK (not
even going through pkg-config manually), in order to meet a safety level
worthy of /usr/include.
In my opinion it's fine if this usage is not supported and a prior #include
<rte_config.h> is required, however in that case we need our header files to
#error out. Adding #include directly as done in this patch was in fact just
as easy.
> When investigating that, I also tried this approach of adding rte_config
> to files explicitly but it did not work for me as expected, as there
> were cases where the build was depending upon the rte_config.h always
> being the first include in the file. Normally, the rte_* headers should
> be last included, so putting it at the top just didn't seem right to me.
> I don't remember the specifics, but it was something like using the RTE_
> defines to determine which system header file to use e.g. BSD vs Linux.
> However, this may be an internal DPDK-build restriction rather than one
> that would affect user-apps our or examples.
Was it perhaps before we added consistency to all public headers?
check-includes.sh was added a few releases ago to make sure each of them
could be included on its own, to ensure they properly fetched all their
dependencies instead of triggering compilation errors.
Rule of thumb being if you need something, #include the file providing it
first. I find it strange rte_config.h is somehow an exception.
> So, with transitioning to meson and pkg-config, this issue becomes less
> significant.
Agreed, however I still think something needs to be done, so:
- Do we want to let applications not rely on our build flags yet still use
our exported headers?
- Is a missing #include <rte_config.h> supposed to trigger an explicit
#error for safety?
- How about putting back #include <rte_config.h> in our exported header
files directly given the above?
Note this discussion is only about exported headers. The rest remains
unaffected and can safely continue to rely on -include rte_config.h.
On Thu, Dec 21, 2017 at 03:50:31PM +0100, Adrien Mazarguil wrote:
> On Thu, Dec 21, 2017 at 02:12:57PM +0000, Bruce Richardson wrote:
> > On Thu, Dec 21, 2017 at 02:00:04PM +0100, Adrien Mazarguil wrote:
> > > Many exported headers rely on definitions found in rte_config.h without
> > > including it, as shown by the following command:
> > >
> > > grep -L '^#include <rte_config.h>' -- \
> > > $(grep -Rl \
> > > $(sed -n '/^#define \([^ ]\+\).*$/{s//\1/;H;};${x;s/\n//;s/\n/\\|/g;p;}' \
> > > build/include/rte_config.h) \
> > > -- build/include/)
> > >
> > > We cannot assume external applications will include rte_config.h on their
> > > own, neither directly nor through a -include parameter like DPDK does
> > > internally.
> > >
> > > This not only causes obvious compilation failures that can be reproduced
> > > with check-includes.sh such as:
> > >
> > > [...]/rte_memory.h:88:43: error: ‘RTE_CACHE_LINE_SIZE’ was not declared in
> > > this scope
> > > #define __rte_cache_aligned __rte_aligned(RTE_CACHE_LINE_SIZE)
> > > ^
> > >
> > > It also results in less visible issues, for instance rte_hash_crc.h relying
> > > on RTE_ARCH_X86_64's presence to provide dedicated inline functions.
> > >
> > > This patch partially reverts the commit below and adds missing include
> > > lines to the remaining files.
> > >
> > > Fixes: f1a7a5c5f404 ("remove include of generated config header")
> > > Cc: Thomas Monjalon <thomas@monjalon.net>
> > >
> > > Signed-off-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>
> > > ---
> >
> > Hi Adrien,
> >
> > Just FYI, when we move to the new DPDK build system and pass the
> > necessary build meta-data to the application using pkg-config, this
> > should be a non-issue, as the pkg-config information will include the
> > "-include rte_config.h" parameter.
>
> Right, actually -include rte_config.h is also provided to applications that
> rely on the current DPDK build system, those are already safe.
>
> The motivation behind this patch is other applications that casually
> #include things without even depending on the build features of DPDK (not
> even going through pkg-config manually), in order to meet a safety level
> worthy of /usr/include.
>
> In my opinion it's fine if this usage is not supported and a prior #include
> <rte_config.h> is required, however in that case we need our header files to
> #error out. Adding #include directly as done in this patch was in fact just
> as easy.
>
> > When investigating that, I also tried this approach of adding rte_config
> > to files explicitly but it did not work for me as expected, as there
> > were cases where the build was depending upon the rte_config.h always
> > being the first include in the file. Normally, the rte_* headers should
> > be last included, so putting it at the top just didn't seem right to me.
> > I don't remember the specifics, but it was something like using the RTE_
> > defines to determine which system header file to use e.g. BSD vs Linux.
> > However, this may be an internal DPDK-build restriction rather than one
> > that would affect user-apps our or examples.
>
> Was it perhaps before we added consistency to all public headers?
>
> check-includes.sh was added a few releases ago to make sure each of them
> could be included on its own, to ensure they properly fetched all their
> dependencies instead of triggering compilation errors.
>
> Rule of thumb being if you need something, #include the file providing it
> first. I find it strange rte_config.h is somehow an exception.
>
> > So, with transitioning to meson and pkg-config, this issue becomes less
> > significant.
>
> Agreed, however I still think something needs to be done, so:
>
> - Do we want to let applications not rely on our build flags yet still use
> our exported headers?
>
> - Is a missing #include <rte_config.h> supposed to trigger an explicit
> #error for safety?
>
> - How about putting back #include <rte_config.h> in our exported header
> files directly given the above?
>
> Note this discussion is only about exported headers. The rest remains
> unaffected and can safely continue to rely on -include rte_config.h.
>
Sure, and to be clear I have no objection to having this patch applied.
Having the include on the compiler cmdline as well as having it the file
is fine, so there is no real conflict. As you say, it will make
individual headers more consumable for those not looking to use all of
DPDK.
/Bruce
@@ -63,6 +63,7 @@
#else
#include <stdint.h>
#include <rte_common.h>
+#include <rte_config.h>
#include <rte_memory.h>
#include <rte_ether.h>
#include <rte_atomic.h>
@@ -61,6 +61,8 @@
#ifndef _CIRBUF_H_
#define _CIRBUF_H_
+#include <rte_config.h>
+
#ifdef __cplusplus
extern "C" {
#endif
@@ -49,6 +49,7 @@ extern "C" {
#include "rte_crypto.h"
#include "rte_dev.h"
#include <rte_common.h>
+#include <rte_config.h>
extern const char **rte_cyptodev_names;
@@ -46,6 +46,7 @@ extern "C" {
#include <string.h>
+#include <rte_config.h>
#include <rte_dev.h>
#include <rte_malloc.h>
#include <rte_mbuf.h>
@@ -40,6 +40,7 @@ extern "C" {
#include <stdint.h>
#include <rte_common.h>
+#include <rte_config.h>
#include <emmintrin.h>
#include "generic/rte_atomic.h"
@@ -40,6 +40,7 @@ extern "C" {
#include <stdint.h>
#include <rte_common.h>
+#include <rte_config.h>
#include "generic/rte_byteorder.h"
#ifndef RTE_BYTE_ORDER
@@ -47,6 +47,7 @@ extern int rte_cycles_vmware_tsc_map;
#include <rte_branch_prediction.h>
#endif
#include <rte_common.h>
+#include <rte_config.h>
static inline uint64_t
rte_rdtsc(void)
@@ -45,6 +45,7 @@
#include <string.h>
#include <rte_vect.h>
#include <rte_common.h>
+#include <rte_config.h>
#ifdef __cplusplus
extern "C" {
@@ -41,6 +41,7 @@
*/
#include <stdint.h>
+#include <rte_config.h>
#include "generic/rte_vect.h"
#if (defined(__ICC) || (__GNUC__ == 4 && __GNUC_MINOR__ < 4))
@@ -51,6 +51,7 @@
#endif
#include <rte_common.h>
+#include <rte_config.h>
/*
* Compile-time endianness detection
@@ -66,6 +66,7 @@ extern "C" {
#include <string.h>
#include <rte_common.h>
+#include <rte_config.h>
#include <rte_debug.h>
#include <rte_memory.h>
#include <rte_branch_prediction.h>
@@ -51,6 +51,8 @@ extern "C" {
#include <errno.h>
#include <limits.h>
+#include <rte_config.h>
+
#ifndef typeof
#define typeof __typeof__
#endif
@@ -49,6 +49,7 @@ extern "C" {
#include <stdio.h>
#include <sys/queue.h>
+#include <rte_config.h>
#include <rte_log.h>
__attribute__((format(printf, 2, 0)))
@@ -43,6 +43,7 @@
#include <stdint.h>
#include <sched.h>
+#include <rte_config.h>
#include <rte_per_lcore.h>
#include <rte_bus.h>
@@ -34,6 +34,7 @@
#ifndef _RTE_EAL_MEMCONFIG_H_
#define _RTE_EAL_MEMCONFIG_H_
+#include <rte_config.h>
#include <rte_tailq.h>
#include <rte_memory.h>
#include <rte_memzone.h>
@@ -39,6 +39,7 @@
#ifndef _KEEPALIVE_H_
#define _KEEPALIVE_H_
+#include <rte_config.h>
#include <rte_memory.h>
#ifndef RTE_KEEPALIVE_MAXCORES
@@ -40,6 +40,7 @@
* API for lcore and socket manipulation
*
*/
+#include <rte_config.h>
#include <rte_per_lcore.h>
#include <rte_eal.h>
#include <rte_launch.h>
@@ -51,6 +51,7 @@ extern "C" {
#include <stdarg.h>
#include <rte_common.h>
+#include <rte_config.h>
struct rte_log_dynamic_type;
@@ -49,6 +49,7 @@ extern "C" {
#endif
#include <rte_common.h>
+#include <rte_config.h>
__extension__
enum rte_page_sizes {
@@ -59,6 +59,7 @@ extern "C" {
#include <stdint.h>
#include <sys/queue.h>
+#include <rte_config.h>
#include <rte_lcore.h>
#define RTE_SERVICE_NAME_MAX 32
@@ -64,6 +64,7 @@
#define RTE_STD_C11
#else
#include <rte_common.h>
+#include <rte_config.h>
#endif
/**
@@ -181,6 +181,7 @@ extern "C" {
#include <rte_devargs.h>
#include <rte_errno.h>
#include <rte_common.h>
+#include <rte_config.h>
#include "rte_ether.h"
#include "rte_eth_ctrl.h"
@@ -37,6 +37,7 @@
#include <rte_malloc.h>
#include <rte_pci.h>
#include <rte_bus_pci.h>
+#include <rte_config.h>
#include <rte_ethdev.h>
/**
@@ -34,6 +34,7 @@
#ifndef _RTE_ETHDEV_VDEV_H_
#define _RTE_ETHDEV_VDEV_H_
+#include <rte_config.h>
#include <rte_malloc.h>
#include <rte_bus_vdev.h>
#include <rte_ethdev.h>
@@ -239,6 +239,7 @@ extern "C" {
#endif
#include <rte_common.h>
+#include <rte_config.h>
#include <rte_memory.h>
#include <rte_errno.h>
@@ -47,6 +47,7 @@ extern "C" {
#include <string.h>
#include <rte_common.h>
+#include <rte_config.h>
#include <rte_dev.h>
#include <rte_log.h>
#include <rte_malloc.h>
@@ -47,6 +47,7 @@ extern "C" {
#include <string.h>
+#include <rte_config.h>
#include <rte_eal.h>
#include <rte_lcore.h>
#include <rte_pci.h>
@@ -46,6 +46,7 @@ extern "C" {
#include <string.h>
+#include <rte_config.h>
#include <rte_debug.h>
#include <rte_eal.h>
#include <rte_bus_vdev.h>
@@ -54,6 +54,7 @@ extern "C" {
#include <string.h>
+#include <rte_config.h>
#ifndef RTE_FBK_HASH_FUNC_DEFAULT
#if defined(RTE_ARCH_X86) || defined(RTE_MACHINE_CPUFLAG_CRC32)
#include <rte_hash_crc.h>
@@ -45,6 +45,7 @@ extern "C" {
#endif
#include <stdint.h>
+#include <rte_config.h>
#include <rte_cpuflags.h>
#include <rte_branch_prediction.h>
#include <rte_common.h>
@@ -48,6 +48,7 @@ extern "C" {
#include <string.h>
#include <limits.h>
+#include <rte_config.h>
#include <rte_log.h>
#include <rte_byteorder.h>
@@ -53,6 +53,7 @@ extern "C" {
#include <stdint.h>
#include <rte_byteorder.h>
+#include <rte_config.h>
#include <rte_ip.h>
#include <rte_common.h>
@@ -48,6 +48,7 @@ extern "C" {
#include <stdint.h>
#include <stdio.h>
+#include <rte_config.h>
#include <rte_malloc.h>
#include <rte_memory.h>
#include <rte_ip.h>
@@ -45,6 +45,7 @@
#include <stdlib.h>
#include <rte_branch_prediction.h>
#include <rte_byteorder.h>
+#include <rte_config.h>
#include <rte_memory.h>
#include <rte_common.h>
#include <rte_vect.h>
@@ -62,6 +62,7 @@
#include <stdint.h>
#include <rte_common.h>
+#include <rte_config.h>
#include <rte_mempool.h>
#include <rte_memory.h>
#include <rte_atomic.h>
@@ -81,6 +81,7 @@ extern "C" {
#include <stdint.h>
#include <rte_common.h>
+#include <rte_config.h>
/** The set ID type that stored internally in hash table based set summary. */
typedef uint16_t member_set_t;
@@ -69,6 +69,7 @@
#include <inttypes.h>
#include <sys/queue.h>
+#include <rte_config.h>
#include <rte_spinlock.h>
#include <rte_log.h>
#include <rte_debug.h>
@@ -96,6 +96,7 @@ extern "C" {
#include <sys/queue.h>
#include <errno.h>
#include <rte_common.h>
+#include <rte_config.h>
#include <rte_memory.h>
#include <rte_lcore.h>
#include <rte_atomic.h>
@@ -38,6 +38,7 @@
extern "C" {
#endif
+#include <rte_config.h>
#ifdef RTE_ARCH_X86_64
#include "rte_lru_x86.h"
#elif defined(RTE_ARCH_ARM64)
@@ -40,6 +40,8 @@ extern "C" {
#include <stdint.h>
+#include <rte_config.h>
+
#ifndef RTE_TABLE_HASH_LRU_STRATEGY
#define RTE_TABLE_HASH_LRU_STRATEGY 2
#endif
@@ -67,6 +67,7 @@
#include <stdint.h>
#include <stddef.h>
#include <rte_common.h>
+#include <rte_config.h>
#ifdef __cplusplus
extern "C" {