Improvements in valgrind Requests

1. Enabled is_zeroed option while creating mempool.
2. Completely removed api from example (example/glue/ObjectModelDelegate.hpp)
3. Fixed some requests, added some new. Added existence condition for requests GC_ObjectHeapIteratorAddressOrderedList::shouldReturnCurrentObject() and nextObjectNoAdvance() which can be moved to GC_ObjectModelBase::isSingleSlotDeadObject and isDeadObject if MM_ExtensionsBase is available.
4. Request logs and stacktraces are now only printed in valgrind console using VALGRIND_PRINTF and VALGRIND_PRINTF_BACKTRACE request. Another configure option has been added --enable-VALGRIND_REQUEST_LOGS to skip them by default.

Issue: eclipse-omr#826
Signed-off-by: Varun Garg <[email protected]>

configure

@@ -656,6 +656,7 @@ CCLINKEXE
 CCLINKSHARED
 CCLINK
 OMR_CROSS_COMPILE
+VALGRIND_REQUEST_LOGS
 OMR_VALGRIND_MEMCHECK
 OMR_GC_IDLE_HEAP_MANAGER
 OMRTHREAD_LIB_UNIX
@@ -885,6 +886,7 @@ enable_OMRTHREAD_LIB_ZOS
 enable_OMRTHREAD_LIB_UNIX
 enable_OMR_GC_IDLE_HEAP_MANAGER
 enable_OMR_VALGRIND_MEMCHECK
+enable_VALGRIND_REQUEST_LOGS
 '
       ac_precious_vars='build_alias
 host_alias
@@ -1672,6 +1674,9 @@ Optional Features:
 
   --enable-OMR_VALGRIND_MEMCHECK
                           Integrate valgrind memcheck API. (Default: no)
+  --enable-VALGRIND_REQUEST_LOGS
+                          Display valgrind request logs in valgrind console.
+                          (Default: no)
 
 Some influential environment variables:
   SPEC        The SPEC to use. Sets up many configure settings without checks.
@@ -7292,6 +7297,25 @@ else
 fi
 
 
+# Check whether --enable-VALGRIND_REQUEST_LOGS was given.
+if test "${enable_VALGRIND_REQUEST_LOGS+set}" = set; then :
+  enableval=$enable_VALGRIND_REQUEST_LOGS; if test "x${enableval}" = xyes; then :
+  VALGRIND_REQUEST_LOGS=1
+
+   $as_echo "#define VALGRIND_REQUEST_LOGS 1" >>confdefs.h
+
+else
+  VALGRIND_REQUEST_LOGS=0
+
+
+fi
+else
+  VALGRIND_REQUEST_LOGS=0
+
+
+fi
+
+
 
 ###
 ### Variable Substitution

configure.ac

