Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark receivers of all constructors as oslots #14419

Merged
merged 1 commit into from
Feb 3, 2022

Conversation

tajila
Copy link
Contributor

@tajila tajila commented Feb 2, 2022

Mark receivers of all constructors as oslots

Signed-off-by: Tobi Ajila [email protected]

@tajila
Copy link
Contributor Author

tajila commented Feb 2, 2022

@gacholio please review these changes

@gacholio
Copy link
Contributor

gacholio commented Feb 3, 2022

jenkins test sanity zlinux jdk17

@DanHeidinga
Copy link
Member

@tajila What's the rationale for this change? I'm curious if there's a spec change or a test case that shows this is a problem

@gacholio gacholio merged commit 457f499 into eclipse-openj9:master Feb 3, 2022
@tajila
Copy link
Contributor Author

tajila commented Feb 3, 2022

@DanHeidinga

This relates to how we implement the J9_STACKWALK_HIDE_EXCEPTION_FRAMES in swalk. we compare the A0 slot with the exception object to determine if its an exception frame.

if (walkState->flags & J9_STACKWALK_HIDE_EXCEPTION_FRAMES) {
			J9ROMMethod * romMethod = J9_ROM_METHOD_FROM_RAM_METHOD(walkState->method);

			if (!(romMethod->modifiers & J9AccStatic)) {
				if (J9UTF8_DATA(J9ROMMETHOD_NAME(romMethod))[0] == '<') {
					if (*walkState->arg0EA == (UDATA) walkState->restartException) {
						return J9_STACKWALK_KEEP_ITERATING;
					}
				}
				walkState->flags &= ~J9_STACKWALK_HIDE_EXCEPTION_FRAMES;
			}
		}

If a GC occurs while we are doing super() calls all the way up the class chain, there is a possibility that the receiver will be moved. This is not a problem if all A0 slots are oslots, but if one of them is an islot it will not updated and the algorithm above fails.

We've always had this problem and it hasn't been an issue until JDK18 as the new reflection implementation makes use of the exception stack traces.

@DanHeidinga
Copy link
Member

Thanks for explaining @tajila. Can I ask about this as well:

We've always had this problem and it hasn't been an issue until JDK18 as the new reflection implementation makes use of the exception stack traces.

the new reflection impl = using MHs - why is that using the exception stack traces?

@tajila
Copy link
Contributor Author

tajila commented Feb 3, 2022

the new reflection impl = using MHs - why is that using the exception stack traces?

Thats correct, the MH impl of reflection. The old reflection impl would treat all errors regarding input args as IllegalArgumentException, however with the new MH impl this is more difficult to detect as they show up as NPEs, classcastexceptions or WronmethodTypeExceptions. So the MH implementation catches all exceptions return from the MH and determines whether to re-throw as an illegalArg exception.

@DanHeidinga
Copy link
Member

Thanks for explaining!

@DanHeidinga
Copy link
Member

I understand we've already branched for Java 18 - should this be ported to the Java 18 branch as well? @tajila

@gacholio
Copy link
Contributor

gacholio commented Feb 4, 2022

It's worth noting that this change is completely safe - we were already doing it for one constructor case, and the new object is necessarily already being kept alive by the caller of the constructor (since constructors are void return) so we're not artificially extended the lifetime of the object.

babsingh added a commit to babsingh/aqa-tests that referenced this pull request Feb 7, 2022
The intermittent failures for IllegalArgumentsTest are fixed via
eclipse-openj9/openj9#14419.

Signed-off-by: Babneet Singh <[email protected]>
babsingh added a commit to babsingh/aqa-tests that referenced this pull request Feb 7, 2022
The intermittent failures for IllegalArgumentsTest are fixed via
eclipse-openj9/openj9#14419.

Signed-off-by: Babneet Singh <[email protected]>
renfeiw pushed a commit to adoptium/aqa-tests that referenced this pull request Feb 7, 2022
The intermittent failures for IllegalArgumentsTest are fixed via
eclipse-openj9/openj9#14419.

Signed-off-by: Babneet Singh <[email protected]>
babsingh added a commit to babsingh/aqa-tests that referenced this pull request Feb 9, 2022
exeCallerAccessTest/CallerAccessTest is disabled on OSX across JDK17, 18 and 19.

Recently, a JDK19 exclude list was created using the JDK18 exclude list. The below
JDK18 changes are being applied to JDK19:

1. SpliteratorTraversingAndSplittingTest is fixed by eclipse-openj9/openj9#14430.
Related: adoptium#3331

2. IllegalArgumentsTest is fixed by eclipse-openj9/openj9#14419.
Related: adoptium#3322

Signed-off-by: Babneet Singh <[email protected]>
renfeiw pushed a commit to adoptium/aqa-tests that referenced this pull request Feb 9, 2022
exeCallerAccessTest/CallerAccessTest is disabled on OSX across JDK17, 18 and 19.

Recently, a JDK19 exclude list was created using the JDK18 exclude list. The below
JDK18 changes are being applied to JDK19:

1. SpliteratorTraversingAndSplittingTest is fixed by eclipse-openj9/openj9#14430.
Related: #3331

2. IllegalArgumentsTest is fixed by eclipse-openj9/openj9#14419.
Related: #3322

Signed-off-by: Babneet Singh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants