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

test/jdk/java/lang/StackWalker/ReflectionFrames.java contribution #7124

Closed
1 of 2 tasks
M-Davies opened this issue Sep 18, 2019 · 8 comments
Closed
1 of 2 tasks

test/jdk/java/lang/StackWalker/ReflectionFrames.java contribution #7124

M-Davies opened this issue Sep 18, 2019 · 8 comments

Comments

@M-Davies
Copy link

M-Davies commented Sep 18, 2019

https://github.com/ibmruntimes/openj9-openjdk-jdk11/blob/4041598f624e2222be156557efbe508b2b682ae9/test/jdk/java/lang/StackWalker/ReflectionFrames.java#L1 is failing for openj9 (see #6724).

@pdbain
Copy link

pdbain commented Sep 19, 2019

Identify which implementation is incorrect

Neither is "incorrect"; each has additional implementation-dependent stack frames which are allowed but not specified by the API. The test case should ignore these implementation-dependent frames.

@M-Davies
Copy link
Author

@pdbain updated ^

@M-Davies
Copy link
Author

M-Davies commented Sep 20, 2019

Some frames might be cleared up by #3627. Will build and test with this to check

EDIT: Build failed with PR

@M-Davies
Copy link
Author

The StackWalker class has an option for StackWalker.getInstance() called SHOW_HIDDEN_FRAMES which, when used with SHOW_REFLECT_FRAMES, hides frames that are implementation specific.

This implies that the frames were supposed to be hidden by default but weren't for some reason because the failing tests were running without SHOW_HIDDEN_FRAMES or SHOW_REFLECT_FRAMES enabled.

@M-Davies
Copy link
Author

This functionality should already be enabled considering the code in the constructor of StackInspector here: https://github.com/ibmruntimes/openj9-openjdk-jdk11/blob/4041598f624e2222be156557efbe508b2b682ae9/test/jdk/java/lang/StackWalker/ReflectionFrames.java#L726

@pdbain thoughts?

@pshipton
Copy link
Member

OpenJ9 doesn't use the OpenJDK implemention for the StackWalker, you have to look at the OpenJ9 code https://github.com/eclipse/openj9/blob/43f86dc478ddbddf25f1b640804baa49dc765dcb/jcl/src/java.base/share/classes/java/lang/StackWalker.java

@M-Davies
Copy link
Author

So the base for implementing a fix for this test is already here in the test and the openj9 StackWalker. filter() will strip out the implementation specific frames which is implemented by parse in the StackInspector's constructor. I'm not sure how to go about using these tools for walkerHide yet however

Filter = https://github.com/ibmruntimes/openj9-openjdk-jdk11/blob/4041598f624e2222be156557efbe508b2b682ae9/test/jdk/java/lang/StackWalker/ReflectionFrames.java#L763

@M-Davies M-Davies changed the title java/lang/StackWalker/ReflectionFrames.java contribution test/jdk/java/lang/StackWalker/ReflectionFrames.java contribution Sep 27, 2019
@M-Davies
Copy link
Author

Looking at the code for both HS and J9, this seems like a lot of effort to fix just one test. The test is only accounting for Hotspot frames to appear and there is no current function within the test that can filter out frames that are OpenJ9 specific (this would have to be made from scratch). OpenJ9 has a very different StackWalker class compared to Hotspot so the structure of the StackInspector class in the test would have to be changed too.

In addition to this, any change made to the test would have to be approved upstream and I find it unlikely that it will be accepted by the community since we would, essentially, be modifying the test towards one implementation.

Closing this issue for the reasons stated above...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants