If a race error is detected, check to see whether the raced-on address

is inside a heap block, and if so, print the allocation point of the
heap block.  It's stupid not to do this considering that the
implementation already keeps track of all mallocs and frees.

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@11089

diff --git a/helgrind/hg_errors.c b/helgrind/hg_errors.c
index 981f4bf9eca76d471f00e86295cdd393e793739c..a4d0bdbae585aa083cc787022a89ce431d4f1dd2 100644 (file)
--- a/helgrind/hg_errors.c
+++ b/helgrind/hg_errors.c
@@ -178,8 +178,15 @@ typedef
             Int         szB;
             Bool        isWrite;
             Thread*     thr;
+            /* descr1/2 provide a description of stack/global locs */
             XArray*     descr1; /* XArray* of HChar */
             XArray*     descr2; /* XArray* of HChar */
+            /* halloc/haddr/hszB describe the addr if it is a heap block. */
+            ExeContext* hctxt;
+            Addr        haddr;
+            SizeT       hszB;
+            /* h1_* and h2_* provide some description of a previously
+               observed access with which we are conflicting. */
             Thread*     h1_ct; /* non-NULL means h1 info present */
             ExeContext* h1_ct_mbsegstartEC;
             ExeContext* h1_ct_mbsegendEC;
@@ -263,33 +270,47 @@ UInt HG_(update_extra) ( Error* err )
       if (0)
          VG_(printf)("HG_(update_extra): "
                      "%d conflicting-event queries\n", xxx);
+
+      tl_assert(!xe->XE.Race.hctxt);
       tl_assert(!xe->XE.Race.descr1);
       tl_assert(!xe->XE.Race.descr2);
 
-      xe->XE.Race.descr1
-         = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr1",
-                       HG_(free), sizeof(HChar) );
-      xe->XE.Race.descr2
-         = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr2",
-                       HG_(free), sizeof(HChar) );
-
-      (void) VG_(get_data_description)( xe->XE.Race.descr1,
-                                        xe->XE.Race.descr2,
-                                        xe->XE.Race.data_addr );
-
-      /* If there's nothing in descr1/2, free it.  Why is it safe to
-         to VG_(indexXA) at zero here?  Because
-         VG_(get_data_description) guarantees to zero terminate
-         descr1/2 regardless of the outcome of the call.  So there's
-         always at least one element in each XA after the call.
-      */
-      if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr1, 0 ))) {
-         VG_(deleteXA)( xe->XE.Race.descr1 );
-         xe->XE.Race.descr1 = NULL;
-      }
-      if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr2, 0 ))) {
-         VG_(deleteXA)( xe->XE.Race.descr2 );
-         xe->XE.Race.descr2 = NULL;
+      /* First, see if it's in any heap block.  Unfortunately this
+         means a linear search through all allocated heap blocks. */
+      HG_(mm_find_containing_block)( 
+         &xe->XE.Race.hctxt, &xe->XE.Race.haddr, &xe->XE.Race.hszB,
+         xe->XE.Race.data_addr
+      );
+
+      if (!xe->XE.Race.hctxt) {
+         /* It's not in any heap block.  See if we can map it to a
+            stack or global symbol. */
+
+         xe->XE.Race.descr1
+            = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr1",
+                          HG_(free), sizeof(HChar) );
+         xe->XE.Race.descr2
+            = VG_(newXA)( HG_(zalloc), "hg.update_extra.Race.descr2",
+                          HG_(free), sizeof(HChar) );
+
+         (void) VG_(get_data_description)( xe->XE.Race.descr1,
+                                           xe->XE.Race.descr2,
+                                           xe->XE.Race.data_addr );
+
+         /* If there's nothing in descr1/2, free it.  Why is it safe to
+            to VG_(indexXA) at zero here?  Because
+            VG_(get_data_description) guarantees to zero terminate
+            descr1/2 regardless of the outcome of the call.  So there's
+            always at least one element in each XA after the call.
+         */
+         if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr1, 0 ))) {
+            VG_(deleteXA)( xe->XE.Race.descr1 );
+            xe->XE.Race.descr1 = NULL;
+         }
+         if (0 == VG_(strlen)( VG_(indexXA)( xe->XE.Race.descr2, 0 ))) {
+            VG_(deleteXA)( xe->XE.Race.descr2 );
+            xe->XE.Race.descr2 = NULL;
+         }
       }
 
       /* And poke around in the conflicting-event map, to see if we
@@ -993,6 +1014,23 @@ void HG_(pp_Error) ( Error* err )
 
       }
 
+      /* If we have a description of the address in terms of a heap
+         block, show it. */
+      if (xe->XE.Race.hctxt) {
+         SizeT delta = err_ga - xe->XE.Race.haddr;
+         if (xml) {
+            emit("  <auxwhat>Address %#lx is %ld bytes inside a block "
+                 "of size %ld alloc'd</auxwhat>\n", err_ga, delta, 
+                 xe->XE.Race.hszB);
+            VG_(pp_ExeContext)( xe->XE.Race.hctxt );
+         } else {
+            emit(" Address %#lx is %ld bytes inside a block "
+                 "of size %ld alloc'd\n", err_ga, delta, 
+                 xe->XE.Race.hszB);
+            VG_(pp_ExeContext)( xe->XE.Race.hctxt );
+         }
+      }
+
       /* If we have a better description of the address, show it.
          Note that in XML mode, it will already by nicely wrapped up
          in tags, either <auxwhat> or <xauxwhat>, so we can just emit

diff --git a/helgrind/hg_errors.h b/helgrind/hg_errors.h
index ee4949185331752c4d1b6cd30a948c7e1f57bef7..359482579d6d3b830bc317edc9c6f953b96cc0f8 100644 (file)
--- a/helgrind/hg_errors.h
+++ b/helgrind/hg_errors.h
@@ -67,6 +67,14 @@ extern ULong HG_(stats__LockN_to_P_get_map_size) ( void );
 extern ULong HG_(stats__string_table_queries);
 extern ULong HG_(stats__string_table_get_map_size) ( void );
 
+/* For error creation: map 'data_addr' to a malloc'd chunk, if any.
+   Slow linear search.  This is an abuse of the normal file structure
+   since this is exported by hg_main.c, not hg_errors.c.  Oh Well. */
+void HG_(mm_find_containing_block)( /*OUT*/ExeContext** where,
+                                    /*OUT*/Addr*        payload,
+                                    /*OUT*/SizeT*       szB,
+                                    Addr                data_addr );
+
 #endif /* ! __HG_ERRORS_H */
 
 /*--------------------------------------------------------------------*/

