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

Support -XX:[+/-]ShowHiddenFrames option #3627

Merged
merged 2 commits into from
Jan 26, 2022

Conversation

fengxue-IS
Copy link
Contributor

@fengxue-IS fengxue-IS commented Nov 9, 2018

  • removed unused VM runtime flags
  • added hiddenFrames count info to J9GetStackTraceUserData
  • added new VM option -XX:[+/-]ShowHiddenFrames
  • added new VM runtime flag J9_RUNTIME_SHOW_HIDDEN_FRAMES for new option
  • Add check to skip hidden frames during walkFrame() call
  • Add new flag to all fillInStackTrace implementations

Fixes: #3394, Fixes: #14132

Signed-off-by: Jack Lu Jack.S.Lu@ibm.com

@fengxue-IS fengxue-IS changed the title WIP: Hide synthetic lambda frame during stack trace creation Hide synthetic lambda frame during stack trace creation Nov 12, 2018
@fengxue-IS
Copy link
Contributor Author

@tajila can you take a look?

runtime/jcl/common/jclexception.c Outdated Show resolved Hide resolved
runtime/jcl/common/jclexception.c Outdated Show resolved Hide resolved
@fengxue-IS
Copy link
Contributor Author

Senior review : @DanHeidinga please take a look

runtime/jcl/common/jclexception.c Outdated Show resolved Hide resolved
runtime/oti/j9consts.h Show resolved Hide resolved
@fengxue-IS
Copy link
Contributor Author

@DanHeidinga can I get a re-review please?

@M-Davies
Copy link

Any update on this @fengxue-IS?

@fengxue-IS
Copy link
Contributor Author

@DanHeidinga Can you take a look at this please

@genie-openj9
Copy link

Can one of the admins verify this patch?

@tajila
Copy link
Contributor

tajila commented Dec 13, 2021

@fengxue-IS Can you please rebase this PR with the latest changes

@tajila
Copy link
Contributor

tajila commented Dec 13, 2021

@DanHeidinga did you have any objections to these changes?

We have noticed some JDK18 tests that require this behaviour

@fengxue-IS
Copy link
Contributor Author

@tajila Can you please take a look.
I made a revert commit for the the original changes and build a new approach as I noticed the old logic will generate a StackTraceElement[] array with null entries at the end for hidden frames removed which breaks some expected behavior of getStackTrace.
The new approach will filter the hidden frames when the internal stacktrace array is created during fillInStackTrace by specifying the newly added flag J9_STACKWALK_SKIP_HIDDEN_FRAMES .

Few points to note:

  1. which Java version should this behavior be enabled on?
  2. which APIs other than Throwable.fillInStackTrace should we specify J9_STACKWALK_SKIP_HIDDEN_FRAMES (I noticed that we have a different impl for j.l.Thread.getStackTrace which doesn't use Throwable.fillInStackTrace to initialize)
  3. should we update StackWalker natives to use the new flag or keeping filtering in the iterator?

We could also continue with the old approach by building a new iterator for the first pass of iterateStackTrace call when calculating the number of frames (https://github.com/eclipse-openj9/openj9/blob/master/runtime/jcl/common/jclexception.cpp#L324).

@fengxue-IS fengxue-IS changed the title Hide synthetic lambda frame during stack trace creation Support -XX:[+/-]ShowHiddenFrames option Dec 15, 2021
@tajila
Copy link
Contributor

tajila commented Dec 15, 2021

which Java version should this behavior be enabled on?

all versions

which APIs other than Throwable.fillInStackTrace should we specify J9_STACKWALK_SKIP_HIDDEN_FRAMES (I noticed that we have a different impl for j.l.Thread.getStackTrace which doesn't use Throwable.fillInStackTrace to initialize)

I think we should try to match the RI as much as possible

should we update StackWalker natives to use the new flag or keeping filtering in the iterator?

Yes, lets handle this in a uniform manner

@fengxue-IS
Copy link
Contributor Author

@tajila can you please take another look?
I've updated the code to have the swalk caller check -XX:ShowHiddenFrames option and set walk flag. Also update the StackWalker API to use the same logic/flags. Personal build passed on sanity.functional/openjdk

Locally tested on my VM that tests below passed:

java/lang/StackTraceElement/WithClassLoaderName.java
java/lang/reflect/MethodHandleAccessorsTest.java#id0
java/lang/reflect/IllegalArgumentsTest.java

@tajila
Copy link
Contributor

tajila commented Jan 20, 2022

@gacholio please review these changes

runtime/oti/j9consts.h Outdated Show resolved Hide resolved
@gacholio
Copy link
Contributor

Was our behaviour incorrect before (assuming the option defaults to off)?

@fengxue-IS
Copy link
Contributor Author

Was our behaviour incorrect before (assuming the option defaults to off)?

OpenJ9 by default doesn't skip hidden frames, though behavior in terms of handling hidden frames is unspecified, the reason for this change is due to many OJDK tests depends on this behavior so we are matching the RI's behavior

@gacholio
Copy link
Contributor

For consistency, please add the print to the start of the stack walker for the new flag bit.

@gacholio
Copy link
Contributor

And please squash the commits once that change is made.

- remove J9_RUNTIME_AOT_STRIPPED/J9_RUNTIME_ARRGESSIVE_VERIFICATON
- remove flag J9_RUNTIME_DFPBD and linked code

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS fengxue-IS force-pushed the stackTrace2 branch 2 times, most recently from e569f65 to 70b44ef Compare January 25, 2022 20:23
- Added new VM option for -XX:[+/-]ShowHiddenFrames
- Added new VM runtime flag J9_RUNTIME_SHOW_HIDDEN_FRAMES for new option
- Add J9_STACKWALK_SKIP_HIDDEN_FRAMES stack walk flag
- Add check to skip hidden frames during walkFrame() call
- Add new flag to all fillInStackTrace implementations
- Add check to getStackTraceForThread
- Update StackWalker to use new J9_STACKWALK_SKIP_HIDDEN_FRAMES flag

Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>
@fengxue-IS
Copy link
Contributor Author

fixed all review comments as suggested, re-compiled locally and passed JTreg test associated.

@gacholio
Copy link
Contributor

jenkins test sanity zlinux jdk17

@gacholio gacholio merged commit f9835f8 into eclipse-openj9:master Jan 26, 2022
fengxue-IS added a commit to fengxue-IS/aqa-tests that referenced this pull request Jan 28, 2022
fengxue-IS added a commit to fengxue-IS/aqa-tests that referenced this pull request Jan 28, 2022
@pshipton
Copy link
Member

@fengxue-IS please create a https://github.com/eclipse-openj9/openj9-docs issue and PR to update the user guide. The new option also needs to be mentioned in the OpenJ9 release notes. https://github.com/eclipse-openj9/openj9/blob/master/doc/release-notes/0.31/0.31.md

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