Add the check of BCV_SPECIAL in generating stackmaps #9419

ChengJin01 · 2020-04-30T17:06:20Z

The change is to add the check of BCV_SPECIAL when
merging & matching stacks to ensure the uninitializedThis
flag is correctly set up during the generation of stackmaps
so as to match the RI's behavior at runtime verification.

Fixes: #9385

Signed-off-by: Cheng Jin jincheng@ca.ibm.com

ChengJin01 · 2020-04-30T17:07:57Z

Verified the failing cases at #9385 and #5676, which works good as expected:

$ ../jdk11_openj9_bcv/bin/java  -cp sootOutput/junit-junit:hamcrest-all-1.3.jar org.junit.runner.JUnitCore org.junit.internal.runners.ErrorReportingRunnerTest
JUnit version 4.13

Time: 0.059
There were 2 failures:
1) givenInvalidTestClassErrorAsCause(org.junit.internal.runners.ErrorReportingRunnerTest)
java.lang.VerifyError: JVMVRFY012 stack shape inconsistent; class=org/junit/internal/runners/ErrorReportingRunner, 
method=<init>(Ljava/lang/Class;Ljava/lang/Throwable;)V, pc=22
	at org.junit.internal.runners.ErrorReportingRunnerTest.givenInvalidTestClassErrorAsCause(ErrorReportingRunnerTest.java:50)
...
2) givenInvalidTestClass_integrationTest(org.junit.internal.runners.ErrorReportingRunnerTest)
java.lang.VerifyError: JVMVRFY012 stack shape inconsistent; class=org/junit/internal/runners/ErrorReportingRunner, 
method=<init>(Ljava/lang/Class;Ljava/lang/Throwable;)V, pc=22
	at org.junit.runners.model.RunnerBuilder.safeRunnerForClass(RunnerBuilder.java:76)
	at org.junit.runners.model.RunnerBuilder.runners(RunnerBuilder.java:125)
	at org.junit.runners.model.RunnerBuilder.runners(RunnerBuilder.java:111)


$ ../jdk11_openj9_bcv/bin/java  -agentpath:YourKit-JavaProfiler-2019.1/bin/linux-x86-64/libyjpagent.so  -cp .:spring-aop-5.16.jar:spring-core-5.1.6.jar:commons-logging-1.2.jar -Xint  Target
[YourKit Java Profiler 2019.1-b117] Log file: /home/jincheng/.yjp/log/Target-20155.log
$ (PASSED without any exception)

ChengJin01 · 2020-04-30T17:08:10Z

Reviewer: @DanHeidinga

ChengJin01 · 2020-04-30T17:48:34Z

@DanHeidinga, please hold off reviewing the code as I detected the VerifyError was still detected by Hotspot after manually changing the class version to 42 in the class file.

ChengJin01 · 2020-05-08T15:07:04Z

@DanHeidinga , the fix is ready for review as all personal builds & Grinder tests passed without any issue specific to verifier.

DanHeidinga · 2020-06-18T18:54:04Z

runtime/bcverify/bcverify.c

