[dpdk-dev,07/11] pmdinfogen: fix build warnings

Message ID 1467905790-10597-8-git-send-email-thomas.monjalon@6wind.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Thomas Monjalon July 7, 2016, 3:36 p.m. UTC
  When compiled with a standard clang, pmdinfogen can raise a warning:
    buildtools/pmdinfogen/pmdinfogen.c:365:1: warning:
    control reaches end of non-void function

Actually there can be more warnings with stricter compilers.
In order to catch them early and fix most of them, the DPDK standard flags
WERROR_FLAGS are used.

The warnings fixed are:
    no previous prototype for ...
    no return statement in function returning non-void
    variable ‘secstrings’ set but not used
    ‘sec_name’ defined but not used
    ‘get_symbol_index’ defined but not used
    pointer of type ‘void *’ used in arithmetic

Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")

Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
---
 buildtools/pmdinfogen/Makefile     |  4 +--
 buildtools/pmdinfogen/pmdinfogen.c | 58 ++++++++++++--------------------------
 2 files changed, 20 insertions(+), 42 deletions(-)
  

Comments

Neil Horman July 7, 2016, 5:55 p.m. UTC | #1
On Thu, Jul 07, 2016 at 05:36:26PM +0200, Thomas Monjalon wrote:
> When compiled with a standard clang, pmdinfogen can raise a warning:
>     buildtools/pmdinfogen/pmdinfogen.c:365:1: warning:
>     control reaches end of non-void function
> 
> Actually there can be more warnings with stricter compilers.
> In order to catch them early and fix most of them, the DPDK standard flags
> WERROR_FLAGS are used.
> 
> The warnings fixed are:
>     no previous prototype for ...
>     no return statement in function returning non-void
>     variable ‘secstrings’ set but not used
>     ‘sec_name’ defined but not used
>     ‘get_symbol_index’ defined but not used
>     pointer of type ‘void *’ used in arithmetic
> 
> Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen utility")
> 
> Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> ---
I'm not opposed to any of these changes, but I'm really starting to wonder how
well used/maintained clang is as a toolchain target.  I assert that because,
with my admittedly broken dependency rule, a native clang build for me errors
out in any number of places:

