Read barrier verification #4037

VermaSh · 2018-12-13T16:37:41Z

This PR provides implementation for ReadBarrierVerifier. This read barrier is designed to catch direct memory read sites. Memory slots are poisoned after GC and only way to read the correct address is through ReadBarrierVerifier. Any read access made outside of this barrier will result in inaccessible address error due to shadow heap address range protection.

Use cases:
The test can be toggled individually or all together, by setting fvtest_enableReadBarrierVerification to a combination of five 1s. Testing for JNI global reference can be toggled by passing 01000. Testing for monitor table object slots can be toggled by passing 00100. Testing for class statics can be toggled by passing 00010. Testing for heap slots can be toggled by passing 00001. Finally, testing for all of the above can be toggled by passing 11111.

Scope of this instrumentation is limited to standard gc policies using non compressed pointers on 64 bit machines. Heap can only be tested on optthruput gc policy. Options, -alwaysCallReadBarrier, -Xgcpolicy:optthruput and -Xint, are needed when testing for heap slots. Root slot testing, however, doesn't need either of those and can be tested on gencon gc policy as well.

Cmd to enable instrumentation for monitor slots and class statics:
./java -Xnocompressedrefs -XXgc:fvtest_enableReadBarrierVerification=00110

Cmd to enable instrumentation for heap slots:
./java -Xnocompressedrefs -Xgcpolicy:optthruput -XXgc:fvtest_enableReadBarrierVerification=00001 -Xint -Xgc:alwaysCallReadBarrier

amicic · 2018-12-13T18:02:50Z

dependent on eclipse-omr/omr#3297

amicic · 2018-12-13T18:04:27Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

