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

Submitted by andrey simiklit on Sept. 11, 2018, 12:42 p.m.

Details

Message ID 1536669727-22009-2-git-send-email-asimiklit.work@gmail.com
State New
Headers show
Series "mesa: fix against several compilation warnings" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Sept. 11, 2018, 12:42 p.m.
From: Andrii Simiklit <andrii.simiklit@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@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(-)

Patch hide | download patch | download mbox

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);
    }
 
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;
    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!");
    fclose(fp);
 
    return assembly;

Comments

On Tuesday, 2018-09-11 15:42:04 +0300, asimiklit.work@gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit@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@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
On Fri, Nov 9, 2018 at 2:45 PM Eric Engestrom <eric.engestrom@intel.com>
wrote:

> On Tuesday, 2018-09-11 15:42:04 +0300, asimiklit.work@gmail.com wrote:
> > From: Andrii Simiklit <andrii.simiklit@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@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?
>

I agree, will fix.


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

Ops, I didn't notice that they are swapped. Thanks a lot :)
I will test it before fix :)


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