[Mesa-dev] [PATCH] i965: Use VALGRIND_MAKE_MEM_x in place of MALLOCLIKE/FREELIKE

[Chris Wilson]

Valgrind doesn't actually implement VALGRIND_FREELIKE_BLOCK as the
exact inverse of VALGRIND_MALLOCLIKE_BLOCK. It makes the block
inaccessible, but still leaves it defined in its allocation tracker i.e.
it will report the mmap as lost despite the call to FREELIKE!

Instead of treating the mmap as an allocation, treat it as changing the
access bits upon the memory, i.e. that it becomes defined (because of
the buffer objects always contain valid content from the user's
perspective) upon mmap and inaccessible upon munmap. This makes memcheck
happy without leaving it thinking there is a very large leak.

Finally for consistency, we treat all the mmap/munmap paths the same
even though valgrind can intercept the regular mmap used for GTT. We
could move this in the drm_mmap/drm_munmap macros, but that quickly
looks ugly given the desire for those to support different OSes, but I
didn't try that hard!

Cc: Kenneth Graunke <kenneth at whitecape.org>
Cc: Matt Turner <mattst88 at gmail.com>
---
 src/mesa/drivers/dri/i965/brw_bufmgr.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c b/src/mesa/drivers/dri/i965/brw_bufmgr.c
index d468011774..a73fea95b1 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
@@ -78,6 +78,16 @@
 #define VG(x)
 #endif
 
+/* VALGRIND_FREELIKE_BLOCK unfortunately does not actually undo the earlier
+ * VALGRIND_MALLOCLIKE_BLOCK but instead leaves vg convinced the memory is
+ * leaked. All because it does not call VG(cli_free) from its
+ * VG_USERREQ__FREELIKE_BLOCK handler. Instead of treating the memory like
+ * and allocation, we mark it available for use upon mmapping and remove
+ * it upon unmapping.
+ */
+#define VG_DEFINED(ptr, size) VG(VALGRIND_MAKE_MEM_DEFINED(ptr, size))
+#define VG_NOACCESS(ptr, size) VG(VALGRIND_MAKE_MEM_NOACCESS(ptr, size))
+
 #define memclear(s) memset(&s, 0, sizeof(s))
 
 #define FILE_DEBUG_FLAG DEBUG_BUFMGR
@@ -531,14 +541,15 @@ bo_free(struct brw_bo *bo)
    struct hash_entry *entry;
 
    if (bo->map_cpu) {
-      VG(VALGRIND_FREELIKE_BLOCK(bo->map_cpu, 0));
+      VG_NOACCESS(bo->map_cpu, bo->size);
       drm_munmap(bo->map_cpu, bo->size);
    }
    if (bo->map_wc) {
-      VG(VALGRIND_FREELIKE_BLOCK(bo->map_wc, 0));
+      VG_NOACCESS(bo->map_wc, bo->size);
       drm_munmap(bo->map_wc, bo->size);
    }
    if (bo->map_gtt) {
+      VG_NOACCESS(bo->map_gtt, bo->size);
       drm_munmap(bo->map_gtt, bo->size);
    }
 
@@ -723,14 +734,16 @@ brw_bo_map_cpu(struct brw_context *brw, struct brw_bo *bo, unsigned flags)
              __FILE__, __LINE__, bo->gem_handle, bo->name);
          return NULL;
       }
-      VG(VALGRIND_MALLOCLIKE_BLOCK(mmap_arg.addr_ptr, mmap_arg.size, 0, 1));
       map = (void *) (uintptr_t) mmap_arg.addr_ptr;
+      VG_DEFINED(map, bo->size);
 
       if (p_atomic_cmpxchg(&bo->map_cpu, NULL, map)) {
-         VG(VALGRIND_FREELIKE_BLOCK(map, 0));
+         VG_NOACCESS(map, bo->size);
          drm_munmap(map, bo->size);
       }
    }
+   assert(bo->map_cpu);
+
    DBG("brw_bo_map_cpu: %d (%s) -> %p, ", bo->gem_handle, bo->name,
        bo->map_cpu);
    print_flags(flags);
@@ -765,9 +778,7 @@ brw_bo_map_gtt(struct brw_context *brw, struct brw_bo *bo, unsigned flags)
          return NULL;
       }
 
-      /* and mmap it.  We don't need to use VALGRIND_MALLOCLIKE_BLOCK
-       * because Valgrind will already intercept this mmap call.
-       */
+      /* and mmap it. */
       map = drm_mmap(0, bo->size, PROT_READ | PROT_WRITE,
                      MAP_SHARED, bufmgr->fd, mmap_arg.offset);
       if (map == MAP_FAILED) {
@@ -776,10 +787,19 @@ brw_bo_map_gtt(struct brw_context *brw, struct brw_bo *bo, unsigned flags)
          return NULL;
       }
 
+      /* We don't need to use VALGRIND_MALLOCLIKE_BLOCK
+       * because Valgrind will already intercept this mmap call. However, for
+       * consistency between all the mmap paths, we mark the pointer as defined
+       * now and mark it as inaccessible afterwards.
+       */
+      VG_DEFINED(map, bo->size);
+
       if (p_atomic_cmpxchg(&bo->map_gtt, NULL, map)) {
+         VG_NOACCESS(map, bo->size);
          drm_munmap(map, bo->size);
       }
    }
+   assert(bo->map_gtt);
 
    DBG("bo_map_gtt: %d (%s) -> %p, ", bo->gem_handle, bo->name, bo->map_gtt);
    print_flags(flags);
-- 
2.13.2