[RFA:] valgrind annotations for ggc-page.c, ggc-common.c

[RFA:] valgrind annotations for ggc-page.c, ggc-common.c

[Hans-Peter Nilsson]

This patch is (being) bootstrapped and checked together with the
previous valgrind "take 2" patch, but is sent separately because
these annotations are optional, in contrast to the cppfiles.c
change in theprevious patch, without which there'd be excessive
false valgrind reports.

Discarding the annotation handles will only result in valgrind
not pointing to the line where the annotation is made, when it
reports badness.  I tried without discarding and bootstrap would
take unnecessarily too long; one file was still not compiled
after 10 hours.  I sampled the running bootstrap by attaching
gdb, checking what was going on and I hit
vg_clientperms.c:vg_alloc_client_block every time.  To
summarize, user annotations in valgrind are kept in an array
which is searched linearly for an empty slot, for every new
annotation request.  For performance reasons, better not
accumulate handles at all.

Regarding whether or not have ENABLE_GC_CHECKING poisoning
memory when running valgrind, I chose to not touch that.
Adding --enable-checking=valgrind should change as little as
possible.  If one doesn't want the memsets, one shouldn't add
--enable-checking=gc.  If an opposite position is taken, then
also consider additional data to keep track of the size of the
annotations, to keep accurate information for e.g. ggc_realloc.
That additional data would of course change the memory
allocation picture, which would likely make any observed memory
problems disappear.

Ok to commit?

	* ggc-common.c (ggc_realloc) [ENABLE_VALGRIND_CHECKING]: Update
	annotations.
	* ggc-page.c (alloc_anon, free_page, ggc_alloc, poison_pages)
	[ENABLE_VALGRIND_CHECKING]: Add machinery to annotate memory.

Index: ggc-common.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ggc-common.c,v
retrieving revision 1.56
diff -p -c -r1.56 ggc-common.c
*** ggc-common.c	16 Sep 2002 18:33:18 -0000	1.56
--- ggc-common.c	19 Nov 2002 01:35:47 -0000
*************** Software Foundation, 59 Temple Place - S
*** 30,35 ****
--- 30,38 ----
  #include "varray.h"
  #include "ggc.h"
  #include "langhooks.h"
+ #ifdef ENABLE_VALGRIND_CHECKING
+ #include <valgrind.h>
+ #endif

  /* Statistics about the allocation.  */
  static ggc_statistics *ggc_stats;
*************** ggc_realloc (x, size)
*** 155,164 ****

    old_size = ggc_get_size (x);
    if (size <= old_size)
!     return x;

    r = ggc_alloc (size);
    memcpy (r, x, old_size);
    return r;
  }

--- 158,199 ----

    old_size = ggc_get_size (x);
    if (size <= old_size)
!     {
! #ifdef ENABLE_VALGRIND_CHECKING
!       /* Mark the unwanted memory as unaccessible.  We also need to make
! 	 the "new" size accessible, since ggc_get_size returns the size of
! 	 the pool, not the size of the individually allocated object, the
! 	 size which was previously made accessible.  Unfortunately, we
! 	 don't know that previously allocated size.  Without that
! 	 knowledge we have to lose some initialization-tracking for the
! 	 old parts of the object.  An alternative is to mark the whole
! 	 old_size as reachable, but that would lose tracking of writes
! 	 after the end of the object (by small offsets).  Discard the
! 	 handle to avoid handle leak.  */
!       VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS ((char *) x + size,
! 						old_size - size));
!       VALGRIND_DISCARD (VALGRIND_MAKE_READABLE (x, size));
! #endif
!       return x;
!     }

    r = ggc_alloc (size);
