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

[andrey simiklit]

On Fri, Nov 9, 2018 at 2:45 PM Eric Engestrom <eric.engestrom at intel.com>
wrote:

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

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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181112/20521351/attachment-0001.html>