RFC: 8229469 JEP 386: Alpine Linux/x64 Port

Fri Sep 4 07:55:07 UTC 2020

Hi Erik, Magnus, Mikael, Nick, David, and Alan,

thank you for looking into it. I grouped together all the comments in
one response to avoid polluting the mailing lists. Here is an updated
version of the patch which addresses the comments:

http://cr.openjdk.java.net/~avoitylov/webrev.8247589.06/

Please also find inline answers to the comments by Mikael, Nick, Alan
and David, in order. Testing is in progress.

[Mikael]

> WB_GetLibcName now returns “glibc” by default (unless MUSL_LIBC is
> defined) which means it may return “glibc” on platforms where the
> default library really isn’t GNU libc. I will work just fine for what
> it’s currently being used for (isMusl), but is potentially a bit
> misleading.

I agree. In the updated version WB_GetLibcName returns the LIBC the JDK
is build for.

[Nick]

> I see the JEP only mentions support for x86_64, so maybe this is out of
> scope, but with this trivial change to os_linux_aarch64.cpp your patch
> works on Alpine/AArch64 too:
I planned to add additional platforms it a follow-up enhancement but
updated the webrev to include AArch64 now. Testing is in progress, and,
if the same level of quality is reached as on x64, I will slightly
update the JEP to make it clear AArch64 is also known to work. I hope
this will be fine.

[Alan]

> .02, .03, .04 seem to be older and seem to include the changes to
> JDK-8252248 that Alexander Scherbatiy had out for review separately so
> I'll ignore those and just look at .01, I hope that is right.
This is correct.
> I agree with David that the comment in java_md.c is excessive and
> unnecessary.
Fixed in the updated version.
>
> The WB API is mostly for the hotspot tests and looks a bit strange to
> be using it in the launcher tests to check for musl. Have you look at
> alternatives? The changes to the Process test to check for busybox is
> okay. 

The WhiteBox API is used in JDK tests for JFR [1],
CollectionUsageThreshold.java [2], TimSortStackSize2.java [3], so we are
not adding an extra dependency.

Tests tools/launcher/Test7029048.java and
tools/launcher/ExecutionEnvironment.java need to change behavior on
systems that alter LD_LIBRARY_PATH (AIX and musl). The alternative we
first considered was to detect the libc of the OS with ldd in the test
and avoid WhiteBox dependency. This approach has a significant drawback:
some distributions bundle glibc and OpenJDK and launch it on a
musl-based Linux OS, so we really need to detect the libc the JDK was
compiled for, not the default libc present in the OS. To avoid such
problems all together it was decided to detect the libc flavor the JDK
was compiled for through WhiteBox.

[David]

> src/hotspot/os/linux/os_linux.cpp
>
>  624   os::Linux::set_libc_version("unknown");
>  625   os::Linux::set_libpthread_version("unknown");
>
> Surely we can do better than "unknown"? Surely different releases of
> Alpine linux must identify different version of the libraries?

The author of musl indicated it currently does not provide a way to
obtain the library version programmatically [4].

> The PaX related error message belongs in release notes not the source
> code - the error message should be much more succint:
>
> "Failed to mark memory page as executable - check if grsecurity/PaX is
> enabled"

Fixed.

> src/hotspot/os/linux/os_linux.hpp
>
> numa_node_to_cpus is too big to be in the header file now.

Fixed.

> src/hotspot/share/prims/whitebox.cpp
>
> I would expect this to use the version string set in os_linux.cpp.

In the updated version of the patch the libc variant now comes from the
build system. The libc version would probably be unused at this point in
WhiteBox, also see answer to your first comment.

> src/hotspot/share/runtime/abstract_vm_version.cpp
>
> Ideally LIBC_STR would come from the build system - as we do for
> FLOAT_ARCH. See flags.m4.

Yes, thank you for the suggestion. It now comes from the build system,
as above.

> src/java.base/unix/native/libjli/java_md.c
>
> The comment is way too long - see the AIX case below it. :)

OK, trimmed it down :)

> src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c
>
>  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.
>
> It is not at all clear to me that this code is not used in a
> multi-threaded environment. The VM is always multi-threaded.  This
> usage was added here:
>
> http://mail.openjdk.java.net/pipermail/serviceability-dev/2015-December/018419.html
>
>
> But this code also uses strerror in another place (as does other code)
> so ...

I checked LinuxDebuggerLocal.java and all attach0 calls using this
functionality are guarded by corresponding syncronized constructs.

That's because nobody claims that ptrace_attach is thread-safe or
re-enterant. It depends to kernel implementation and attempt to use it
from multiple threads could lead to unpredictable results. If
ptrace_attach is run from different threads but with the same PID, it
could crash the target process or fail. And it doesn't have any sense
from serviceability agent point of view.

Running ptrace_attach from multiple threads with different PIDs should
be more or less OK - I trust modern kernel, but I don't see any usecase
or command line support for using hotspot agent with multiple PIDs.

So it was concluded that using strerror instead of strerror_r to
mitigate compatibility issues should not be a problem.

>
> test/hotspot/jtreg/runtime/StackGuardPages/exeinvoke.c
>
> Why is this change needed?

Both  tests fail without it because

res = (*_jvm)->AttachCurrentThread(_jvm, (void **)&env, NULL);

returned with an error (res != JNI_OK).

AttachCurrentThread uses the current thread as JavaThread and allocates
a java level Thread object and needs to run initializing java code
(JavaThread::allocate_threadObj). In order to run java code, the
remaining stack space is checked. There must be sufficient  space for an
interpreter frame, the java frame and shadow pages (
JavaCalls::call_helper() ). The default for the number of shadow pages
is 10 for a release build. If this check fails a StackOverflowException
is thrown. As we return with a pending exception the attach fails and we
return JNI_ERR.

This is only a problem as we call AttachCurrentThread from a thread that
we created with the default stacksize. Threads created by the JVM are
created with a much higher stacksize.

> In general the busybox changes are bit unpleasant in the tests. Pity
> Alpine didn't try to present a familiar binary environment. :(

By doing so, it managed to significantly trim down the image size.

> test/jdk/java/lang/ProcessBuilder/RedirectWithLongFilename.java
>
> @comment is not needed
>
> That said I don't think that test should be using the Basic class.

Fixed by removing the dependency on Basic class and adding windows to
@requires.

Thanks,

-Aleksei

[1]
https://hg.openjdk.java.net/jdk/jdk/file/8f642d0b0f63/test/jdk/jdk/jfr/event/compiler/TestCodeCacheConfig.java
[2]
https://hg.openjdk.java.net/jdk/jdk/file/8f642d0b0f63/test/jdk/java/lang/management/MemoryMXBean/CollectionUsageThreshold.java
[3]
https://hg.openjdk.java.net/jdk/jdk/file/8f642d0b0f63/test/jdk/java/util/Arrays/TimSortStackSize2.java
[4] https://www.openwall.com/lists/musl/2020/05/27/2