/home/nhorman/git/dpdk/lib/librte_eal/linuxapp/eal/eal_pci.c:392:37: error:
equality comparison with extraneous parentheses [-Werror,-Wparentheses-equality]
 if (((&pci_device_list)->tqh_first == ((void*)0))) {
      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
/home/nhorman/git/dpdk/lib/librte_eal/linuxapp/eal/eal_pci.c:392:37: note:
remove extraneous parentheses around the comparison to silence this warning
 if (((&pci_device_list)->tqh_first == ((void*)0))) {
     ~                              ^            ~
/home/nhorman/git/dpdk/lib/librte_eal/linuxapp/eal/eal_pci.c:392:37: note: use
'=' to turn this equality comparison into an assignment
 if (((&pci_device_list)->tqh_first == ((void*)0))) {

along with several others
...

The above are (in part) asserted based on the definition of the TAILQ_EMPTY macro which is
from glibc, and has been unchanged for a number of years.  This error occurs on
clang 3.7.0 for me.  Not saying we can't fix these warnings your getting, but if
clang has been tripping over the above for I don't know how long, I have to
question how important clang is.

Neil


>  buildtools/pmdinfogen/Makefile     |  4 +--
>  buildtools/pmdinfogen/pmdinfogen.c | 58 ++++++++++++--------------------------
>  2 files changed, 20 insertions(+), 42 deletions(-)
> 
> diff --git a/buildtools/pmdinfogen/Makefile b/buildtools/pmdinfogen/Makefile
> index 125901b..3885d3b 100644
> --- a/buildtools/pmdinfogen/Makefile
> +++ b/buildtools/pmdinfogen/Makefile
> @@ -41,9 +41,9 @@ HOSTAPP = pmdinfogen
>  #
>  SRCS-y += pmdinfogen.c
>  
> -HOST_EXTRA_CFLAGS += -g -I${RTE_OUTPUT}/include
> +HOST_CFLAGS += $(WERROR_FLAGS) -g
> +HOST_CFLAGS += -I$(RTE_OUTPUT)/include
>  
>  DEPDIRS-y += lib/librte_eal
>  
>  include $(RTE_SDK)/mk/rte.hostapp.mk
> -
> diff --git a/buildtools/pmdinfogen/pmdinfogen.c b/buildtools/pmdinfogen/pmdinfogen.c
> index 0947dc6..beb06f1 100644
> --- a/buildtools/pmdinfogen/pmdinfogen.c
> +++ b/buildtools/pmdinfogen/pmdinfogen.c
> @@ -15,6 +15,7 @@
>  #include <limits.h>
>  #include <stdbool.h>
>  #include <errno.h>
> +#include <rte_common.h>
>  #include "pmdinfogen.h"
>  
>  #ifdef RTE_ARCH_64
> @@ -32,7 +33,7 @@ static const char *sym_name(struct elf_info *elf, Elf_Sym *sym)
>  		return "(unknown)";
>  }
>  
> -void *grab_file(const char *filename, unsigned long *size)
> +static void *grab_file(const char *filename, unsigned long *size)
>  {
>  	struct stat st;
>  	void *map = MAP_FAILED;
> @@ -59,7 +60,7 @@ failed:
>    * spaces in the beginning of the line is trimmed away.
>    * Return a pointer to a static buffer.
>    **/
> -void release_file(void *file, unsigned long size)
> +static void release_file(void *file, unsigned long size)
>  {
>  	munmap(file, size);
>  }
> @@ -67,9 +68,8 @@ void release_file(void *file, unsigned long size)
>  
>  static void *get_sym_value(struct elf_info *info, const Elf_Sym *sym)
>  {
> -	void *ptr = (void *)info->hdr + info->sechdrs[sym->st_shndx].sh_offset;
> -
> -	return (void *)(ptr + sym->st_value);
> +	return RTE_PTR_ADD(info->hdr,
> +		info->sechdrs[sym->st_shndx].sh_offset + sym->st_value);
>  }
>  
>  static Elf_Sym *find_sym_in_symtab(struct elf_info *info,
> @@ -95,7 +95,6 @@ static int parse_elf(struct elf_info *info, const char *filename)
>  	Elf_Ehdr *hdr;
>  	Elf_Shdr *sechdrs;
>  	Elf_Sym  *sym;
> -	const char *secstrings;
>  	int endian;
>  	unsigned int symtab_idx = ~0U, symtab_shndx_idx = ~0U;
>  
> @@ -140,7 +139,7 @@ static int parse_elf(struct elf_info *info, const char *filename)
>  	hdr->e_shnum     = TO_NATIVE(endian, 16, hdr->e_shnum);
>  	hdr->e_shstrndx  = TO_NATIVE(endian, 16, hdr->e_shstrndx);
>  
> -	sechdrs = (void *)hdr + hdr->e_shoff;
> +	sechdrs = RTE_PTR_ADD(hdr, hdr->e_shoff);
>  	info->sechdrs = sechdrs;
>  
>  	/* Check if file offset is correct */
> @@ -191,7 +190,6 @@ static int parse_elf(struct elf_info *info, const char *filename)
>  			TO_NATIVE(endian, ADDR_SIZE, sechdrs[i].sh_entsize);
>  	}
>  	/* Find symbol table. */
> -	secstrings = (void *)hdr + sechdrs[info->secindex_strings].sh_offset;
>  	for (i = 1; i < info->num_sections; i++) {
>  		int nobits = sechdrs[i].sh_type == SHT_NOBITS;
>  
> @@ -206,22 +204,22 @@ static int parse_elf(struct elf_info *info, const char *filename)
>  		if (sechdrs[i].sh_type == SHT_SYMTAB) {
>  			unsigned int sh_link_idx;
>  			symtab_idx = i;
> -			info->symtab_start = (void *)hdr +
> -			    sechdrs[i].sh_offset;
> -			info->symtab_stop  = (void *)hdr +
> -			    sechdrs[i].sh_offset + sechdrs[i].sh_size;
> +			info->symtab_start = RTE_PTR_ADD(hdr,
> +				sechdrs[i].sh_offset);
> +			info->symtab_stop  = RTE_PTR_ADD(hdr,
> +				sechdrs[i].sh_offset + sechdrs[i].sh_size);
>  			sh_link_idx = sechdrs[i].sh_link;
> -			info->strtab       = (void *)hdr +
> -			    sechdrs[sh_link_idx].sh_offset;
> +			info->strtab       = RTE_PTR_ADD(hdr,
> +				sechdrs[sh_link_idx].sh_offset);
>  		}
>  
>  		/* 32bit section no. table? ("more than 64k sections") */
>  		if (sechdrs[i].sh_type == SHT_SYMTAB_SHNDX) {
>  			symtab_shndx_idx = i;
> -			info->symtab_shndx_start = (void *)hdr +
> -			    sechdrs[i].sh_offset;
> -			info->symtab_shndx_stop  = (void *)hdr +
> -			    sechdrs[i].sh_offset + sechdrs[i].sh_size;
> +			info->symtab_shndx_start = RTE_PTR_ADD(hdr,
> +				sechdrs[i].sh_offset);
> +			info->symtab_shndx_stop  = RTE_PTR_ADD(hdr,
> +				sechdrs[i].sh_offset + sechdrs[i].sh_size);
>  		}
>  	}
>  	if (!info->symtab_start)
> @@ -262,28 +260,6 @@ static void parse_elf_finish(struct elf_info *info)
>  	}
>  }
>  
> -static const char *sec_name(struct elf_info *elf, int secindex)
> -{
> -	Elf_Shdr *sechdrs = elf->sechdrs;
> -	return (void *)elf->hdr +
> -		elf->sechdrs[elf->secindex_strings].sh_offset +
> -		sechdrs[secindex].sh_name;
> -}
> -
> -static int get_symbol_index(struct elf_info *info, Elf_Sym *sym)
> -{
> -	const char *name =  sym_name(info, sym);
> -	const char *idx;
> -
> -	idx = name;
> -	while (idx) {
> -		if (isdigit(*idx))
> -			return atoi(idx);
> -		idx++;
> -	}
> -	return -1;
> -}
> -
>  struct opt_tag {
>  	const char *suffix;
>  	const char *json_id;
> @@ -362,6 +338,8 @@ static int locate_pmd_entries(struct elf_info *info)
>  			}
>  		}
>  	} while (last);
> +
> +	return 0;
>  }
>  
>  static void output_pmd_info_string(struct elf_info *info, char *outfile)
> -- 
> 2.7.0
> 
>
  
