RFR: JDK-8247589: Implementation of Alpine Linux/x64 Port

David Holmes dholmes at openjdk.java.net
Wed Sep 9 00:11:23 UTC 2020


On Mon, 7 Sep 2020 11:23:28 GMT, Aleksei Voitylov <avoitylov at openjdk.org> 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


More information about the hotspot-runtime-dev mailing list