+
+ #ifdef ENABLE_VALGRIND_CHECKING
+   /* Since ggc_get_size returns the size of the pool, not the size of the
+      individually allocated object, we'd access parts of the old object
+      that were marked invalid with the memcpy below.  We lose a bit of the
+      initialization-tracking since some of it may be uninitialized.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_READABLE (x, old_size));
+ #endif
+
    memcpy (r, x, old_size);
+
+ #ifdef ENABLE_VALGRIND_CHECKING
+   /* The old object is not supposed to be used anymore.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (x, old_size));
+ #endif
+
    return r;
  }

Index: ggc-page.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ggc-page.c,v
retrieving revision 1.56
diff -p -c -r1.56 ggc-page.c
*** ggc-page.c	12 Nov 2002 00:27:31 -0000	1.56
--- ggc-page.c	19 Nov 2002 01:35:48 -0000
*************** Software Foundation, 59 Temple Place - S
*** 29,34 ****
--- 29,37 ----
  #include "ggc.h"
  #include "timevar.h"
  #include "params.h"
+ #ifdef ENABLE_VALGRIND_CHECKING
+ #include <valgrind.h>
+ #endif

  /* Prefer MAP_ANON(YMOUS) to /dev/zero, since we don't need to keep a
     file open.  Prefer either to valloc.  */
*************** alloc_anon (pref, size)
*** 524,529 ****
--- 527,539 ----
    /* Remember that we allocated this memory.  */
    G.bytes_mapped += size;

+ #ifdef ENABLE_VALGRIND_CHECKING
+   /* Pretend we don't have access to the allocated pages.  We'll enable
+      access to smaller pieces of the area in ggc_alloc.  Discard the
+      handle to avoid handle leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (page, size));
+ #endif
+
    return page;
  }
  #endif
*************** free_page (entry)
*** 750,755 ****
--- 760,771 ----
  	     "Deallocating page at %p, data %p-%p\n", (PTR) entry,
  	     entry->page, entry->page + entry->bytes - 1);

+ #ifdef ENABLE_VALGRIND_CHECKING
+   /* Mark the page as inaccessible.  Discard the handle to avoid handle
+      leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (entry->page, entry->bytes));
+ #endif
+
    set_page_table_entry (entry->page, NULL);

  #ifdef USING_MALLOC_PAGE_GROUPS
*************** ggc_alloc (size)
*** 943,951 ****
--- 959,989 ----
    result = entry->page + object_offset;

  #ifdef ENABLE_GC_CHECKING
+ #ifdef ENABLE_VALGRIND_CHECKING
+   /* Keep poisoning-by-writing-0xaf the object, in an attempt to keep the
+      exact same semantics in presence of memory bugs, regardless of
+      ENABLE_VALGRIND_CHECKING.  We override this request below.  Drop the
+      handle to avoid handle leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_WRITABLE (result, OBJECT_SIZE (order)));
+ #endif
+
    /* `Poison' the entire allocated object, including any padding at
       the end.  */
    memset (result, 0xaf, OBJECT_SIZE (order));
+
+ #ifdef ENABLE_VALGRIND_CHECKING
+   /* Make the bytes after the end of the object unaccessible.  Discard the
+      handle to avoid handle leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS ((char *) result + size,
+ 					    OBJECT_SIZE (order) - size));
+ #endif
+ #endif
+
+ #ifdef ENABLE_VALGRIND_CHECKING
+   /* Tell Valgrind that the memory is there, but its content isn't
+      defined.  The bytes at the end of the object are still marked
+      unaccessible.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_WRITABLE (result, size));
  #endif

    /* Keep track of how many bytes are being allocated.  This
*************** poison_pages ()
*** 1433,1439 ****
  	      word = i / HOST_BITS_PER_LONG;
  	      bit = i % HOST_BITS_PER_LONG;
  	      if (((p->in_use_p[word] >> bit) & 1) == 0)
! 		memset (p->page + i * size, 0xa5, size);
  	    }
  	}
      }
--- 1471,1493 ----
  	      word = i / HOST_BITS_PER_LONG;
  	      bit = i % HOST_BITS_PER_LONG;
  	      if (((p->in_use_p[word] >> bit) & 1) == 0)
! 		{
! 		  char *object = p->page + i * size;
!
! #ifdef ENABLE_VALGRIND_CHECKING
! 		  /* Keep poison-by-write when we expect to use Valgrind,
! 		     so the exact same memory semantics is kept, in case
! 		     there are memory errors.  We override this request
! 		     below.  */
! 		  VALGRIND_DISCARD (VALGRIND_MAKE_WRITABLE (object, size));
! #endif
! 		  memset (object, 0xa5, size);
!
! #ifdef ENABLE_VALGRIND_CHECKING
! 		  /* Drop the handle to avoid handle leak.  */
! 		  VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (object, size));
! #endif
! 		}
  	    }
  	}
      }

