[PATCH 00/10] fix compilation with -Og

[PATCH 00/10] fix compilation with -Og

[Olivier Matz]

In developer mode (RTE_DEVEL_BUILD=y) where -Werror is passed in the
CFLAGS, the compilation fails with gcc-6.3.0 and EXTRA_CFLAGS=-Og. Some
errors are real bugs (but not critical), while some are false positives
(gcc bugs?).

The solution often consists in initializing a local variable to
ensure the compiler won't complain.

The patchset contains all the fixes needed to properly compile with -Og.
Feedback is welcome to decide if:
1/ we include all of them, even if some are workarounds for
   gcc bugs
2/ we only include the real fixes, without fixing the compilation with
   -Og.


Olivier Matz (10):
  net/bnxt: fix compilation with -Og
  net/qede: fix compilation with -Og
  net/virtio: fix compilation with -Og
  net/i40e: fix compilation with -Og
  uio: fix compilation with -Og
  cmdline: fix compilation with -Og
  metrics: fix compilation with -Og
  lpm6: fix compilation with -Og
  app/test-crypto-perf: fix memory leak
  app/test-crypto-perf: fix compilation with -Og

 app/test-crypto-perf/cperf_test_verify.c  | 5 +++++
 drivers/net/bnxt/rte_pmd_bnxt.c           | 2 +-
 drivers/net/i40e/base/i40e_adminq.c       | 2 +-
 drivers/net/qede/qede_rxtx.c              | 2 +-
 drivers/net/virtio/virtio_rxtx.c          | 2 +-
 lib/librte_cmdline/cmdline_parse.c        | 2 +-
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 2 +-
 lib/librte_lpm/rte_lpm6.c                 | 2 +-
 lib/librte_metrics/rte_metrics.c          | 2 +-
 9 files changed, 13 insertions(+), 8 deletions(-)

-- 
2.11.0

[PATCH 01/10] net/bnxt: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC rte_pmd_bnxt.o
  rte_pmd_bnxt.c: In function ‘rte_pmd_bnxt_set_all_queues_drop_en’:
  rte_pmd_bnxt.c:116:6: error: ‘rc’ may be used uninitialized in this
                        function [-Werror=maybe-uninitialized]
    int rc;
        ^~

This can happen if both bp->nr_vnics and bp->pf.active_vfs are 0.
Fix it by initializing rc to -EINVAL.

Fixes: 49947a13ba9e ("net/bnxt: support Tx loopback, set VF MAC and queues drop")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/bnxt/rte_pmd_bnxt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/bnxt/rte_pmd_bnxt.c b/drivers/net/bnxt/rte_pmd_bnxt.c
index c343d9033..484a1cb63 100644
--- a/drivers/net/bnxt/rte_pmd_bnxt.c
+++ b/drivers/net/bnxt/rte_pmd_bnxt.c
@@ -113,7 +113,7 @@ int rte_pmd_bnxt_set_all_queues_drop_en(uint8_t port, uint8_t on)
 	struct rte_eth_dev *eth_dev;
 	struct bnxt *bp;
 	uint32_t i;
-	int rc;
+	int rc = -EINVAL;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port, -ENODEV);
 
-- 
2.11.0

[PATCH 02/10] net/qede: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC qede_rxtx.o
  qede_rxtx.c: In function ‘qede_start_queues’:
  qede_rxtx.c:797:9: error: ‘rc’ may be used uninitialized in this
                            function [-Werror=maybe-uninitialized]
    return rc;
           ^~

If there is no Rx or Tx queue, rc will not be initialized. Fix it
by initializing rc to -1.

