[dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Viacheslav Ovsiienko]

Some compilers (i.e Intel icc) do not recognize GCC diagnostic
pragma, the compiler check is added.

Fixes: a46a42b5cd03 ("net/mlx5: add VF LAG mode bonding device recognition")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 drivers/net/mlx5/mlx5.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 951b9f5..7a3f654 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -2296,11 +2296,15 @@ struct mlx5_dev_spawn_data {
 	if (!file)
 		return -1;
 	MKSTR(format, "%c%us", '%', (unsigned int)(sizeof(ifname) - 1));
-
-	/* Use safe format to check maximal buffer length. */
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
+#pragma GCC diagnostic push
 #pragma GCC diagnostic ignored "-Wformat-nonliteral"
+#endif
+	/* Use safe format to check maximal buffer length. */
 	while (fscanf(file, format, ifname) == 1) {
-#pragma GCC diagnostic error "-Wformat-nonliteral"
+#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
+#pragma GCC diagnostic pop
+#endif
 		char tmp_str[IF_NAMESIZE + 32];
 		struct rte_pci_addr pci_addr;
 		struct mlx5_switch_info	info;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Stephen Hemminger]

On Tue,  1 Oct 2019 11:10:23 +0000
Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:

> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> +#pragma GCC diagnostic push
>  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> +#endif
> +	/* Use safe format to check maximal buffer length. */
>  	while (fscanf(file, format, ifname) == 1) {
> -#pragma GCC diagnostic error "-Wformat-nonliteral"
> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> +#pragma GCC diagnostic pop
> +#endif

This is messy, is there not a better way to do this?

Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Slava Ovsiienko]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, October 1, 2019 17:54
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh <rasland@mellanox.com>; ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc
> pragma
> 
> On Tue,  1 Oct 2019 11:10:23 +0000
> Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> #pragma GCC
> > +diagnostic push
> >  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> > +#endif
> > +	/* Use safe format to check maximal buffer length. */
> >  	while (fscanf(file, format, ifname) == 1) { -#pragma GCC diagnostic
> > error "-Wformat-nonliteral"
> > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> #pragma GCC
> > +diagnostic pop #endif
> 
> This is messy, is there not a better way to do this?

At least I did not find one.

The GCC compile-time format checking feature is nice in general and it worth 
to be engaged. The legitimate fscanf() usage with variable format parameter
causes GCC to emit error/warning, so we should suppress these ones for this
single line. ICC does not emit warning and does not recognize GCC pragmas.
Clang just does not recognize fscanf().

Should we use "#ifndef __INTEL_COMPILER" (typical workaround for
GCC diagnostic pragma in DPDK)? I'm not sure, It is not completely correct.

The alternative I see is to implement dedicated routine to read words from the file,
but it means more code and more run-time resources. It seems not to be 
the right way to push compile-time issues resolving to the run-time.

Defining the macro is not relevant here because this is a single case.

WBR, Slava

Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Stephen Hemminger]

On Tue, 1 Oct 2019 17:15:46 +0000
Slava Ovsiienko <viacheslavo@mellanox.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Tuesday, October 1, 2019 17:54
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> > Darawsheh <rasland@mellanox.com>; ferruh.yigit@intel.com
> > Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc
> > pragma
> > 
> > On Tue,  1 Oct 2019 11:10:23 +0000
> > Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> >   
> > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)  
> > #pragma GCC  
> > > +diagnostic push
> > >  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> > > +#endif
> > > +	/* Use safe format to check maximal buffer length. */
> > >  	while (fscanf(file, format, ifname) == 1) { -#pragma GCC diagnostic
> > > error "-Wformat-nonliteral"
> > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)  
> > #pragma GCC  
> > > +diagnostic pop #endif  
> > 
> > This is messy, is there not a better way to do this?  
> 
> At least I did not find one.
> 
> The GCC compile-time format checking feature is nice in general and it worth 
> to be engaged. The legitimate fscanf() usage with variable format parameter
> causes GCC to emit error/warning, so we should suppress these ones for this
> single line. ICC does not emit warning and does not recognize GCC pragmas.
> Clang just does not recognize fscanf().
> 
> Should we use "#ifndef __INTEL_COMPILER" (typical workaround for
> GCC diagnostic pragma in DPDK)? I'm not sure, It is not completely correct.
> 
> The alternative I see is to implement dedicated routine to read words from the file,
> but it means more code and more run-time resources. It seems not to be 
> the right way to push compile-time issues resolving to the run-time.
> 
> Defining the macro is not relevant here because this is a single case.
> 
> WBR, Slava
> 
> 

