Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Mon, 14 Sep 2020 06:30:50 GMT, Aleksei Voitylov wrote: >> Marked as reviewed by dholmes (Reviewer). > > thank you Alan, Erik, and David! When the JEP becomes Targeted, I'll use this > PR to integrate the changes. I added the contributors that could be found in the portola project commits. If anyone knows some other contributors I missed, I'll be happy to stand corrected. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Mon, 14 Sep 2020 04:18:39 GMT, David Holmes wrote: >> Aleksei Voitylov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8247589: Implementation of Alpine Linux/x64 Port > > Marked as reviewed by dholmes (Reviewer). thank you Alan, Erik, and David! When the JEP becomes Targeted, I'll use this PR to integrate the changes. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Wed, 9 Sep 2020 00:08:35 GMT, David Holmes wrote: >> Aleksei Voitylov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> JDK-8247589: Implementation of Alpine Linux/x64 Port > > Attempting to use the GitHub UI for further review. If this doesn't work out > well I will revert to direct email. Updates look good. Nothing further from me. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Fri, 11 Sep 2020 07:36:57 GMT, Aleksei Voitylov wrote: >> test/hotspot/jtreg/runtime/StackGuardPages/exeinvoke.c line 282: >> >>> 280: >>> 281: pthread_attr_init(&thread_attr); >>> 282: pthread_attr_setstacksize(&thread_attr, stack_size); >> >> 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). > > 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. Thanks for the additional info on this. - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Fri, 11 Sep 2020 07:03:37 GMT, Aleksei Voitylov wrote: >> continuing the review thread from here >> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html >> >>> The download side of using JNI in these tests is that it complicates the >>> setup a bit for those that run jtreg directly and/or just build the JDK >>> and not the test libraries. You could reduce this burden a bit by >>> limiting the load library/isMusl check to Linux only, meaning isMusl >>> would not be called on other platforms. >>> >>> The alternative you suggest above might indeed be better. I assume you >>> don't mean splitting the tests but rather just adding a second @test >>> description so that the vm.musl case runs the test with a system >>> property that allows the test know the expected load library path behavior. >> >> I have updated the PR to split the two tests in multiple @test s. >> >>> The updated comment in java_md.c in this looks good. A minor comment on >>> Platform.isBusybox is Files.isSymbolicLink returning true implies that >>> the link exists so no need to check for exists too. Also the >>> if-then-else style for the new class in ProcessBuilder/Basic.java is >>> inconsistent with the rest of the test so it stands out. >> >> Thank you, these changes are done in the updated PR. >> >>> Given the repo transition this weekend then I assume you'll create a PR >>> for the final review at least. Also I see JEP 386 hasn't been targeted >>> yet but I assume Boris, as owner, will propose-to-target and wait for it >>> to be targeted before it is integrated. >> >> Yes. How can this be best accomplished with the new git workflow? >> - we can continue the review process till the end and I will request the >> integration to happen only after the JEP is >> targeted. I guess this step is now done by typing "slash integrate" in a >> comment. >> - we can pause the review process now until the JEP is targeted. >> >> In the first case I'm kindly asking the Reviewers who already chimed in on >> that to re-confirm the review here. > > Aleksei Voitylov has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8247589: Implementation of Alpine Linux/x64 Port Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
On Tue, 8 Sep 2020 23:44:58 GMT, David Holmes wrote:
>> Aleksei Voitylov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> JDK-8247589: Implementation of Alpine Linux/x64 Port
>
> make/autoconf/platform.m4 line 536:
>
>> 534: AC_SUBST(HOTSPOT_$1_CPU_DEFINE)
>> 535:
>> 536: if test "x$OPENJDK_$1_LIBC" = "xmusl"; then
>
> I'm not clear why we only check for musl when setting the HOTSPOT_$1_LIBC
> variable
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 line 624:
>
>> 622: // confstr() from musl libc returns EINVAL for
>> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
>> 624: os::Linux::set_libc_version("unknown");
>
> This should be "musl - unknown" as we don't know an exact version but we do
> know that it is musl.
Right, this should be more consistent with glibc which here returns name and
version. Updated as suggested.
> src/hotspot/os/linux/os_linux.cpp line 625:
>
>> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
>> 624: os::Linux::set_libc_version("unknown");
>> 625: os::Linux::set_libpthread_version("unknown");
>
> This should be "musl - unknown" as we don't know an exact version but we do
> know that it is musl.
The pthread version is also updated to "musl - unknown". Reason being, pthread
functionality for musl is built into the
library.
> src/hotspot/share/runtime/abstract_vm_version.cpp line 263:
>
>> 261: #define LIBC_STR "-" XSTR(LIBC)
>> 262: #else
>> 263: #define LIBC_STR ""
>
> Again I'm not clear why we do nothing in the non-musl case? Shouldn't we be
> reporting glibc or musl?
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.
> src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c line 284:
>
>> 282: // To improve portability across platforms and avoid conflicts
>> 283: // between GNU and XSI versions of strerror_r, plain strerror is
>> used.
>> 284: // It's safe because this code is not used in any multithreaded
>> environment.
>
> 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.
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.
> test/hotspot/jtreg/runtime/StackGuardPages/exeinvoke.c line 282:
>
>> 280:
>> 281: pthread_attr_init(&thread_attr);
>> 282: pthread_attr_setstacksize(&thread_attr, stack_size);
>
> 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).
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.
> test/hotspot/jtreg/runtime/TLS/exestack-tls.c line 60:
>
>> 58: }
>> 59:
>> 60: #if defined(__GLIBC)
>
> Why do we use this form here but at line 30 we have:
> #ifdef __GLIBC__
> ?
Fixed to be consistent.
-
PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port [v2]
> continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Aleksei Voitylov has updated the pull request incrementally with one additional commit since the last revision: JDK-8247589: Implementation of Alpine Linux/x64 Port - Changes: - all: https://git.openjdk.java.net/jdk/pull/49/files - new: https://git.openjdk.java.net/jdk/pull/49/files/f61f546a..d5994cb5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=49&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=49&range=00-01 Stats: 19 lines in 4 files changed: 7 ins; 4 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/49.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/49/head:pull/49 PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port
On Mon, 7 Sep 2020 11:23:28 GMT, Aleksei Voitylov wrote:
> continuing the review thread from here
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html
>
>> The download side of using JNI in these tests is that it complicates the
>> setup a bit for those that run jtreg directly and/or just build the JDK
>> and not the test libraries. You could reduce this burden a bit by
>> limiting the load library/isMusl check to Linux only, meaning isMusl
>> would not be called on other platforms.
>>
>> The alternative you suggest above might indeed be better. I assume you
>> don't mean splitting the tests but rather just adding a second @test
>> description so that the vm.musl case runs the test with a system
>> property that allows the test know the expected load library path behavior.
>
> I have updated the PR to split the two tests in multiple @test s.
>
>> The updated comment in java_md.c in this looks good. A minor comment on
>> Platform.isBusybox is Files.isSymbolicLink returning true implies that
>> the link exists so no need to check for exists too. Also the
>> if-then-else style for the new class in ProcessBuilder/Basic.java is
>> inconsistent with the rest of the test so it stands out.
>
> Thank you, these changes are done in the updated PR.
>
>> Given the repo transition this weekend then I assume you'll create a PR
>> for the final review at least. Also I see JEP 386 hasn't been targeted
>> yet but I assume Boris, as owner, will propose-to-target and wait for it
>> to be targeted before it is integrated.
>
> Yes. How can this be best accomplished with the new git workflow?
> - we can continue the review process till the end and I will request the
> integration to happen only after the JEP is
> targeted. I guess this step is now done by typing "slash integrate" in a
> comment.
> - we can pause the review process now until the JEP is targeted.
>
> In the first case I'm kindly asking the Reviewers who already chimed in on
> that to re-confirm the review here.
Attempting to use the GitHub UI for further review. If this doesn't work out
well I will revert to direct email.
make/autoconf/platform.m4 line 536:
> 534: AC_SUBST(HOTSPOT_$1_CPU_DEFINE)
> 535:
> 536: if test "x$OPENJDK_$1_LIBC" = "xmusl"; then
I'm not clear why we only check for musl when setting the HOTSPOT_$1_LIBC
variable
src/hotspot/os/linux/os_linux.cpp line 624:
> 622: // confstr() from musl libc returns EINVAL for
> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
> 624: os::Linux::set_libc_version("unknown");
This should be "musl - unknown" as we don't know an exact version but we do
know that it is musl.
src/hotspot/os/linux/os_linux.cpp line 625:
> 623: // _CS_GNU_LIBC_VERSION and _CS_GNU_LIBPTHREAD_VERSION
> 624: os::Linux::set_libc_version("unknown");
> 625: os::Linux::set_libpthread_version("unknown");
This should be "musl - unknown" as we don't know an exact version but we do
know that it is musl.
src/hotspot/share/runtime/abstract_vm_version.cpp line 263:
> 261: #define LIBC_STR "-" XSTR(LIBC)
> 262: #else
> 263: #define LIBC_STR ""
Again I'm not clear why we do nothing in the non-musl case? Shouldn't we be
reporting glibc or musl?
src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c line 284:
> 282: // To improve portability across platforms and avoid conflicts
> 283: // between GNU and XSI versions of strerror_r, plain strerror is
> used.
> 284: // It's safe because this code is not used in any multithreaded
> environment.
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.
test/hotspot/jtreg/runtime/StackGuardPages/exeinvoke.c line 282:
> 280:
> 281: pthread_attr_init(&thread_attr);
> 282: pthread_attr_setstacksize(&thread_attr, stack_size);
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).
test/hotspot/jtreg/runtime/TLS/exestack-tls.c line 60:
> 58: }
> 59:
> 60: #if defined(__GLIBC)
Why do we use this form here but at line 30 we have:
#ifdef __GLIBC__
?
-
Changes requested by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/49
Re: RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port
On Mon, 7 Sep 2020 11:23:28 GMT, Aleksei Voitylov wrote: > continuing the review thread from here > https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/068546.html > >> The download side of using JNI in these tests is that it complicates the >> setup a bit for those that run jtreg directly and/or just build the JDK >> and not the test libraries. You could reduce this burden a bit by >> limiting the load library/isMusl check to Linux only, meaning isMusl >> would not be called on other platforms. >> >> The alternative you suggest above might indeed be better. I assume you >> don't mean splitting the tests but rather just adding a second @test >> description so that the vm.musl case runs the test with a system >> property that allows the test know the expected load library path behavior. > > I have updated the PR to split the two tests in multiple @test s. > >> The updated comment in java_md.c in this looks good. A minor comment on >> Platform.isBusybox is Files.isSymbolicLink returning true implies that >> the link exists so no need to check for exists too. Also the >> if-then-else style for the new class in ProcessBuilder/Basic.java is >> inconsistent with the rest of the test so it stands out. > > Thank you, these changes are done in the updated PR. > >> Given the repo transition this weekend then I assume you'll create a PR >> for the final review at least. Also I see JEP 386 hasn't been targeted >> yet but I assume Boris, as owner, will propose-to-target and wait for it >> to be targeted before it is integrated. > > Yes. How can this be best accomplished with the new git workflow? > - we can continue the review process till the end and I will request the > integration to happen only after the JEP is > targeted. I guess this step is now done by typing "slash integrate" in a > comment. > - we can pause the review process now until the JEP is targeted. > > In the first case I'm kindly asking the Reviewers who already chimed in on > that to re-confirm the review here. Build changes look ok. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/49
