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

[Neil Horman <nhorman@tuxdriver.com>]

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
> 
>