John McNamara July 7, 2016, 9:25 p.m. UTC | #2
> -----Original Message-----

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman

> Sent: Thursday, July 7, 2016 6:55 PM

> To: Thomas Monjalon <thomas.monjalon@6wind.com>

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH 07/11] pmdinfogen: fix build warnings

> 

> On Thu, Jul 07, 2016 at 05:36:26PM +0200, Thomas Monjalon wrote:

> > When compiled with a standard clang, pmdinfogen can raise a warning:

> >     buildtools/pmdinfogen/pmdinfogen.c:365:1: warning:

> >     control reaches end of non-void function

> >

> > Actually there can be more warnings with stricter compilers.

> > In order to catch them early and fix most of them, the DPDK standard

> > flags WERROR_FLAGS are used.

> >

> > The warnings fixed are:

> >     no previous prototype for ...

> >     no return statement in function returning non-void

> >     variable ‘secstrings’ set but not used

> >     ‘sec_name’ defined but not used

> >     ‘get_symbol_index’ defined but not used

> >     pointer of type ‘void *’ used in arithmetic

> >

> > Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen

> > utility")

> >

> > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>

> > ---

> I'm not opposed to any of these changes, but I'm really starting to wonder

> how well used/maintained clang is as a toolchain target.  I assert that

> because, with my admittedly broken dependency rule, a native clang build

> for me errors out in any number of places:

> 

> /home/nhorman/git/dpdk/lib/librte_eal/linuxapp/eal/eal_pci.c:392:37:

> error:

> equality comparison with extraneous parentheses [-Werror,-Wparentheses-

> equality]  if (((&pci_device_list)->tqh_first == ((void*)0))) {

>       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~

> /home/nhorman/git/dpdk/lib/librte_eal/linuxapp/eal/eal_pci.c:392:37: note:

> remove extraneous parentheses around the comparison to silence this

> warning  if (((&pci_device_list)->tqh_first == ((void*)0))) {



It is due to the clang/ccache "issue" that is tripping up everyone. Exporting CCACHE_CPP2=yes should fix it. There was a thread about this earlier in the week.

John.
  
Neil Horman July 8, 2016, 2:51 p.m. UTC | #3
On Thu, Jul 07, 2016 at 09:25:27PM +0000, Mcnamara, John wrote:
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Neil Horman
> > Sent: Thursday, July 7, 2016 6:55 PM
> > To: Thomas Monjalon <thomas.monjalon@6wind.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH 07/11] pmdinfogen: fix build warnings
> > 
> > On Thu, Jul 07, 2016 at 05:36:26PM +0200, Thomas Monjalon wrote:
> > > When compiled with a standard clang, pmdinfogen can raise a warning:
> > >     buildtools/pmdinfogen/pmdinfogen.c:365:1: warning:
> > >     control reaches end of non-void function
> > >
> > > Actually there can be more warnings with stricter compilers.
> > > In order to catch them early and fix most of them, the DPDK standard
> > > flags WERROR_FLAGS are used.
> > >
> > > The warnings fixed are:
> > >     no previous prototype for ...
> > >     no return statement in function returning non-void
> > >     variable ‘secstrings’ set but not used
> > >     ‘sec_name’ defined but not used
> > >     ‘get_symbol_index’ defined but not used
> > >     pointer of type ‘void *’ used in arithmetic
> > >
> > > Fixes: 98b0fdb0ffc6 ("pmdinfogen: add buildtools and pmdinfogen
> > > utility")
> > >
> > > Signed-off-by: Thomas Monjalon <thomas.monjalon@6wind.com>
> > > ---
> > I'm not opposed to any of these changes, but I'm really starting to wonder
> > how well used/maintained clang is as a toolchain target.  I assert that
> > because, with my admittedly broken dependency rule, a native clang build
> > for me errors out in any number of places:
> > 
> > /home/nhorman/git/dpdk/lib/librte_eal/linuxapp/eal/eal_pci.c:392:37:
> > error:
> > equality comparison with extraneous parentheses [-Werror,-Wparentheses-
> > equality]  if (((&pci_device_list)->tqh_first == ((void*)0))) {
> >       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~
> > /home/nhorman/git/dpdk/lib/librte_eal/linuxapp/eal/eal_pci.c:392:37: note:
> > remove extraneous parentheses around the comparison to silence this
> > warning  if (((&pci_device_list)->tqh_first == ((void*)0))) {
> 
> 
> It is due to the clang/ccache "issue" that is tripping up everyone. Exporting CCACHE_CPP2=yes should fix it. There was a thread about this earlier in the week.
> 
> John.
> 
> 

Ah, that did it, yes.  Though I didnt' get any of the warnings the Thomas
encountered when he posted those changes to pmdinfogen either 

Regardless, theres nothing egregious in the chagnes, so I don't see the harm.

Acked-by: Neil Horman <nhorman@tuxdriver.com>
  

Patch

diff --git a/buildtools/pmdinfogen/Makefile b/buildtools/pmdinfogen/Makefile
index 125901b..3885d3b 100644
--- a/buildtools/pmdinfogen/Makefile
+++ b/buildtools/pmdinfogen/Makefile
@@ -41,9 +41,9 @@  HOSTAPP = pmdinfogen
 #
 SRCS-y += pmdinfogen.c
 
-HOST_EXTRA_CFLAGS += -g -I${RTE_OUTPUT}/include
+HOST_CFLAGS += $(WERROR_FLAGS) -g
+HOST_CFLAGS += -I$(RTE_OUTPUT)/include
 
 DEPDIRS-y += lib/librte_eal
 
 include $(RTE_SDK)/mk/rte.hostapp.mk
-
diff --git a/buildtools/pmdinfogen/pmdinfogen.c b/buildtools/pmdinfogen/pmdinfogen.c
index 0947dc6..beb06f1 100644
--- a/buildtools/pmdinfogen/pmdinfogen.c
+++ b/buildtools/pmdinfogen/pmdinfogen.c
@@ -15,6 +15,7 @@ 
 #include <limits.h>
 #include <stdbool.h>
 #include <errno.h>
+#include <rte_common.h>
 #include "pmdinfogen.h"
 
 #ifdef RTE_ARCH_64
@@ -32,7 +33,7 @@  static const char *sym_name(struct elf_info *elf, Elf_Sym *sym)
 		return "(unknown)";
 }
 
-void *grab_file(const char *filename, unsigned long *size)
+static void *grab_file(const char *filename, unsigned long *size)
 {
 	struct stat st;
 	void *map = MAP_FAILED;
@@ -59,7 +60,7 @@  failed:
   * spaces in the beginning of the line is trimmed away.
   * Return a pointer to a static buffer.
   **/
-void release_file(void *file, unsigned long size)
+static void release_file(void *file, unsigned long size)
 {
 	munmap(file, size);
 }
@@ -67,9 +68,8 @@  void release_file(void *file, unsigned long size)
 
 static void *get_sym_value(struct elf_info *info, const Elf_Sym *sym)
 {
-	void *ptr = (void *)info->hdr + info->sechdrs[sym->st_shndx].sh_offset;
-
-	return (void *)(ptr + sym->st_value);
+	return RTE_PTR_ADD(info->hdr,
+		info->sechdrs[sym->st_shndx].sh_offset + sym->st_value);
 }
 
 static Elf_Sym *find_sym_in_symtab(struct elf_info *info,
@@ -95,7 +95,6 @@  static int parse_elf(struct elf_info *info, const char *filename)
 	Elf_Ehdr *hdr;
 	Elf_Shdr *sechdrs;
 	Elf_Sym  *sym;
-	const char *secstrings;
 	int endian;
 	unsigned int symtab_idx = ~0U, symtab_shndx_idx = ~0U;
 
@@ -140,7 +139,7 @@  static int parse_elf(struct elf_info *info, const char *filename)
 	hdr->e_shnum     = TO_NATIVE(endian, 16, hdr->e_shnum);
 	hdr->e_shstrndx  = TO_NATIVE(endian, 16, hdr->e_shstrndx);
 
-	sechdrs = (void *)hdr + hdr->e_shoff;
+	sechdrs = RTE_PTR_ADD(hdr, hdr->e_shoff);
 	info->sechdrs = sechdrs;
 
 	/* Check if file offset is correct */
@@ -191,7 +190,6 @@  static int parse_elf(struct elf_info *info, const char *filename)
 			TO_NATIVE(endian, ADDR_SIZE, sechdrs[i].sh_entsize);
 	}
 	/* Find symbol table. */
-	secstrings = (void *)hdr + sechdrs[info->secindex_strings].sh_offset;
 	for (i = 1; i < info->num_sections; i++) {
 		int nobits = sechdrs[i].sh_type == SHT_NOBITS;
 
@@ -206,22 +204,22 @@  static int parse_elf(struct elf_info *info, const char *filename)
 		if (sechdrs[i].sh_type == SHT_SYMTAB) {
 			unsigned int sh_link_idx;
 			symtab_idx = i;
-			info->symtab_start = (void *)hdr +
-			    sechdrs[i].sh_offset;
-			info->symtab_stop  = (void *)hdr +
-			    sechdrs[i].sh_offset + sechdrs[i].sh_size;
+			info->symtab_start = RTE_PTR_ADD(hdr,
+				sechdrs[i].sh_offset);
+			info->symtab_stop  = RTE_PTR_ADD(hdr,
+				sechdrs[i].sh_offset + sechdrs[i].sh_size);
 			sh_link_idx = sechdrs[i].sh_link;
-			info->strtab       = (void *)hdr +
-			    sechdrs[sh_link_idx].sh_offset;
+			info->strtab       = RTE_PTR_ADD(hdr,
+				sechdrs[sh_link_idx].sh_offset);
 		}
 
 		/* 32bit section no. table? ("more than 64k sections") */
 		if (sechdrs[i].sh_type == SHT_SYMTAB_SHNDX) {
 			symtab_shndx_idx = i;
-			info->symtab_shndx_start = (void *)hdr +
-			    sechdrs[i].sh_offset;
-			info->symtab_shndx_stop  = (void *)hdr +
-			    sechdrs[i].sh_offset + sechdrs[i].sh_size;
+			info->symtab_shndx_start = RTE_PTR_ADD(hdr,
+				sechdrs[i].sh_offset);
+			info->symtab_shndx_stop  = RTE_PTR_ADD(hdr,
+				sechdrs[i].sh_offset + sechdrs[i].sh_size);
 		}
 	}
 	if (!info->symtab_start)
@@ -262,28 +260,6 @@  static void parse_elf_finish(struct elf_info *info)
 	}
 }
 
-static const char *sec_name(struct elf_info *elf, int secindex)
-{
-	Elf_Shdr *sechdrs = elf->sechdrs;
-	return (void *)elf->hdr +
-		elf->sechdrs[elf->secindex_strings].sh_offset +
-		sechdrs[secindex].sh_name;
-}
-
-static int get_symbol_index(struct elf_info *info, Elf_Sym *sym)
-{
-	const char *name =  sym_name(info, sym);
-	const char *idx;
-
-	idx = name;
-	while (idx) {
-		if (isdigit(*idx))
-			return atoi(idx);
-		idx++;
-	}
-	return -1;
-}
-
 struct opt_tag {
 	const char *suffix;
 	const char *json_id;
@@ -362,6 +338,8 @@  static int locate_pmd_entries(struct elf_info *info)
 			}
 		}
 	} while (last);
+
+	return 0;
 }
 
 static void output_pmd_info_string(struct elf_info *info, char *outfile)