+						if ((*targetStackPtr != (UDATA) (BCV_BASE_TYPE_TOP))
+						&& !(*targetStackPtr & BCV_SPECIAL)


Isn't the local targetItem already initialized to be *targetStackPtr? Can the local be used here?

Also, please use explicit boolean conditions

Suggested change

if ((*targetStackPtr != (UDATA) (BCV_BASE_TYPE_TOP))

&& !(*targetStackPtr & BCV_SPECIAL)

if ((targetItem != (UDATA) (BCV_BASE_TYPE_TOP))

&& ((targetItem & BCV_SPECIAL) == 0)

Is this really checking for (targetItem & BCV_SPECIAL_INIT) != 0 to avoid changing to BCV_BASE_TYPE_TOP in that case?

Isn't the local targetItem already initialized to be *targetStackPtr? Can the local be used here? Also, please use explicit boolean conditions

Yes, we should use targetItem instead of *targetStackPtr in this case. Updated as suggested.

Is this really checking for (targetItem & BCV_SPECIAL_INIT) != 0 to avoid changing to BCV_BASE_TYPE_TOP in that case?

Yes, it was based on the thorough debugging with the class files in question. It is obvious that the intention of the original implementation was to reserve the BCV_SPECIAL bits so as to set up uninitializedThis in setInitializedThisStatus but somehow the bits were wipe out here accidentally to deal with other situations. The BCV_SPECIAL bits can't be restored back or reset up later in simulateStack once they are removed here. That's why we need to handle the case on both mergeStack() and matchStack() to ensure the BCV_SPECIAL bits from the generated stackmaps are correctly addressed.

DanHeidinga · 2020-06-18T18:56:20Z

runtime/bcverify/j9bcverify.tdf

@@ -114,7 +114,7 @@ TraceExit=Trc_RTV_findAndMatchStack_Exit Overhead=1 Level=3 Template="findAndMat
 TraceEntry=Trc_RTV_matchStack_Entry Overhead=1 Level=3 Template="matchStack - inlineMatch is %i"
 TraceException=Trc_RTV_matchStack_DepthMismatchException Overhead=1 Level=1 Template="matchStack - %.*s %.*s%.*s mismatched stack depths, live = %i, target = %i"
 TraceException=Trc_RTV_matchStack_IncompatibleClassException Overhead=1 Level=1 Template="matchStack - %.*s %.*s%.*s incompatible objects at offset %i, live = 0x%X, target = 0x%X"
-TraceException=Trc_RTV_matchStack_PrimitiveMismatchException Overhead=1 Level=1 Template="matchStack - %.*s %.*s%.*s incompatible primitives at offset %i, live = 0x%X, target = 0x%X"
+TraceException=Trc_RTV_matchStack_PrimitiveOrSpecialMismatchException Overhead=1 Level=1 Template="matchStack - %.*s %.*s%.*s incompatible primitives or special at offset %i, live = 0x%X, target = 0x%X"


Tracepoints aren't changed. If necessary, the current one is obsoleted and a new one is introduced at the end of the file so the index & format never change

Agree and moved the new tracepoint to the end of the file.

DanHeidinga · 2020-06-18T20:09:52Z

runtime/bcverify/rtverify.c

+			/* Only skip the check on the target slot with BCV_SPECIAL for the generated stackmaps
+			 * as this slot is set up based on the bytecode itself rather than decompressed stackmaps.
+			 */
+			} else if ((*targetPtr != BCV_BASE_TYPE_TOP) && !((*targetPtr & BCV_SPECIAL) && verifyData->createdStackMap)) {
+				Trc_RTV_matchStack_PrimitiveOrSpecialMismatchException(verifyData->vmStruct,


Suggested change

/* Only skip the check on the target slot with BCV_SPECIAL for the generated stackmaps

* as this slot is set up based on the bytecode itself rather than decompressed stackmaps.

*/

} else if ((*targetPtr != BCV_BASE_TYPE_TOP) && !((*targetPtr & BCV_SPECIAL) && verifyData->createdStackMap)) {

Trc_RTV_matchStack_PrimitiveOrSpecialMismatchException(verifyData->vmStruct,

} else if (*targetPtr != BCV_BASE_TYPE_TOP) {

if (((*targetPtr & BCV_SPECIAL) != 0) && verifyData->createdStackMap) {

/* Generated stackmaps can skip the check on the target slot with BCV_SPECIAL

* as this slot is set up based on the bytecode itself rather than decompressed stackmaps.

*/

} else {

Trc_RTV_matchStack_PrimitiveOrSpecialMismatchException(verifyData->vmStruct,

.....

rc = BCV_FAIL; /* fail - primitive or special mismatch */

goto _incompatibleType;

}

}

Reorganizing this may be clearer about when the check is being skipped vs not.

Agree and updated against the suggestion above.

DanHeidinga · 2020-06-19T01:59:11Z

runtime/bcverify/j9bcverify.tdf

@@ -114,7 +114,6 @@ TraceExit=Trc_RTV_findAndMatchStack_Exit Overhead=1 Level=3 Template="findAndMat
 TraceEntry=Trc_RTV_matchStack_Entry Overhead=1 Level=3 Template="matchStack - inlineMatch is %i"
 TraceException=Trc_RTV_matchStack_DepthMismatchException Overhead=1 Level=1 Template="matchStack - %.*s %.*s%.*s mismatched stack depths, live = %i, target = %i"
 TraceException=Trc_RTV_matchStack_IncompatibleClassException Overhead=1 Level=1 Template="matchStack - %.*s %.*s%.*s incompatible objects at offset %i, live = 0x%X, target = 0x%X"
-TraceException=Trc_RTV_matchStack_PrimitiveMismatchException Overhead=1 Level=1 Template="matchStack - %.*s %.*s%.*s incompatible primitives at offset %i, live = 0x%X, target = 0x%X"


The old tracepoint has to stay here

The change is to add the check of BCV_SPECIAL when merging & matching stacks to ensure the uninitializedThis flag is correctly set up during the generation of stackmaps so as to match the RI's behavior at runtime verification.[ci skip] Fixes: eclipse-openj9#9385 Signed-off-by: Cheng Jin <jincheng@ca.ibm.com>

DanHeidinga

lgtm

DanHeidinga · 2020-06-19T04:02:37Z

Jenkins test sanity plinux,zlinux jdk8

DanHeidinga · 2020-06-19T04:02:47Z

Jenkins test sanity win,osx jdk11

DanHeidinga · 2020-06-19T04:03:00Z

Jenkins test sanity xlinux jdk14

DanHeidinga · 2020-06-19T13:52:06Z

Ported to 0.21 release branch in #9951

ChengJin01 changed the title ~~Ignore the check on uninitializedThis for class version < 49~~ Add the check of BCV_SPECIAL in generating stackmaps May 5, 2020

ChengJin01 force-pushed the bcv_ignore_init_flag_check branch 6 times, most recently from d65321b to 657b969 Compare May 7, 2020 23:24

DanHeidinga reviewed Jun 18, 2020

View reviewed changes

ChengJin01 force-pushed the bcv_ignore_init_flag_check branch from 657b969 to f2335ae Compare June 18, 2020 21:03

DanHeidinga reviewed Jun 19, 2020

View reviewed changes

ChengJin01 force-pushed the bcv_ignore_init_flag_check branch from f2335ae to a811a1a Compare June 19, 2020 02:54

DanHeidinga approved these changes Jun 19, 2020

View reviewed changes

DanHeidinga self-assigned this Jun 19, 2020

DanHeidinga added the comp:vm label Jun 19, 2020

DanHeidinga added this to the Release 0.21 (Java 8, 11, 14) July refresh milestone Jun 19, 2020

DanHeidinga merged commit e8d7c56 into eclipse-openj9:master Jun 19, 2020

DanHeidinga mentioned this pull request Jun 19, 2020

(0.21.0) Add the check of BCV_SPECIAL in generating stackmaps #9951

Merged

This was referenced Nov 3, 2020

Reduce the optimization level of bcverify.c in compilation #11086

Merged

JDK15 java/util/StringJoiner/StringJoinerTest.java - Type=Segmentation error vmState=0x00060000 #10947

Closed

ChengJin01 mentioned this pull request Jan 21, 2021

J9 (1.8.0_275-b01) crashed while J9 (9.0.4) and J9 (11.0.9+11) did not. #11683

Closed

ChengJin01 deleted the bcv_ignore_init_flag_check branch March 17, 2022 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the check of BCV_SPECIAL in generating stackmaps #9419

Add the check of BCV_SPECIAL in generating stackmaps #9419

ChengJin01 commented Apr 30, 2020 •

edited

ChengJin01 commented Apr 30, 2020

ChengJin01 commented Apr 30, 2020

ChengJin01 commented Apr 30, 2020 •

edited

ChengJin01 commented May 8, 2020

DanHeidinga Jun 18, 2020

DanHeidinga Jun 18, 2020

ChengJin01 Jun 18, 2020

ChengJin01 Jun 18, 2020 •

edited

DanHeidinga Jun 18, 2020

ChengJin01 Jun 18, 2020

DanHeidinga Jun 18, 2020

ChengJin01 Jun 18, 2020

DanHeidinga Jun 19, 2020

ChengJin01 Jun 19, 2020

DanHeidinga left a comment

DanHeidinga commented Jun 19, 2020

DanHeidinga commented Jun 19, 2020

DanHeidinga commented Jun 19, 2020

DanHeidinga commented Jun 19, 2020

		if ((*targetStackPtr != (UDATA) (BCV_BASE_TYPE_TOP))
		&& !(*targetStackPtr & BCV_SPECIAL)

Add the check of BCV_SPECIAL in generating stackmaps #9419

Add the check of BCV_SPECIAL in generating stackmaps #9419

Conversation

ChengJin01 commented Apr 30, 2020 • edited

ChengJin01 commented Apr 30, 2020

ChengJin01 commented Apr 30, 2020

ChengJin01 commented Apr 30, 2020 • edited

ChengJin01 commented May 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChengJin01 Jun 18, 2020 • edited

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

DanHeidinga left a comment

Choose a reason for hiding this comment

DanHeidinga commented Jun 19, 2020

DanHeidinga commented Jun 19, 2020

DanHeidinga commented Jun 19, 2020

DanHeidinga commented Jun 19, 2020

ChengJin01 commented Apr 30, 2020 •

edited

ChengJin01 commented Apr 30, 2020 •

edited

ChengJin01 Jun 18, 2020 •

edited