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

Replace JITSERVER_SUPPORT with J9VM_OPT_JITSERVER #8656

Merged
merged 2 commits into from
Feb 27, 2020

Conversation

ashu-mehra
Copy link
Contributor

Replace macro JITSERVER_SUPPORT with J9VM_OPT_JITSERVER so as to have
a single macro to enable the JITServer support both in VM and JIT.

Closes: #8229

Signed-off-by: Ashutosh Mehra mehra.ashutosh@ibm.com

@keithc-ca
Copy link
Contributor

Please fix the copyright dates.

@keithc-ca
Copy link
Contributor

Pull requests will be required for jdk14 and jdk(next).

Replace macro JITSERVER_SUPPORT with J9VM_OPT_JITSERVER so as to have
a single macro to enable the JITServer support both in VM and JIT.

Closes: eclipse-openj9#8229

Signed-off-by: Ashutosh Mehra <mehra.ashutosh@ibm.com>
@ashu-mehra
Copy link
Contributor Author

ashu-mehra commented Feb 25, 2020

PR for openj9-openjdk-jdk: ibmruntimes/openj9-openjdk-jdk#173
PR for openj9-openjdk-jdk14: ibmruntimes/openj9-openjdk-jdk14#16
PR openj9-openjdk-jdk11: for ibmruntimes/openj9-openjdk-jdk11#269
PR for openj9-openjdk-jdk8: ibmruntimes/openj9-openjdk-jdk8#379

Note that these changes should not be merged until the referenced PRs are not merged.

@ashu-mehra
Copy link
Contributor Author

Do we need similar change in openj9-openjdk-jdk13 as well or is it discontinued?

@ashu-mehra
Copy link
Contributor Author

@mpirvu fyi

@pshipton
Copy link
Member

Do we need similar change in openj9-openjdk-jdk13 as well or is it discontinued?

jdk13 is out of support

@keithc-ca
Copy link
Contributor

This removes build_jitserver from j9.flags: the changes in the extension repos should remove references to that flag.

@ashu-mehra
Copy link
Contributor Author

@keithc-ca If I remove build_jitserver and JITSERVER_SUPPORT from the extension repos, we would need this PR and extension repo PRs to all go in at the same time to avoid any jitserver build breaks. I thought I would keep the existing variables in extension repos until this PR is merged, and then create a new PR to remove them.
Do you think all the changes can instead be delivered together?

@keithc-ca
Copy link
Contributor

It wasn't clear that your intent was to remove the references to build_jitserver later: that plan works for me. I'll review the extension repo PRs accordingly.

@pshipton
Copy link
Member

Do you think all the changes can instead be delivered together?

It's also not hard to do this. PR testing can cover dependent changes. There is no acceptance build to promote, so all the PRs can be delivered at approximately the same time.

@keithc-ca
Copy link
Contributor

@pshipton Is your preference that we just do one round of changes to the extension repos for this?

@pshipton
Copy link
Member

I have no preference, just saying it's possible to merge it all at once, it doesn't need to be staged like OMR changes do.

@ashu-mehra
Copy link
Contributor Author

it's possible to merge it all at once, it doesn't need to be staged like OMR changes do.

Ok, then let me update the pr in extension repos to remove old variables and then all can be merged together.

@ashu-mehra
Copy link
Contributor Author

Updated the extensions repo to remove old flag.

@@ -138,10 +138,6 @@ ifeq ($(HOST_ARCH),x)
CX_DEFINES+=J9HAMMER
CX_FLAGS+=-m64 -fPIC
endif

ifneq ($(JITSERVER_SUPPORT),)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need these defines? Are the definitions added in some other place?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is for defining the macro JITSERVER_SUPPORT. We should instead use J9VM_OPT_JITSERVER which gets defined in j9cfg.h (when using UMA build system) or by CMakeLists.txt in VM and JIT.
Normally j9cfg.h gets included via j9.h and any code using J9VM_OPT_JITSERVER should now include j9.h.
Before this PR, JIT files didn't include j9.h if there isn't any dependency on it, but with this PR if the JIT files need to see J9VM_OPT_JITSERVER, then j9.h needs to be included.
A case in point is https://github.com/eclipse/openj9/pull/8656/files#diff-65a548d2a6dcefcbcdd234568cac76ab where I have to explicitly include j9.h.

@ashu-mehra ashu-mehra changed the title WIP: Replace JITSERVER_SUPPORT with J9VM_OPT_JITSERVER Replace JITSERVER_SUPPORT with J9VM_OPT_JITSERVER Feb 26, 2020
runtime/compiler/CMakeLists.txt Outdated Show resolved Hide resolved
runtime/compiler/runtime/CMakeLists.txt Outdated Show resolved Hide resolved
@keithc-ca keithc-ca self-assigned this Feb 26, 2020
Instead of adding -DJ9VM_OPT_JITSERVER, add it to j9cfg.h.in
so that cmake system can convert it to define or undef depending
on the env variable with same name.

Signed-off-by: Ashutosh Mehra <mehra.ashutosh@ibm.com>
@ashu-mehra
Copy link
Contributor Author

@keithc-ca I added a new commit with the suggested changes. Please review.

@keithc-ca
Copy link
Contributor

Jenkins test sanity,extended plinux,xlinuxcm jdk11 depends ibmruntimes/openj9-openjdk-jdk11#269

@keithc-ca keithc-ca merged commit 944f9dd into eclipse-openj9:master Feb 27, 2020
@ashu-mehra ashu-mehra deleted the change_macro branch April 14, 2020 19:17
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

Successfully merging this pull request may close these issues.

Use single flag to enable JITServer changes
5 participants