brgds, H-P

Re: [RFA:] valgrind annotations for ggc-page.c, ggc-common.c

[Richard Henderson]

On Mon, Nov 18, 2002 at 10:14:20PM -0500, Hans-Peter Nilsson wrote:
> 	* ggc-common.c (ggc_realloc) [ENABLE_VALGRIND_CHECKING]: Update
> 	annotations.
> 	* ggc-page.c (alloc_anon, free_page, ggc_alloc, poison_pages)
> 	[ENABLE_VALGRIND_CHECKING]: Add machinery to annotate memory.

Ok.


r~

Re: [RFA:] valgrind annotations for ggc-page.c, ggc-common.c

[Hans-Peter Nilsson]

On Tue, 19 Nov 2002, Richard Henderson wrote:
> On Mon, Nov 18, 2002 at 10:14:20PM -0500, Hans-Peter Nilsson wrote:
> > 	* ggc-common.c (ggc_realloc) [ENABLE_VALGRIND_CHECKING]: Update
> > 	annotations.
> > 	* ggc-page.c (alloc_anon, free_page, ggc_alloc, poison_pages)
> > 	[ENABLE_VALGRIND_CHECKING]: Add machinery to annotate memory.
>
> Ok.

Thanks for the review.

Likewise as for cppfiles.c, I committed this instead, as an
obvious change.  Tested together with the cppfiles.c change.

	* ggc-common.c [!ENABLE_VALGRIND_CHECKING] (VALGRIND_DISCARD):
	Define as empty.
	(ggc_realloc): Update valgrind annotations.
	* ggc-page.c [!ENABLE_VALGRIND_CHECKING] (VALGRIND_DISCARD):
	Define as empty.
 	(alloc_anon, free_page, ggc_alloc, poison_pages): Add machinery to
	valgrind-annotate memory.

Index: ggc-page.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ggc-page.c,v
retrieving revision 1.56
diff -p -c -r1.56 ggc-page.c
*** ggc-page.c	12 Nov 2002 00:27:31 -0000	1.56
--- ggc-page.c	20 Nov 2002 00:02:38 -0000
*************** Software Foundation, 59 Temple Place - S
*** 29,34 ****
--- 29,40 ----
  #include "ggc.h"
  #include "timevar.h"
  #include "params.h"
+ #ifdef ENABLE_VALGRIND_CHECKING
+ #include <valgrind.h>
+ #else
+ /* Avoid #ifdef:s when we can help it.  */
+ #define VALGRIND_DISCARD(x)
+ #endif

  /* Prefer MAP_ANON(YMOUS) to /dev/zero, since we don't need to keep a
     file open.  Prefer either to valloc.  */
*************** alloc_anon (pref, size)
*** 524,529 ****
--- 530,540 ----
    /* Remember that we allocated this memory.  */
    G.bytes_mapped += size;

+   /* Pretend we don't have access to the allocated pages.  We'll enable
+      access to smaller pieces of the area in ggc_alloc.  Discard the
+      handle to avoid handle leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (page, size));
+
    return page;
  }
  #endif
*************** free_page (entry)
*** 750,755 ****
--- 761,770 ----
  	     "Deallocating page at %p, data %p-%p\n", (PTR) entry,
  	     entry->page, entry->page + entry->bytes - 1);

+   /* Mark the page as inaccessible.  Discard the handle to avoid handle
+      leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (entry->page, entry->bytes));
+
    set_page_table_entry (entry->page, NULL);

  #ifdef USING_MALLOC_PAGE_GROUPS
*************** ggc_alloc (size)
*** 943,953 ****
--- 958,984 ----
    result = entry->page + object_offset;

  #ifdef ENABLE_GC_CHECKING
+   /* Keep poisoning-by-writing-0xaf the object, in an attempt to keep the
+      exact same semantics in presence of memory bugs, regardless of
+      ENABLE_VALGRIND_CHECKING.  We override this request below.  Drop the
+      handle to avoid handle leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_WRITABLE (result, OBJECT_SIZE (order)));
+
    /* `Poison' the entire allocated object, including any padding at
       the end.  */
    memset (result, 0xaf, OBJECT_SIZE (order));
