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

Enhancement to report any unrecognized options that begin with "-XX:" (Issue2977) #4435

Merged
merged 1 commit into from Mar 8, 2019

Conversation

@AidanHa
Copy link
Contributor

commented Jan 24, 2019

Add an enhancement option that allows users to enter the option '-XX:-IgnoreUnrecognizedXXColonOptions' to ignore all unrecognized options that start with '-XX:'

Steps:
Removed ' { VMOPT_XX, STARTSWITH_MATCH }' from the Ignore table in runtime/vm/jvminit.c (Line 184)
Add a new flag in j9options that is set when the argument is supposed to be ignored (ARG_IGNORED)
Add necessary macros to support the new flag
Modify CheckArgsConsumed and findArgInVMArgs to be able to set and read the new flag

Doc Issue eclipse/openj9-docs#212

Fixes: #2977

Signed-off-by: Aidan Ha qbha@edu.uwaterloo.ca

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch 2 times, most recently from d28d882 to cb255f7 Jan 24, 2019

@dsouzai

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

Thank you for your contribution @AidanHa :). Could you please update the PR with a more descriptive title.

@dsouzai dsouzai requested a review from DanHeidinga Jan 25, 2019

@dsouzai dsouzai added the comp:vm label Jan 25, 2019

@AidanHa AidanHa changed the title Issue2977 Enhancement to report any unrecognized options that begin with "-XX:" (Issue2977) Jan 25, 2019

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch 4 times, most recently from 940cc6a to a6033d2 Jan 25, 2019

Show resolved Hide resolved runtime/vm/jvminit.c Outdated
Show resolved Hide resolved runtime/vm/jvminit.c Outdated
@tajila

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Can you please update the commit message and the PR description so it matches the most recent changes

@tajila

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

Also, I think you can greatly simplify this PR by introducing a new function called findArgInVMArgsImpl. This function will take in the extra parameter for ignoredArgs.

The existing findArgInVMArgs will have the same existing method params and will call the new function and pass in false for ignoredArgs. In the one place where you need the new behaviour you can call the new function. In all other places the code stays the same as it is.

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch 2 times, most recently from a062456 to 050fccd Feb 5, 2019

Show resolved Hide resolved runtime/oti/j9nonbuilder.h Outdated
@tajila

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Can you show the results of running the following -XInvalid, -XX:Invalid, -XInvalid -XX:Invalid and -XInvalid -XX:Invalid -XX:Invalid -XInvalidwith:

  1. Default options
  2. -XX:+IgnoreUnrecognizedVMOptions
  3. -XX:-IgnoreUnrecognizedVMOptions
  4. -XX:+IgnoreUnrecognizedXXOptions
  5. -XX:-IgnoreUnrecognizedXXOptions
@AidanHa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Sure, the changes I implemented are giving errors while building the jdk, so I will look into that first.

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch 2 times, most recently from 6d984b1 to 8459210 Feb 6, 2019

@AidanHa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

image
image

@tajila

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

Thanks running that @AidanHa.

I made an error in my last comment. The last one was supposed to be -XX:Invalid -XInvalid not -XInvalid -XX:Invalid. Can you please show those results as well

@AidanHa

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

Show resolved Hide resolved runtime/util/vmargs.c Outdated
Show resolved Hide resolved runtime/oti/j9nonbuilder.h Outdated
Show resolved Hide resolved runtime/vm/intfunc.c Outdated
Show resolved Hide resolved runtime/vm/jvminit.c Outdated
Show resolved Hide resolved runtime/vm/jvminit.c Outdated
@tajila

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

Some minor comments, rest LGTM

@pshipton

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

We'll need a doc item created for this.

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch 3 times, most recently from 4d6b3d1 to 4a1c6b4 Feb 14, 2019

@DanHeidinga DanHeidinga self-assigned this Feb 28, 2019

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch from 51bfa11 to 493fd3d Mar 1, 2019

@AidanHa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

Seems like one of the extended tests failed :https://ci.eclipse.org/openj9/job/PullRequest-Extended-JDK8-linux_ppc-64_cmprssptrs_le-OpenJ9/172/console

The test id was test -XX:+PageAlignDirectMemory -XX:-PageAlignDirectMemory

I tried rebasing my fork to see if anything has changed...

Could we try running the extended test once more?

image

@DanHeidinga

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

Jenkins test extended plinux jdk8

Show resolved Hide resolved runtime/vm/jvminit.c Outdated
Show resolved Hide resolved runtime/vm/jvminit.c Outdated

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch 9 times, most recently from 5bf6f1b to cef8eb4 Mar 4, 2019

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch 4 times, most recently from 0138da1 to 87b239a Mar 6, 2019

@AidanHa

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2019

The fixes have been addressed.

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch from 87b239a to a4a2d74 Mar 7, 2019

-XX:IgnoreUnrecognizedXXOptions ignores all unrecognized -XX options
Steps:
Removed ' { VMOPT_XX, STARTSWITH_MATCH }'  from the Ignore table in runtime/vm/jvminit.c (Line 184)
Add a new flag in j9options that is set when the argument is supposed to be ignored (ARG_IGNORED)
Add necessary macros to support the new flag
Modify CheckArgsConsumed and findArgInVMArgs to be able to set and read the new flag

Fixes: #2977
Signed-off-by: Aidan Ha <qbha@edu.uwaterloo.ca>

@AidanHa AidanHa force-pushed the AidanHa:Issue2977 branch from a4a2d74 to 112ecba Mar 7, 2019

@pshipton

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Jenkins test extended plinux jdk8

@pshipton

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@llxia can you please review the test material

@llxia

llxia approved these changes Mar 8, 2019

Copy link
Contributor

left a comment

LGTM

@pshipton pshipton merged commit 0bd089b into eclipse:master Mar 8, 2019

5 checks passed

Copyright Check Copyrights appear to be up to date
Details
Extended - JDK8 - linux_ppc-64_cmprssptrs_le Build finished.
Details
Line Endings Check Line endings appear to be correct
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details

@AidanHa AidanHa deleted the AidanHa:Issue2977 branch Mar 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.