-
Notifications
You must be signed in to change notification settings - Fork 5.8k
8300139 : [AIX] Use pthreads to avoid JNI_createVM call from primordial thread #12302
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
Conversation
👋 Welcome back varada1110! A progress list of the required criteria for merging this PR into |
@varada1110 The following labels will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command. |
9722c9d
to
0a5256e
Compare
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to see the code duplication, but we don't have a sharing mechanism for the native parts of tests so that can't be helped.
Changes look good.
Please update the Oracle copyright lines so that the second year is 2023 if needed. Thanks.
@varada1110 This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 210 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@dholmes-ora, @tstuefe) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Thanks @dholmes-ora . I will update Oracle copyright lines. |
It may be interesting to invest some time to find out if the "don't start on primordial thread" rule still holds on modern AIX versions. I added this rule 18 years ago with our original (internal) AIX port because - IIRC - we were unable to find out the stack dimensions of the primordial thread, and we could not place guard pages reliably. And maybe some other problems too; the primordial thread behaved a lot different from pthread back then. Maybe modern AIX variants don't have that problem anymore. We could cut down all this coding. Cheers, Thomas |
test/hotspot/jtreg/runtime/jni/CalleeSavedRegisters/FPRegs.java
Outdated
Show resolved
Hide resolved
It just occurred to me that we may also need to update the test makefiles to link to libpthread for these modified tests? |
These tests is passing without adding lpthread in JtregNativeHotspot.gmk. Do I need to update the makefile? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nits remain, nothing major.
test/hotspot/jtreg/runtime/jni/CalleeSavedRegisters/FPRegs.java
Outdated
Show resolved
Hide resolved
test/jdk/java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java
Outdated
Show resolved
Hide resolved
test/hotspot/jtreg/runtime/jni/CalleeSavedRegisters/exeFPRegs.c
Outdated
Show resolved
Hide resolved
test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c
Outdated
Show resolved
Hide resolved
test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks.
I'm somewhat surprised that the tests pass without explicit linkage, but perhaps libpthread is a default on AIX. If things are working in your AIX testing then that is fine. |
/integrate |
@varada1110 |
/sponsor |
Going to push as commit cb81073.
Your commit was automatically rebased without conflicts. |
@tstuefe @varada1110 Pushed as commit cb81073. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
test/jdk/jni/nullCaller/NullCallerTest.java
test/jdk/java/lang/reflect/exeCallerAccessTest/CallerAccessTest.java
test/hotspot/jtreg/runtime/jni/CalleeSavedRegisters/FPRegs.java
The above tests were blocked on AIX [@require os.family != "aix"] because these tests are failing to call JNI_CreateJavaVM. This is solved by implementing JNI_CreateJavaVM call via POSIX threads.
Similarly there are tests which are not blocked and still failing to call JNI_CreateJavaVM on AIX :
test/hotspot/jtreg/runtime/jni/daemonDestroy/TestDaemonDestroy.java { PR : 12006 }
test/lib-test/jdk/test/lib/process/TestNativeProcessBuilder.java
The reported issue : 8300139
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/12302/head:pull/12302
$ git checkout pull/12302
Update a local copy of the PR:
$ git checkout pull/12302
$ git pull https://git.openjdk.org/jdk pull/12302/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 12302
View PR using the GUI difftool:
$ git pr show -t 12302
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/12302.diff