7121373: Clean up CollectedHeap::is_in

Fixed G1CollectedHeap::is_in, added tests, cleaned up comments and made Space::is_in pure virtual.

Reviewed-by: brutisso, tonyp, jcoomes

hotspot/src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.hpp

@@ -336,12 +336,6 @@ class CompactibleFreeListSpace: public CompactibleSpace {
                      unallocated_block() : end());
   }
 
-  // This is needed because the default implementation uses block_start()
-  // which can;t be used at certain times (for example phase 3 of mark-sweep).
-  // A better fix is to change the assertions in phase 3 of mark-sweep to
-  // use is_in_reserved(), but that is deferred since the is_in() assertions
-  // are buried through several layers of callers and are used elsewhere
-  // as well.
   bool is_in(const void* p) const {
     return used_region().contains(p);
   }

hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp

@@ -2411,8 +2411,11 @@ void G1CollectedHeap::collect(GCCause::Cause cause) {
 }
 
 bool G1CollectedHeap::is_in(const void* p) const {
-  HeapRegion* hr = _hrs.addr_to_region((HeapWord*) p);
-  if (hr != NULL) {
+  if (_g1_committed.contains(p)) {
+    // Given that we know that p is in the committed space,
+    // heap_region_containing_raw() should successfully
+    // return the containing region.
+    HeapRegion* hr = heap_region_containing_raw(p);
     return hr->is_in(p);
   } else {
     return _perm_gen->as_gen()->is_in(p);

hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp

@@ -1196,7 +1196,7 @@ public:
                                        HumongousRegionSet* humongous_proxy_set,
                                        bool par);
 
-  // Returns "TRUE" iff "p" points into the allocated area of the heap.
+  // Returns "TRUE" iff "p" points into the committed areas of the heap.
   virtual bool is_in(const void* p) const;
 
   // Return "TRUE" iff the given object address is within the collection

hotspot/src/share/vm/gc_interface/collectedHeap.cpp

@@ -471,3 +471,26 @@ oop CollectedHeap::Class_obj_allocate(KlassHandle klass, int size, KlassHandle r
 
   return mirror;
 }
+
+/////////////// Unit tests ///////////////
+
+#ifndef PRODUCT
+void CollectedHeap::test_is_in() {
+  CollectedHeap* heap = Universe::heap();
+
+  // Test that NULL is not in the heap.
+  assert(!heap->is_in(NULL), "NULL is unexpectedly in the heap");
+
+  // Test that a pointer to before the heap start is reported as outside the heap.
+  assert(heap->_reserved.start() >= (void*)MinObjAlignment, "sanity");
+  void* before_heap = (void*)((intptr_t)heap->_reserved.start() - MinObjAlignment);
+  assert(!heap->is_in(before_heap),
+      err_msg("before_heap: " PTR_FORMAT " is unexpectedly in the heap", before_heap));
+
+  // Test that a pointer to after the heap end is reported as outside the heap.
+  assert(heap->_reserved.end() <= (void*)(uintptr_t(-1) - (uint)MinObjAlignment), "sanity");
+  void* after_heap = (void*)((intptr_t)heap->_reserved.end() + MinObjAlignment);
+  assert(!heap->is_in(after_heap),
+      err_msg("after_heap: " PTR_FORMAT " is unexpectedly in the heap", after_heap));
+}
+#endif

hotspot/src/share/vm/gc_interface/collectedHeap.hpp

@@ -217,8 +217,8 @@ class CollectedHeap : public CHeapObj {
     return p == NULL || is_in_reserved(p);
   }
 
-  // Returns "TRUE" if "p" points to the head of an allocated object in the
-  // heap. Since this method can be expensive in general, we restrict its
+  // Returns "TRUE" iff "p" points into the committed areas of the heap.
+  // Since this method can be expensive in general, we restrict its
   // use to assertion checking only.
   virtual bool is_in(const void* p) const = 0;
 
@@ -648,6 +648,10 @@ class CollectedHeap : public CHeapObj {
   // reduce the occurrence of ParallelGCThreads to uses where the
   // actual number may be germane.
   static bool use_parallel_gc_threads() { return ParallelGCThreads > 0; }
+
+  /////////////// Unit tests ///////////////
+
+  NOT_PRODUCT(static void test_is_in();)
 };
 
 // Class to set and reset the GC cause for a CollectedHeap.

hotspot/src/share/vm/memory/genCollectedHeap.cpp

@@ -957,7 +957,7 @@ bool GenCollectedHeap::is_in_young(oop p) {
   return result;
 }
 
-// Returns "TRUE" iff "p" points into the allocated area of the heap.
+// Returns "TRUE" iff "p" points into the committed areas of the heap.
 bool GenCollectedHeap::is_in(const void* p) const {
   #ifndef ASSERT
   guarantee(VerifyBeforeGC   ||

hotspot/src/share/vm/memory/genCollectedHeap.hpp

@@ -198,7 +198,7 @@ public:
   // Mostly used for testing purposes. Caller does not hold the Heap_lock on entry.
   void collect(GCCause::Cause cause, int max_level);
 
-  // Returns "TRUE" iff "p" points into the allocated area of the heap.
+  // Returns "TRUE" iff "p" points into the committed areas of the heap.
   // The methods is_in(), is_in_closed_subset() and is_in_youngest() may
   // be expensive to compute in general, so, to prevent
   // their inadvertent use in product jvm's, we restrict their use to

hotspot/src/share/vm/memory/generation.hpp

@@ -220,7 +220,7 @@ class Generation: public CHeapObj {
   MemRegion prev_used_region() const { return _prev_used_region; }
   virtual void  save_used_region()   { _prev_used_region = used_region(); }
 
-  // Returns "TRUE" iff "p" points into an allocated object in the generation.
+  // Returns "TRUE" iff "p" points into the committed areas in the generation.
   // For some kinds of generations, this may be an expensive operation.
   // To avoid performance problems stemming from its inadvertent use in
   // product jvm's, we restrict its use to assertion checking or

hotspot/src/share/vm/memory/space.cpp

@@ -304,11 +304,6 @@ void ContiguousSpace::clear(bool mangle_space) {
   CompactibleSpace::clear(mangle_space);
 }
 
-bool Space::is_in(const void* p) const {
-  HeapWord* b = block_start_const(p);
-  return b != NULL && block_is_obj(b);
-}
-
 bool ContiguousSpace::is_in(const void* p) const {
   return _bottom <= p && p < _top;
 }

hotspot/src/share/vm/memory/space.hpp

@@ -187,7 +187,7 @@ class Space: public CHeapObj {
   // expensive operation. To prevent performance problems
   // on account of its inadvertent use in product jvm's,
   // we restrict its use to assertion checks only.
-  virtual bool is_in(const void* p) const;
+  virtual bool is_in(const void* p) const = 0;
 
   // Returns true iff the given reserved memory of the space contains the
   // given address.

hotspot/src/share/vm/oops/arrayOop.cpp

@@ -38,9 +38,7 @@ bool arrayOopDesc::check_max_length_overflow(BasicType type) {
   return (julong)(size_t)bytes == bytes;
 }
 
-bool arrayOopDesc::test_max_array_length() {
-  tty->print_cr("test_max_array_length");
-
+void arrayOopDesc::test_max_array_length() {
   assert(check_max_length_overflow(T_BOOLEAN), "size_t overflow for boolean array");
   assert(check_max_length_overflow(T_CHAR), "size_t overflow for char array");
   assert(check_max_length_overflow(T_FLOAT), "size_t overflow for float array");
@@ -54,8 +52,6 @@ bool arrayOopDesc::test_max_array_length() {
   assert(check_max_length_overflow(T_NARROWOOP), "size_t overflow for narrowOop array");
 
   // T_VOID and T_ADDRESS are not supported by max_array_length()
-
-  return true;
 }
 
 

hotspot/src/share/vm/oops/arrayOop.hpp

@@ -128,7 +128,7 @@ class arrayOopDesc : public oopDesc {
 #ifndef PRODUCT
   static bool check_max_length_overflow(BasicType type);
   static int32_t old_max_array_length(BasicType type);
-  static bool test_max_array_length();
+  static void test_max_array_length();
 #endif
 };
 

hotspot/src/share/vm/prims/jni.cpp

@@ -5037,16 +5037,25 @@ _JNI_IMPORT_OR_EXPORT_ jint JNICALL JNI_GetDefaultJavaVMInitArgs(void *args_) {
 
 #ifndef PRODUCT
 
+#include "gc_interface/collectedHeap.hpp"
 #include "utilities/quickSort.hpp"
 
+#define run_unit_test(unit_test_function_call)              \
+  tty->print_cr("Running test: " #unit_test_function_call); \
+  unit_test_function_call
+
 void execute_internal_vm_tests() {
   if (ExecuteInternalVMTests) {
-    assert(QuickSort::test_quick_sort(), "test_quick_sort failed");
-    assert(arrayOopDesc::test_max_array_length(), "test_max_array_length failed");
+    tty->print_cr("Running internal VM tests");
+    run_unit_test(arrayOopDesc::test_max_array_length());
+    run_unit_test(CollectedHeap::test_is_in());
+    run_unit_test(QuickSort::test_quick_sort());
     tty->print_cr("All internal VM tests passed");
   }
 }
 
+#undef run_unit_test
+
 #endif
 
 #ifndef USDT2

hotspot/src/share/vm/utilities/quickSort.cpp

@@ -93,8 +93,7 @@ bool QuickSort::sort_and_compare(int* arrayToSort, int* expectedResult, int leng
   return compare_arrays(arrayToSort, expectedResult, length);
 }
 
-bool QuickSort::test_quick_sort() {
-  tty->print_cr("test_quick_sort");
+void QuickSort::test_quick_sort() {
   {
     int* test_array = NULL;
     int* expected_array = NULL;
@@ -214,7 +213,6 @@ bool QuickSort::test_quick_sort() {
     delete[] test_array;
     delete[] expected_array;
   }
-  return true;
 }
 
 #endif

hotspot/src/share/vm/utilities/quickSort.hpp

@@ -130,7 +130,7 @@ class QuickSort : AllStatic {
   static void print_array(const char* prefix, int* array, int length);
   static bool compare_arrays(int* actual, int* expected, int length);
   template <class C> static bool sort_and_compare(int* arrayToSort, int* expectedResult, int length, C comparator, bool idempotent = false);
-  static bool test_quick_sort();
+  static void test_quick_sort();
 #endif
 };