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

Update RAS documentation regarding Hidden frames #910

Closed
fengxue-IS opened this issue Mar 14, 2022 · 15 comments
Closed

Update RAS documentation regarding Hidden frames #910

fengxue-IS opened this issue Mar 14, 2022 · 15 comments

Comments

@fengxue-IS
Copy link
Contributor

Issue or pull request number:
eclipse-openj9/openj9#14688

Overview:
Code change updated the stacktrace logic on processing Hidden frames, they are only skipped in getStackTrace calls and StackWalker API based on -XX:[+|-]ShowHiddenFrames option, RAS triggers will not be affected by the use of -XX:[+|-]ShowHiddenFrames option and alway show hidden frames to the filter

Release target:
E.g. Eclipse OpenJ9 0.31.0

Applies to the following JDK versions:
all

Applies to the following platforms:
all

@gacholio
Copy link
Contributor

This applies from 0.31 onwards.

@doveye
Copy link
Contributor

doveye commented Apr 12, 2022

@fengxue-IS I assume no doc updates have been done for this issue? (I can't see any.)
Sounds like updates are required in https://eclipse-openj9.github.io/openj9-docs/xxshowhiddenframes/ and also https://eclipse-openj9.github.io/openj9-docs/version0.31/. The ID team can make these updates.

@fengxue-IS
Copy link
Contributor Author

Yes, I haven't had time to write the doc update for this issue.
As -XX:[+|-]ShowHiddenFrames is also added as part of 0.31, update to https://eclipse-openj9.github.io/openj9-docs/version0.31/ may not be needed
It would be really appreciated if the ID team can make them. Thanks

@doveye
Copy link
Contributor

doveye commented Apr 14, 2022

@fengxue-IS I did some more digging into this item and have some comments/questions:

  1. It looks like we're supporting/reimplementing the HotSpot ShowHiddenFrames option. So the docs would need to change to match what we've done before, eg in https://www.eclipse.org/openj9/docs/version0.23/#support-for-openjdk-hotspot-options. We'd also need to add the option to the table in https://www.eclipse.org/openj9/docs/cmdline_migration/#compatible-options.
  2. Given the above, are we "supporting" this HotSpot option or "reimplementing" it, and are there any differences in behaviour between the OpenJ9 and Hotspot versions of ShowHiddenFrames?
  3. https://github.com/openjdk/jdk/blob/master/src/hotspot/share/runtime/globals.hpp lists the option as a diagnostic option and says that diagnostic options are not documented and users can enable them with guidance from support. So are we sure we want to document this option at all?

@fengxue-IS
Copy link
Contributor Author

@doveye thanks for looking into this issue

  1. we are technically reimplementing this option, and it is different from the OpenJDK option which requires use of +UnlockDiagnosticVMOptions together
  2. As noted in this issue, for OpenJ9, this option is doesn't affect output of dump, (only JCL API's behave similar to RI's use for this option and other uses are not guaranteed)
  3. My understanding is that we should doc it as this is a useful debug option (also since this is -XX option which already implies implementation specific) @gacholio @pshipton do you have any thoughts on this?

@pshipton
Copy link
Member

I don't have a problem documenting the option. This type of option helps user's diagnose problems themselves without needing input from service. We could mention it's only expected to be used for diagnostics.

@gacholio
Copy link
Contributor

This really isn't a diagnostic option (in fact, it should not be an option at all). If the default is to not show hidden frames. then using the option will result in failed security checks, etc, so it's almost impossible that any modern application could function.

@tajila
Copy link
Contributor

tajila commented Apr 19, 2022

Running with -XX:+ShowHiddenFrames on JDK18+ will result in functional issues. We should probably raise a bug with OpenJDK. Either they need to update the new MH implementation to work properly without hiding frames or the option needs to be removed.

@pshipton
Copy link
Member

I think @gacholio comments are specific to jdk18+. Using the option for 8, 11, 17 restores the behaviour of previous releases so may help diagnose a problem.

Sreekala-Gopakumar added a commit to Sreekala-Gopakumar/openj9-docs that referenced this issue Apr 20, 2022
eclipse-openj9#910

Updated the ShowHiddenFrames content to mention HotSpot for consistency. Added that this option doesn't affect the dump file. Added this option to the table in the Switching to OpenJ9 topic.

Signed-off-by: Sreekala Gopakumar <[email protected]>
@Sreekala-Gopakumar
Copy link
Contributor

@fengxue-IS - Following changes have been made:

  • updated the ShowHiddenFrames content (-XX:[+|-]ShowHiddenFrames topic) to mention HotSpot for consistency.
  • Added that this option doesn't affect the dump file.
  • Added this option to the table in the Switching to OpenJ9 topic.

Please check and let me know, if the changes are ok.

@doveye
Copy link
Contributor

doveye commented Apr 21, 2022

I've merged the PR but we didn't add any content for the Java 18 issue. If it's an OpenJDK problem then we could put it in the Known Issues table in the release notes at https://github.com/eclipse-openj9/openj9/blob/master/doc/release-notes/0.32/0.32.md. Eg:

Issue #: 14803
Description: Using the -XX:+ShowHiddenFrames option in an OpenJ9 release built with OpenJDK 18 causes errors due to underlying problems in OpenJDK
Platform: All platforms
Impact: MethodHandle frames cannot be shown in stack traces
Workaround: Avoid using the -XX:+ShowHiddenFrames option with OpenJDK 18

@tajila @pshipton @gacholio Are you happy with this please?

@pshipton
Copy link
Member

The impact is not correct. I don't think we need to add this to mention the problems, but I'll defer to Tobi. It's already stated it's a diagnostic/debugging option.

@tajila
Copy link
Contributor

tajila commented Apr 21, 2022

Impact: MethodHandle frames cannot be shown in stack traces

The actual impact is that the wrong exception may be thrown when using Reflection API.

Sreekala-Gopakumar added a commit to Sreekala-Gopakumar/openj9 that referenced this issue Apr 26, 2022
In addition, in the 0.32 release note, an item is added in the Known Issues table based on the feedback received in issue eclipse-openj9#910.
eclipse-openj9/openj9-docs#910

In the 0.31 release note, the full commit history line at the end of the topic deleted based on the comment received from the reviewer.
eclipse-openj9#14939

[skip ci]

Signed-off-by: Sreekala Gopakumar [email protected]
@doveye
Copy link
Contributor

doveye commented Apr 26, 2022

It might be a diagnostic option but if users on Java 18 use it and it breaks their app, shouldn't we warn them about that? Otherwise they could be wasting time trying to debug what's actually a known issue. Although throwing the wrong exception doesn't sound like the inability to function that Graham mentioned earlier.

@doveye
Copy link
Contributor

doveye commented Apr 27, 2022

It looks like the release notes got updated anyway, so I'll close this issue.

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

No branches or pull requests

6 participants