<div dir="ltr"><div dir="ltr"><div dir="ltr"> <div class="gmail_quote"><div dir="ltr">On Fri, Nov 9, 2018 at 2:45 PM Eric Engestrom <<a href="mailto:eric.engestrom@intel.com">eric.engestrom@intel.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tuesday, 2018-09-11 15:42:04 +0300, <a href="mailto:asimiklit.work@gmail.com" target="_blank">asimiklit.work@gmail.com</a> wrote: > From: Andrii Simiklit <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>> > > 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 <<a href="mailto:andrii.simiklit@globallogic.com" target="_blank">andrii.simiklit@globallogic.com</a>> > --- > 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 </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > 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? </blockquote><div> </div><div>I agree, will fix. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > 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); </blockquote><div> </div>Ops, I didn't notice that they are swapped. Thanks a lot :) I will test it before fix :)<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> > fclose(fp); > > return assembly; > -- > 2.7.4 > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></div></div></div></div>