Support -XX:[+/-]ShowHiddenFrames option by fengxue-IS · Pull Request #3627 · eclipse-openj9/openj9

fengxue-IS · 2018-11-09T19:46:24Z

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 · 2018-11-28T17:38:33Z

@tajila can you take a look?

fengxue-IS · 2018-11-28T19:43:34Z

Senior review : @DanHeidinga please take a look

fengxue-IS · 2018-12-05T16:37:47Z

@DanHeidinga can I get a re-review please?

M-Davies · 2019-09-11T11:25:39Z

Any update on this @fengxue-IS?

fengxue-IS · 2019-09-11T15:51:52Z

@DanHeidinga Can you take a look at this please

genie-openj9 · 2021-05-17T14:21:31Z

Can one of the admins verify this patch?

tajila · 2021-12-13T19:26:47Z

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

tajila · 2021-12-13T19:27:19Z

@DanHeidinga did you have any objections to these changes?

We have noticed some JDK18 tests that require this behaviour

fengxue-IS · 2021-12-15T07:09:03Z

@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:

which Java version should this behavior be enabled on?
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)
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).

tajila · 2021-12-15T14:58:36Z

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 · 2022-01-19T20:34:00Z

@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 · 2022-01-20T22:56:09Z

@gacholio please review these changes

gacholio · 2022-01-21T16:36:07Z

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

fengxue-IS · 2022-01-21T20:41:50Z

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 · 2022-01-24T16:52:27Z

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

gacholio · 2022-01-24T17:10:49Z

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>

- 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 · 2022-01-25T21:18:15Z

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

gacholio · 2022-01-26T16:32:51Z

jenkins test sanity zlinux jdk17

eclipse-openj9/openj9#3627 Fixed: - eclipse-openj9/openj9#14132 - eclipse-openj9/openj9#3394 - eclipse-openj9/openj9#14084 (Also depends on ibmruntimes/openj9-openjdk-jdk18#12) Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

pshipton · 2022-01-31T23:10:50Z

@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

eclipse-openj9/openj9#3627 Fixed: - eclipse-openj9/openj9#14132 - eclipse-openj9/openj9#3394 - eclipse-openj9/openj9#14084 (Also depends on ibmruntimes/openj9-openjdk-jdk18#12) Signed-off-by: Jack Lu <Jack.S.Lu@ibm.com>

fengxue-IS force-pushed the stackTrace2 branch from f731ee1 to a5a7b08 Compare November 9, 2018 19:49

pshipton added the comp:vm label Nov 9, 2018

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

tajila approved these changes Nov 28, 2018

View reviewed changes

Comment thread runtime/jcl/common/jclexception.c Outdated

Comment thread runtime/jcl/common/jclexception.c Outdated

fengxue-IS force-pushed the stackTrace2 branch from a5a7b08 to 46e5528 Compare November 28, 2018 19:38

DanHeidinga reviewed Nov 28, 2018

View reviewed changes

Comment thread runtime/jcl/common/jclexception.c Outdated

Comment thread runtime/oti/j9consts.h Outdated

fengxue-IS force-pushed the stackTrace2 branch from 46e5528 to 56b0038 Compare November 28, 2018 21:47

fengxue-IS force-pushed the stackTrace2 branch 2 times, most recently from b04e059 to 58b43cb Compare August 22, 2019 20:35

DanHeidinga mentioned this pull request Sep 6, 2019

JTReg Test Failure: java/lang/StackWalker/DumpStackTest.java #6680

Closed

M-Davies mentioned this pull request Sep 20, 2019

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

Closed

2 tasks

This was referenced Dec 13, 2021

JDK18 java/lang/StackTraceElement/WithClassLoaderName.java - AssertionError: Actual: null Excepted: app mismatched class loader name #14132

Closed

OpenJ9 supports -XX:[+|-]ShowHiddenFrames? #13951

Closed

fengxue-IS force-pushed the stackTrace2 branch 4 times, most recently from 01aa5eb to b115a21 Compare December 15, 2021 05:57

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

pshipton added doc:externals and removed doc:externals labels Dec 15, 2021

tajila reviewed Jan 20, 2022

View reviewed changes

Comment thread runtime/jcl/common/java_lang_StackWalker.cpp Outdated

gacholio reviewed Jan 21, 2022

View reviewed changes

Comment thread runtime/oti/j9consts.h Outdated

Clean unused runtime flags

8f57aa0

- 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 force-pushed the stackTrace2 branch 2 times, most recently from e569f65 to 70b44ef Compare January 25, 2022 20:23

fengxue-IS force-pushed the stackTrace2 branch from 70b44ef to 18375dd Compare January 25, 2022 20:28

gacholio approved these changes Jan 26, 2022

View reviewed changes

gacholio merged commit f9835f8 into eclipse-openj9:master Jan 26, 2022

fengxue-IS mentioned this pull request Jan 28, 2022

Re-enable hidden frames related tests in JDK18 adoptium/aqa-tests#3302

Merged

fengxue-IS mentioned this pull request Jan 31, 2022

New java option doc for -XX:[+/-]ShowHiddenFrames eclipse-openj9/openj9-docs#887

Closed

This was referenced Feb 7, 2022

Add release documentation for -XX:[+|-]ShowHiddenFrames #14452

Merged

Re-enable java/lang/StackWalker/DumpStackTest.java adoptium/aqa-tests#3348

Merged

pshipton mentioned this pull request Mar 7, 2022

JDK17.0.2.0 CE java/lang/StackWalker/DumpStackTest.java RuntimeException: StackTraceElements mismatch at index 3 #14660

Closed

babsingh mentioned this pull request Mar 8, 2022

JDK18 DaaLoadTest FAILED - Cannot invoke "jdk.internal.reflect.DirectMethodHandleAccessor.invokeImpl #14395

Closed

fengxue-IS mentioned this pull request Mar 10, 2022

Skip hidden frames during iterateStackTrace call #14688

Merged

pshipton changed the title ~~Support -XX:[+/-]ShowHiddenFrames option~~ (22_02) Support -XX:[+/-]ShowHiddenFrames option Mar 23, 2022

pshipton changed the title ~~(22_02) Support -XX:[+/-]ShowHiddenFrames option~~ Support -XX:[+/-]ShowHiddenFrames option Mar 23, 2022

Uh oh!

Conversation

fengxue-IS commented Nov 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fengxue-IS commented Nov 28, 2018

Uh oh!

Uh oh!

Uh oh!

fengxue-IS commented Nov 28, 2018

Uh oh!

Uh oh!

Uh oh!

fengxue-IS commented Dec 5, 2018

Uh oh!

M-Davies commented Sep 11, 2019

Uh oh!

fengxue-IS commented Sep 11, 2019

Uh oh!

genie-openj9 commented May 17, 2021

Uh oh!

tajila commented Dec 13, 2021

Uh oh!

tajila commented Dec 13, 2021

Uh oh!

fengxue-IS commented Dec 15, 2021

Uh oh!

tajila commented Dec 15, 2021

Uh oh!

fengxue-IS commented Jan 19, 2022

Uh oh!

Uh oh!

tajila commented Jan 20, 2022

Uh oh!

Uh oh!

gacholio commented Jan 21, 2022

Uh oh!

fengxue-IS commented Jan 21, 2022

Uh oh!

gacholio commented Jan 24, 2022

Uh oh!

gacholio commented Jan 24, 2022

Uh oh!

fengxue-IS commented Jan 25, 2022

Uh oh!

gacholio commented Jan 26, 2022

Uh oh!

pshipton commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

fengxue-IS commented Nov 9, 2018 •

edited

Loading