diff mbox series

eal: remove variable length array

Message ID 20181214163827.9403-1-jeffrey.b.shaw@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series eal: remove variable length array |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing fail Performance Testing issues

Commit Message

Jeff Shaw Dec. 14, 2018, 4:38 p.m. UTC 
  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(-)

Comments

Stephen Hemminger Dec. 14, 2018, 6:36 p.m. UTC | #1 
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.
Mattias Rönnblom Dec. 14, 2018, 6:36 p.m. UTC | #2 
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.
Jeff Shaw Dec. 14, 2018, 6:59 p.m. UTC | #3 
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 :)
Jeff Shaw Dec. 14, 2018, 7:07 p.m. UTC | #4 
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.
Jeff Shaw Dec. 14, 2018, 7:17 p.m. UTC | #5 
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.
Mattias Rönnblom Dec. 14, 2018, 8:28 p.m. UTC | #6 
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.
Thomas Monjalon Dec. 19, 2018, 9:45 p.m. UTC | #7 
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?
Mattias Rönnblom Dec. 20, 2018, 10:53 a.m. UTC | #8 
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?
Thomas Monjalon Dec. 20, 2018, 11:03 a.m. UTC | #9 
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?
diff mbox series

Patch

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