7121373: Clean up CollectedHeap::is_in

Summary: Fixed G1CollectedHeap::is_in, added tests, cleaned up comments and made Space::is_in pure virtual.
Reviewed-by: brutisso, tonyp, jcoomes

15 files changed, 54 insertions, 32 deletions

diff --git a/src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.hpp b/src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.hpp
index 81e220ba0..8b7e10772 100644
--- a/src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.hpp
+++ b/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);
   }
diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
index 4632a9242..9a3871292 100644
--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
+++ b/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);
diff --git a/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp b/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp
index a0a22d2f7..3f79a4d16 100644
--- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.hpp
+++ b/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
diff --git a/src/share/vm/gc_interface/collectedHeap.cpp b/src/share/vm/gc_interface/collectedHeap.cpp
index 3623e9b52..52c6e4792 100644
--- a/src/share/vm/gc_interface/collectedHeap.cpp
+++ b/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
diff --git a/src/share/vm/gc_interface/collectedHeap.hpp b/src/share/vm/gc_interface/collectedHeap.hpp
index 726a30e30..bf01c640d 100644
--- a/src/share/vm/gc_interface/collectedHeap.hpp
+++ b/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.
diff --git a/src/share/vm/memory/genCollectedHeap.cpp b/src/share/vm/memory/genCollectedHeap.cpp
index babb0068b..0ad9199ef 100644
--- a/src/share/vm/memory/genCollectedHeap.cpp
+++ b/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   ||
diff --git a/src/share/vm/memory/genCollectedHeap.hpp b/src/share/vm/memory/genCollectedHeap.hpp
index e7bea6476..ad424bae3 100644
--- a/src/share/vm/memory/genCollectedHeap.hpp
+++ b/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
diff --git a/src/share/vm/memory/generation.hpp b/src/share/vm/memory/generation.hpp
index 4561f5ad3..8eb995326 100644
--- a/src/share/vm/memory/generation.hpp
+++ b/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
diff --git a/src/share/vm/memory/space.cpp b/src/share/vm/memory/space.cpp
index ff91ba30c..7f3aceb32 100644
--- a/src/share/vm/memory/space.cpp
+++ b/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;
 }
diff --git a/src/share/vm/memory/space.hpp b/src/share/vm/memory/space.hpp
index e1fbc2389..2d718c2a5 100644
--- a/src/share/vm/memory/space.hpp
+++ b/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.
diff --git a/src/share/vm/oops/arrayOop.cpp b/src/share/vm/oops/arrayOop.cpp
index c15943835..c8239c3d9 100644
--- a/src/share/vm/oops/arrayOop.cpp
+++ b/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;
 }
 
 
diff --git a/src/share/vm/oops/arrayOop.hpp b/src/share/vm/oops/arrayOop.hpp
index e1699904c..f1b4def6a 100644
--- a/src/share/vm/oops/arrayOop.hpp
+++ b/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
 };
 
diff --git a/src/share/vm/prims/jni.cpp b/src/share/vm/prims/jni.cpp
index aef42cb38..fc35714bb 100644
--- a/src/share/vm/prims/jni.cpp
+++ b/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
diff --git a/src/share/vm/utilities/quickSort.cpp b/src/share/vm/utilities/quickSort.cpp
index bf68af1fc..e3cfa1efa 100644
--- a/src/share/vm/utilities/quickSort.cpp
+++ b/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
diff --git a/src/share/vm/utilities/quickSort.hpp b/src/share/vm/utilities/quickSort.hpp
index 17eaf4693..ba131aad6 100644
--- a/src/share/vm/utilities/quickSort.hpp
+++ b/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
 };