+
+   /* Make the bytes after the end of the object unaccessible.  Discard the
+      handle to avoid handle leak.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS ((char *) result + size,
+ 					    OBJECT_SIZE (order) - size));
  #endif

+   /* Tell Valgrind that the memory is there, but its content isn't
+      defined.  The bytes at the end of the object are still marked
+      unaccessible.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_WRITABLE (result, size));
+
    /* Keep track of how many bytes are being allocated.  This
       information is used in deciding when to collect.  */
    G.allocated += OBJECT_SIZE (order);
*************** poison_pages ()
*** 1433,1439 ****
  	      word = i / HOST_BITS_PER_LONG;
  	      bit = i % HOST_BITS_PER_LONG;
  	      if (((p->in_use_p[word] >> bit) & 1) == 0)
! 		memset (p->page + i * size, 0xa5, size);
  	    }
  	}
      }
--- 1464,1482 ----
  	      word = i / HOST_BITS_PER_LONG;
  	      bit = i % HOST_BITS_PER_LONG;
  	      if (((p->in_use_p[word] >> bit) & 1) == 0)
! 		{
! 		  char *object = p->page + i * size;
!
! 		  /* Keep poison-by-write when we expect to use Valgrind,
! 		     so the exact same memory semantics is kept, in case
! 		     there are memory errors.  We override this request
! 		     below.  */
! 		  VALGRIND_DISCARD (VALGRIND_MAKE_WRITABLE (object, size));
! 		  memset (object, 0xa5, size);
!
! 		  /* Drop the handle to avoid handle leak.  */
! 		  VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (object, size));
! 		}
  	    }
  	}
      }
Index: ggc-common.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/ggc-common.c,v
retrieving revision 1.56
diff -p -c -r1.56 ggc-common.c
*** ggc-common.c	16 Sep 2002 18:33:18 -0000	1.56
--- ggc-common.c	20 Nov 2002 00:02:38 -0000
*************** Software Foundation, 59 Temple Place - S
*** 30,35 ****
--- 30,41 ----
  #include "varray.h"
  #include "ggc.h"
  #include "langhooks.h"
+ #ifdef ENABLE_VALGRIND_CHECKING
+ #include <valgrind.h>
+ #else
+ /* Avoid #ifdef:s when we can help it.  */
+ #define VALGRIND_DISCARD(x)
+ #endif

  /* Statistics about the allocation.  */
  static ggc_statistics *ggc_stats;
*************** ggc_realloc (x, size)
*** 155,164 ****

    old_size = ggc_get_size (x);
    if (size <= old_size)
!     return x;

    r = ggc_alloc (size);
    memcpy (r, x, old_size);
    return r;
  }

--- 161,196 ----

    old_size = ggc_get_size (x);
    if (size <= old_size)
!     {
!       /* Mark the unwanted memory as unaccessible.  We also need to make
! 	 the "new" size accessible, since ggc_get_size returns the size of
! 	 the pool, not the size of the individually allocated object, the
! 	 size which was previously made accessible.  Unfortunately, we
! 	 don't know that previously allocated size.  Without that
! 	 knowledge we have to lose some initialization-tracking for the
! 	 old parts of the object.  An alternative is to mark the whole
! 	 old_size as reachable, but that would lose tracking of writes
! 	 after the end of the object (by small offsets).  Discard the
! 	 handle to avoid handle leak.  */
!       VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS ((char *) x + size,
! 						old_size - size));
!       VALGRIND_DISCARD (VALGRIND_MAKE_READABLE (x, size));
!       return x;
!     }

    r = ggc_alloc (size);
+
+   /* Since ggc_get_size returns the size of the pool, not the size of the
+      individually allocated object, we'd access parts of the old object
+      that were marked invalid with the memcpy below.  We lose a bit of the
+      initialization-tracking since some of it may be uninitialized.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_READABLE (x, old_size));
+
    memcpy (r, x, old_size);
+
+   /* The old object is not supposed to be used anymore.  */
+   VALGRIND_DISCARD (VALGRIND_MAKE_NOACCESS (x, old_size));
+
    return r;
  }

brgds, H-P