@@ -517,6 +517,7 @@ AS_IF([[test "x$enable_DDR" = "xyes"] && [test "$cross_compiling" != "yes"] && [
 
 OMRCFG_DEFINE_FLAG_OFF([OMR_GC_IDLE_HEAP_MANAGER])
 OMRCFG_DEFINE_FLAG_OFF([OMR_VALGRIND_MEMCHECK], [], [Integrate valgrind memcheck API. (Default: no)])
+OMRCFG_DEFINE_FLAG_OFF([VALGRIND_REQUEST_LOGS], [], [Display valgrind request logs in valgrind console. (Default: no)])
 
 ###
 ### Variable Substitution

doc/Valgrind Memcheck API.md

@@ -17,25 +17,27 @@ We have used following valgrind API requests:
 
 3. `VALGRIND_MAKE_MEM_UNDEFINED(lowAddress, size)` - This marks address ranges as accessible but containing undefined data. This request is used in `MM_HeapVirtualMemory::heapRemoveRange` to let Valgrind know that the memory is no longer in use for the heap.
 
-4. `VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)` - pool provides the anchor address for our memory pool. We have set it to  `handle->getMemoryBase(),` in `MM_MemoryManager::newInstance` and saved it to `valgrindMempoolAddr` in `MM_ExtensionsBase` to identify the mempool whenever we add or remove objects.
+4. `VALGRIND_CREATE_MEMPOOL(pool, rzB, is_zeroed)` - pool provides the anchor address for our memory pool. We have set it to  `handle->getMemoryBase(),` in `MM_MemoryManager::newInstance` and saved it to `valgrindMempoolAddr` in `MM_ExtensionsBase` to identify the mempool whenever we add or remove objects. We also set is_zeroed to 1 to tell valgrind that object is defined just after allocation request is used.
 
 5. `VALGRIND_MEMPOOL_ALLOC(pool, addr, size)` - This request is used to inform valgrind that object of `size` has been allocated at `addr` inside `pool`.  This also marks the object as `DEFINED`. The request is added to `allocateAndInitializeObject(OMR_VMThread *omrVMThread)`.
 
 6. `VALGRIND_MEMPOOL_FREE(pool, addr)` - This request is used to inform valgrind that object at `addr` has been freed from the `pool`.  This also marks the object as `NOACCESS`. The request is to be used in `MM_SweepPoolManagerAddressOrderedListBase::addFreeMemory(env, *sweepChunk, *address, size)`
 
 7. `VALGRIND_DESTROY_MEMPOOL(pool)` - This request destroys valgrind `pool` along with objects allocated on it. This request is used in `MM_MemoryManager::destroyVirtualMemory`. So to avoid any area getting marked as noaccess fafter a GC cycle is complete, it is nececcary that all objects inside it to be freed before (or inside) `MM_HeapVirtualMemory::heapRemoveRange` that marks the area as undefined at end.
 
+8. `VALGRIND_PRINTF(format, ...)` and `VALGRIND_PRINTF_BACKTRACE(format, ...)` - These requests print messages on valgrind terminal. We use them to when `--enable-VALGRIND_REQUEST_LOGS` is used to print request logs with stack trace.
+
 ## Different Ways of freeing objects in Valgrind.
 
 API request `VALGRIND_MEMPOOL_FREE(pool, addr)` requires us to have base address of object that we are attempting to free. GC keeps a record of the heap areas that are available or free. The GC is informed of the live objects during the marking phase of the GC in `MM_MarkingDelegate::scanRoots`, where all live objects are noted. The GC combines all unused memory into a "free list" which is used for future allocations. Proper Valgrind integration requires `VALGRIND_MEMPOOL_FREE` to be called on each object freed during a garbage collection, which is not something that the GC tracks
 
 There are 3 workarounds for this issue.
 
-1. Using `MM_MarkingDelegate::masterCleanupAfterGC`. Target language has record of objects in a object table, and this method is called after GC cycle has been completed with a record of marked objects. While it is freeing the dead objects from object table we are also requesting valgrind to free them. **Disadvantage:** free requests should have been made inside the GC (where we also request allocations) and not in target.
+1. Using `MM_MarkingDelegate::masterCleanupAfterGC`. Target language has record of objects in a object table, and this method is called after GC cycle has been completed with a record of marked objects. While it is freeing the dead objects from object table we are also requesting valgrind to free them. **Disadvantage:** free requests should have been made inside the GC (where we also request allocations) and not in target language.
 
- 2. Using a list to track allocated objects. This workaround allows us to free objects from GC directly. This is how we are freeing them currently. **Disadvantage** we haven't needed such a list in current Implementation, it is a overhead to maintain it later and increases complexity of code. 
+ 2. Using a list to track allocated objects. This workaround allows us to free objects from GC directly. This is how we are freeing them currently. **Disadvantage** we haven't needed such a list in current Implementation, it is a overhead to maintain it later and increases complexity of code. Another disadvantage is we cannot access this list everywhere in GC (where `MM_ExtensionsBase` is absent)
 
- 3. Adding new valgrind API: Valgrind already has the list of allocated objects, so it should be possible for it to find the objects in a range and also free them. Andrew Young has written a patch for this and it is in their mailing lists for review. https://sourceforge.net/p/valgrind/mailman/message/35996446/ . This does not require to have a track of allocated objects. This should be best solution, but until it is merged in their code, it will be a bit difficult to use.
+ 3. Adding new valgrind API: Valgrind already has the list of allocated objects, so it should be possible for it to find the objects in a range and also free them. A patch has been made for this and it is in their mailing lists for review. https://sourceforge.net/p/valgrind/mailman/message/35996446/ . This does not require to have a track of allocated objects. This should be best solution, but until it is merged in their code, it will be difficult to use.
 
  ## Preperation
 
@@ -44,8 +46,9 @@ There are 3 workarounds for this issue.
 
     So for given example, makefiles are generated by: 
 
-        make -f run_configure.mk SPEC=linux_x86-64 OMRGLUE=./example/glue EXTRA_CONFIGURE_ARGS='--disable-optimized --enable-OMR_VALGRIND_MEMCHECK`
+        make -f run_configure.mk SPEC=linux_x86-64 OMRGLUE=./example/glue EXTRA_CONFIGURE_ARGS='--disable-optimized --enable-OMR_VALGRIND_MEMCHECK'
 
+    If you are debugging API integration or GC itself, option `--enable-VALGRIND_REQUEST_LOGS` will dump each valgrind request with callstack from GC. This option is not needed when developing target languages.
 
 
 1. **Preparing GC Tests** - Currently following 4 tests work are using valgrind requests. They are:
@@ -71,7 +74,7 @@ There are 3 workarounds for this issue.
 2. **Error Count** - last line of output indicates errors. Example:  
     > ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
 
-3. **Invalid Read / Write errors** - these are errors that  are most helpful. They are detailed and may also contain location of previous objects they are occurring, Example:
+3. **Invalid Read / Write errors** - these are errors that are very helpful. They are detailed and may also contain location of previous objects they are occurring, Example:
 
         Invalid read of size 8
         Address 0x6d71d18 is 1,104 bytes inside a block of size 1,448 free'd  

example/glue/ObjectModelDelegate.hpp

@@ -23,10 +23,6 @@
 
 #include "ForwardedHeader.hpp"
 
-#if defined(OMR_VALGRIND_MEMCHECK)
-#include <valgrind/memcheck.h>
-#endif /* defined(OMR_VALGRIND_MEMCHECK) */
-
 class MM_AllocateInitialization;
 class MM_EnvironmentBase;
 
@@ -73,14 +69,7 @@ class GC_ObjectModelDelegate
 	uintptr_t
 	extractSizeFromObjectHeaderSlot(fomrobject_t headerSlot)
 	{
-#if defined(OMR_VALGRIND_MEMCHECK)
-		VALGRIND_MAKE_MEM_DEFINED(&headerSlot + _objectHeaderSlotSizeShift,sizeof(uintptr_t));
-#endif /* defined(OMR_VALGRIND_MEMCHECK) */
-		uintptr_t size = headerSlot >> _objectHeaderSlotSizeShift;
-#if defined(OMR_VALGRIND_MEMCHECK)
-		VALGRIND_MAKE_MEM_NOACCESS(&headerSlot + _objectHeaderSlotSizeShift,sizeof(uintptr_t));
-#endif /* defined(OMR_VALGRIND_MEMCHECK) */
-		return size;
+		return headerSlot >> _objectHeaderSlotSizeShift;
 	}
 
 protected:
@@ -153,14 +142,7 @@ class GC_ObjectModelDelegate
 	getObjectSizeInBytesWithHeader(omrobjectptr_t objectPtr)
 	{
 		fomrobject_t *headerSlotAddress = (fomrobject_t *)objectPtr + _objectHeaderSlotOffset;
-#if defined(OMR_VALGRIND_MEMCHECK)
-		VALGRIND_MAKE_MEM_DEFINED(headerSlotAddress,sizeof(fomrobject_t));
-#endif /* defined(OMR_VALGRIND_MEMCHECK) */
-		uintptr_t objectSize = extractSizeFromObjectHeaderSlot(*headerSlotAddress);
-#if defined(OMR_VALGRIND_MEMCHECK)
-		VALGRIND_MAKE_MEM_NOACCESS(headerSlotAddress,sizeof(fomrobject_t));
-#endif /* defined(OMR_VALGRIND_MEMCHECK) */
-		return objectSize;
+		return extractSizeFromObjectHeaderSlot(*headerSlotAddress);
 	}
 
 	/**

gc/base/AllocateInitialization.hpp

@@ -201,11 +201,12 @@ class MM_AllocateInitialization : public MM_Base
 
 			if (NULL != heapBytes) {
 #if defined(OMR_VALGRIND_MEMCHECK)
-				/* Allocate object in Valgrind memory pool before modifying it */
+#if defined(VALGRIND_REQUEST_LOGS)
+				VALGRIND_PRINTF_BACKTRACE("VALGRIND: Allocated object at 0x%lx of size %lu\n", (uintptr_t)heapBytes,_allocateDescription.getBytesRequested());
+#endif /* defined(VALGRIND_REQUEST_LOGS) */
+				/* Allocate object in Valgrind memory pool. */ 		
 				VALGRIND_MEMPOOL_ALLOC(env->getExtensions()->valgrindMempoolAddr,heapBytes,_allocateDescription.getBytesRequested());
 				env->getExtensions()->_allocatedObjects.insert((uintptr_t)heapBytes);
-				OMRPORT_ACCESS_FROM_OMRPORT(env->getPortLibrary());
-				omrtty_printf("VALGRIND: Allocated object at %x of size %d\n", heapBytes,_allocateDescription.getBytesRequested());
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 
 				/* wipe allocated space if requested and allowed (NON_ZERO_TLH flag set inhibits zeroing) */

gc/base/HeapLinkedFreeHeader.hpp

@@ -86,27 +86,29 @@ class MM_HeapLinkedFreeHeader
 	MMINLINE uintptr_t
 	getNextImpl()
 	{
-#if defined(SPLIT_NEXT_POINTER)		
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF_BACKTRACE("Marking area defined at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */
 		VALGRIND_MAKE_MEM_DEFINED(this, sizeof(*this));
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
-	    uintptr_t lowBits = _next;
+
+#if defined(SPLIT_NEXT_POINTER)		
+		uintptr_t lowBits = _next;
 		uintptr_t highBits = _nextHighBits;
-#if defined(OMR_VALGRIND_MEMCHECK)
-		uintptr_t temp = (highBits << 32) | lowBits;
-		VALGRIND_MAKE_MEM_NOACCESS(this, sizeof(*this));
-		return temp;
-#endif /* defined(OMR_VALGRIND_MEMCHECK) */
-	    return (highBits << 32) | lowBits;
+		uintptr_t result = (highBits << 32) | lowBits;
 #else /* defined(SPLIT_NEXT_POINTER) */
+		uintptr_t result = _next;
+#endif /* defined(SPLIT_NEXT_POINTER) */
+
 #if defined(OMR_VALGRIND_MEMCHECK)
-		VALGRIND_MAKE_MEM_DEFINED(this, sizeof(*this));
-	    uintptr_t next = _next;
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF("Marking area noaccess at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */				
 		VALGRIND_MAKE_MEM_NOACCESS(this, sizeof(*this));
-		return next;
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
-		return _next;
-#endif /* defined(SPLIT_NEXT_POINTER) */
+
+		return result;
 	}
 
 	/**
@@ -121,15 +123,23 @@ class MM_HeapLinkedFreeHeader
 	setNextImpl(uintptr_t value)
 	{
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF_BACKTRACE("Marking area defined at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */
 		VALGRIND_MAKE_MEM_DEFINED(this, sizeof(*this));
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
+
 #if defined(SPLIT_NEXT_POINTER)
 		_next = (uint32_t)value;
 		_nextHighBits = (uint32_t)(value >> 32);
 #else /* defined(SPLIT_NEXT_POINTER) */
 		_next = value;
 #endif /* defined(SPLIT_NEXT_POINTER) */
+
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF("Marking area noaccess at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */				
 		VALGRIND_MAKE_MEM_NOACCESS(this, sizeof(*this));
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 	}
@@ -168,9 +178,15 @@ class MM_HeapLinkedFreeHeader
 	 */
 	MMINLINE uintptr_t getSize()	{
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF_BACKTRACE("Marking area defined at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */		
 		VALGRIND_MAKE_MEM_DEFINED(this, sizeof(*this));
-	    uintptr_t size = _size;
-	    VALGRIND_MAKE_MEM_NOACCESS(this, sizeof(*this));
+		uintptr_t size = _size;
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF("Marking area noaccess at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */	
+		VALGRIND_MAKE_MEM_NOACCESS(this, sizeof(*this));
 	    return size;
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 	    return _size;
@@ -181,10 +197,16 @@ class MM_HeapLinkedFreeHeader
 	 */
 	MMINLINE void setSize(uintptr_t size) {
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF_BACKTRACE("Marking area defined at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */		
 		VALGRIND_MAKE_MEM_DEFINED(this, sizeof(*this));
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 		_size = size;
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF("Marking area noaccess at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */			
 	    VALGRIND_MAKE_MEM_NOACCESS(this, sizeof(*this));
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 	}
@@ -194,10 +216,16 @@ class MM_HeapLinkedFreeHeader
 	 */
 	MMINLINE void expandSize(uintptr_t increment) {
 #if defined(OMR_VALGRIND_MEMCHECK)
-	    VALGRIND_MAKE_MEM_DEFINED(this, sizeof(*this));
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF_BACKTRACE("Marking area defined at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */	    
+		VALGRIND_MAKE_MEM_DEFINED(this, sizeof(*this));
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 		_size += increment;
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF("Marking area noaccess at 0x%p of size %lu\n",this,sizeof(*this));
+#endif /* defined(VALGRIND_REQUEST_LOGS) */				
 	    VALGRIND_MAKE_MEM_NOACCESS(this, sizeof(*this));
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 	}
@@ -218,7 +246,11 @@ class MM_HeapLinkedFreeHeader
 	fillWithSingleSlotHoles(void* addrBase, uintptr_t freeEntrySize)
 	{
 #if defined(OMR_VALGRIND_MEMCHECK)
-		VALGRIND_MAKE_MEM_DEFINED(addrBase, freeEntrySize);
+		uintptr_t vgSizeUnchanged = freeEntrySize;
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF_BACKTRACE("Marking area defined at 0x%p of size %lu\n",addrBase,vgSizeUnchanged);
+#endif /* defined(VALGRIND_REQUEST_LOGS) */		
+		VALGRIND_MAKE_MEM_DEFINED(addrBase, vgSizeUnchanged);
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 #if defined(SPLIT_NEXT_POINTER)
 		uint32_t *freeSlot = (uint32_t *) addrBase;
@@ -234,7 +266,10 @@ class MM_HeapLinkedFreeHeader
 		}
 #endif /* defined(SPLIT_NEXT_POINTER) */
 #if defined(OMR_VALGRIND_MEMCHECK)
-		VALGRIND_MAKE_MEM_NOACCESS(addrBase, freeEntrySize);
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF("Marking area noaccess at 0x%p of size %lu\n",addrBase, vgSizeUnchanged);		
+#endif /* defined(VALGRIND_REQUEST_LOGS) */	
+		VALGRIND_MAKE_MEM_NOACCESS(addrBase, vgSizeUnchanged);
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 	}
 
@@ -248,6 +283,9 @@ class MM_HeapLinkedFreeHeader
 	fillWithHoles(void* addrBase, uintptr_t freeEntrySize)
 	{
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF_BACKTRACE("Marking area defined at 0x%p of size %lu\n",addrBase,freeEntrySize);
+#endif /* defined(VALGRIND_REQUEST_LOGS) */	
 		VALGRIND_MAKE_MEM_DEFINED(addrBase, freeEntrySize);
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 		MM_HeapLinkedFreeHeader *freeEntry = NULL;
@@ -262,6 +300,9 @@ class MM_HeapLinkedFreeHeader
 			freeEntry->setSize(freeEntrySize);
 		}
 #if defined(OMR_VALGRIND_MEMCHECK)
+#if defined(VALGRIND_REQUEST_LOGS)
+		VALGRIND_PRINTF("Marking area noaccess at 0x%p of size %lu\n",addrBase, freeEntrySize);		
+#endif /* defined(VALGRIND_REQUEST_LOGS) */	
 		VALGRIND_MAKE_MEM_NOACCESS(addrBase, freeEntrySize);
 #endif /* defined(OMR_VALGRIND_MEMCHECK) */
 		return freeEntry;