You are going to a lot of effort to solve a problem of interface name length
which can not happen.  The maximum interface name in linux and bsd is always
15 characters plus null. Therefore there is no need to build a dynamic
format string at all here. Or you could use the assignment allocation modifier
so that the resulting string from fscanf was allocated.

Could you try one of these instead.

[dpdk-dev] [PATCH v2] net/mlx5: fix compilation issue with gcc pragma

[Viacheslav Ovsiienko]

The GCC compiler might generate warning or error if
format parameter of fscanf is not literal. This was
suppressed with GCC specific pragms. Some compilers
(i.e Intel icc) do not recognize GCC diagnostic
pragma. The code was refactored with stringification,
pragmas are not needed anymore.

Fixes: a46a42b5cd03 ("net/mlx5: add VF LAG mode bonding device recognition")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
v2: code rewritten with stringification
v1: http://patches.dpdk.org/patch/60310/

 drivers/net/mlx5/mlx5.c       | 6 +-----
 drivers/net/mlx5/mlx5_utils.h | 4 ++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 951b9f5..f751728 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -2295,12 +2295,8 @@ struct mlx5_dev_spawn_data {
 	}
 	if (!file)
 		return -1;
-	MKSTR(format, "%c%us", '%', (unsigned int)(sizeof(ifname) - 1));
-
 	/* Use safe format to check maximal buffer length. */
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-	while (fscanf(file, format, ifname) == 1) {
-#pragma GCC diagnostic error "-Wformat-nonliteral"
+	while (fscanf(file, "%" STRINGIFY(IF_NAMESIZE) "s", ifname) == 1) {
 		char tmp_str[IF_NAMESIZE + 32];
 		struct rte_pci_addr pci_addr;
 		struct mlx5_switch_info	info;
diff --git a/drivers/net/mlx5/mlx5_utils.h b/drivers/net/mlx5/mlx5_utils.h
index 97092c7..8bafeaa 100644
--- a/drivers/net/mlx5/mlx5_utils.h
+++ b/drivers/net/mlx5/mlx5_utils.h
@@ -150,6 +150,10 @@
 	\
 	snprintf(name, sizeof(name), __VA_ARGS__)
 
+/* Stringification macros. */
+#define STRINGIFY1(x) #x
+#define STRINGIFY(x) STRINGIFY1(x)
+
 /**
  * Return nearest power of two above input value.
  *
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Slava Ovsiienko]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, October 2, 2019 2:41
> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh <rasland@mellanox.com>; ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc
> pragma
> 
> On Tue, 1 Oct 2019 17:15:46 +0000
> Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > Sent: Tuesday, October 1, 2019 17:54
> > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh
> > > <rasland@mellanox.com>; ferruh.yigit@intel.com
> > > Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with
> > > gcc pragma
> > >
> > > On Tue,  1 Oct 2019 11:10:23 +0000
> > > Viacheslav Ovsiienko <viacheslavo@mellanox.com> wrote:
> > >
> > > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> > > #pragma GCC
> > > > +diagnostic push
> > > >  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> > > > +#endif
> > > > +	/* Use safe format to check maximal buffer length. */
> > > >  	while (fscanf(file, format, ifname) == 1) { -#pragma GCC
> > > > diagnostic error "-Wformat-nonliteral"
> > > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> > > #pragma GCC
> > > > +diagnostic pop #endif
> > >
> > > This is messy, is there not a better way to do this?
> >
> > At least I did not find one.
> >
> > The GCC compile-time format checking feature is nice in general and it
> > worth to be engaged. The legitimate fscanf() usage with variable
> > format parameter causes GCC to emit error/warning, so we should
> > suppress these ones for this single line. ICC does not emit warning and does
> not recognize GCC pragmas.
> > Clang just does not recognize fscanf().
> >
> > Should we use "#ifndef __INTEL_COMPILER" (typical workaround for GCC
> > diagnostic pragma in DPDK)? I'm not sure, It is not completely correct.
> >
> > The alternative I see is to implement dedicated routine to read words
> > from the file, but it means more code and more run-time resources. It
> > seems not to be the right way to push compile-time issues resolving to the
> run-time.
> >
> > Defining the macro is not relevant here because this is a single case.
> >
> > WBR, Slava
> >
> >
> 
> You are going to a lot of effort to solve a problem of interface name length
> which can not happen.  The maximum interface name in linux and bsd is
> always 15 characters plus null.

We just have a definition IF_NAMESIZE. If we have the definition - we should follow, right?

> Therefore there is no need to build a dynamic format
> string at all here. Or you could use the assignment allocation modifier so that
> the resulting string from fscanf was allocated.

The allocation modifier has questionable compatibility either,
does involve implicit memory allocations and requires explicit free call.
It seems to be less robust than using a standard length modifier.

> 
> Could you try one of these instead.
It seems there is better solution - stringification,
please see:  http://patches.dpdk.org/patch/60415/
I like stringification not too much, but it seems there is the right place to use one.

WBR, Slava
 

Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Slava Ovsiienko]

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Slava Ovsiienko
> Sent: Wednesday, October 2, 2019 9:15
> To: Stephen Hemminger <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh <rasland@mellanox.com>; ferruh.yigit@intel.com
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc
> pragma
> 
> > -----Original Message-----
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Sent: Wednesday, October 2, 2019 2:41
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh
> > <rasland@mellanox.com>; ferruh.yigit@intel.com
> > Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with
> > gcc pragma
> >
> > On Tue, 1 Oct 2019 17:15:46 +0000
> > Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Stephen Hemminger <stephen@networkplumber.org>
> > > > Sent: Tuesday, October 1, 2019 17:54
> > > > To: Slava Ovsiienko <viacheslavo@mellanox.com>
> > > > Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> > Darawsheh
> > > > <rasland@mellanox.com>; ferruh.yigit@intel.com
> > > > Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue
> > > > with gcc pragma
> > > >
> > > > On Tue,  1 Oct 2019 11:10:23 +0000 Viacheslav Ovsiienko
> > > > <viacheslavo@mellanox.com> wrote:
> > > >
> > > > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> > > > #pragma GCC
> > > > > +diagnostic push
> > > > >  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> > > > > +#endif
> > > > > +	/* Use safe format to check maximal buffer length. */
> > > > >  	while (fscanf(file, format, ifname) == 1) { -#pragma GCC
> > > > > diagnostic error "-Wformat-nonliteral"
> > > > > +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> > > > #pragma GCC
> > > > > +diagnostic pop #endif
> > > >
> > > > This is messy, is there not a better way to do this?
> > >
> > > At least I did not find one.
> > >
> > > The GCC compile-time format checking feature is nice in general and
> > > it worth to be engaged. The legitimate fscanf() usage with variable
> > > format parameter causes GCC to emit error/warning, so we should
> > > suppress these ones for this single line. ICC does not emit warning
> > > and does
> > not recognize GCC pragmas.
> > > Clang just does not recognize fscanf().
> > >
> > > Should we use "#ifndef __INTEL_COMPILER" (typical workaround for GCC
> > > diagnostic pragma in DPDK)? I'm not sure, It is not completely correct.
> > >
> > > The alternative I see is to implement dedicated routine to read
> > > words from the file, but it means more code and more run-time
> > > resources. It seems not to be the right way to push compile-time
> > > issues resolving to the
> > run-time.
> > >
> > > Defining the macro is not relevant here because this is a single case.
> > >
> > > WBR, Slava
> > >
> > >
> >
> > You are going to a lot of effort to solve a problem of interface name
> > length which can not happen.  The maximum interface name in linux and
> > bsd is always 15 characters plus null.
> 
> We just have a definition IF_NAMESIZE. If we have the definition - we should
> follow, right?
> 
> > Therefore there is no need to build a dynamic format string at all
> > here. Or you could use the assignment allocation modifier so that the
> > resulting string from fscanf was allocated.
> 
> The allocation modifier has questionable compatibility either, does involve
> implicit memory allocations and requires explicit free call.
> It seems to be less robust than using a standard length modifier.
> 
> >
> > Could you try one of these instead.
> It seems there is better solution - stringification, please see:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatche
> s.dpdk.org%2Fpatch%2F60415%2F&amp;data=02%7C01%7Cviacheslavo%40
> mellanox.com%7Cfad3629b2e694dde62f908d746ffe45a%7Ca652971c7d2e4
> d9ba6a4d149256f461b%7C0%7C0%7C637055937198169033&amp;sdata=vx
> EXTvYh12PJdU9ZmlqAzIThILKmAG23r4OyKE0DBvU%3D&amp;reserved=0
> I like stringification not too much, but it seems there is the right place to use
> one.

Also, I would add something like this:

assert(atol(STRINGIFY(IF_NAMESIZE)) == IF_NAMESIZE);

to make sure IF_NAMESIZE is not defined like as "BASESIZE+1".
What do you think ?

WBR, Slava

Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Ferruh Yigit]

On 10/2/2019 7:55 AM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Slava Ovsiienko
>> Sent: Wednesday, October 2, 2019 9:15
>> To: Stephen Hemminger <stephen@networkplumber.org>
>> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
>> Darawsheh <rasland@mellanox.com>; ferruh.yigit@intel.com
>> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc
>> pragma
>>
>>> -----Original Message-----
>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>> Sent: Wednesday, October 2, 2019 2:41
>>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
>>> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
>> Darawsheh
>>> <rasland@mellanox.com>; ferruh.yigit@intel.com
>>> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with
>>> gcc pragma
>>>
>>> On Tue, 1 Oct 2019 17:15:46 +0000
>>> Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
>>>
>>>>> -----Original Message-----
>>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>>> Sent: Tuesday, October 1, 2019 17:54
>>>>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
>>>>> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
>>> Darawsheh
>>>>> <rasland@mellanox.com>; ferruh.yigit@intel.com
>>>>> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue
>>>>> with gcc pragma
>>>>>
>>>>> On Tue,  1 Oct 2019 11:10:23 +0000 Viacheslav Ovsiienko
>>>>> <viacheslavo@mellanox.com> wrote:
>>>>>
>>>>>> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
>>>>> #pragma GCC
>>>>>> +diagnostic push
>>>>>>  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
>>>>>> +#endif
>>>>>> +	/* Use safe format to check maximal buffer length. */
>>>>>>  	while (fscanf(file, format, ifname) == 1) { -#pragma GCC
>>>>>> diagnostic error "-Wformat-nonliteral"
>>>>>> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
>>>>> #pragma GCC
>>>>>> +diagnostic pop #endif
>>>>>
>>>>> This is messy, is there not a better way to do this?
>>>>
>>>> At least I did not find one.
>>>>
>>>> The GCC compile-time format checking feature is nice in general and
>>>> it worth to be engaged. The legitimate fscanf() usage with variable
>>>> format parameter causes GCC to emit error/warning, so we should
>>>> suppress these ones for this single line. ICC does not emit warning
>>>> and does
>>> not recognize GCC pragmas.
>>>> Clang just does not recognize fscanf().
>>>>
>>>> Should we use "#ifndef __INTEL_COMPILER" (typical workaround for GCC
>>>> diagnostic pragma in DPDK)? I'm not sure, It is not completely correct.
>>>>
>>>> The alternative I see is to implement dedicated routine to read
>>>> words from the file, but it means more code and more run-time
>>>> resources. It seems not to be the right way to push compile-time
>>>> issues resolving to the
>>> run-time.
>>>>
>>>> Defining the macro is not relevant here because this is a single case.
>>>>
>>>> WBR, Slava
>>>>
>>>>
>>>
>>> You are going to a lot of effort to solve a problem of interface name
>>> length which can not happen.  The maximum interface name in linux and
>>> bsd is always 15 characters plus null.
>>
>> We just have a definition IF_NAMESIZE. If we have the definition - we should
>> follow, right?
>>
>>> Therefore there is no need to build a dynamic format string at all
>>> here. Or you could use the assignment allocation modifier so that the
>>> resulting string from fscanf was allocated.
>>
>> The allocation modifier has questionable compatibility either, does involve
>> implicit memory allocations and requires explicit free call.
>> It seems to be less robust than using a standard length modifier.
>>
>>>
>>> Could you try one of these instead.
>> It seems there is better solution - stringification, please see:
>> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatche
>> s.dpdk.org%2Fpatch%2F60415%2F&amp;data=02%7C01%7Cviacheslavo%40
>> mellanox.com%7Cfad3629b2e694dde62f908d746ffe45a%7Ca652971c7d2e4
>> d9ba6a4d149256f461b%7C0%7C0%7C637055937198169033&amp;sdata=vx
>> EXTvYh12PJdU9ZmlqAzIThILKmAG23r4OyKE0DBvU%3D&amp;reserved=0
>> I like stringification not too much, but it seems there is the right place to use
>> one.

+1, this is better than the pragma

But there is already 'RTE_STR' for stringify, can you please use it?


> 
> Also, I would add something like this:
> 
> assert(atol(STRINGIFY(IF_NAMESIZE)) == IF_NAMESIZE);
> 
> to make sure IF_NAMESIZE is not defined like as "BASESIZE+1".
> What do you think ?

I think fscanf() will give a build error in that case, so may not need assertion.

> 
> WBR, Slava
> 

[dpdk-dev] [PATCH v3] net/mlx5: fix compilation issue with gcc pragma

[Viacheslav Ovsiienko]

The GCC compiler might generate warning or error if
format parameter of fscanf is not literal. This was
suppressed with GCC specific pragms. Some compilers
(i.e Intel icc) do not recognize GCC diagnostic
pragma, so the code was refactored with stringification,
pragmas are not needed anymore.

Fixes: a46a42b5cd03 ("net/mlx5: add VF LAG mode bonding device recognition")

Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
---
 v3: replace with RTE_STR stringifier

 v2: http://patches.dpdk.org/patch/60415/
     code rewritten with stringification

 v1: http://patches.dpdk.org/patch/60310/
     initial version with pragmas and compiler type check

 drivers/net/mlx5/mlx5.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/net/mlx5/mlx5.c b/drivers/net/mlx5/mlx5.c
index 951b9f5..13d112e 100644
--- a/drivers/net/mlx5/mlx5.c
+++ b/drivers/net/mlx5/mlx5.c
@@ -2295,12 +2295,9 @@ struct mlx5_dev_spawn_data {
 	}
 	if (!file)
 		return -1;
-	MKSTR(format, "%c%us", '%', (unsigned int)(sizeof(ifname) - 1));
-
 	/* Use safe format to check maximal buffer length. */
-#pragma GCC diagnostic ignored "-Wformat-nonliteral"
-	while (fscanf(file, format, ifname) == 1) {
-#pragma GCC diagnostic error "-Wformat-nonliteral"
+	assert(atol(RTE_STR(IF_NAMESIZE)) == IF_NAMESIZE);
+	while (fscanf(file, "%" RTE_STR(IF_NAMESIZE) "s", ifname) == 1) {
 		char tmp_str[IF_NAMESIZE + 32];
 		struct rte_pci_addr pci_addr;
 		struct mlx5_switch_info	info;
-- 
1.8.3.1

Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc pragma

[Slava Ovsiienko]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Wednesday, October 2, 2019 14:54
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Stephen Hemminger
> <stephen@networkplumber.org>
> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh <rasland@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with gcc
> pragma
> 
> On 10/2/2019 7:55 AM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: dev <dev-bounces@dpdk.org> On Behalf Of Slava Ovsiienko
> >> Sent: Wednesday, October 2, 2019 9:15
> >> To: Stephen Hemminger <stephen@networkplumber.org>
> >> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> Darawsheh
> >> <rasland@mellanox.com>; ferruh.yigit@intel.com
> >> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with
> >> gcc pragma
> >>
> >>> -----Original Message-----
> >>> From: Stephen Hemminger <stephen@networkplumber.org>
> >>> Sent: Wednesday, October 2, 2019 2:41
> >>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> >>> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> >> Darawsheh
> >>> <rasland@mellanox.com>; ferruh.yigit@intel.com
> >>> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue with
> >>> gcc pragma
> >>>
> >>> On Tue, 1 Oct 2019 17:15:46 +0000
> >>> Slava Ovsiienko <viacheslavo@mellanox.com> wrote:
> >>>
> >>>>> -----Original Message-----
> >>>>> From: Stephen Hemminger <stephen@networkplumber.org>
> >>>>> Sent: Tuesday, October 1, 2019 17:54
> >>>>> To: Slava Ovsiienko <viacheslavo@mellanox.com>
> >>>>> Cc: dev@dpdk.org; Matan Azrad <matan@mellanox.com>; Raslan
> >>> Darawsheh
> >>>>> <rasland@mellanox.com>; ferruh.yigit@intel.com
> >>>>> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix compilation issue
> >>>>> with gcc pragma
> >>>>>
> >>>>> On Tue,  1 Oct 2019 11:10:23 +0000 Viacheslav Ovsiienko
> >>>>> <viacheslavo@mellanox.com> wrote:
> >>>>>
> >>>>>> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> >>>>> #pragma GCC
> >>>>>> +diagnostic push
> >>>>>>  #pragma GCC diagnostic ignored "-Wformat-nonliteral"
> >>>>>> +#endif
> >>>>>> +	/* Use safe format to check maximal buffer length. */
> >>>>>>  	while (fscanf(file, format, ifname) == 1) { -#pragma GCC
> >>>>>> diagnostic error "-Wformat-nonliteral"
> >>>>>> +#if defined(RTE_TOOLCHAIN_GCC) && (GCC_VERSION >= 40600)
> >>>>> #pragma GCC
> >>>>>> +diagnostic pop #endif
> >>>>>
> >>>>> This is messy, is there not a better way to do this?
> >>>>
> >>>> At least I did not find one.
> >>>>
> >>>> The GCC compile-time format checking feature is nice in general and
> >>>> it worth to be engaged. The legitimate fscanf() usage with variable
> >>>> format parameter causes GCC to emit error/warning, so we should
> >>>> suppress these ones for this single line. ICC does not emit warning
> >>>> and does
> >>> not recognize GCC pragmas.
> >>>> Clang just does not recognize fscanf().
> >>>>
> >>>> Should we use "#ifndef __INTEL_COMPILER" (typical workaround for
> >>>> GCC diagnostic pragma in DPDK)? I'm not sure, It is not completely
> correct.
> >>>>
> >>>> The alternative I see is to implement dedicated routine to read
> >>>> words from the file, but it means more code and more run-time
> >>>> resources. It seems not to be the right way to push compile-time
> >>>> issues resolving to the
> >>> run-time.
> >>>>
> >>>> Defining the macro is not relevant here because this is a single case.
> >>>>
> >>>> WBR, Slava
> >>>>
> >>>>
> >>>
> >>> You are going to a lot of effort to solve a problem of interface
> >>> name length which can not happen.  The maximum interface name in
> >>> linux and bsd is always 15 characters plus null.
> >>
> >> We just have a definition IF_NAMESIZE. If we have the definition - we
> >> should follow, right?
> >>
> >>> Therefore there is no need to build a dynamic format string at all
> >>> here. Or you could use the assignment allocation modifier so that
> >>> the resulting string from fscanf was allocated.
> >>
> >> The allocation modifier has questionable compatibility either, does
> >> involve implicit memory allocations and requires explicit free call.
> >> It seems to be less robust than using a standard length modifier.
> >>
> >>>
> >>> Could you try one of these instead.
> >> It seems there is better solution - stringification, please see:
> >>
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fpatc
> >> he
> >>
> s.dpdk.org%2Fpatch%2F60415%2F&amp;data=02%7C01%7Cviacheslavo%40
> >>
> mellanox.com%7Cfad3629b2e694dde62f908d746ffe45a%7Ca652971c7d2e4
> >>
> d9ba6a4d149256f461b%7C0%7C0%7C637055937198169033&amp;sdata=vx
> >> EXTvYh12PJdU9ZmlqAzIThILKmAG23r4OyKE0DBvU%3D&amp;reserved=0
> >> I like stringification not too much, but it seems there is the right
> >> place to use one.
> 
> +1, this is better than the pragma
> 
> But there is already 'RTE_STR' for stringify, can you please use it?
Thanks for the clue, I did not find it with "grep stringi".

> 
> 
 >
> > Also, I would add something like this:
> >
> > assert(atol(STRINGIFY(IF_NAMESIZE)) == IF_NAMESIZE);
> >
> > to make sure IF_NAMESIZE is not defined like as "BASESIZE+1".
> > What do you think ?
> 
> I think fscanf() will give a build error in that case, so may not need assertion.
Not always, something like "#define IF_NAMESIZE data_len" passes
the compiler check, so I've added assert.

With best regards, Slava

Re: [dpdk-dev] [PATCH v3] net/mlx5: fix compilation issue with gcc pragma

[Ferruh Yigit]

On 10/2/2019 1:36 PM, Viacheslav Ovsiienko wrote:
> The GCC compiler might generate warning or error if
> format parameter of fscanf is not literal. This was
> suppressed with GCC specific pragms. Some compilers
> (i.e Intel icc) do not recognize GCC diagnostic
> pragma, so the code was refactored with stringification,
> pragmas are not needed anymore.
> 
> Fixes: a46a42b5cd03 ("net/mlx5: add VF LAG mode bonding device recognition")
> 
> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>

Squashed into relevant commit in next-net, thanks.