[Valgrind-developers] valgrind: r12548: fix 298943 massif asserts with --pages-as-heap=yes when brk is changing by value different of page size

[<sv...@va...>]

philippe	2012-05-01 21:02:30 +0100 (Tue, 01 May 2012)

  New Revision: 12548

  Log:
    fix 298943  massif asserts with --pages-as-heap=yes when brk is changing by value different of page size
    
    * add a massif test to (somewhat) validate --pages-as-heap=yes
      with calls to brk not being a multiple of a page size
    
    * fix the assert:
       only record new pages or unrecord old pages if at least one new
       full page (or one full old page) is added/removed.

  Added files:
    trunk/massif/tests/pages_as_heap.c
    trunk/massif/tests/pages_as_heap.stderr.exp
    trunk/massif/tests/pages_as_heap.vgtest
  Modified files:
    trunk/NEWS
    trunk/massif/ms_main.c
    trunk/massif/tests/Makefile.am


  Modified: trunk/massif/ms_main.c (+17 -4)
===================================================================
--- trunk/massif/ms_main.c	2012-05-01 20:51:07 +01:00 (rev 12547)
+++ trunk/massif/ms_main.c	2012-05-01 21:02:30 +01:00 (rev 12548)
@@ -1884,8 +1884,15 @@
 static
 void ms_new_mem_brk ( Addr a, SizeT len, ThreadId tid )
 {
-   tl_assert(VG_IS_PAGE_ALIGNED(len));
-   ms_record_page_mem(a, len);
+   // brk limit is not necessarily aligned on a page boundary.
+   // If new memory being brk-ed implies to allocate a new page,
+   // then call ms_record_page_mem with page aligned parameters
+   // otherwise just ignore.
+   Addr old_bottom_page = VG_PGROUNDDN(a - 1);
+   Addr new_top_page = VG_PGROUNDDN(a + len - 1);
+   if (old_bottom_page != new_top_page)
+      ms_record_page_mem(VG_PGROUNDDN(a),
+                         (new_top_page - old_bottom_page));
 }
 
 static
@@ -1906,8 +1913,14 @@
 static
 void ms_die_mem_brk( Addr a, SizeT len )
 {
-   tl_assert(VG_IS_PAGE_ALIGNED(len));
-   ms_unrecord_page_mem(a, len);
+   // Call ms_unrecord_page_mem only if one or more pages are de-allocated.
+   // See ms_new_mem_brk for more details.
+   Addr new_bottom_page = VG_PGROUNDDN(a - 1);
+   Addr old_top_page = VG_PGROUNDDN(a + len - 1);
+   if (old_top_page != new_bottom_page)
+      ms_unrecord_page_mem(VG_PGROUNDDN(a),
+                           (old_top_page - new_bottom_page));
+
 }
 
 //------------------------------------------------------------//

  Modified: trunk/NEWS (+1 -0)
===================================================================
--- trunk/NEWS	2012-05-01 20:51:07 +01:00 (rev 12547)
+++ trunk/NEWS	2012-05-01 21:02:30 +01:00 (rev 12548)
@@ -84,6 +84,7 @@
 297992  Support systems missing WIFCONTINUED (e.g. pre-2.6.10 Linux) 
 297993  Fix compilation of valgrind with gcc -g3.
 298394  s390x: Don't bail out on an unknown machine model. Assume it's a new model.
+298943  massif asserts with --pages-as-heap=yes when brk is chaning by value different of page size
 
 Release 3.7.0 (5 November 2011)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  Added: trunk/massif/tests/pages_as_heap.vgtest (+6 -0)
===================================================================
--- trunk/massif/tests/pages_as_heap.vgtest	2012-05-01 20:51:07 +01:00 (rev 12547)
+++ trunk/massif/tests/pages_as_heap.vgtest	2012-05-01 21:02:30 +01:00 (rev 12548)
@@ -0,0 +1,6 @@
+prog: pages_as_heap
+vgopts: --stacks=no --time-unit=B --heap-admin=0 --pages-as-heap=yes --massif-out-file=massif.out --detailed-freq=3
+vgopts: --ignore-fn=mmap
+# would be nice to test that pages as heap works properly using
+# post: perl ../../massif/ms_print massif.out | ../../tests/filter_addresses
+cleanup: rm massif.out

  Added: trunk/massif/tests/pages_as_heap.stderr.exp (+2 -0)
===================================================================
--- trunk/massif/tests/pages_as_heap.stderr.exp	2012-05-01 20:51:07 +01:00 (rev 12547)
+++ trunk/massif/tests/pages_as_heap.stderr.exp	2012-05-01 21:02:30 +01:00 (rev 12548)
@@ -0,0 +1,2 @@
+
+

  Modified: trunk/massif/tests/Makefile.am (+2 -0)
===================================================================
--- trunk/massif/tests/Makefile.am	2012-05-01 20:51:07 +01:00 (rev 12547)
+++ trunk/massif/tests/Makefile.am	2012-05-01 21:02:30 +01:00 (rev 12548)
@@ -28,6 +28,7 @@
 	null.post.exp null.stderr.exp null.vgtest \
 	one.post.exp one.post.exp2 one.stderr.exp one.vgtest \
 	overloaded-new.post.exp overloaded-new.stderr.exp overloaded-new.vgtest \
+	pages_as_heap.stderr.exp pages_as_heap.vgtest \
 	peak.post.exp peak.stderr.exp peak.vgtest \
 	peak2.post.exp peak2.stderr.exp peak2.vgtest \
 	realloc.post.exp realloc.stderr.exp realloc.vgtest \
@@ -57,6 +58,7 @@
 	null \
 	one \
 	overloaded-new \
+	pages_as_heap \
 	peak \
 	realloc \
 	thresholds \

  Added: trunk/massif/tests/pages_as_heap.c (+33 -0)
===================================================================
--- trunk/massif/tests/pages_as_heap.c	2012-05-01 20:51:07 +01:00 (rev 12547)
+++ trunk/massif/tests/pages_as_heap.c	2012-05-01 21:02:30 +01:00 (rev 12548)
@@ -0,0 +1,33 @@
+#include <stdio.h>
+#include <unistd.h>
+
+#define MAX 20000
+
+int main () {
+  char* ptr;
+  int i;
+  int inc_dec;
+  int delta;
+  int brk_stat;
+
+  // loop to first increase, then decrease
+  for (inc_dec = 1; inc_dec >= -1; inc_dec-=2) {
+     // loop to increase(decrease) with small then big delta
+     for (delta = 1; delta <= 400; delta+=399) {
+        if (0) printf("initial brk value for inc_dec %d delta %d: %p\n",
+               inc_dec, delta, sbrk(0));
+        for (i=0; i<MAX; i++) {
+           brk_stat = brk(sbrk(0) + inc_dec * delta);
+           if (brk_stat == -1) {
+              printf("brk value at failure: %p\n", sbrk(0));
+              perror ("brk() failed!\n");
+              return 0;
+           }
+        }
+        if (0) printf("resulting brk value for inc_dec %d delta %d: %p\n",
+               inc_dec, delta, sbrk(0));
+     }
+  }
+
+  return 0;
+}