Pass in a mask of segment kinds to VG_(get_segment_starts)

and VG_(am_get_segment_starts) to indicate which segments
should be collected. That should solve the following problem:
in m_main.c we used to:

      seg_starts = VG_(get_segment_starts)( &n_seg_starts );

      for (i = 0; i < n_seg_starts; i++) {
         Word j, n;
         NSegment const* seg
            = VG_(am_find_nsegment)( seg_starts[i] );
         vg_assert(seg);
         if (seg->kind == SkFileC || seg->kind == SkAnonC) {

         ...
         // ... dynamic memory allocation for valgrind
         ...
      }

This caused the vassert(seg) to fire because the dynamic memory
allocation further down the loop changed segments such that a
valgrind segment which used to be non-SkFree suddenly became
SkFree and hence VG_(am_find_nsegment) returned NULL. Whoom.

With this revision we only collect the segments we're really
interested in. For the example above that is all client segments.
So if V allocates memory -- fine. That will not change the layout
of client segments.

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

diff --git a/coregrind/m_aspacehl.c b/coregrind/m_aspacehl.c
index 36fb49d1d115522c7f7fa841314ba2e809ef6efd..17eccce2a4b9b091e61b87a427bc6662863f9d8e 100644 (file)
--- a/coregrind/m_aspacehl.c
+++ b/coregrind/m_aspacehl.c
@@ -38,7 +38,9 @@
 // addresses.  The vector is dynamically allocated and should be freed
 // by the caller when done.  REQUIRES m_mallocfree to be running.
 // Writes the number of addresses required into *n_acquired.
-Addr* VG_(get_segment_starts) ( /*OUT*/Int* n_acquired )
+// Only those segments are considered whose kind matches any of the kinds
+// given in KIND_MASK.
+Addr* VG_(get_segment_starts) ( UInt kind_mask, /*OUT*/Int* n_acquired )
 {
    Addr* starts;
    Int   n_starts, r = 0;
@@ -46,7 +48,7 @@ Addr* VG_(get_segment_starts) ( /*OUT*/Int* n_acquired )
    n_starts = 1;
    while (True) {
       starts = VG_(malloc)( "main.gss.1", n_starts * sizeof(Addr) );
-      r = VG_(am_get_segment_starts)( starts, n_starts );
+      r = VG_(am_get_segment_starts)( kind_mask, starts, n_starts );
       if (r >= 0)
          break;
       VG_(free)(starts);

diff --git a/coregrind/m_aspacemgr/aspacemgr-linux.c b/coregrind/m_aspacemgr/aspacemgr-linux.c
index f3eb7ceb52ed2ab3f06f87f3c63eccd3aac35d5b..25f304958b6dd562f1969fe405cf7fc824379e8f 100644 (file)
--- a/coregrind/m_aspacemgr/aspacemgr-linux.c
+++ b/coregrind/m_aspacemgr/aspacemgr-linux.c
@@ -624,7 +624,8 @@ const HChar* VG_(am_get_filename)( NSegment const * seg )
    return (i < 0) ? NULL : segnames + i;
 }
 
-/* Collect up the start addresses of all non-free, non-resvn segments.
+/* Collect up the start addresses of segments whose kind matches one of
+   the kinds specified in kind_mask.
    The interface is a bit strange in order to avoid potential
    segment-creation races caused by dynamic allocation of the result
    buffer *starts.
@@ -638,7 +639,7 @@ const HChar* VG_(am_get_filename)( NSegment const * seg )
    Correct use of this function may mean calling it multiple times in
    order to establish a suitably-sized buffer. */
 
-Int VG_(am_get_segment_starts)( Addr* starts, Int nStarts )
+Int VG_(am_get_segment_starts)( UInt kind_mask, Addr* starts, Int nStarts )
 {
    Int i, j, nSegs;
 
@@ -647,9 +648,8 @@ Int VG_(am_get_segment_starts)( Addr* starts, Int nStarts )
 
    nSegs = 0;
    for (i = 0; i < nsegments_used; i++) {
-      if (nsegments[i].kind == SkFree || nsegments[i].kind == SkResvn)
-         continue;
-      nSegs++;
+      if ((nsegments[i].kind & kind_mask) != 0)
+         nSegs++;
    }
 
    if (nSegs > nStarts) {
@@ -663,10 +663,8 @@ Int VG_(am_get_segment_starts)( Addr* starts, Int nStarts )
 
    j = 0;
    for (i = 0; i < nsegments_used; i++) {
-      if (nsegments[i].kind == SkFree || nsegments[i].kind == SkResvn)
-         continue;
-      starts[j] = nsegments[i].start;
-      j++;
+      if ((nsegments[i].kind & kind_mask) != 0)
+         starts[j++] = nsegments[i].start;
    }
 
    aspacem_assert(j == nSegs); /* this should not fail */

diff --git a/coregrind/m_coredump/coredump-elf.c b/coregrind/m_coredump/coredump-elf.c
index f0034ccacd38cb1e745435bf8725a07de6fa9b96..2c59bba940ad45587c059d8d81ed1931dfd9a7ef 100644 (file)
--- a/coregrind/m_coredump/coredump-elf.c
+++ b/coregrind/m_coredump/coredump-elf.c
@@ -635,8 +635,9 @@ void make_elf_coredump(ThreadId tid, const vki_siginfo_t *si, ULong max_size)
         return;                /* can't create file */
    }
 
-   /* Get the segments */
-   seg_starts = VG_(get_segment_starts)(&n_seg_starts);
+   /* Get the client segments */
+   seg_starts = VG_(get_segment_starts)(SkFileC | SkAnonC | SkShmC,
+                                        &n_seg_starts);
 
    /* First, count how many memory segments to dump */
    num_phdrs = 1;              /* start with notes */

diff --git a/coregrind/m_main.c b/coregrind/m_main.c
index b956ae8487121ba59c7c0ae883d467afa98138ad..82aa00a7f993b31a49bbdaba6bacad8e5e2f603f 100644 (file)
--- a/coregrind/m_main.c
+++ b/coregrind/m_main.c
@@ -2185,7 +2185,7 @@ Int valgrind_main ( Int argc, HChar **argv, HChar **envp )
      Int   n_seg_starts;
      Addr_n_ULong anu;
 
-     seg_starts = VG_(get_segment_starts)( &n_seg_starts );
+     seg_starts = VG_(get_segment_starts)( SkFileC | SkFileV, &n_seg_starts );
      vg_assert(seg_starts && n_seg_starts >= 0);
 
      /* show them all to the debug info reader.  allow_SkFileV has to
@@ -2207,7 +2207,7 @@ Int valgrind_main ( Int argc, HChar **argv, HChar **envp )
 #  elif defined(VGO_darwin)
    { Addr* seg_starts;
      Int   n_seg_starts;
-     seg_starts = VG_(get_segment_starts)( &n_seg_starts );
+     seg_starts = VG_(get_segment_starts)( SkFileC, &n_seg_starts );
      vg_assert(seg_starts && n_seg_starts >= 0);
 
      /* show them all to the debug info reader.  
@@ -2291,38 +2291,20 @@ Int valgrind_main ( Int argc, HChar **argv, HChar **envp )
      vg_assert(VG_(running_tid) == VG_INVALID_THREADID);
      VG_(running_tid) = tid_main;
 
-     seg_starts = VG_(get_segment_starts)( &n_seg_starts );
+     seg_starts = VG_(get_segment_starts)( SkFileC | SkAnonC | SkShmC,
+                                           &n_seg_starts );
      vg_assert(seg_starts && n_seg_starts >= 0);
 
-     /* show interesting ones to the tool */
+     /* Show client segments to the tool */
      for (i = 0; i < n_seg_starts; i++) {
         Word j, n;
         NSegment const* seg 
            = VG_(am_find_nsegment)( seg_starts[i] );
         vg_assert(seg);
-        if (seg->kind == SkFileC || seg->kind == SkAnonC) {
-          /* This next assertion is tricky.  If it is placed
-             immediately before this 'if', it very occasionally fails.
-             Why?  Because previous iterations of the loop may have
-             caused tools (via the new_mem_startup calls) to do
-             dynamic memory allocation, and that may affect the mapped
-             segments; in particular it may cause segment merging to
-             happen.  Hence we cannot assume that seg_starts[i], which
-             reflects the state of the world before we started this
-             loop, is the same as seg->start, as the latter reflects
-             the state of the world (viz, mappings) at this particular
-             iteration of the loop.
-
-             Why does moving it inside the 'if' make it safe?  Because
-             any dynamic memory allocation done by the tools will
-             affect only the state of Valgrind-owned segments, not of
-             Client-owned segments.  And the 'if' guards against that
-             -- we only get in here for Client-owned segments.
-
-             In other words: the loop may change the state of
-             Valgrind-owned segments as it proceeds.  But it should
-             not cause the Client-owned segments to change. */
-           vg_assert(seg->start == seg_starts[i]);
+        vg_assert(seg->kind == SkFileC || seg->kind == SkAnonC ||
+                  seg->kind == SkShmC);
+        vg_assert(seg->start == seg_starts[i]);
+        {
            VG_(debugLog)(2, "main", 
                             "tell tool about %010lx-%010lx %c%c%c\n",
                              seg->start, seg->end,

diff --git a/include/pub_tool_aspacehl.h b/include/pub_tool_aspacehl.h
index 00709c930bf2ac2fa9c351b3745d397bc4cd7812..acc05a701c6e24b278e375b1b9539022d963e908 100644 (file)
--- a/include/pub_tool_aspacehl.h
+++ b/include/pub_tool_aspacehl.h
@@ -37,7 +37,9 @@
 // addresses.  The vector is dynamically allocated and should be freed
 // by the caller when done.  REQUIRES m_mallocfree to be running.
 // Writes the number of addresses required into *n_acquired.
-extern Addr* VG_(get_segment_starts) ( /*OUT*/Int* n_acquired );
+// Only those segments are considered whose kind matches any of the kinds
+// given in KIND_MASK.
+extern Addr* VG_(get_segment_starts)( UInt kind_mask, /*OUT*/Int* n_acquired );
 
 #endif   // __PUB_TOOL_ASPACEHL_H
 

diff --git a/include/pub_tool_aspacemgr.h b/include/pub_tool_aspacemgr.h
index 5edcb030fb343b95c8e52ecce09f769cb85109e2..75d9f9f321175ca3af6116fb8ef3043f0f4b791b 100644 (file)
--- a/include/pub_tool_aspacemgr.h
+++ b/include/pub_tool_aspacemgr.h
@@ -36,16 +36,17 @@
 //--------------------------------------------------------------
 // Definition of address-space segments
 
-/* Describes segment kinds. */
+/* Describes segment kinds. Enumerators are one-hot encoded so they
+   can be or'ed together. */
 typedef
    enum {
-      SkFree,   // unmapped space
-      SkAnonC,  // anonymous mapping belonging to the client
-      SkAnonV,  // anonymous mapping belonging to valgrind
-      SkFileC,  // file mapping belonging to the client
-      SkFileV,  // file mapping belonging to valgrind
-      SkShmC,   // shared memory segment belonging to the client
-      SkResvn   // reservation
+      SkFree  = 0x01,  // unmapped space
+      SkAnonC = 0x02,  // anonymous mapping belonging to the client
+      SkAnonV = 0x04,  // anonymous mapping belonging to valgrind
+      SkFileC = 0x08,  // file mapping belonging to the client
+      SkFileV = 0x10,  // file mapping belonging to valgrind
+      SkShmC  = 0x20,  // shared memory segment belonging to the client
+      SkResvn = 0x40   // reservation
    }
    SegKind;
 
@@ -115,7 +116,8 @@ typedef
    NSegment;
 
 
-/* Collect up the start addresses of all non-free, non-resvn segments.
+/* Collect up the start addresses of segments whose kind matches one of
+   the kinds specified in kind_mask.
    The interface is a bit strange in order to avoid potential
    segment-creation races caused by dynamic allocation of the result
    buffer *starts.
@@ -128,7 +130,8 @@ typedef
 
    Correct use of this function may mean calling it multiple times in
    order to establish a suitably-sized buffer. */
-extern Int VG_(am_get_segment_starts)( Addr* starts, Int nStarts );
+extern Int VG_(am_get_segment_starts)( UInt kind_mask, Addr* starts,
+                                       Int nStarts );
 
 /* Finds the segment containing 'a'.  Only returns file/anon/resvn
    segments.  This returns a 'NSegment const *' - a pointer to

diff --git a/memcheck/mc_leakcheck.c b/memcheck/mc_leakcheck.c
index 4e6e28e67daae03b80e1d457da33968a26198754..671fff5eb4bf0165f4b55b18725cb805063732c2 100644 (file)
--- a/memcheck/mc_leakcheck.c
+++ b/memcheck/mc_leakcheck.c
@@ -1612,7 +1612,8 @@ static void scan_memory_root_set(Addr searched, SizeT szB)
 {
    Int   i;
    Int   n_seg_starts;
-   Addr* seg_starts = VG_(get_segment_starts)( &n_seg_starts );
+   Addr* seg_starts = VG_(get_segment_starts)( SkFileC | SkAnonC | SkShmC,
+                                               &n_seg_starts );
 
    tl_assert(seg_starts && n_seg_starts > 0);
 
@@ -1624,8 +1625,9 @@ static void scan_memory_root_set(Addr searched, SizeT szB)
       SizeT seg_size;
       NSegment const* seg = VG_(am_find_nsegment)( seg_starts[i] );
       tl_assert(seg);
+      tl_assert(seg->kind == SkFileC || seg->kind == SkAnonC ||
+                seg->kind == SkShmC);
 
-      if (seg->kind != SkFileC && seg->kind != SkAnonC) continue;
       if (!(seg->hasR && seg->hasW))                    continue;
       if (seg->isCH)                                    continue;