eal: remove variable length array
PATCH 1/1 eal: remove variable length array
Compilers that do not support the C11 standard, or do not implement gcc extensions, may not support variable length arrays.
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using alloca().
--- lib/librte_eal/common/include/rte_dev.h | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
lib/librte_eal/common/include/rte_dev.h +9 -10
@@ -47,22 +47,21 @@ __attribute__((format(printf, 2, 0)))static inline void rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) { +char *buffer; +int buf_len;va_list ap;va_start (ap, fmt); + buf_len =vsnprintf (NULL ,0 , fmt, ap) +1 ; +va_end (ap); - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; + buffer = (char *)alloca (buf_len); -va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); +va_start (ap, fmt); +vsnprintf (buffer, buf_len, fmt, ap); +va_end (ap); -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } +rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , func_name, buffer); }/*
Signature · 1 line
2.14.3
Quoted 26 lines · On Fri, 14 Dec 2018 08:38:27 -0800 Jeff Shaw <[email protected]> wrote:
Compilers that do not support the C11 standard, or do not implement gcc extensions, may not support variable length arrays.
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Signed-off-by Jeff Shaw <[email protected]>--- lib/librte_eal/common/include/rte_dev.h | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)lib/librte_eal/common/include/rte_dev.h +1
-0@@ -47,22 +47,21 @@ __attribute__((format(printf, 2, 0)))static inline void rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) { +char *buffer;
lib/librte_eal/common/include/rte_dev.h __attribute__((format(printf, 2, 0))) · line 58
Quoted 10 lines
lib/librte_eal/common/include/rte_dev.h +4
-2+int buf_len;va_list ap;va_start (ap, fmt); + buf_len =vsnprintf (NULL ,0 , fmt, ap) +1 ; +va_end (ap); - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; + buffer = (char *)alloca (buf_len);
alloca is void * so cast is not necessary.
You might be able to skip the buffering step entirely by using rte_log a little more creatively, see how other logging works.
But to go further since rte_pmd_debug_trace is not used anywhere in current code base in DPDK, it should just be removed.
Quoted 36 lines · Stephen Hemminger
Quoted 33 lines · On Fri, 14 Dec 2018 08:38:27 -0800 Jeff Shaw <[email protected]> wrote:
Compilers that do not support the C11 standard, or do not implement gcc extensions, may not support variable length arrays.
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Signed-off-by Jeff Shaw <[email protected]>--- lib/librte_eal/common/include/rte_dev.h | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)lib/librte_eal/common/include/rte_dev.h +4
-0@@ -47,22 +47,21 @@ __attribute__((format(printf, 2, 0)))static inline void rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) { +char *buffer; +int buf_len;va_list ap;va_start (ap, fmt); + buf_len =vsnprintf (NULL ,0 , fmt, ap) +1 ; +va_end (ap);
Quoted 5 lines
lib/librte_eal/common/include/rte_dev.h __attribute__((format(printf, 2, 0))) · line 58
Quoted 3 lines
lib/librte_eal/common/include/rte_dev.h +1
-2- { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; + buffer = (char *)alloca (buf_len);alloca is void * so cast is not necessary.
I get a compiler warning on Windows, hence the cast. I suppose we could disable the warning.
Quoted 3 lines
You might be able to skip the buffering step entirely by using
rte_loga little more creatively, see how other logging works.
Probably right... this eventuall needs to be done in ~100 other places, most of which aren't used with rte_log(), so it seemed fitting.
Quoted 3 lines
But to go further since
rte_pmd_debug_traceis not used anywhere in current code base in DPDK, it should just be removed.
Much better solution :)
Quoted 50 lines · Jeff Shaw
Quoted 36 lines · Stephen Hemminger
Quoted 33 lines · On Fri, 14 Dec 2018 08:38:27 -0800 Jeff Shaw <[email protected]> wrote:
Compilers that do not support the C11 standard, or do not implement gcc extensions, may not support variable length arrays.
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Signed-off-by Jeff Shaw <[email protected]>--- lib/librte_eal/common/include/rte_dev.h | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)lib/librte_eal/common/include/rte_dev.h +4
-0@@ -47,22 +47,21 @@ __attribute__((format(printf, 2, 0)))static inline void rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) { +char *buffer; +int buf_len;va_list ap;va_start (ap, fmt); + buf_len =vsnprintf (NULL ,0 , fmt, ap) +1 ; +va_end (ap);Quoted 5 lines
lib/librte_eal/common/include/rte_dev.h __attribute__((format(printf, 2, 0))) · line 58
Quoted 3 lines
lib/librte_eal/common/include/rte_dev.h +1
-2- { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; + buffer = (char *)alloca (buf_len);alloca is void * so cast is not necessary.
I get a compiler warning on Windows, hence the cast. I suppose we could disable the warning.
Quoted 3 lines
You might be able to skip the buffering step entirely by using
rte_loga little more creatively, see how other logging works.
Quoted 8 lines
Probably right... this eventuall needs to be done in ~100 other places, most of which aren't used with
rte_log(), so it seemed fitting.Quoted 3 lines
But to go further since
rte_pmd_debug_traceis not used anywhere in current code base in DPDK, it should just be removed.Much better solution :)
Actually, it cannot be removed. It is used by librte_eventdev. When RTE_LIBRTE_EVENTDEV_DEBUG is defined, the RTE_PMD_DEBUG_TRACE macro uses rte_pmd_debug_trace.
I will remove the cast and update the commit message to reference C99 instead of C11 as requested by Mattias R.
Quoted 3 lines · Jeff Shaw
Compilers that do not support the C11 standard, or do not implement gcc extensions, may not support variable length arrays.
VLAs are a C99 thing.
Quoted 9 lines
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().
Is alloca() even included in *any* C standard? As far as I see, it just achieves the same thing in an uglier, less portable way than VLAs.
Quoted 6 lines · Mattias Rönnblom
Quoted 3 lines · Jeff Shaw
Compilers that do not support the C11 standard, or do not implement gcc extensions, may not support variable length arrays.
VLAs are a C99 thing.
You're right, my mistake.
Quoted 6 lines
Quoted 6 lines
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
Quoted 7 lines
Quoted 4 lines
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Is
alloca()even included in *any* C standard? As far as I see, it just achieves the same thing in an uglier, less portable way than VLAs.
I agree that it is much less elegant than a VLA. This is in preparation for DPDK on Windows, which using the Microsoft Visual C++ (MSVC) compiler. MSVC does not support variable length arrays. It does, however, support alloca(), as does GCC/ICC.
For this particular instance, the point is moot, since the function is not used anywhere and can just as easily be removed. Though it does not address the issue for the ~100 other instances where VLAs are used. We will be submitting patches for those as more common files are ported to Windows.
Quoted 18 lines · Jeff Shaw
Quoted 5 lines
Quoted 5 lines
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
Quoted 7 lines
Quoted 4 lines
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Is
alloca()even included in *any* C standard? As far as I see, it just achieves the same thing in an uglier, less portable way than VLAs.I agree that it is much less elegant than a VLA. This is in preparation for DPDK on Windows, which using the Microsoft Visual C++ (MSVC) compiler. MSVC does not support variable length arrays. It does, however, support
alloca(), as does GCC/ICC.
Quoted 6 lines
For this particular instance, the point is moot, since the function is not used anywhere and can just as easily be removed. Though it does not address the issue for the ~100 other instances where VLAs are used. We will be submitting patches for those as more common files are ported to Windows.
If Microsoft's C compiler doesn't support C99, some 20 years after its release, maybe it's time to find a new compiler, instead of messing up the DPDK code in a ~100 instances.
PATCH 1/1 eal: simplify RTE_PMD_DEBUG_TRACE
Use rte_log directly, eliminating no longer used rte_pmd_dev_trace function. This removes variable length array which is problem on Windows and other compilers not doing C99.
Also, drop unused RTE_PROC_PRIMARY macros.
--- lib/librte_eal/common/include/rte_dev.h | 43 ++----------------------- 1 file changed, 3 insertions(+), 40 deletions(-)
lib/librte_eal/common/include/rte_dev.h +3 -40
@@ -43,54 +43,17 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,enum rte_dev_event_type event, void *cb_arg); -__attribute__((format (printf,2 ,0 ))) -static inline void -rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) -{ -va_list ap; - -va_start (ap, fmt); - - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; - -va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); - -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } -} -/* * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the * RTE_*_RET() macros defined below is compiled in debug mode. */ #if defined(RTE_LIBRTE_EVENTDEV_DEBUG ) -#define RTE_PMD_DEBUG_TRACE (...) \ - rte_pmd_debug_trace(__func__, __VA_ARGS__) +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \ + rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, ## args)#else -#define RTE_PMD_DEBUG_TRACE (...) (void)0 +#define RTE_PMD_DEBUG_TRACE (...) do { } while(0)#endif -/* Macros for checking for restricting functions to primary instance only */ -#define RTE_PROC_PRIMARY_OR_ERR_RET (retval) do { \ - if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \ - RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ - return retval; \ - } \ -} while (0) - -#define RTE_PROC_PRIMARY_OR_RET () do { \ - if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \ - RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ - return; \ - } \ -} while (0) -/* Macros to check for invalid function pointers */ #define RTE_FUNC_PTR_OR_ERR_RET (func, retval) do { \ if ((func) == NULL) { \
Signature · 1 line
2.19.2
Re: [PATCH] eal: simplify RTE_PMD_DEBUG_TRACE
Quoted 32 lines · Stephen Hemminger
Use
rte_logdirectly, eliminating no longer usedrte_pmd_dev_tracefunction. This removes variable length array which is problem on Windows and other compilers not doing C99.Also, drop unused
RTE_PROC_PRIMARYmacros.Reported-by Jeff Shaw <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>--- lib/librte_eal/common/include/rte_dev.h | 43 ++----------------------- 1 file changed, 3 insertions(+), 40 deletions(-)lib/librte_eal/common/include/rte_dev.h +0
-12@@ -43,54 +43,17 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,enum rte_dev_event_type event, void *cb_arg); -__attribute__((format (printf,2 ,0 ))) -static inline void -rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) -{ -va_list ap; - -va_start (ap, fmt); - - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; - -va_end (ap);
lib/librte_eal/common/include/rte_dev.h typedef void (*rte_dev_event_cb_fn)(const char *device_name, · line 67
Quoted 10 lines
lib/librte_eal/common/include/rte_dev.h +0
-10- -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); - -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } -} -
Will this break applications that try to use this function? Because it is not a documented function, is there no guarantee it will be present?
lib/librte_eal/common/include/rte_dev.h typedef void (*rte_dev_event_cb_fn)(const char *device_name, · line 52
Quoted 9 lines
lib/librte_eal/common/include/rte_dev.h +2
-2/* * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the * RTE_*_RET() macros defined below is compiled in debug mode. */ #if defined(RTE_LIBRTE_EVENTDEV_DEBUG ) -#define RTE_PMD_DEBUG_TRACE (...) \ - rte_pmd_debug_trace(__func__, __VA_ARGS__) +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \ + rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, ## args)
Actually, MSVC does not support named variable arguments either. I think this will work instead:
#define RTE_PMD_DEBUG_TRACE(fmt, ...) \
rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, __VA_ARGS__)The previous behavior was "%s: ..." not "%s():". I'm not sure if you meant to change how the messages are displayed. I don't care either way, but maybe users of the function would prefer the same format.
Re: [PATCH] eal: simplify RTE_PMD_DEBUG_TRACE
Quoted 38 lines · On Fri, 14 Dec 2018 13:20:07 -0800 Jeff Shaw <[email protected]> wrote:
Quoted 37 lines · Stephen Hemminger
Use
rte_logdirectly, eliminating no longer usedrte_pmd_dev_tracefunction. This removes variable length array which is problem on Windows and other compilers not doing C99.Also, drop unused
RTE_PROC_PRIMARYmacros.Reported-by Jeff Shaw <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>--- lib/librte_eal/common/include/rte_dev.h | 43 ++----------------------- 1 file changed, 3 insertions(+), 40 deletions(-)lib/librte_eal/common/include/rte_dev.h +0
-17@@ -43,54 +43,17 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,enum rte_dev_event_type event, void *cb_arg); -__attribute__((format (printf,2 ,0 ))) -static inline void -rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) -{ -va_list ap; - -va_start (ap, fmt); - - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; - -va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); -
Quoted 8 lines
lib/librte_eal/common/include/rte_dev.h typedef void (*rte_dev_event_cb_fn)(const char *device_name, · line 67
Quoted 5 lines
lib/librte_eal/common/include/rte_dev.h +0
-5-rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } -} -Will this break applications that try to use this function? Because it is not a documented function, is there no guarantee it will be present?
It shouldn't be visible as part of EAL. Any code that was built that had old MACRO would still run (ABI compatible) because it was inline.
Re: [PATCH] eal: simplify RTE_PMD_DEBUG_TRACE
In reply to Jeff ShawQuoted 60 lines · Jeff Shaw
Quoted 32 lines · Stephen Hemminger
Use
rte_logdirectly, eliminating no longer usedrte_pmd_dev_tracefunction. This removes variable length array which is problem on Windows and other compilers not doing C99.Also, drop unused
RTE_PROC_PRIMARYmacros.Reported-by Jeff Shaw <[email protected]>Signed-off-by Stephen Hemminger <[email protected]>--- lib/librte_eal/common/include/rte_dev.h | 43 ++----------------------- 1 file changed, 3 insertions(+), 40 deletions(-)lib/librte_eal/common/include/rte_dev.h +0
-12@@ -43,54 +43,17 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,enum rte_dev_event_type event, void *cb_arg); -__attribute__((format (printf,2 ,0 ))) -static inline void -rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) -{ -va_list ap; - -va_start (ap, fmt); - - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; - -va_end (ap);lib/librte_eal/common/include/rte_dev.h typedef void (*rte_dev_event_cb_fn)(const char *device_name, · line 67
Quoted 10 lines
lib/librte_eal/common/include/rte_dev.h +0
-10- -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); - -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } -} -Will this break applications that try to use this function? Because it is not a documented function, is there no guarantee it will be present?
lib/librte_eal/common/include/rte_dev.h typedef void (*rte_dev_event_cb_fn)(const char *device_name, · line 52
Quoted 9 lines
lib/librte_eal/common/include/rte_dev.h +2
-2/* * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the * RTE_*_RET() macros defined below is compiled in debug mode. */ #if defined(RTE_LIBRTE_EVENTDEV_DEBUG ) -#define RTE_PMD_DEBUG_TRACE (...) \ - rte_pmd_debug_trace(__func__, __VA_ARGS__) +#define RTE_PMD_DEBUG_TRACE(fmt, args...) \ + rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, ## args)Actually, MSVC does not support named variable arguments either. I think this will work instead:
Quoted 7 lines
#define RTE_PMD_DEBUG_TRACE(fmt, ...) \ rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s():" fmt, __func__, __VA_ARGS__)The previous behavior was "%s: ..." not "%s():". I'm not sure if you meant to change how the messages are displayed. I don't care either way, but maybe users of the function would prefer the same format.
+1 to remove "rte_pmd_debug_trace()" option, but I guess a new version is required, to switch to '__VA_ARGS__' and perhaps to keep the format same, %s vs %s().
Who can send a new version? Jeff?
PATCH v2 1/1 eal: simplify RTE_PMD_DEBUG_TRACE
In reply to Stephen HemmingerFrom: Stephen Hemminger <[email protected]>
Use rte_log directly, eliminating no longer used rte_pmd_dev_trace function. This removes variable length array which is problem on Windows and other compilers not doing C99.
Also, drop unused RTE_PROC_PRIMARY macros.
---
V2:
- Changed named variable "args..." to use "..." with ##__VA_LIST__. Pasting is necessary to support case where only the format string, with no arguments, is passed.
--- lib/librte_eal/common/include/rte_dev.h | 44 +++------------------------------ 1 file changed, 4 insertions(+), 40 deletions(-)
lib/librte_eal/common/include/rte_dev.h +4 -40
@@ -43,54 +43,18 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,enum rte_dev_event_type event, void *cb_arg); -__attribute__((format (printf,2 ,0 ))) -static inline void -rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) -{ -va_list ap; - -va_start (ap, fmt); - - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; - -va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); - -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } -} -/* * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the * RTE_*_RET() macros defined below is compiled in debug mode. */ #if defined(RTE_LIBRTE_EVENTDEV_DEBUG ) -#define RTE_PMD_DEBUG_TRACE (...) \ - rte_pmd_debug_trace(__func__, __VA_ARGS__) +#define RTE_PMD_DEBUG_TRACE (fmt, ...) \ + rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: " fmt, __func__, \ + ##__VA_ARGS__)#else -#define RTE_PMD_DEBUG_TRACE (...) (void)0 +#define RTE_PMD_DEBUG_TRACE (...) do { } while(0)#endif -/* Macros for checking for restricting functions to primary instance only */ -#define RTE_PROC_PRIMARY_OR_ERR_RET (retval) do { \ - if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \ - RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ - return retval; \ - } \ -} while (0) - -#define RTE_PROC_PRIMARY_OR_RET () do { \ - if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \ - RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ - return; \ - } \ -} while (0) -/* Macros to check for invalid function pointers */ #define RTE_FUNC_PTR_OR_ERR_RET (func, retval) do { \ if ((func) == NULL) { \
Signature · 1 line
2.14.3
PATCH v3 1/1 eal: simplify RTE_PMD_DEBUG_TRACE
From: Stephen Hemminger <[email protected]>
Use rte_log directly, eliminating no longer used rte_pmd_dev_trace function. This removes variable length array which is problem on Windows and other compilers not doing C99.
Also, drop unused RTE_PROC_PRIMARY macros.
---
V3:
- Fix checkpatch error:
ERROR:SPACING: space required before the open parenthesis '('
V2:
- Changed named variable "args..." to use "..." with ##__VA_LIST__. Pasting is necessary to support case where only the format string, with no arguments, is passed.
--- lib/librte_eal/common/include/rte_dev.h | 44 +++------------------------------ 1 file changed, 4 insertions(+), 40 deletions(-)
lib/librte_eal/common/include/rte_dev.h +4 -40
@@ -43,54 +43,18 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,enum rte_dev_event_type event, void *cb_arg); -__attribute__((format (printf,2 ,0 ))) -static inline void -rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) -{ -va_list ap; - -va_start (ap, fmt); - - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; - -va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); - -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } -} -/* * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the * RTE_*_RET() macros defined below is compiled in debug mode. */ #if defined(RTE_LIBRTE_EVENTDEV_DEBUG ) -#define RTE_PMD_DEBUG_TRACE (...) \ - rte_pmd_debug_trace(__func__, __VA_ARGS__) +#define RTE_PMD_DEBUG_TRACE (fmt, ...) \ + rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: " fmt, __func__, \ + ##__VA_ARGS__)#else -#define RTE_PMD_DEBUG_TRACE (...) (void)0 +#define RTE_PMD_DEBUG_TRACE (...) do { } while (0)#endif -/* Macros for checking for restricting functions to primary instance only */ -#define RTE_PROC_PRIMARY_OR_ERR_RET (retval) do { \ - if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \ - RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ - return retval; \ - } \ -} while (0) - -#define RTE_PROC_PRIMARY_OR_RET () do { \ - if (rte_eal_process_type() != RTE_PROC_PRIMARY) { \ - RTE_PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ - return; \ - } \ -} while (0) -/* Macros to check for invalid function pointers */ #define RTE_FUNC_PTR_OR_ERR_RET (func, retval) do { \ if ((func) == NULL) { \
Signature · 1 line
2.14.3
Re: [PATCH v3] eal: simplify RTE_PMD_DEBUG_TRACE
Quoted 55 lines · Jeff Shaw
From: Stephen Hemminger <[email protected]>
Use
rte_logdirectly, eliminating no longer usedrte_pmd_dev_tracefunction. This removes variable length array which is problem on Windows and other compilers not doing C99.Also, drop unused
RTE_PROC_PRIMARYmacros.Signed-off-by Stephen Hemminger <[email protected]>Signed-off-by Jeff Shaw <[email protected]>---
V3:
- Fix checkpatch error:
ERROR:SPACING: space required before the open parenthesis '('V2:
- Changed named variable "args..." to use "..." with ##__VA_LIST__. Pasting is necessary to support case where only the format string, with no arguments, is passed.--- lib/librte_eal/common/include/rte_dev.h | 44 +++------------------------------ 1 file changed, 4 insertions(+), 40 deletions(-)lib/librte_eal/common/include/rte_dev.h +0
-22@@ -43,54 +43,18 @@ typedef void (*rte_dev_event_cb_fn)(const char *device_name,enum rte_dev_event_type event, void *cb_arg); -__attribute__((format (printf,2 ,0 ))) -static inline void -rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) -{ -va_list ap; - -va_start (ap, fmt); - - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; - -va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); - -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } -} -
lib/librte_eal/common/include/rte_dev.h typedef void (*rte_dev_event_cb_fn)(const char *device_name, · line 53
Quoted 10 lines
lib/librte_eal/common/include/rte_dev.h +3
-2/* * Enable RTE_PMD_DEBUG_TRACE() when at least one component relying on the * RTE_*_RET() macros defined below is compiled in debug mode. */ #if defined(RTE_LIBRTE_EVENTDEV_DEBUG ) -#define RTE_PMD_DEBUG_TRACE (...) \ - rte_pmd_debug_trace(__func__, __VA_ARGS__) +#define RTE_PMD_DEBUG_TRACE (fmt, ...) \ + rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: " fmt, __func__, \ + ##__VA_ARGS__)
When RTE_PMD_DEBUG_TRACE enabled, mlx4/5 drivers are causing build error because of '-pedantic' argument [1].
error is in 'RTE_FUNC_PTR_OR_ERR_RET' & 'RTE_FUNC_PTR_OR_RET', what do you think removing 'RTE_PMD_DEBUG_TRACE' completely from these macros? Since 'RTE_PMD_DEBUG_TRACE' already enabled only with 'RTE_LIBRTE_EVENTDEV_DEBUG' it is already disabled mostly.
Another option can be replacing 'RTE_PMD_DEBUG_TRACE' with 'rte_log', but with DEBUG log type to not introduce new logs [2].
[1]
error: token pasting of ',' and __VA_ARGS__ is a GNU extension [-Werror,-Wgnu-zero-variadic-macro-arguments]
[2]
- RTE_PMD_DEBUG_TRACE("Function not supported\n"); \
+ rte_log(RTE_LOG_ERR, RTE_LOGTYPE_PMD, "%s: Function not supported\n",__func__); \
14/12/2018 21:28, Mattias Rönnblom:
Quoted 24 lines · Jeff Shaw
Quoted 17 lines
Quoted 11 lines
Quoted 8 lines
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Is
alloca()even included in *any* C standard? As far as I see, it just achieves the same thing in an uglier, less portable way than VLAs.I agree that it is much less elegant than a VLA. This is in preparation for DPDK on Windows, which using the Microsoft Visual C++ (MSVC) compiler. MSVC does not support variable length arrays. It does, however, support
alloca(), as does GCC/ICC.Quoted 5 lines
For this particular instance, the point is moot, since the function is not used anywhere and can just as easily be removed. Though it does not address the issue for the ~100 other instances where VLAs are used. We will be submitting patches for those as more common files are ported to Windows.
Quoted 3 lines
If Microsoft's C compiler doesn't support C99, some 20 years after its release, maybe it's time to find a new compiler, instead of messing up the DPDK code in a ~100 instances.
If think there is no reasonnable compiler for Windows. Yes I know, it's crazy.
Microsoft, should we wait 10 more years to have C99 supported in MSVC?
Quoted 25 lines · Thomas Monjalon
14/12/2018 21:28, Mattias Rönnblom:
Quoted 24 lines · Jeff Shaw
Quoted 22 lines
Quoted 11 lines
Quoted 8 lines
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Is
alloca()even included in *any* C standard? As far as I see, it just achieves the same thing in an uglier, less portable way than VLAs.I agree that it is much less elegant than a VLA. This is in preparation for DPDK on Windows, which using the Microsoft Visual C++ (MSVC) compiler. MSVC does not support variable length arrays. It does, however, support
alloca(), as does GCC/ICC.For this particular instance, the point is moot, since the function is not used anywhere and can just as easily be removed. Though it does not address the issue for the ~100 other instances where VLAs are used. We will be submitting patches for those as more common files are ported to Windows.
Quoted 7 lines
Quoted 3 lines
If Microsoft's C compiler doesn't support C99, some 20 years after its release, maybe it's time to find a new compiler, instead of messing up the DPDK code in a ~100 instances.
If think there is no reasonnable compiler for Windows. Yes I know, it's crazy.
With's wrong with the Windows version of Clang?
20/12/2018 11:53, Mattias Rönnblom:
Quoted 26 lines · Thomas Monjalon
Quoted 25 lines
14/12/2018 21:28, Mattias Rönnblom:
Quoted 24 lines · Jeff Shaw
Quoted 22 lines
Quoted 11 lines
Quoted 8 lines
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Is
alloca()even included in *any* C standard? As far as I see, it just achieves the same thing in an uglier, less portable way than VLAs.I agree that it is much less elegant than a VLA. This is in preparation for DPDK on Windows, which using the Microsoft Visual C++ (MSVC) compiler. MSVC does not support variable length arrays. It does, however, support
alloca(), as does GCC/ICC.For this particular instance, the point is moot, since the function is not used anywhere and can just as easily be removed. Though it does not address the issue for the ~100 other instances where VLAs are used. We will be submitting patches for those as more common files are ported to Windows.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
If Microsoft's C compiler doesn't support C99, some 20 years after its release, maybe it's time to find a new compiler, instead of messing up the DPDK code in a ~100 instances.
If think there is no reasonnable compiler for Windows. Yes I know, it's crazy.
With's wrong with the Windows version of Clang?
I agree it should be the preferred compiler for DPDK on Windows.
But Microsoft told there are some issues working with clang. Jason, Harini, please, could you elaborate?
PATCH v2 1/1 eal: remove variable length array
In reply to Jeff ShawCompilers that do not support the C99 standard, or do not implement gcc extensions, may not support variable length arrays.
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using alloca().
---
V2:
- Reference C99 in commit message instead of C11.
- Remove unnecessary cast of alloca() returning void *.
--- lib/librte_eal/common/include/rte_dev.h | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)
lib/librte_eal/common/include/rte_dev.h +9 -10
@@ -47,22 +47,21 @@ __attribute__((format(printf, 2, 0)))static inline void rte_pmd_debug_trace (const char *func_name,const char *fmt, ...) { +char *buffer; +int buf_len;va_list ap;va_start (ap, fmt); + buf_len =vsnprintf (NULL ,0 , fmt, ap) +1 ; +va_end (ap); - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; + buffer =alloca (buf_len); -va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); +va_start (ap, fmt); +vsnprintf (buffer, buf_len, fmt, ap); +va_end (ap); -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } +rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , func_name, buffer); }/*
Signature · 1 line
2.14.3
Quoted 40 lines · Jeff Shaw
Compilers that do not support the C99 standard, or do not implement gcc extensions, may not support variable length arrays.
The code prior to this commit produced the following warning when compiled with "-Wvla -std=c90".
warning: ISO C90 forbids variable length array ‘array’ [-Wvla]
This commit removes the variable length array from the PMD debug trace function by allocating memory dynamically on the stack using
alloca().Signed-off-by Jeff Shaw <[email protected]>---
V2:
- Reference C99 in commit message instead of C11.
- Remove unnecessary cast ofalloca()returning void *.---
lib/librte_eal/common/include/rte_dev.h| 19 +++++++++----------1 file changed, 9 insertions(+), 10 deletions(-)lib/librte_eal/common/include/rte_dev.h +0
-0@@ -47,22 +47,21 @@ __attribute__((format(printf, 2, 0)))static inline void
rte_pmd_debug_trace(const char *func_name, const char *fmt, ...){lib/librte_eal/common/include/rte_dev.h +2
-0+char *buffer; +int buf_len;va_list ap; va_start(ap, fmt);
lib/librte_eal/common/include/rte_dev.h __attribute__((format(printf, 2, 0))) · line 58
Quoted 6 lines
lib/librte_eal/common/include/rte_dev.h +3
-2+ buf_len =vsnprintf (NULL ,0 , fmt, ap) +1 ; +va_end (ap); - { -char buffer[vsnprintf (NULL ,0 , fmt, ap) +1 ]; + buffer =alloca (buf_len);
Looks like some formatting problems with tab and/or spaces. DPDK is tabs of size 8 and no spaces before tabs.
The code looks fine to me, except for the calling vsnprintf twice. Could we have used char buffer[SOME_NUMBER_THAT_IS_REASONABLE];
This would remove the two calls to vsnprintf() and alloca usage.
If everyone is OK with it then I am ok after the formatting problems get fixed.
Quoted 16 lines
lib/librte_eal/common/include/rte_dev.h +4
-8-va_end (ap); - -va_start (ap, fmt); -vsnprintf (buffer,sizeof (buffer), fmt, ap); -va_end (ap); +va_start (ap, fmt); +vsnprintf (buffer, buf_len, fmt, ap); +va_end (ap); -rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , - func_name, buffer); - } +rte_log (RTE_LOG_ERR ,RTE_LOGTYPE_PMD ,"%s: %s" , func_name, buffer);}
Quoted 4 lines
/*
Signature · 1 line
2.14.3
Regards,
Keith