@@ -748,6 +748,9 @@ MM_StandardAccessBarrier::preMonitorTableSlotRead(J9VMThread *vmThread, j9object
 {
 	omrobjectptr_t object = (omrobjectptr_t)*srcAddress;

+	if (NULL == _extensions->scavenger) { /* This check is needed if gc_alwaysCallObjectAccessBarrier build flag is enabled */
+		return true;
+	}


remove these changes. they are correct, but we'll consider them as part of another PR

amicic · 2018-12-13T18:31:48Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	if (shadowHeapTop > (uintptr_t)object && (shadowHeapBase <= (uintptr_t)object)) {
+		healedAddress = heapBase + ((uintptr_t)object - shadowHeapBase);
+
+		// TODO: instead of atomicWriteReference, write back to srcAddress. But maybe we might need it to be atomic


let's play safe and use atomic update here. I belive that there could be a race where another thread can write a different value between the moment this thread read a poissoned value and the moment if wants to update with heal value. so, this thread should fail on update if the field has been modified meanwhile.

update two other spots too

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

dmitripivkine · 2018-12-13T19:27:27Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	GC_SlotObject slotObject(env->getOmrVM(), srcAddress);
+
+	if ((shadowHeapTop > (uintptr_t)object && shadowHeapBase <= (uintptr_t)object)) {
+		healedAddress = heapBase + ((uintptr_t)object - shadowHeapBase);


this variable should be defined here to narrow it's scope

dmitripivkine · 2018-12-13T19:28:08Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	uintptr_t heapBase = (uintptr_t)_extensions->heap->getHeapBase();
+	uintptr_t shadowHeapBase = (uintptr_t)_extensions->shadowHeapBase;
+	uintptr_t shadowHeapTop = (uintptr_t)_extensions->shadowHeapTop;
+	uintptr_t healedAddress;


remove this, define it inside if statement

dmitripivkine · 2018-12-13T19:29:13Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	uintptr_t heapTop = (uintptr_t)extensions->heap->getHeapTop();
+	uintptr_t shadowHeapBase = (uintptr_t)extensions->shadowHeapBase;
+
+	uintptr_t poisonedAddress;


define this inside if statement

dmitripivkine · 2018-12-13T19:35:27Z

runtime/gc_glue_java/GlobalCollectorDelegate.cpp

-		_extensions->accessBarrier = MM_StandardAccessBarrier::newInstance(env);
+
+#if defined(OMR_ENV_DATA64) && !defined(OMR_GC_COMPRESSED_POINTERS)
+		if (1 == (1 & _extensions->fvtest_enableReadBarrierVerification)) {


if this flag has value 1 set only, so there is no need to mask it. It would make sense if this field might have different different bit pattern set but looks like this is not a case

dmitripivkine · 2018-12-13T19:52:55Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	uintptr_t shadowHeapBase = (uintptr_t)extensions->shadowHeapBase;
+	uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;
+	uintptr_t healedAddress;
+	j9object_t object = *srcAddress;


move these two definitions inside if statement

dmitripivkine · 2018-12-13T19:53:42Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	uintptr_t shadowHeapBase = (uintptr_t)extensions->shadowHeapBase;
+	uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;
+
+	uintptr_t healedHeapAddress;


define this inside if statement

dmitripivkine · 2018-12-13T20:22:49Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	uintptr_t shadowHeapBase = (uintptr_t)_extensions->shadowHeapBase;
+	uintptr_t shadowHeapTop = (uintptr_t)_extensions->shadowHeapTop;
+	uintptr_t healedAddress;
+	j9object_t object = *srcAddress;


move these two definitions inside if statement

dmitripivkine · 2018-12-13T20:53:44Z

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp

+		GC_ClassHeapIterator classHeapIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm), segment);
+		J9Class *clazz = NULL;
+
+		while(NULL != (clazz = classHeapIterator.nextClass())) {


please add space between while and bracket

dmitripivkine · 2018-12-13T20:53:56Z

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp

+	OMR_VMThread *omrVMThread = env->getOmrVMThread();
+	GC_SegmentIterator segmentIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm)->classMemorySegments, MEMORY_TYPE_RAM_CLASS);
+
+	while(J9MemorySegment *segment = segmentIterator.nextSegment()) {


please add space between while and bracket

dmitripivkine · 2018-12-13T20:54:04Z

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp

+		GC_ClassHeapIterator classHeapIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm), segment);
+		J9Class *clazz = NULL;
+
+		while(NULL != (clazz = classHeapIterator.nextClass())) {


please add space between while and bracket

dmitripivkine · 2018-12-13T20:54:10Z

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp

+	OMR_VMThread *omrVMThread = env->getOmrVMThread();
+	GC_SegmentIterator segmentIterator(static_cast<J9JavaVM*>(omrVMThread->_vm->_language_vm)->classMemorySegments, MEMORY_TYPE_RAM_CLASS);
+
+	while(J9MemorySegment *segment = segmentIterator.nextSegment()) {


please add space between while and bracket

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp

amicic · 2018-12-13T22:46:27Z

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp

+
+			volatile omrobjectptr_t *slotPtr = NULL;
+			GC_ClassStaticsIterator classStaticsIterator(env, clazz);
+			while (NULL != (slotPtr = (omrobject_t*)classStaticsIterator.nextSlot())) {


shouldn't be casting to 'omrobjectptr_t *'?

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp

amicic · 2018-12-13T22:56:26Z

runtime/gc_modron_standard/StandardAccessBarrier.cpp

@@ -853,6 +853,7 @@ bool
 MM_StandardAccessBarrier::preObjectRead(J9VMThread *vmThread, J9Object *srcObject, fj9object_t *srcAddress)
 {
 	MM_EnvironmentStandard *env = MM_EnvironmentStandard::getEnvironment(vmThread->omrVMThread);
+


don't change spacing. this file just simply needs no change

amicic · 2018-12-13T22:57:07Z

runtime/gc_modron_standard/StandardAccessBarrier.hpp

@@ -44,7 +43,7 @@

 class MM_StandardAccessBarrier : public MM_ObjectAccessBarrier
 {
-private:
+protected:


this is probably unnecessary, now that Verifier does not have its initialize/teardown

amicic · 2018-12-13T23:03:44Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;
+	uintptr_t referenceFromSlot = (uintptr_t)*slot;
+
+	if (shadowHeapTop > referenceFromSlot && (shadowHeapBase => referenceFromSlot)) {


extra brackets around first comparisson

does operator '=>' really exist?

amicic · 2018-12-13T23:04:02Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+	uintptr_t shadowHeapTop = (uintptr_t)extensions->shadowHeapTop;
+	omrobjectptr_t object = convertPointerFromToken(*srcAddress);
+
+	if (shadowHeapTop > (uintptr_t)object && (shadowHeapBase <= (uintptr_t)object)) {


extra brackets around first comparison

dmitripivkine · 2018-12-14T15:39:30Z

runtime/gc_modron_standard/ReadBarrierVerifier.cpp

+
+bool
+MM_ReadBarrierVerifier::preObjectRead(J9VMThread *vmThread, J9Object *srcObject, fj9object_t *srcAddress)
+{


In general these virtual methods should call parent's functionality as well. It happen to be no need for now because of limited scope. "As is" is ok for now, but we should not forget about it

dmitripivkine

I believe it is good enough to submit

This PR provides implementation for ReadBarrierVerifier. This read barrier is designed to catch direct memory read sites. Memory slots are poisoned after GC and only way to read the correct address is through ReadBarrierVerifier. Any read access made outside of this barrier will result in inaccessible address error due to shadow heap address range protection. Use cases: The test can be toggled individually or all together, by setting fvtest_enableReadBarrierVerification to a combination of five 1s. Testing for JNI global reference can be toggled by passing 01000. Testing for monitor table object slots can be toggled by passing 00100. Testing for class statics can be toggled by passing 00010. Testing for heap slots can be toggled by passing 00001. Finally, testing for all of the above can be toggled by passing 11111. Scope of this instrumentation is limited to standard gc policies using non compressed pointers on 64 bit machines. Heap can only be tested on optthruput gc policy. Options, -alwaysCallReadBarrier, -Xgcpolicy:optthruput and -Xint, are needed when testing for heap slots. Root slot testing, however, doesn't need either of those and can be tested on gencon gc policy as well. Cmd to enable instrumentation for monitor slots and class statics: ./java -Xnocompressedrefs -XXgc:fvtest_enableReadBarrierVerification=00110 Cmd to enable instrumentation for heap slots: ./java -Xnocompressedrefs -Xgcpolicy:optthruput -XXgc:fvtest_enableReadBarrierVerification=00001 -Xint -Xgc:alwaysCallReadBarrier Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>

amicic · 2019-01-09T22:43:55Z

Jenkins test sanity xLinux jdk8

amicic · 2019-01-09T22:52:32Z

Jenkins test sanity xLinux jdk8 depends eclipse-omr/omr#3362

amicic requested a review from dmitripivkine December 13, 2018 18:01

amicic reviewed Dec 13, 2018

View reviewed changes

runtime/gc_modron_standard/ReadBarrierVerifier.cpp Show resolved Hide resolved

amicic reviewed Dec 13, 2018

View reviewed changes

runtime/gc_modron_standard/ReadBarrierVerifier.cpp Outdated Show resolved Hide resolved

dmitripivkine added comp:gc depends:omr Pull request is dependent on a corresponding change in OMR labels Dec 13, 2018

dmitripivkine reviewed Dec 13, 2018

View reviewed changes

amicic reviewed Dec 13, 2018

View reviewed changes

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp Show resolved Hide resolved

amicic reviewed Dec 13, 2018

View reviewed changes

runtime/gc_modron_standard/RootScannerReadBarrierVerifier.cpp Show resolved Hide resolved

amicic reviewed Dec 13, 2018

View reviewed changes

VermaSh force-pushed the readBarrierVerification branch 3 times, most recently from 010bb3d to f9b77df Compare December 14, 2018 04:42

dmitripivkine reviewed Dec 14, 2018

View reviewed changes

dmitripivkine approved these changes Dec 14, 2018

View reviewed changes

VermaSh changed the title ~~WIP: Read barrier verification~~ Read barrier verification Dec 14, 2018

VermaSh force-pushed the readBarrierVerification branch 2 times, most recently from 03d3326 to 1987b32 Compare December 14, 2018 20:06

VermaSh force-pushed the readBarrierVerification branch from 1987b32 to 54f04a8 Compare December 14, 2018 20:10

amicic mentioned this pull request Dec 17, 2018

Read barrier verification eclipse-omr/omr#3362

Merged

amicic merged commit 066af9b into eclipse-openj9:master Jan 10, 2019

VermaSh deleted the readBarrierVerification branch November 11, 2019 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read barrier verification #4037

Read barrier verification #4037

VermaSh commented Dec 13, 2018 •

edited

Loading

amicic commented Dec 13, 2018

amicic Dec 13, 2018

amicic Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

dmitripivkine Dec 13, 2018

amicic Dec 13, 2018

amicic Dec 13, 2018

amicic Dec 13, 2018

amicic Dec 13, 2018

amicic Dec 13, 2018

amicic Dec 13, 2018

dmitripivkine Dec 14, 2018

dmitripivkine left a comment

amicic commented Jan 9, 2019

amicic commented Jan 9, 2019

Read barrier verification #4037

Read barrier verification #4037

Conversation

VermaSh commented Dec 13, 2018 • edited Loading

amicic commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitripivkine left a comment

Choose a reason for hiding this comment

amicic commented Jan 9, 2019

amicic commented Jan 9, 2019

VermaSh commented Dec 13, 2018 •

edited

Loading