[PATCH] eal: remove variable length array

[PATCH] eal: remove variable length array

[Jeff Shaw]

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 <jeffrey.b.shaw@intel.com>
---
 lib/librte_eal/common/include/rte_dev.h | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index a9724dc91..af772872b 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -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);
 }
 
 /*
-- 
2.14.3

Re: [PATCH] eal: remove variable length array

[Stephen Hemminger]

On Fri, 14 Dec 2018 08:38:27 -0800
Jeff Shaw <jeffrey.b.shaw@intel.com> 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 <jeffrey.b.shaw@intel.com>
> ---
>  lib/librte_eal/common/include/rte_dev.h | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index a9724dc91..af772872b 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -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);

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.

Re: [PATCH] eal: remove variable length array

[Jeff Shaw]

On Fri, Dec 14, 2018 at 10:36:03AM -0800, Stephen Hemminger wrote:
> On Fri, 14 Dec 2018 08:38:27 -0800
> Jeff Shaw <jeffrey.b.shaw@intel.com> 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 <jeffrey.b.shaw@intel.com>
> > ---
> >  lib/librte_eal/common/include/rte_dev.h | 19 +++++++++----------
> >  1 file changed, 9 insertions(+), 10 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> > index a9724dc91..af772872b 100644
> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -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);
> 
> 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.

> 
> You might be able to skip the buffering step entirely by using rte_log
> a 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.

> 
> But to go further since rte_pmd_debug_trace is not used anywhere in
> current code base in DPDK, it should just be removed.

Much better solution :)

Re: [PATCH] eal: remove variable length array

[Jeff Shaw]

On Fri, Dec 14, 2018 at 10:59:28AM -0800, Jeff Shaw wrote:
> On Fri, Dec 14, 2018 at 10:36:03AM -0800, Stephen Hemminger wrote:
> > On Fri, 14 Dec 2018 08:38:27 -0800
> > Jeff Shaw <jeffrey.b.shaw@intel.com> 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 <jeffrey.b.shaw@intel.com>
> > > ---
> > >  lib/librte_eal/common/include/rte_dev.h | 19 +++++++++----------
> > >  1 file changed, 9 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> > > index a9724dc91..af772872b 100644
> > > --- a/lib/librte_eal/common/include/rte_dev.h
> > > +++ b/lib/librte_eal/common/include/rte_dev.h
> > > @@ -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);
> > 
> > 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.
> 
> > 
> > You might be able to skip the buffering step entirely by using rte_log
> > a 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.
> 
> > 
> > But to go further since rte_pmd_debug_trace is 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.

Re: [PATCH] eal: remove variable length array

[Mattias Rönnblom]

On 2018-12-14 17:38, Jeff Shaw wrote:
> 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.

> 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.

Re: [PATCH] eal: remove variable length array

[Jeff Shaw]

On Fri, Dec 14, 2018 at 07:36:38PM +0100, Mattias Rönnblom wrote:
> On 2018-12-14 17:38, Jeff Shaw wrote:
> > 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.

> 
> > 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.

Re: [PATCH] eal: remove variable length array

[Mattias Rönnblom]

On 2018-12-14 20:07, Jeff Shaw wrote:
>>> 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.
> 

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] eal: simplify RTE_PMD_DEBUG_TRACE

[Stephen Hemminger]

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.

Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
 1 file changed, 3 insertions(+), 40 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index a9724dc9181c..e496da440028 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -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) { \
-- 
2.19.2

Re: [PATCH] eal: simplify RTE_PMD_DEBUG_TRACE

[Jeff Shaw]

On Fri, Dec 14, 2018 at 12:50:55PM -0800, Stephen Hemminger wrote:
> 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.
> 
> Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
>  1 file changed, 3 insertions(+), 40 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index a9724dc9181c..e496da440028 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -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);
> -	}
> -}
> -

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?

>  /*
>   * 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

[Stephen Hemminger]

On Fri, 14 Dec 2018 13:20:07 -0800
Jeff Shaw <jeffrey.b.shaw@intel.com> wrote:

> On Fri, Dec 14, 2018 at 12:50:55PM -0800, Stephen Hemminger wrote:
> > 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.
> > 
> > Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> >  lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
> >  1 file changed, 3 insertions(+), 40 deletions(-)
> > 
> > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> > index a9724dc9181c..e496da440028 100644
> > --- a/lib/librte_eal/common/include/rte_dev.h
> > +++ b/lib/librte_eal/common/include/rte_dev.h
> > @@ -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);
> > -	}
> > -}
> > -  
> 
> 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

[Ferruh Yigit]

On 12/14/2018 9:20 PM, Jeff Shaw wrote:
> On Fri, Dec 14, 2018 at 12:50:55PM -0800, Stephen Hemminger wrote:
>> 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.
>>
>> Reported-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
>> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
>> ---
>>  lib/librte_eal/common/include/rte_dev.h | 43 ++-----------------------
>>  1 file changed, 3 insertions(+), 40 deletions(-)
>>
>> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
>> index a9724dc9181c..e496da440028 100644
>> --- a/lib/librte_eal/common/include/rte_dev.h
>> +++ b/lib/librte_eal/common/include/rte_dev.h
>> @@ -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);
>> -	}
>> -}
>> -
> 
> 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?
> 
>>  /*
>>   * 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.
> 

+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] eal: simplify RTE_PMD_DEBUG_TRACE

[Jeff Shaw]

From: Stephen Hemminger <stephen@networkplumber.org>

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.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
---

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(-)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index a9724dc91..366beb7c0 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -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) { \
-- 
2.14.3

[PATCH v3] eal: simplify RTE_PMD_DEBUG_TRACE

[Jeff Shaw]

From: Stephen Hemminger <stephen@networkplumber.org>

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.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
---

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(-)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index a9724dc91..d7b3c1576 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -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) { \
-- 
2.14.3

Re: [PATCH v3] eal: simplify RTE_PMD_DEBUG_TRACE

[Ferruh Yigit]

On 12/21/2018 6:18 PM, Jeff Shaw wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> 
> 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.
> 
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Jeff Shaw <jeffrey.b.shaw@intel.com>
> ---
> 
> 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(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index a9724dc91..d7b3c1576 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -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__)

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__); \

Re: [PATCH] eal: remove variable length array

[Thomas Monjalon]

14/12/2018 21:28, Mattias Rönnblom:
> On 2018-12-14 20:07, Jeff Shaw wrote:
> >>> 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.
> 
> 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?

Re: [PATCH] eal: remove variable length array

[Mattias Rönnblom]

On 2018-12-19 22:45, Thomas Monjalon wrote:
> 14/12/2018 21:28, Mattias Rönnblom:
>> On 2018-12-14 20:07, Jeff Shaw wrote:
>>>>> 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.
>>
>> 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?

Re: [PATCH] eal: remove variable length array

[Thomas Monjalon]

20/12/2018 11:53, Mattias Rönnblom:
> On 2018-12-19 22:45, Thomas Monjalon wrote:
> > 14/12/2018 21:28, Mattias Rönnblom:
> >> On 2018-12-14 20:07, Jeff Shaw wrote:
> >>>>> 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.
> >>
> >> 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] eal: remove variable length array

[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 <jeffrey.b.shaw@intel.com>
---

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(-)

diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
index a9724dc91..6d7a220b0 100644
--- a/lib/librte_eal/common/include/rte_dev.h
+++ b/lib/librte_eal/common/include/rte_dev.h
@@ -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);
 }
 
 /*
-- 
2.14.3

Re: [PATCH v2] eal: remove variable length array

[Wiles, Keith]

> On Dec 14, 2018, at 2:40 PM, Jeff Shaw <jeffrey.b.shaw@intel.com> wrote:
> 
> 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 <jeffrey.b.shaw@intel.com>
> ---
> 
> 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(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h
> index a9724dc91..6d7a220b0 100644
> --- a/lib/librte_eal/common/include/rte_dev.h
> +++ b/lib/librte_eal/common/include/rte_dev.h
> @@ -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);

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.
> 
> -		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);
> }
> 
> /*
> -- 
> 2.14.3
> 

Regards,
Keith