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.

Sign up for GitHub

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

Jump to bottom

Add NULL restricted check #18331

Merged
merged 1 commit into from
Oct 27, 2023
Merged

Add NULL restricted check #18331

merged 1 commit into from
Oct 27, 2023

Conversation

hangshao0
Copy link
Contributor

Q type is going to be removed. Instead we should do null restricted modifier check.
In order for existing test to work, this change does not remove q type checks from the existing code. NULL restricted check is appended to places where Q type check is done.

issue #18157

@hangshao0 hangshao0 requested review from theresa-m and tajila October 23, 2023 15:05
@hangshao0 hangshao0 added comp:vm project:valhalla Used to track Project Valhalla related work labels Oct 23, 2023
@hangshao0 hangshao0 marked this pull request as ready for review October 23, 2023 15:06
tajila
tajila reviewed Oct 23, 2023
View reviewed changes
runtime/vm/ValueTypeHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/ValueTypeHelpers.cpp Outdated Show resolved Hide resolved
runtime/vm/ValueTypeHelpers.hpp Outdated
goto done;
}
} else {
/* Nullable class type or interface type */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the else block needed? we seem to handle the non-flattened case in the if (J9ROMFIELD_IS_NULL_RESTRICTED(result->field)) { as well. Perhaps L can look like what Q used to look like since "null restricted" doesn't add any semantic differences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the else block needed?

Yes, the else block is needed for nullable L types. The code in the else block is the same as previous L case.

Perhaps L can look like what Q used to look like since "null restricted" doesn't add any semantic differences.

The code of L in the null restricted case is the same as what Q used to be.

For the nullable case, the L code is doing VM_ValueTypeHelpers::acmp() that contains an additional J9_IS_J9CLASS_VALUETYPE(lhsClass) && (rhsClass == lhsClass) comparison before isSubstitutable(), while the previous Q case is directly callingisSubstitutable(). I guess they are different.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a NULL check would be needed to collapse those statements.

runtime/vm/resolvesupport.cpp Show resolved Hide resolved
tajila
tajila reviewed Oct 24, 2023
View reviewed changes
runtime/vm/ValueTypeHelpers.hpp Outdated
rc = true;
} else {
/* When unflattened, we get our object from the specified offset, then increment past the header to the first field. */
rc = isSubstitutable(currentThread, objectAccessBarrier, lhsFieldObject, rhsFieldObject, J9VMTHREAD_OBJECT_HEADER_SIZE(currentThread), fieldClass);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After looking at it some more, I think this needs to be VM_ValueTypeHelpers::acmp since value classes can have super classes that aren't j.l.Object.

runtime/vm/ValueTypeHelpers.hpp

if (!VM_ValueTypeHelpers::acmp(currentThread, objectAccessBarrier, lhsObject, rhsObject)) {
case 'L': {
if (J9ROMFIELD_IS_NULL_RESTRICTED(result->field)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(J9_IS_FIELD_FLATTENED(fieldClass, result->field) implies null restricted and if you replace isSubstitutable with acmp then you can handle both null restricted and nullabe, so the if (J9ROMFIELD_IS_NULL_RESTRICTED(result->field)) { is not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change is updated to remove the J9ROMFIELD_IS_NULL_RESTRICTED check and handle the unflattened cases (nullable and null free) together.

@tajila
Copy link
Contributor

tajila commented Oct 24, 2023

Jenkins test sanity win jdk8

@tajila
Copy link
Contributor

tajila commented Oct 24, 2023

Jenkins test sanity,extended xlinuxval jdknext

@tajila
Copy link
Contributor

tajila commented Oct 24, 2023

Jenkins test sanity plinuxvalst jdknext

@hangshao0
Copy link
Contributor Author

For the compilation error:
error: 'fieldClass' was not declared in this scope

We need to move:

J9Class *fieldClass = findJ9ClassInFlattenedClassCache() before J9_IS_FIELD_FLATTENED().
Looking at the code in findJ9ClassInFlattenedClassCache(), it is asserting foundClass is not NULL, which means it can only be called after the Q (null restricted) type check.

Do you want to remove the assertion in findJ9ClassInFlattenedClassCache() or add the Q (null restricted) check back ? @TobiAjila

@tajila
Copy link
Contributor

tajila commented Oct 24, 2023

Do you want to remove the assertion in findJ9ClassInFlattenedClassCache() or add the Q (null restricted) check back ?

Ah okay, the original null restricted check is better.

@hangshao0
Add NULL restricted check
25058c6 
Q type is going to be removed. Instead we should do null restricted
modifier check. 
In order for existing test to work, this change does not remove q type
checks from the existing code. NULL restricted check is appended to
places where Q type check is done.

issue eclipse-openj9#18157

Signed-off-by: Hang Shao <[email protected]>
@hangshao0
Copy link
Contributor Author

Ah okay, the original null restricted check is better.

The null restricted check is added back.

@tajila
Copy link
Contributor

tajila commented Oct 25, 2023

Jenkins test sanity,extended xlinuxval jdknext

@tajila
Copy link
Contributor

tajila commented Oct 25, 2023

Jenkins test sanity plinuxvalst jdknext

@tajila
Copy link
Contributor

tajila commented Oct 25, 2023

Jenkins test sanity win jdk8

@hangshao0
Copy link
Contributor Author

The failures in the PR build:

jep454/downcall/InvalidDownCallTests.java:35: error: Arena is a preview API and is disabled by default.
12:47:55      [javac] import java.lang.foreign.Arena;
12:47:55      [javac]                         ^
12:47:55      [javac]   (use --enable-preview to enable preview APIs)

is not related to this change. They are caused by --enable-preview missing in the build flag.

@JasonFengJ9
Copy link
Member

They are caused by --enable-preview missing in the build flag.

Valhalla extension doesn't have the latest API changes either.

16:44:01      [javac] /home/jenkins/workspace/Grinder/aqa-tests/functional/Java22andUp/src/org/openj9/test/jep454/upcall/UpcallMHWithPrimTests.java:689: error: cannot find symbol
16:44:01      [javac] 			MemorySegment doubleSegmt = arena.allocateFrom(JAVA_DOUBLE, 1159.748D);
16:44:01      [javac] 			                                 ^
16:44:01      [javac]   symbol:   method allocateFrom(OfDouble,double)
16:44:01      [javac]   location: variable arena of type Arena

@JasonFengJ9
Copy link
Member

Temporarily disable JEP 454 tests

@hangshao0
Copy link
Contributor Author

@TobiAjila Do you have more review comments ?

@tajila
Copy link
Contributor

tajila commented Oct 27, 2023

No Ill merge

@tajila tajila merged commit bc054dc into eclipse-openj9:master Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@theresa-m theresa-m theresa-m approved these changes

@tajila tajila tajila approved these changes

Assignees
No one assigned
Labels
comp:vm project:valhalla Used to track Project Valhalla related work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hangshao0 @tajila @JasonFengJ9 @theresa-m