[Mesa-dev] [PATCH 1/4] intel/tools: avoid 'ignoring return value' and 'unused variable' warnings

[Eric Engestrom]

On Tuesday, 2018-09-11 15:42:04 +0300, asimiklit.work at gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit at globallogic.com>
> 
> 1. tools/i965_disasm.c:58:4: warning:
>     ignoring return value of ‘fread’,
>     declared with attribute warn_unused_result
>     fread(assembly, *end, 1, fp);
> 
> 2. tools/aub_read.c:271:31: warning: unused variable ‘end’
>     const uint32_t *p = data, *end = data + data_len, *next;
> 
> 3. tools/aub_mem.c:292:13: warning: unused variable ‘res’
>        void *res = mmap((uint8_t *)bo.map + map_offset, 4096, PROT_READ,
>    tools/aub_mem.c:357:13: warning: unused variable ‘res’
>        void *res = mmap((uint8_t *)bo.map + (page - bo.addr), 4096, PROT_READ,
> 
> Signed-off-by: Andrii Simiklit <andrii.simiklit at globallogic.com>
> ---
>  src/intel/tools/aub_mem.c     | 10 ++++++----
>  src/intel/tools/aub_read.c    |  2 +-
>  src/intel/tools/i965_disasm.c |  3 ++-
>  3 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/src/intel/tools/aub_mem.c b/src/intel/tools/aub_mem.c
> index 58b51b7..98e1421 100644
> --- a/src/intel/tools/aub_mem.c
> +++ b/src/intel/tools/aub_mem.c
> @@ -289,8 +289,9 @@ aub_mem_get_ggtt_bo(void *_mem, uint64_t address)
>           continue;
>  
>        uint32_t map_offset = i->virt_addr - address;
> -      void *res = mmap((uint8_t *)bo.map + map_offset, 4096, PROT_READ,
> -                       MAP_SHARED | MAP_FIXED, mem->mem_fd, phys_mem->fd_offset);
> +      MAYBE_UNUSED void *res =
> +            mmap((uint8_t *)bo.map + map_offset, 4096, PROT_READ,
> +                  MAP_SHARED | MAP_FIXED, mem->mem_fd, phys_mem->fd_offset);
>        assert(res != MAP_FAILED);
>     }
>  
> @@ -354,8 +355,9 @@ aub_mem_get_ppgtt_bo(void *_mem, uint64_t address)
>     for (uint64_t page = address; page < end; page += 4096) {
>        struct phys_mem *phys_mem = ppgtt_walk(mem, mem->pml4, page);
>  
> -      void *res = mmap((uint8_t *)bo.map + (page - bo.addr), 4096, PROT_READ,
> -                       MAP_SHARED | MAP_FIXED, mem->mem_fd, phys_mem->fd_offset);
> +      MAYBE_UNUSED void *res =
> +            mmap((uint8_t *)bo.map + (page - bo.addr), 4096, PROT_READ,
> +                  MAP_SHARED | MAP_FIXED, mem->mem_fd, phys_mem->fd_offset);
>        assert(res != MAP_FAILED);
>     }
>  

R-b on the above

> diff --git a/src/intel/tools/aub_read.c b/src/intel/tools/aub_read.c
> index 5b704e8..0a1f84a 100644
> --- a/src/intel/tools/aub_read.c
> +++ b/src/intel/tools/aub_read.c
> @@ -268,7 +268,7 @@ handle_memtrace_mem_write(struct aub_read *read, const uint32_t *p)
>  int
>  aub_read_command(struct aub_read *read, const void *data, uint32_t data_len)
>  {
> -   const uint32_t *p = data, *end = data + data_len, *next;
> +   MAYBE_UNUSED const uint32_t *p = data, *end = data + data_len, *next;

You're flagging all of them as MAYBE_UNUSED; please split out the one
variable for which it applies?

>     uint32_t h, header_length, bias;
>  
>     assert(data_len >= 4);
> diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
> index 73a6760..e207def 100644
> --- a/src/intel/tools/i965_disasm.c
> +++ b/src/intel/tools/i965_disasm.c
> @@ -55,7 +55,8 @@ i965_disasm_read_binary(FILE *fp, size_t *end)
>     if (assembly == NULL)
>        return NULL;
>  
> -   fread(assembly, *end, 1, fp);
> +   MAYBE_UNUSED size_t size = fread(assembly, *end, 1, fp);
> +   assert((size == *end) && "error: unable to read all elements!");

Please split this out in a separate patch, and I don't think this
`size == *end` is right

>From `man 3 fread`:
> On success, fread() and fwrite() return the number of items read or
> written. This number equals the number of bytes transferred only when
> size is 1.

but here, size is `*end` and nmemb is `1`, so I would expect fread to
return 1 on success and 0 on failure.
Have you tested this? What value did you see?

You can test the complete EOF failure path by adding this just before
fread():
  fseek(fp, 0, SEEK_END);

And the more insidious "just a bit too short" failure path with:
  fseek(fp, 1, SEEK_CUR);

>     fclose(fp);
>  
>     return assembly;
> -- 
> 2.7.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev