Implement truncated return bytecodes #1831

dnakamura · 2018-05-03T20:27:59Z

Add code which emmits the new bytecodes for truncated return types.
See #999

Signed-off-by: Devin Nakamura [email protected]

fjeremic · 2018-05-22T14:21:34Z

I had a comment on the PR which enabled JIT support before I knew this PR existed here (#1943 (comment)). @dnakamura would it be possible to add some unit tests which generate these new bytecodes to this PR?

DanHeidinga · 2018-05-22T18:29:16Z

runtime/bcverify/rtverify.c

+							returnType = BCV_BASE_TYPE_INT_BIT;
+							break;
+						default:
+							//TODO mark unreachable


Can you add an Assert unreachable here?

Do we have a common solution for that? I tried doing a quick search of the code and It looks like its usually a module or function specific tracepoint. Wasn't sure if there was a more generic solution, hence the TODO

usually a module or function specific tracepoint

That's the generic solution :) Most (many?) tdf files have the basic asserts in them at this point. Copying over an existing one if it isn't present already should be fairly straightforward

DanHeidinga · 2018-05-22T18:34:19Z

runtime/stackmap/fixreturns.c

+			break;
+		default:
+			if (romMethod->modifiers & J9AccSynchronized) {
+				return JBsyncReturn0 + (U_8) *returnSlots;


This change introduces multiple return points - can we keep it to a single return?

DanHeidinga · 2018-05-22T18:38:34Z

runtime/bcverify/rtverify.c

+							//TODO mark unreachable
+							break;
+						}
+						inconsistentStack |= ( returnType == baseTypeCharConversion[returnChar - 'A']);


Does this match the RIs behaviour? Some of the primitives can be automatically converted to others as long as the conversion doesn't lose info. ie: byte -> int. Will this fail verification in those cases?

Does this actually matter, since these return bytecodes are only generated internally?

I think you're right here - the check that the return type is compatible is already handled above this code so all this is doing is validating we put the right bytecode in for the signature.

DanHeidinga · 2018-05-31T02:04:29Z

runtime/bcverify/rtverify.c

+							break;
+
+						case JBreturn1:
+						case JBsyncReturn1:


Can you add a comment here that the synchronized {b,c,s,z} will always be handled by JBgenericReturn? It will make it more obvious to anyone debugging through the verifier that this is correct without requiring them to check how the bytecodes are written

DanHeidinga · 2018-05-31T02:14:46Z

Can you also look into why the copyright check is failing?

DanHeidinga · 2018-06-08T04:22:34Z

Thanks for fixing the copyright check. There's one remaining review comment and some unit tests required. When do you think this could be completed?

dnakamura · 2018-06-11T17:46:49Z

Hope to get it done by the end of the week, I have a standalone test case that needs to be polished a bit and integrated with the test framework

dnakamura · 2018-06-15T18:38:27Z

Currently hitting an issue with shared classes. I believe the issue is that there is an old shared class cache which has the old bytecodes.

DanHeidinga · 2018-06-17T03:40:17Z

@pshipton @hangshao0 What's the process for incrementing the SCC version number when the ROMClass format changes?

pshipton · 2018-06-17T04:33:52Z

In OSCache.hpp, you'll find the following define:

#define OSCACHE_CURRENT_CACHE_GEN

To add a new generation, increment the OSCACHE_CURRENT_CACHE_GEN by 2. There are 3 case statements which need to be updated with the old generation numbers, search for "case OSCACHE_CURRENT_CACHE_GEN".

The cache generation number is appended to the cache file name. The effect of changing the generation number is that the JVM will create a new cache file, rather than trying to open the old one. We'll have to document / release note this, as it leaves a extra file in the file system, until the cache is destroyed (which destroys older generations as well).

dnakamura · 2018-06-18T18:01:21Z

@pshipton Is there any reason the switch statement are not re-structured as ifs ? Would make life easier since they wont need to be changed every time the version is bumped

hangshao0 · 2018-06-18T18:26:14Z

Re-structure to if...else should be fine.

dnakamura · 2018-07-26T14:56:27Z

@DanHeidinga this should be good to go

pshipton · 2018-10-11T02:30:42Z

@dnakamura there are some conflicts to be resolved.
What happened about changing the shared cache version?

pshipton · 2018-10-11T02:33:12Z

runtime/vm/BytecodeInterpreter.hpp

-					} else if (isConstructor) {
-						newBytecode = JBreturnFromConstructor;
+					/* are we dealing with a case where we should use a truncated return bytecode? */
+					if(truncatedReturnBytecode != 0){


There is a missing space after the if

and also before the curly bracket.

pshipton · 2018-10-11T02:33:55Z

runtime/vm/BytecodeInterpreter.hpp

-					} else if (isConstructor) {
-						newBytecode = JBreturnFromConstructor;
+					/* are we dealing with a case where we should use a truncated return bytecode? */
+					if (truncatedReturnBytecode != 0){


Missing space before the curly bracket here too.

DanHeidinga · 2018-10-11T03:10:40Z

runtime/bcverify/rtverify.c

+
+					/* check methods that return char, byte, short, or bool use the right opcode */
+					if (BCV_BASE_TYPE_INT == type){
+						switch(bc){


nitpicks on the spacing here as well. Between the ){ here and line above.

dnakamura · 2018-10-11T16:23:53Z

@pshipton looking at the conflicts and comments now. There turned out to be a bug in my earlier code which caused issues with the shared classes cache. However this shouldn't break compatibility, since all returns are converted to the generic return opcode before being put in the cache

DanHeidinga · 2018-10-11T16:48:24Z

since all returns are converted to the generic return opcode before being put in the cache

@dnakamura that's backwards actually. We run fixreturns to ensure that genericReturn is never present in the sharedclasses cache (apart from contstructors). Otherwise, the returns are subject to patching - see:

https://github.com/eclipse/openj9/blob/f031ac4012794306c16a6ba8fcc9907ca6fbcf90/runtime/vm/BytecodeInterpreter.hpp#L7662-L7672

DanHeidinga · 2018-10-12T20:26:34Z

No, this isn't ready to go yet. Some of the review comments haven't been addressed and I believe this will leave JBgenericReturn bytecodes in romclasses from the sharedcache which we'll try to fix at runtime.

The bytecode patching in JBgenericReturn in the interpreter will need to be modified if we're genericreturns in SCC romclasses

pshipton · 2018-10-12T20:33:54Z

Then it seems it should be removed from the 0.11.0 plan and deferred to 0.12.0

dnakamura · 2018-10-15T19:45:13Z

IT looks like I forgot to push the changes to the branch (or maybe pushed them to the wrong branch). Latest changes with fixes for comments should be there

@dnakamura that's backwards actually. We run fixreturns to ensure that genericReturn is never present in the sharedclasses cache (apart from contstructors). Otherwise, the returns are subject to patching - see:

Oh yeah, thats right. I knew there was some issue involving patching generic returns and the shared classes cache. The problem is addressed in these changes
https://github.com/eclipse/openj9/pull/1831/files#diff-5ca144eb3d6b1334698c9840002b59c3R7671

pshipton · 2018-10-15T20:09:42Z

@DanHeidinga ^

DanHeidinga

@smlambert @sophia-guo Can one of you give the tests a look? It looks OK to me

DanHeidinga · 2018-10-16T01:49:20Z

runtime/bcverify/rtverify.c

+								__LINE__);
+							break;
+						}
+						inconsistentStack |= !( returnType == baseTypeCharConversion[returnChar - 'A']);


Isn't this just inconsistentStack |= (returnType != baseTypeCharConversion[returnChar - 'A']); or is there some other reason to negate the whole expression?

DanHeidinga · 2018-10-16T01:57:21Z

Jenkins test sanity plinux,win,zlinux jdk8

sophia-guo · 2018-10-16T12:05:23Z

test/functional/Java8andUp/testng.xml

@@ -30,6 +30,7 @@
 	<test name="generalTest">
 		<classes>
 			<class name="org.openj9.test.gpu.SortTest" />
+			<class name="org.openj9.test.truncatedReturn.TestTruncatedReturn" />


We are planning to rename the generalTest to GPUSortTest to be more straightforward. Each testname could indicate kind of its feature just like all other tests. So probably adding a new test naming truncateReturn would be better.

sophia-guo · 2018-10-16T12:06:12Z

test/functional/Java8andUp/playlist.xml

@@ -27,7 +27,7 @@
 	<test>
 		<testCaseName>generalSanityTest</testCaseName>
 		<command>$(JAVA_COMMAND) $(JVM_OPTIONS) \
-	-cp $(Q)$(RESOURCES_DIR)$(P)$(TESTNG)$(P)$(TEST_RESROOT)$(D)GeneralTest.jar$(Q) \
+	-cp $(Q)$(RESOURCES_DIR)$(P)$(TESTNG)$(P)$(LIB_DIR)$(D)asm-all.jar$(P)$(TEST_RESROOT)$(D)GeneralTest.jar$(Q) \


Same as comment for testng.xml. Adding a separate testcase would be better.

DanHeidinga · 2018-10-16T19:51:55Z

@sophia-guo Are you ok with the updated tests?

Add code which emmits the new bytecodes for truncated return types. See eclipse-openj9#999 Signed-off-by: Devin Nakamura <[email protected]>

sophia-guo

LGTM

DanHeidinga · 2018-10-16T20:40:01Z

Jenkins test sanity plinux,xlinux,win jdk8

DanHeidinga · 2018-10-16T20:40:21Z

Jenkins test extended xlinux jdk8

When new bytecodes were added in eclipse-openj9#1831 for the truncated returns, the SCC generation number wasn't incremented which can lead to crashes. Currently the VM will attempt to destroy and recreate the SCC if the SHA in the header is different but if can't, it will continue to use the cache. The only sure way (today) to prevent this issue is to increment the cache generation. Do this now to prevent VerifyErrors due to unknown bytecodes while we work out the right behaviour. issue: eclipse-openj9#5380 Signed-off-by: Dan Heidinga <[email protected]>

dnakamura force-pushed the ret_op3 branch from 9d8a014 to a7caf81 Compare May 3, 2018 20:29

pshipton added the comp:vm label May 8, 2018

DanHeidinga mentioned this pull request May 22, 2018

Add bytecodes for returning byte,char,short and bool #1943

Merged

DanHeidinga reviewed May 22, 2018

View reviewed changes

dnakamura force-pushed the ret_op3 branch from a7caf81 to ada41e1 Compare May 30, 2018 21:43

DanHeidinga reviewed May 31, 2018

View reviewed changes

dnakamura force-pushed the ret_op3 branch from ada41e1 to 10b74f8 Compare May 31, 2018 14:51

dnakamura changed the title ~~WIP: Implement truncated return bytecodes~~ Implement truncated return bytecodes May 31, 2018

dnakamura force-pushed the ret_op3 branch from 10b74f8 to fdd6e6e Compare June 4, 2018 22:55

dnakamura force-pushed the ret_op3 branch from fdd6e6e to b176626 Compare June 25, 2018 15:16

tajila mentioned this pull request Jul 16, 2018

Correctly truncate boolean values in the appropriate bytecodes #2049

Closed

12 tasks

dnakamura force-pushed the ret_op3 branch from b176626 to 768e60c Compare July 26, 2018 14:56

dnakamura force-pushed the ret_op3 branch from 768e60c to 169879d Compare July 26, 2018 22:11

pshipton reviewed Oct 11, 2018

View reviewed changes

DanHeidinga reviewed Oct 11, 2018

View reviewed changes

DanHeidinga added this to the Release 0.11.0 milestone Oct 11, 2018

dnakamura force-pushed the ret_op3 branch 2 times, most recently from a13e67b to eec7ffb Compare October 15, 2018 19:40

DanHeidinga reviewed Oct 16, 2018

View reviewed changes

sophia-guo reviewed Oct 16, 2018

View reviewed changes

dnakamura force-pushed the ret_op3 branch from eec7ffb to 0b4b71a Compare October 16, 2018 19:42

dnakamura force-pushed the ret_op3 branch from 0b4b71a to 43f70b6 Compare October 16, 2018 19:52

Implement truncated return bytecodes

43f70b6

Add code which emmits the new bytecodes for truncated return types. See eclipse-openj9#999 Signed-off-by: Devin Nakamura <[email protected]>

sophia-guo approved these changes Oct 16, 2018

View reviewed changes

DanHeidinga self-assigned this Oct 16, 2018

DanHeidinga approved these changes Oct 17, 2018

View reviewed changes

DanHeidinga merged commit 8c6d5e2 into eclipse-openj9:master Oct 17, 2018

DanHeidinga mentioned this pull request Oct 17, 2018

The nightly build (jdk-9+181-20 on January 22 2018) has different behaviors from OpenJDK #999

Closed

dnakamura deleted the ret_op3 branch November 13, 2018 19:08

hangshao0 mentioned this pull request Apr 5, 2019

The shared cache compatibility broken #5380

Closed

DanHeidinga mentioned this pull request Apr 9, 2019

Increment the SharedClasses cache generation number #5422

Merged

pshipton mentioned this pull request Apr 9, 2019

Shared cache generation number change eclipse-openj9/openj9-docs#248

Closed

DanHeidinga mentioned this pull request Apr 10, 2019

(v0.14.0) Increment the SharedClasses cache generation number #5434

Merged

Implement truncated return bytecodes #1831

Implement truncated return bytecodes #1831

Conversation

dnakamura commented May 3, 2018

fjeremic commented May 22, 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

DanHeidinga commented May 31, 2018

DanHeidinga commented Jun 8, 2018

dnakamura commented Jun 11, 2018

dnakamura commented Jun 15, 2018

DanHeidinga commented Jun 17, 2018

pshipton commented Jun 17, 2018

dnakamura commented Jun 18, 2018

hangshao0 commented Jun 18, 2018

dnakamura commented Jul 26, 2018

pshipton commented Oct 11, 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

dnakamura commented Oct 11, 2018

DanHeidinga commented Oct 11, 2018

DanHeidinga commented Oct 12, 2018

pshipton commented Oct 12, 2018

dnakamura commented Oct 15, 2018 • edited Loading

pshipton commented Oct 15, 2018

DanHeidinga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanHeidinga commented Oct 16, 2018

sophia-guo left a comment

Choose a reason for hiding this comment

DanHeidinga commented Oct 16, 2018

DanHeidinga commented Oct 16, 2018

dnakamura commented Oct 15, 2018 •

edited

Loading