diff --git a/helgrind/hg_main.c b/helgrind/hg_main.c
index 19dd1f968eac8a30cfc34ce0c01e7cc712e598f4..2f4e13b280532f9b360b95f1511f5df130d0768d 100644 (file)
--- a/helgrind/hg_main.c
+++ b/helgrind/hg_main.c
@@ -3868,6 +3868,36 @@ static SizeT hg_cli_malloc_usable_size ( ThreadId tid, void* p )
 }
 
 
+/* For error creation: map 'data_addr' to a malloc'd chunk, if any.
+   Slow linear search. */
+
+static inline Bool addr_is_in_MM_Chunk( MallocMeta* mm, Addr a )
+{
+   if (a < mm->payload) return False;
+   if (a >= mm->payload + mm->szB) return False;
+   return True;
+}
+
+void HG_(mm_find_containing_block)( /*OUT*/ExeContext** where,
+                                    /*OUT*/Addr*        payload,
+                                    /*OUT*/SizeT*       szB,
+                                    Addr                data_addr )
+{
+   MallocMeta* mm;
+   /* Well, this totally sucks.  But without using an interval tree or
+      some such, it's hard to see how to do better. */
+   VG_(HT_ResetIter)(hg_mallocmeta_table);
+   while ( (mm = VG_(HT_Next)(hg_mallocmeta_table)) ) {
+      if (UNLIKELY(addr_is_in_MM_Chunk(mm, data_addr))) {
+         *where   = mm->where;
+         *payload = mm->payload;
+         *szB     = mm->szB;
+         return;
+      }
+   }
+}
+
+
 /*--------------------------------------------------------------*/
 /*--- Instrumentation                                        ---*/
 /*--------------------------------------------------------------*/