On Tue, 8 Sep 2020 23:44:58 GMT, David Holmes <dhol...@openjdk.org> 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
