diff mbox series

[v6,07/12] app/test: clean LTO build warnings (maybe-uninitialized)

Message ID 20191029141212.4907-8-aostruszka@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Add an option to use LTO for DPDK build |

Checks

Context Check Description
ci/Intel-compilation fail Compilation issues
ci/checkpatch success coding style OK

Commit Message

Andrzej Ostruszka [C] Oct. 29, 2019, 2:12 p.m. UTC 
  During LTO build compiler reports some 'false positive' warnings about
variables being possibly used uninitialized.  This patch silences these
warnings.

Exemplary compiler warning to suppress (with LTO enabled):
error: ‘stats.greatest_free_size’ may be used uninitialized in this
function [-Werror=maybe-uninitialized]
  return len - overhead;

Signed-off-by: Andrzej Ostruszka <aostruszka@marvell.com>
---
 app/test/test_hash_readwrite.c     | 2 +-
 app/test/test_link_bonding_mode4.c | 8 +++++++-
 app/test/test_memzone.c            | 3 ++-
 3 files changed, 10 insertions(+), 3 deletions(-)

Comments

Wang, Yipeng1 Nov. 1, 2019, 5:15 p.m. UTC | #1 
>-----Original Message-----
>From: Andrzej Ostruszka [mailto:aostruszka@marvell.com]
>Sent: Tuesday, October 29, 2019 7:12 AM
>To: dev@dpdk.org; Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh <sameh.gobriel@intel.com>; Richardson, Bruce
><bruce.richardson@intel.com>; Chas Williams <chas3@att.com>; Burakov, Anatoly <anatoly.burakov@intel.com>
>Cc: mattias.ronnblom@ericsson.com; stephen@networkplumber.org
>Subject: [PATCH v6 07/12] app/test: clean LTO build warnings (maybe-uninitialized)
>
>During LTO build compiler reports some 'false positive' warnings about
>variables being possibly used uninitialized.  This patch silences these
>warnings.
>
>Exemplary compiler warning to suppress (with LTO enabled):
>error: ‘stats.greatest_free_size’ may be used uninitialized in this
>function [-Werror=maybe-uninitialized]
>  return len - overhead;
>
>Signed-off-by: Andrzej Ostruszka <aostruszka@marvell.com>
>---
> app/test/test_hash_readwrite.c     | 2 +-
> app/test/test_link_bonding_mode4.c | 8 +++++++-
> app/test/test_memzone.c            | 3 ++-
> 3 files changed, 10 insertions(+), 3 deletions(-)
>
>diff --git a/app/test/test_hash_readwrite.c b/app/test/test_hash_readwrite.c
>index 4376b099b..615767fb6 100644
>--- a/app/test/test_hash_readwrite.c
>+++ b/app/test/test_hash_readwrite.c
>@@ -298,7 +298,7 @@ test_rw_reader(void *arg)
>
> 	begin = rte_rdtsc_precise();
> 	for (i = 0; i < read_cnt; i++) {
>-		void *data;
>+		void *data = arg;

[Wang, Yipeng] 
Hi Andrzej, thanks for the fix! Maybe you could initialize the data to be "NULL"? 
I think it makes more sense to be NULL rather than arg.
Andrzej Ostruszka Nov. 4, 2019, 1:48 p.m. UTC | #2 
Yipeng

Thank you for your comment.  See my reply below.

On 11/1/19 6:15 PM, Wang, Yipeng1 wrote:
>> -----Original Message-----
[...]
>> diff --git a/app/test/test_hash_readwrite.c b/app/test/test_hash_readwrite.c
>> index 4376b099b..615767fb6 100644
>> --- a/app/test/test_hash_readwrite.c
>> +++ b/app/test/test_hash_readwrite.c
>> @@ -298,7 +298,7 @@ test_rw_reader(void *arg)
>>
>> 	begin = rte_rdtsc_precise();
>> 	for (i = 0; i < read_cnt; i++) {
>> -		void *data;
>> +		void *data = arg;
> 
> [Wang, Yipeng] 
> Hi Andrzej, thanks for the fix! Maybe you could initialize the data to be "NULL"? 
> I think it makes more sense to be NULL rather than arg.

That actually is not a good idea.  The conditional test below does not
check return value from lookup and expects the returned pointer to data
to be equal to the iteration count (these pointers do not point at
anything).  For the first loop iteration "data = NULL" could miss the
problem of the key not being in the hash (key not in the hash, return
value not checked but the pointer "data" is NULL as we expected since i
= 0).

What I used is a bit hackish - I assumed that arg pointer (which is
pointing to the "read_cnt") will not be within [0-read_cnt] address
range.  If that is too hackish then it would be better to rework this
function to actually test for return value of hash lookup.  Let me know
what you think.

Regards
Andrzej
Wang, Yipeng1 Nov. 7, 2019, 5:48 p.m. UTC | #3 
>-----Original Message-----
>From: Andrzej Ostruszka [mailto:amo@semihalf.com]
>Sent: Monday, November 4, 2019 5:48 AM
>To: Wang, Yipeng1 <yipeng1.wang@intel.com>; dev@dpdk.org; Gobriel, Sameh <sameh.gobriel@intel.com>; Richardson, Bruce
><bruce.richardson@intel.com>; Chas Williams <chas3@att.com>; Burakov, Anatoly <anatoly.burakov@intel.com>
>Cc: mattias.ronnblom@ericsson.com; stephen@networkplumber.org
>Subject: Re: [dpdk-dev] [PATCH v6 07/12] app/test: clean LTO build warnings (maybe-uninitialized)
>
>Yipeng
>
>Thank you for your comment.  See my reply below.
>
>On 11/1/19 6:15 PM, Wang, Yipeng1 wrote:
>>> -----Original Message-----
>[...]
>>> diff --git a/app/test/test_hash_readwrite.c b/app/test/test_hash_readwrite.c
>>> index 4376b099b..615767fb6 100644
>>> --- a/app/test/test_hash_readwrite.c
>>> +++ b/app/test/test_hash_readwrite.c
>>> @@ -298,7 +298,7 @@ test_rw_reader(void *arg)
>>>
>>> 	begin = rte_rdtsc_precise();
>>> 	for (i = 0; i < read_cnt; i++) {
>>> -		void *data;
>>> +		void *data = arg;
>>
>> [Wang, Yipeng]
>> Hi Andrzej, thanks for the fix! Maybe you could initialize the data to be "NULL"?
>> I think it makes more sense to be NULL rather than arg.
>
>That actually is not a good idea.  The conditional test below does not
>check return value from lookup and expects the returned pointer to data
>to be equal to the iteration count (these pointers do not point at
>anything).  For the first loop iteration "data = NULL" could miss the
>problem of the key not being in the hash (key not in the hash, return
>value not checked but the pointer "data" is NULL as we expected since i
>= 0).
>
>What I used is a bit hackish - I assumed that arg pointer (which is
>pointing to the "read_cnt") will not be within [0-read_cnt] address
>range.  If that is too hackish then it would be better to rework this
>function to actually test for return value of hash lookup.  Let me know
>what you think.
>
>Regards
>Andrzej
 [Wang, Yipeng] Thank you for the explanation. I think what you said makes sense.
diff mbox series

Patch

diff --git a/app/test/test_hash_readwrite.c b/app/test/test_hash_readwrite.c
index 4376b099b..615767fb6 100644
--- a/app/test/test_hash_readwrite.c
+++ b/app/test/test_hash_readwrite.c
@@ -298,7 +298,7 @@  test_rw_reader(void *arg)
 
 	begin = rte_rdtsc_precise();
 	for (i = 0; i < read_cnt; i++) {
-		void *data;
+		void *data = arg;
 		rte_hash_lookup_data(tbl_rw_test_param.h,
 				tbl_rw_test_param.keys + i,
 				&data);
diff --git a/app/test/test_link_bonding_mode4.c b/app/test/test_link_bonding_mode4.c
index ff54d7b91..cf12f026d 100644
--- a/app/test/test_link_bonding_mode4.c
+++ b/app/test/test_link_bonding_mode4.c
@@ -585,7 +585,13 @@  bond_get_update_timeout_ms(void)
 {
 	struct rte_eth_bond_8023ad_conf conf;
 
-	rte_eth_bond_8023ad_conf_get(test_params.bonded_port_id, &conf);
+	if (rte_eth_bond_8023ad_conf_get(test_params.bonded_port_id, &conf) < 0) {
+		RTE_LOG(DEBUG, EAL, "Failed to get bonding configuration: "
+				    "%s at %d\n", __func__, __LINE__);
+		RTE_TEST_TRACE_FAILURE(__FILE__, __LINE__, __func__);
+		return 0;
+	}
+
 	return conf.update_timeout_ms;
 }
 
diff --git a/app/test/test_memzone.c b/app/test/test_memzone.c
index 4d8744455..0343b0326 100644
--- a/app/test/test_memzone.c
+++ b/app/test/test_memzone.c
@@ -475,7 +475,8 @@  find_max_block_free_size(unsigned int align, unsigned int socket_id)
 	struct rte_malloc_socket_stats stats;
 	size_t len, overhead;
 
-	rte_malloc_get_socket_stats(socket_id, &stats);
+	if (rte_malloc_get_socket_stats(socket_id, &stats) < 0)
+		return 0;
 
 	len = stats.greatest_free_size;
 	overhead = MALLOC_ELEM_OVERHEAD;