Fixes: 4c4bdadfa9e7 ("net/qede: refactoring multi-queue implementation")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/qede/qede_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
index 5c3613c7c..76e24abff 100644
--- a/drivers/net/qede/qede_rxtx.c
+++ b/drivers/net/qede/qede_rxtx.c
@@ -780,7 +780,7 @@ int qede_start_queues(struct rte_eth_dev *eth_dev)
 {
 	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
 	uint8_t id;
-	int rc;
+	int rc = -1;
 
 	for_each_rss(id) {
 		rc = qede_rx_queue_start(eth_dev, id);
-- 
2.11.0

[PATCH 03/10] net/virtio: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC virtio_rxtx.o
  virtio_rxtx.c: In function ‘virtio_rx_offload’:
  virtio_rxtx.c:680:10: error: ‘csum’ may be used uninitialized in
                        this function [-Werror=maybe-uninitialized]
       csum = ~csum;
       ~~~~~^~~~~~~

The function rte_raw_cksum_mbuf() may indeed return an error, and
in this case, csum won't be initialized. Fix it by initializing csum
to 0.

Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_rxtx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index 45a9c919a..cab3a1675 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -671,7 +671,7 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
 			 * In case of SCTP, this will be wrong since it's a CRC
 			 * but there's nothing we can do.
 			 */
-			uint16_t csum, off;
+			uint16_t csum = 0, off;
 
 			rte_raw_cksum_mbuf(m, hdr->csum_start,
 				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
-- 
2.11.0

[PATCH 04/10] net/i40e: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC i40e_adminq.o
  i40e_adminq.c: In function ‘i40e_clean_arq_element’:
  i40e_adminq.c:1145:56: error: ‘ntu’ may be used uninitialized in
                         this function [-Werror=maybe-uninitialized]
     *pending = (ntc > ntu ? hw->aq.arq.count : 0) + (ntu - ntc);
                                                     ~~~~~^~~~~~

Depending on what is defined, ntu actually be used without being
initialized. Fix it by initializing it to 0, despite this is probably
not the proper fix. Moreover, the error is located in base/ directory,
which contains driver code common to several platforms (not only dpdk).

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/i40e/base/i40e_adminq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/i40e/base/i40e_adminq.c b/drivers/net/i40e/base/i40e_adminq.c
index 8cc8c5eca..27c133323 100644
--- a/drivers/net/i40e/base/i40e_adminq.c
+++ b/drivers/net/i40e/base/i40e_adminq.c
@@ -1047,7 +1047,7 @@ enum i40e_status_code i40e_clean_arq_element(struct i40e_hw *hw,
 	u16 desc_idx;
 	u16 datalen;
 	u16 flags;
-	u16 ntu;
+	u16 ntu = 0;
 
 	/* pre-clean the event info */
 	i40e_memset(&e->desc, 0, sizeof(e->desc), I40E_NONDMA_MEM);
-- 
2.11.0

[PATCH 05/10] uio: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC eal_pci_uio.o
  eal_pci_uio.c: In function ‘pci_get_uio_de ’:
  eal_pci_uio.c:221:9: error: ‘uio_num’ may be used uninitialized in
                       this function [-Werror=maybe-uninitialized]
  return uio_num;
         ^~~~~~~

This is a false positive: gcc is not able to see that when e != NULL,
uio_num is always set. Fix the warning by initializing uio_num to -1,
and by the way, change the type to int.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci_uio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
index fa10329fd..b639ab938 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci_uio.c
@@ -154,7 +154,7 @@ pci_get_uio_dev(struct rte_pci_device *dev, char *dstbuf,
 			   unsigned int buflen, int create)
 {
 	struct rte_pci_addr *loc = &dev->addr;
-	unsigned int uio_num;
+	int uio_num = -1;
 	struct dirent *e;
 	DIR *dir;
 	char dirname[PATH_MAX];
-- 
2.11.0

[PATCH 06/10] cmdline: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC cmdline_parse.o
  cmdline_parse.c: In function ‘match_inst’:
  cmdline_parse.c:227:5: error: ‘token_p’ may be used uninitialized in
                         this function [-Werror=maybe-uninitialized]
    if (token_p) {
       ^

This is a false positive, gcc is not able to see that we always go in
the loop at least once, initializing token_p.

Fix the warning by initializing token_p to NULL.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_cmdline/cmdline_parse.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_cmdline/cmdline_parse.c b/lib/librte_cmdline/cmdline_parse.c
index 56491eacd..3e12ee54f 100644
--- a/lib/librte_cmdline/cmdline_parse.c
+++ b/lib/librte_cmdline/cmdline_parse.c
@@ -163,7 +163,7 @@ static int
 match_inst(cmdline_parse_inst_t *inst, const char *buf,
 	   unsigned int nb_match_token, void *resbuf, unsigned resbuf_size)
 {
-	cmdline_parse_token_hdr_t * token_p;
+	cmdline_parse_token_hdr_t *token_p = NULL;
 	unsigned int i=0;
 	int n = 0;
 	struct cmdline_token_hdr token_hdr;
-- 
2.11.0

[PATCH 07/10] metrics: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC rte_metrics.o
  rte_metrics.c: In function
  ‘rte_metrics_reg_names’: rte_metrics.c:153:22: error: ‘entry’
                           may be used uninitialized in this function
                           [-Werror=maybe-uninitialized]
    entry->idx_next_set = 0;
    ~~~~~~~~~~~~~~~~~~~~^~~

This is a false positive, gcc is not able to see that we always go in
the loop at least once, initializing entry.

Fix the warning by initializing entry to NULL.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_metrics/rte_metrics.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_metrics/rte_metrics.c b/lib/librte_metrics/rte_metrics.c
index b66a72bb0..d9404001a 100644
--- a/lib/librte_metrics/rte_metrics.c
+++ b/lib/librte_metrics/rte_metrics.c
@@ -115,7 +115,7 @@ rte_metrics_reg_name(const char *name)
 int
 rte_metrics_reg_names(const char * const *names, uint16_t cnt_names)
 {
-	struct rte_metrics_meta_s *entry;
+	struct rte_metrics_meta_s *entry = NULL;
 	struct rte_metrics_data_s *stats;
 	const struct rte_memzone *memzone;
 	uint16_t idx_name;
-- 
2.11.0

[PATCH 08/10] lpm6: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC rte_lpm6.o
  rte_lpm6.c: In function ‘rte_lpm6_add_v1705’:
  rte_lpm6.c:442:11: error: ‘tbl_next’ may be used uninitialized in
                             this function [-Werror=maybe-uninitialized]
     if (!tbl[tbl_index].valid) {
             ^
  rte_lpm6.c:521:29: note: ‘tbl_next’ was declared here
    struct rte_lpm6_tbl_entry *tbl_next;
                               ^~~~~~~~

This is a false positive from gcc. Fix it by initializing tbl_next
to NULL.

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 lib/librte_lpm/rte_lpm6.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
index b4a7df348..f9496fe1b 100644
--- a/lib/librte_lpm/rte_lpm6.c
+++ b/lib/librte_lpm/rte_lpm6.c
@@ -518,7 +518,7 @@ rte_lpm6_add_v1705(struct rte_lpm6 *lpm, uint8_t *ip, uint8_t depth,
 		uint32_t next_hop)
 {
 	struct rte_lpm6_tbl_entry *tbl;
-	struct rte_lpm6_tbl_entry *tbl_next;
+	struct rte_lpm6_tbl_entry *tbl_next = NULL;
 	int32_t rule_index;
 	int status;
 	uint8_t masked_ip[RTE_LPM6_IPV6_ADDR_SIZE];
-- 
2.11.0

[PATCH 09/10] app/test-crypto-perf: fix memory leak

[Olivier Matz]

data is allocated but never freed.

Fixes: f8be1786b1b8 ("app/crypto-perf: introduce performance test application")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test-crypto-perf/cperf_test_verify.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/app/test-crypto-perf/cperf_test_verify.c b/app/test-crypto-perf/cperf_test_verify.c
index a314646c2..5221f2251 100644
--- a/app/test-crypto-perf/cperf_test_verify.c
+++ b/app/test-crypto-perf/cperf_test_verify.c
@@ -386,6 +386,7 @@ cperf_verify_op(struct rte_crypto_op *op,
 					options->digest_sz);
 	}
 
+	rte_free(data);
 	return !!res;
 }
 
-- 
2.11.0

[PATCH 10/10] app/test-crypto-perf: fix compilation with -Og

[Olivier Matz]

The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
error:

  CC cperf_test_verify.o
cperf_test_verify.c: In function ‘cperf_verify_op’:
cperf_test_verify.c:382:5: error: ‘auth’ may be used uninitialized
                           in this function
                           [-Werror=maybe-uninitialized]
  if (auth == 1) {
     ^
cperf_test_verify.c:371:5: error: ‘cipher’ may be used uninitialized
                           in this function
			   [-Werror=maybe-uninitialized]
  if (cipher == 1) {
     ^
cperf_test_verify.c:384:11: error: ‘auth_offset’ may be used
			    uninitialized in this function
			    [-Werror=maybe-uninitialized]
    res += memcmp(data + auth_offset,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~
      vector->digest.data,
      ~~~~~~~~~~~~~~~~~~~~
      options->digest_sz);
      ~~~~~~~~~~~~~~~~~~~
cperf_test_verify.c:377:11: error: ‘cipher_offset’ may be used
                            uninitialized in this function
                            [-Werror=maybe-uninitialized]
    res += memcmp(data + cipher_offset,
           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
      vector->plaintext.data,
      ~~~~~~~~~~~~~~~~~~~~~~~
      options->test_buffer_size);
      ~~~~~~~~~~~~~~~~~~~~~~~~~~

There is no default case in the switch statement, so if options->op_type
is an unknown value, the function will use uninitialized values. Fix it
by adding a default.

Fixes: f8be1786b1b8 ("app/crypto-perf: introduce performance test application")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test-crypto-perf/cperf_test_verify.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/test-crypto-perf/cperf_test_verify.c b/app/test-crypto-perf/cperf_test_verify.c
index 5221f2251..36be7b864 100644
--- a/app/test-crypto-perf/cperf_test_verify.c
+++ b/app/test-crypto-perf/cperf_test_verify.c
@@ -366,6 +366,9 @@ cperf_verify_op(struct rte_crypto_op *op,
 		auth = 1;
 		auth_offset = vector->aad.length + options->test_buffer_size;
 		break;
+	default:
+		res = 1;
+		goto out;
 	}
 
 	if (cipher == 1) {
@@ -386,6 +389,7 @@ cperf_verify_op(struct rte_crypto_op *op,
 					options->digest_sz);
 	}
 
+out:
 	rte_free(data);
 	return !!res;
 }
-- 
2.11.0

Re: [PATCH 00/10] fix compilation with -Og

[Bruce Richardson]

On Mon, Sep 11, 2017 at 05:13:23PM +0200, Olivier Matz wrote:
> In developer mode (RTE_DEVEL_BUILD=y) where -Werror is passed in the
> CFLAGS, the compilation fails with gcc-6.3.0 and EXTRA_CFLAGS=-Og. Some
> errors are real bugs (but not critical), while some are false positives
> (gcc bugs?).
> 
> The solution often consists in initializing a local variable to
> ensure the compiler won't complain.
> 
> The patchset contains all the fixes needed to properly compile with -Og.
> Feedback is welcome to decide if:
> 1/ we include all of them, even if some are workarounds for
>    gcc bugs
> 2/ we only include the real fixes, without fixing the compilation with
>    -Og.
> 
Unless it's in a performance critical code path, where additional tests
may be needed to ensure it's not affecting the performance via extra
writes to the variable, I'd say apply them all! No point in leaving
known errors/warnings around when the fixes are generally trivial.

/Bruce

Re: [PATCH 02/10] net/qede: fix compilation with -Og

[Patil, Harish]

-----Original Message-----
From: dev <dev-bounces@dpdk.org> on behalf of Olivier Matz
<olivier.matz@6wind.com>
Date: Monday, September 11, 2017 at 8:13 AM
To: "dev@dpdk.org" <dev@dpdk.org>
Subject: [dpdk-dev] [PATCH 02/10] net/qede: fix compilation with -Og

>The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
>error:
>
>  CC qede_rxtx.o
>  qede_rxtx.c: In function ‘qede_start_queues’:
>  qede_rxtx.c:797:9: error: ‘rc’ may be used uninitialized in this
>                            function [-Werror=maybe-uninitialized]
>    return rc;
>           ^~
>
>If there is no Rx or Tx queue, rc will not be initialized. Fix it
>by initializing rc to -1.
>
>Fixes: 4c4bdadfa9e7 ("net/qede: refactoring multi-queue implementation")
>
>Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>---
> drivers/net/qede/qede_rxtx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/qede/qede_rxtx.c b/drivers/net/qede/qede_rxtx.c
>index 5c3613c7c..76e24abff 100644
>--- a/drivers/net/qede/qede_rxtx.c
>+++ b/drivers/net/qede/qede_rxtx.c
>@@ -780,7 +780,7 @@ int qede_start_queues(struct rte_eth_dev *eth_dev)
> {
> 	struct qede_dev *qdev = QEDE_INIT_QDEV(eth_dev);
> 	uint8_t id;
>-	int rc;
>+	int rc = -1;
> 
> 	for_each_rss(id) {
> 		rc = qede_rx_queue_start(eth_dev, id);
>-- 
>2.11.0

Acked-by: Harish Patil <harish.patil@cavium.com>

>

Re: [PATCH 09/10] app/test-crypto-perf: fix memory leak

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Monday, September 11, 2017 4:14 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 09/10] app/test-crypto-perf: fix memory leak
> 
> data is allocated but never freed.
> 
> Fixes: f8be1786b1b8 ("app/crypto-perf: introduce performance test
> application")
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Re: [PATCH 10/10] app/test-crypto-perf: fix compilation with -Og

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Olivier Matz
> Sent: Monday, September 11, 2017 4:14 PM
> To: dev@dpdk.org
> Subject: [dpdk-dev] [PATCH 10/10] app/test-crypto-perf: fix compilation
> with -Og
> 
> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
> error:
> 
>   CC cperf_test_verify.o
> cperf_test_verify.c: In function ‘cperf_verify_op’:
> cperf_test_verify.c:382:5: error: ‘auth’ may be used uninitialized
>                            in this function
>                            [-Werror=maybe-uninitialized]
>   if (auth == 1) {
>      ^
> cperf_test_verify.c:371:5: error: ‘cipher’ may be used uninitialized
>                            in this function
> 			   [-Werror=maybe-uninitialized]
>   if (cipher == 1) {
>      ^
> cperf_test_verify.c:384:11: error: ‘auth_offset’ may be used
> 			    uninitialized in this function
> 			    [-Werror=maybe-uninitialized]
>     res += memcmp(data + auth_offset,
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~
>       vector->digest.data,
>       ~~~~~~~~~~~~~~~~~~~~
>       options->digest_sz);
>       ~~~~~~~~~~~~~~~~~~~
> cperf_test_verify.c:377:11: error: ‘cipher_offset’ may be used
>                             uninitialized in this function
>                             [-Werror=maybe-uninitialized]
>     res += memcmp(data + cipher_offset,
>            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>       vector->plaintext.data,
>       ~~~~~~~~~~~~~~~~~~~~~~~
>       options->test_buffer_size);
>       ~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> There is no default case in the switch statement, so if options->op_type is
> an unknown value, the function will use uninitialized values. Fix it by adding
> a default.
> 
> Fixes: f8be1786b1b8 ("app/crypto-perf: introduce performance test
> application")
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Acked-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Before applying this patch and patch 9, title should be renamed to
"app/crypto-perf", since that's the convention that we are using.

Thanks,
Pablo

Re: [PATCH 03/10] net/virtio: fix compilation with -Og

[Ferruh Yigit]

On 9/11/2017 4:13 PM, Olivier Matz wrote:
> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
> error:
> 
>   CC virtio_rxtx.o
>   virtio_rxtx.c: In function ‘virtio_rx_offload’:
>   virtio_rxtx.c:680:10: error: ‘csum’ may be used uninitialized in
>                         this function [-Werror=maybe-uninitialized]
>        csum = ~csum;
>        ~~~~~^~~~~~~
> 
> The function rte_raw_cksum_mbuf() may indeed return an error, and
> in this case, csum won't be initialized. Fix it by initializing csum
> to 0.

After this fix if rte_raw_cksum_mbuf() returns error csum still will be
wrong and since compiler warning suppressed it may be harder to find the
root cause.

For this case checking rte_raw_cksum_mbuf() return value seems better
idea but I will cc the maintainer for decision.

> 
> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/virtio/virtio_rxtx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 45a9c919a..cab3a1675 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -671,7 +671,7 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>  			 * In case of SCTP, this will be wrong since it's a CRC
>  			 * but there's nothing we can do.
>  			 */
> -			uint16_t csum, off;
> +			uint16_t csum = 0, off;
>  
>  			rte_raw_cksum_mbuf(m, hdr->csum_start,
>  				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
> 

Re: [PATCH 04/10] net/i40e: fix compilation with -Og

[Ferruh Yigit]

On 9/11/2017 4:13 PM, Olivier Matz wrote:
> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
> error:
> 
>   CC i40e_adminq.o
>   i40e_adminq.c: In function ‘i40e_clean_arq_element’:
>   i40e_adminq.c:1145:56: error: ‘ntu’ may be used uninitialized in
>                          this function [-Werror=maybe-uninitialized]
>      *pending = (ntc > ntu ? hw->aq.arq.count : 0) + (ntu - ntc);
>                                                      ~~~~~^~~~~~
> 
> Depending on what is defined, ntu actually be used without being
> initialized. Fix it by initializing it to 0, despite this is probably
> not the proper fix. Moreover, the error is located in base/ directory,
> which contains driver code common to several platforms (not only dpdk).

This practically seems false positive because driver makefile hard-coded
both defines.

Also ntu used before this location, not sure warning generated for here.

Overall I am for fixing the compile warning, but for decision maintainer
cc'ed because this being base code update requires maintainer
communicate other channels to reflect update into base code.

> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/i40e/base/i40e_adminq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/i40e/base/i40e_adminq.c b/drivers/net/i40e/base/i40e_adminq.c
> index 8cc8c5eca..27c133323 100644
> --- a/drivers/net/i40e/base/i40e_adminq.c
> +++ b/drivers/net/i40e/base/i40e_adminq.c
> @@ -1047,7 +1047,7 @@ enum i40e_status_code i40e_clean_arq_element(struct i40e_hw *hw,
>  	u16 desc_idx;
>  	u16 datalen;
>  	u16 flags;
> -	u16 ntu;
> +	u16 ntu = 0;
>  
>  	/* pre-clean the event info */
>  	i40e_memset(&e->desc, 0, sizeof(e->desc), I40E_NONDMA_MEM);
> 

Re: [PATCH 03/10] net/virtio: fix compilation with -Og

[Ferruh Yigit]

On 10/6/2017 12:17 AM, Ferruh Yigit wrote:
> On 9/11/2017 4:13 PM, Olivier Matz wrote:
>> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
>> error:
>>
>>   CC virtio_rxtx.o
>>   virtio_rxtx.c: In function ‘virtio_rx_offload’:
>>   virtio_rxtx.c:680:10: error: ‘csum’ may be used uninitialized in
>>                         this function [-Werror=maybe-uninitialized]
>>        csum = ~csum;
>>        ~~~~~^~~~~~~
>>
>> The function rte_raw_cksum_mbuf() may indeed return an error, and
>> in this case, csum won't be initialized. Fix it by initializing csum
>> to 0.
> 
> After this fix if rte_raw_cksum_mbuf() returns error csum still will be
> wrong and since compiler warning suppressed it may be harder to find the
> root cause.
> 
> For this case checking rte_raw_cksum_mbuf() return value seems better
> idea but I will cc the maintainer for decision.

Trying harder to add maintainer!

> 
>>
>> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
>> ---
>>  drivers/net/virtio/virtio_rxtx.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
>> index 45a9c919a..cab3a1675 100644
>> --- a/drivers/net/virtio/virtio_rxtx.c
>> +++ b/drivers/net/virtio/virtio_rxtx.c
>> @@ -671,7 +671,7 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>>  			 * In case of SCTP, this will be wrong since it's a CRC
>>  			 * but there's nothing we can do.
>>  			 */
>> -			uint16_t csum, off;
>> +			uint16_t csum = 0, off;
>>  
>>  			rte_raw_cksum_mbuf(m, hdr->csum_start,
>>  				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
>>
> 

Re: [PATCH 08/10] lpm6: fix compilation with -Og

[Ferruh Yigit]

On 9/11/2017 4:13 PM, Olivier Matz wrote:
> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
> error:
> 
>   CC rte_lpm6.o
>   rte_lpm6.c: In function ‘rte_lpm6_add_v1705’:
>   rte_lpm6.c:442:11: error: ‘tbl_next’ may be used uninitialized in
>                              this function [-Werror=maybe-uninitialized]
>      if (!tbl[tbl_index].valid) {
>              ^
>   rte_lpm6.c:521:29: note: ‘tbl_next’ was declared here
>     struct rte_lpm6_tbl_entry *tbl_next;
>                                ^~~~~~~~
> 
> This is a false positive from gcc. Fix it by initializing tbl_next
> to NULL.

This is hard to trace, and it seems there is a way to have it, not sure
practically possible:

rte_lpm6_add_v1705
    add_step
        if (depth <= bits_covered) {
            ...
            return 0 <--- so tbl_next stays untouched.
    tbl = tbl_next
    add_step
        tbl[tbl_index]


So same comment as previous, if there is a practically available path,
this patch makes it harder to find by hiding compiler warning, so adding
maintainer for decision.


> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  lib/librte_lpm/rte_lpm6.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_lpm/rte_lpm6.c b/lib/librte_lpm/rte_lpm6.c
> index b4a7df348..f9496fe1b 100644
> --- a/lib/librte_lpm/rte_lpm6.c
> +++ b/lib/librte_lpm/rte_lpm6.c
> @@ -518,7 +518,7 @@ rte_lpm6_add_v1705(struct rte_lpm6 *lpm, uint8_t *ip, uint8_t depth,
>  		uint32_t next_hop)
>  {
>  	struct rte_lpm6_tbl_entry *tbl;
> -	struct rte_lpm6_tbl_entry *tbl_next;
> +	struct rte_lpm6_tbl_entry *tbl_next = NULL;
>  	int32_t rule_index;
>  	int status;
>  	uint8_t masked_ip[RTE_LPM6_IPV6_ADDR_SIZE];
> 

Re: [PATCH 00/10] fix compilation with -Og

[Ferruh Yigit]

On 9/11/2017 4:13 PM, Olivier Matz wrote:
> In developer mode (RTE_DEVEL_BUILD=y) where -Werror is passed in the
> CFLAGS, the compilation fails with gcc-6.3.0 and EXTRA_CFLAGS=-Og. Some
> errors are real bugs (but not critical), while some are false positives
> (gcc bugs?).
> 
> The solution often consists in initializing a local variable to
> ensure the compiler won't complain.
> 
> The patchset contains all the fixes needed to properly compile with -Og.
> Feedback is welcome to decide if:
> 1/ we include all of them, even if some are workarounds for
>    gcc bugs
> 2/ we only include the real fixes, without fixing the compilation with
>    -Og.
> 
> 
> Olivier Matz (10):
>   net/bnxt: fix compilation with -Og
>   net/qede: fix compilation with -Og
>   net/virtio: fix compilation with -Og
>   net/i40e: fix compilation with -Og
>   uio: fix compilation with -Og
>   cmdline: fix compilation with -Og
>   metrics: fix compilation with -Og
>   lpm6: fix compilation with -Og
>   app/test-crypto-perf: fix memory leak
>   app/test-crypto-perf: fix compilation with -Og

Series applied to dpdk-next-net/master, thanks.

Except 3/10, 4/10, 8/10. Normally we don't get partial sets, that makes
life harder for everyone, but since mentioned ones requires some more
input I didn't want to make rest wait.

When those three concluded would you mind sending another set for them?

Re: [PATCH 03/10] net/virtio: fix compilation with -Og

[Maxime Coquelin]

On 09/11/2017 05:13 PM, Olivier Matz wrote:
> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
> error:
> 
>    CC virtio_rxtx.o
>    virtio_rxtx.c: In function ‘virtio_rx_offload’:
>    virtio_rxtx.c:680:10: error: ‘csum’ may be used uninitialized in
>                          this function [-Werror=maybe-uninitialized]
>         csum = ~csum;
>         ~~~~~^~~~~~~
> 
> The function rte_raw_cksum_mbuf() may indeed return an error, and
> in this case, csum won't be initialized. Fix it by initializing csum
> to 0.
> 
> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>   drivers/net/virtio/virtio_rxtx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 45a9c919a..cab3a1675 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -671,7 +671,7 @@ virtio_rx_offload(struct rte_mbuf *m, struct virtio_net_hdr *hdr)
>   			 * In case of SCTP, this will be wrong since it's a CRC
>   			 * but there's nothing we can do.
>   			 */
> -			uint16_t csum, off;
> +			uint16_t csum = 0, off;
>   
>   			rte_raw_cksum_mbuf(m, hdr->csum_start,
>   				rte_pktmbuf_pkt_len(m) - hdr->csum_start,
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH 00/10] fix compilation with -Og

[Olivier MATZ]

Hi Ferruh,

On Fri, Oct 06, 2017 at 01:26:17AM +0100, Ferruh Yigit wrote:
> On 9/11/2017 4:13 PM, Olivier Matz wrote:
> > In developer mode (RTE_DEVEL_BUILD=y) where -Werror is passed in the
> > CFLAGS, the compilation fails with gcc-6.3.0 and EXTRA_CFLAGS=-Og. Some
> > errors are real bugs (but not critical), while some are false positives
> > (gcc bugs?).
> > 
> > The solution often consists in initializing a local variable to
> > ensure the compiler won't complain.
> > 
> > The patchset contains all the fixes needed to properly compile with -Og.
> > Feedback is welcome to decide if:
> > 1/ we include all of them, even if some are workarounds for
> >    gcc bugs
> > 2/ we only include the real fixes, without fixing the compilation with
> >    -Og.
> > 
> > 
> > Olivier Matz (10):
> >   net/bnxt: fix compilation with -Og
> >   net/qede: fix compilation with -Og
> >   net/virtio: fix compilation with -Og
> >   net/i40e: fix compilation with -Og
> >   uio: fix compilation with -Og
> >   cmdline: fix compilation with -Og
> >   metrics: fix compilation with -Og
> >   lpm6: fix compilation with -Og
> >   app/test-crypto-perf: fix memory leak
> >   app/test-crypto-perf: fix compilation with -Og
> 
> Series applied to dpdk-next-net/master, thanks.
> 
> Except 3/10, 4/10, 8/10. Normally we don't get partial sets, that makes
> life harder for everyone, but since mentioned ones requires some more
> input I didn't want to make rest wait.

Thanks

> When those three concluded would you mind sending another set for them?

Sure, will do as soon as we have the feedback


Olivier

Re: [PATCH 03/10] net/virtio: fix compilation with -Og

[Ferruh Yigit]

On 10/6/2017 7:43 AM, Maxime Coquelin wrote:
> 
> 
> On 09/11/2017 05:13 PM, Olivier Matz wrote:
>> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
>> error:
>>
>>    CC virtio_rxtx.o
>>    virtio_rxtx.c: In function ‘virtio_rx_offload’:
>>    virtio_rxtx.c:680:10: error: ‘csum’ may be used uninitialized in
>>                          this function [-Werror=maybe-uninitialized]
>>         csum = ~csum;
>>         ~~~~~^~~~~~~
>>
>> The function rte_raw_cksum_mbuf() may indeed return an error, and
>> in this case, csum won't be initialized. Fix it by initializing csum
>> to 0.
>>
>> Fixes: 96cb6711939e ("net/virtio: support Rx checksum offload")
>> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied to dpdk-next-net/master, thanks.

Re: [PATCH 04/10] net/i40e: fix compilation with -Og

[Wu, Jingjing]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, October 6, 2017 7:25 AM
> To: Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> Subject: Re: [dpdk-dev] [PATCH 04/10] net/i40e: fix compilation with -Og
> 
> On 9/11/2017 4:13 PM, Olivier Matz wrote:
> > The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the
> > following
> > error:
> >
> >   CC i40e_adminq.o
> >   i40e_adminq.c: In function ‘i40e_clean_arq_element’:
> >   i40e_adminq.c:1145:56: error: ‘ntu’ may be used uninitialized in
> >                          this function [-Werror=maybe-uninitialized]
> >      *pending = (ntc > ntu ? hw->aq.arq.count : 0) + (ntu - ntc);
> >                                                      ~~~~~^~~~~~
> >
> > Depending on what is defined, ntu actually be used without being
> > initialized. Fix it by initializing it to 0, despite this is probably
> > not the proper fix. Moreover, the error is located in base/ directory,
> > which contains driver code common to several platforms (not only dpdk).
> 
> This practically seems false positive because driver makefile hard-coded both
> defines.
> 
> Also ntu used before this location, not sure warning generated for here.
> 
> Overall I am for fixing the compile warning, but for decision maintainer cc'ed
> because this being base code update requires maintainer communicate other
> channels to reflect update into base code.
> 
Right, for base code, we are getting them from internal release.
In principle, based code would keep unchanged until the base code update.

I will go to report this issue to base code develop team, and then make
It be fixed in next base code drop. Is that OK?

Thanks
Jingjing

Re: [PATCH 04/10] net/i40e: fix compilation with -Og

[Olivier MATZ]

On Wed, Oct 11, 2017 at 06:20:35AM +0000, Wu, Jingjing wrote:
> 
> 
> > -----Original Message-----
> > From: Yigit, Ferruh
> > Sent: Friday, October 6, 2017 7:25 AM
> > To: Olivier Matz <olivier.matz@6wind.com>; dev@dpdk.org; Wu, Jingjing
> > <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH 04/10] net/i40e: fix compilation with -Og
> > 
> > On 9/11/2017 4:13 PM, Olivier Matz wrote:
> > > The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the
> > > following
> > > error:
> > >
> > >   CC i40e_adminq.o
> > >   i40e_adminq.c: In function ‘i40e_clean_arq_element’:
> > >   i40e_adminq.c:1145:56: error: ‘ntu’ may be used uninitialized in
> > >                          this function [-Werror=maybe-uninitialized]
> > >      *pending = (ntc > ntu ? hw->aq.arq.count : 0) + (ntu - ntc);
> > >                                                      ~~~~~^~~~~~
> > >
> > > Depending on what is defined, ntu actually be used without being
> > > initialized. Fix it by initializing it to 0, despite this is probably
> > > not the proper fix. Moreover, the error is located in base/ directory,
> > > which contains driver code common to several platforms (not only dpdk).
> > 
> > This practically seems false positive because driver makefile hard-coded both
> > defines.
> > 
> > Also ntu used before this location, not sure warning generated for here.
> > 
> > Overall I am for fixing the compile warning, but for decision maintainer cc'ed
> > because this being base code update requires maintainer communicate other
> > channels to reflect update into base code.
> > 
> Right, for base code, we are getting them from internal release.
> In principle, based code would keep unchanged until the base code update.
> 
> I will go to report this issue to base code develop team, and then make
> It be fixed in next base code drop. Is that OK?

Fine to me, thanks.

Re: [PATCH 08/10] lpm6: fix compilation with -Og

[Bruce Richardson]

On Fri, Oct 06, 2017 at 01:18:00AM +0100, Ferruh Yigit wrote:
> On 9/11/2017 4:13 PM, Olivier Matz wrote:
> > The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
> > error:
> > 
> >   CC rte_lpm6.o
> >   rte_lpm6.c: In function ‘rte_lpm6_add_v1705’:
> >   rte_lpm6.c:442:11: error: ‘tbl_next’ may be used uninitialized in
> >                              this function [-Werror=maybe-uninitialized]
> >      if (!tbl[tbl_index].valid) {
> >              ^
> >   rte_lpm6.c:521:29: note: ‘tbl_next’ was declared here
> >     struct rte_lpm6_tbl_entry *tbl_next;
> >                                ^~~~~~~~
> > 
> > This is a false positive from gcc. Fix it by initializing tbl_next
> > to NULL.
> 
> This is hard to trace, and it seems there is a way to have it, not sure
> practically possible:
> 
> rte_lpm6_add_v1705
>     add_step
>         if (depth <= bits_covered) {
>             ...
>             return 0 <--- so tbl_next stays untouched.
>     tbl = tbl_next
>     add_step
>         tbl[tbl_index]
> 
> 
> So same comment as previous, if there is a practically available path,
> this patch makes it harder to find by hiding compiler warning, so adding
> maintainer for decision.
> 
> 
+Cristian, who has more knowledge of this library than I.

I don't think there is an issue here, since there is an additional check
before the second and subsequent calls to add_step(). The condition
check on the loop in add_v1705 is:

"i < RTE_LPM6_IPV6_ADDR_SIZE && status == 1;"

The "return 0" sets status to 0, causing the loop to break, preventing
any more calls to "tbl = tbl_next".

Therefore I don't think this masks an issue, and the fix is ok.

In the absense of any other objections or comments on this from
Cristian:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH 08/10] lpm6: fix compilation with -Og

[Ferruh Yigit]

On 9/11/2017 8:13 AM, Olivier Matz wrote:
> The compilation with gcc-6.3.0 and EXTRA_CFLAGS=-Og gives the following
> error:
> 
>   CC rte_lpm6.o
>   rte_lpm6.c: In function ‘rte_lpm6_add_v1705’:
>   rte_lpm6.c:442:11: error: ‘tbl_next’ may be used uninitialized in
>                              this function [-Werror=maybe-uninitialized]
>      if (!tbl[tbl_index].valid) {
>              ^
>   rte_lpm6.c:521:29: note: ‘tbl_next’ was declared here
>     struct rte_lpm6_tbl_entry *tbl_next;
>                                ^~~~~~~~
> 
> This is a false positive from gcc. Fix it by initializing tbl_next
> to NULL.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Applied to dpdk-next-net/master, thanks.