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