JDK-8247589: Implementation of Alpine Linux/x64 Port #49
Conversation
|
👋 Welcome back avoitylov! A progress list of the required criteria for merging this PR into |
|
@voitylov 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 |
openjdk
bot
added
jdk
Webrevs
|
|
/reviewer add @erikj79 |
|
@voitylov Could not parse
|
|
@voitylov |
|
@voitylov 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 34 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 (@AlanBateman, @erikj79, @dholmes-ora) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@voitylov |
|
This change was in review on core-libs-dev and other mailing lists before the switch to skara/git. The issues that I brought up have been added in the PR and I don't have any further comments. |
openjdk
bot
added
hotspot
make/autoconf/platform.m4
Outdated
| @@ -493,6 +533,13 @@ AC_DEFUN([PLATFORM_SETUP_LEGACY_VARS_HELPER], | |||
| fi | |||
| AC_SUBST(HOTSPOT_$1_CPU_DEFINE) | |||
|
|
|||
| if test "x$OPENJDK_$1_LIBC" = "xmusl"; then | |||
There was a problem hiding this comment.
I'm not clear why we only check for musl when setting the HOTSPOT_$1_LIBC variable
There was a problem hiding this comment.
this check is removed in the updated version. As a consequence, LIBC variable is added to the release file for all platforms, which is probably a good thing.
src/hotspot/os/linux/os_linux.cpp
Outdated
| #ifdef MUSL_LIBC | ||
| // confstr() from musl libc returns EINVAL for | ||
| // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION | ||
| os::Linux::set_libc_version("unknown"); |
There was a problem hiding this comment.
This should be "musl - unknown" as we don't know an exact version but we do know that it is musl.
There was a problem hiding this comment.
Right, this should be more consistent with glibc which here returns name and version. Updated as suggested.
src/hotspot/os/linux/os_linux.cpp
Outdated
| // confstr() from musl libc returns EINVAL for | ||
| // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION | ||
| os::Linux::set_libc_version("unknown"); | ||
| os::Linux::set_libpthread_version("unknown"); |
There was a problem hiding this comment.
This should be "musl - unknown" as we don't know an exact version but we do know that it is musl.
There was a problem hiding this comment.
The pthread version is also updated to "musl - unknown". Reason being, pthread functionality for musl is built into the library.
| #ifdef MUSL_LIBC | ||
| #define LIBC_STR "-" XSTR(LIBC) | ||
| #else | ||
| #define LIBC_STR "" |
There was a problem hiding this comment.
Again I'm not clear why we do nothing in the non-musl case? Shouldn't we be reporting glibc or musl?
There was a problem hiding this comment.
Unlike the case above, I think it's best to keep it as is. I'd expect there to be a bunch of scripts in the wild which parse it and may get broken when facing a triplet for existing platforms.
| char* msg = strerror_r(errno, buf, sizeof(buf)); | ||
| // To improve portability across platforms and avoid conflicts | ||
| // between GNU and XSI versions of strerror_r, plain strerror is used. | ||
| // It's safe because this code is not used in any multithreaded environment. |
There was a problem hiding this comment.
I still question this assertion. The issue is not that the current code path that leads to strerror use may be executed concurrently but that any other strerror use could be concurrent with this one. I would consider this a "must fix" if not for the fact we already use strerror in the code and so this doesn't really change the exposure to the problem.
There was a problem hiding this comment.
You are right! The updated version #ifdefs the XSI or GNU versions of strerror_r in this place. Note to self: file a bug to address the usage of strerror in other places, at least for HotSpot.
| pthread_attr_t thread_attr; | ||
|
|
||
| pthread_attr_init(&thread_attr); | ||
| pthread_attr_setstacksize(&thread_attr, stack_size); |
There was a problem hiding this comment.
Just a comment in response to the explanation as to why this change is needed. If the default thread stacksize under musl is insufficient to successfully attach such a thread to the VM then this will cause problems for applications that embed the VM directly (or which otherwise directly attach existing threads).
There was a problem hiding this comment.
This fix https://git.musl-libc.org/cgit/musl/commit/src/aio/aio.c?id=1a6d6f131bd60ec2a858b34100049f0c042089f2 addresses the problem for recent versions of musl. The test passes on a recent Alpine Linux 3.11.6 (musl 1.1.24) and fails on Alpine Linux 3.8.2 (musl 1.1.19) without this test fix.
There are still older versions of the library in the wild, hence the test fix. The mitigation for such users would be a distro upgrade.
There was a problem hiding this comment.
Thanks for the additional info on this.
| @@ -54,6 +57,7 @@ JNIEnv* create_vm(JavaVM **jvm, char* argTLS) { | |||
| return env; | |||
| } | |||
|
|
|||
| #if defined(__GLIBC) | |||
There was a problem hiding this comment.
Why do we use this form here but at line 30 we have:
#ifdef GLIBC
?
|
Updates look good. Nothing further from me. |
|
thank you Alan, Erik, and David! When the JEP becomes Targeted, I'll use this PR to integrate the changes. |
|
/contributor add @vidmik |
|
@voitylov |
|
@voitylov |
|
@voitylov |
|
|
|
@voitylov For future reference please don't force-push commits on open PRs as it breaks the commit history. I can no longer just look at the two most recent commits and see what they added relative to what I had previously reviewed. Thanks. |
|
@voitylov this pull request can not be integrated into git checkout JDK-8247589
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push |
openjdk
bot
added
merge-conflict
openjdk
bot
added
ready
|
@dholmes-ora yes, sorry about that. I updated the branch to pull the recent changes to enable pre-submit tests and ensure everything is well before integration and though the branch looked good it confused the pull request. So I had to force push it back to the original state. |
|
@iignatev I resolved the conflict in whitebox.cpp and fixed a minor style nit on the way. Could you take a look? |
LGTM |
|
/integrate |
|
/sponsor |
openjdk
bot
added
integrated
|
@AlexanderScherbatiy @voitylov Since your change was applied there have been 64 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 63009f9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Thanks everyone! |
* [PEA] Clear allocation state before late inlines
continuing the review thread from here https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html
I have updated the PR to split the two tests in multiple @test s.
Thank you, these changes are done in the updated PR.
Yes. How can this be best accomplished with the new git workflow?
In the first case I'm kindly asking the Reviewers who already chimed in on that to re-confirm the review here.
Progress
Testing
Issue
Reviewers
Contributors
<mikael@openjdk.org><alexsch@openjdk.org><asiebenborn@openjdk.org><avoitylov@openjdk.org>Download
$ git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49$ git checkout pull/49