Hi Aleksei,
Overall this all seems okay. A few minor comments below.
On 1/09/2020 9:41 pm, Aleksei Voitylov wrote:
Hi,
JEP 386 is now Candidate [1] and as we resolved all outstanding issues
within the Portola project I'd like to ask for comments from HotSpot,
Core Libs (changes in libjli/java_md.c), and Build groups before moving
the JEP to Proposed to Target:
http://cr.openjdk.java.net/~avoitylov/webrev.8247589.01/
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 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"
--
src/hotspot/os/linux/os_linux.hpp
numa_node_to_cpus is too big to be in the header file now.
---
src/hotspot/share/prims/whitebox.cpp
I would expect this to use the version string set in os_linux.cpp.
---
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.
---
src/java.base/unix/native/libjli/java_md.c
The comment is way too long - see the AIX case below it. :)
---
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 ...
---
test/hotspot/jtreg/runtime/StackGuardPages/exeinvoke.c
Why is this change needed?
---
In general the busybox changes are bit unpleasant in the tests. Pity
Alpine didn't try to present a familiar binary environment. :(
---
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.
---
Thanks,
David
Testing:
JTReg, VM TestBase on all platforms (Linux x86_64, Linux x86, Linux Arm,
Linux AArch64, Linux PPC, Windows x86_64, Windows x86, Mac) with no
regressions. A downport of these changes to 14 passes JCK 14 on these
platforms.
The port was tested on Alpine Linux 3.8 and 3.11 with JTReg, VM
TestBase. There are no differences with Linux x86_64 with the exception
of SA which is not supported as per the JEP. A downport of these changes
to 14 passes JCK 14 on Alpine Linux.
Thanks,
-Aleksei
[1] https://bugs.openjdk.java.net/browse/JDK-8229469
