Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
On 04/09/2020 21:55, Aleksei Voitylov wrote: Alan, here is a simpler version which, for the two tests in question, refers to a local helper class with a native method: http://cr.openjdk.java.net/~avoitylov/webrev.8247589.07/ If there is a preference to avoid JNI, there is yet another alternative: split the two launcher tests in question into tests with @requires vm.musl | os.family = "aix" and with @requires !vm.musl & os.family != "aix". 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. 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. 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. -Alan
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
Alan, here is a simpler version which, for the two tests in question, refers to a local helper class with a native method: http://cr.openjdk.java.net/~avoitylov/webrev.8247589.07/ If there is a preference to avoid JNI, there is yet another alternative: split the two launcher tests in question into tests with @requires vm.musl | os.family = "aix" and with @requires !vm.musl & os.family != "aix". -Aleksei On 04/09/2020 15:51, Aleksei Voitylov wrote: > Alan, > > in this case I'm leaning towards a new class jdk.test.lib.LibcHelper > with a native implementation which calls (*env)->NewStringUTF(env, > libc), which will be used by the tests which require it. Otherwise we'd > have to specify nativepath for all tests which use > jdk.test.lib.Platform. What do you think? > > -Aleksei > > On 04/09/2020 12:08, Alan Bateman wrote: >> On 04/09/2020 10:00, Alan Bateman wrote: >>> On 04/09/2020 08:55, Aleksei Voitylov wrote: : 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. >>> I really dislike the changes to the launcher tests and I don't think >>> the WB API is the right place for this. I think we need to find >>> something cleaner and maybe expose it as a method on >>> jdk.test.lib.Platform. >>> >> or alternatively, a new class in jdk.test.lib that gives provide >> methods to introspect the runtime, whatever is simplest and doesn't >> depend on the WB API as it's independent of HotSpot. >> >> -Alan
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
Alan, in this case I'm leaning towards a new class jdk.test.lib.LibcHelper with a native implementation which calls (*env)->NewStringUTF(env, libc), which will be used by the tests which require it. Otherwise we'd have to specify nativepath for all tests which use jdk.test.lib.Platform. What do you think? -Aleksei On 04/09/2020 12:08, Alan Bateman wrote: > On 04/09/2020 10:00, Alan Bateman wrote: >> On 04/09/2020 08:55, Aleksei Voitylov wrote: >>> : >>> 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. >>> >> I really dislike the changes to the launcher tests and I don't think >> the WB API is the right place for this. I think we need to find >> something cleaner and maybe expose it as a method on >> jdk.test.lib.Platform. >> > or alternatively, a new class in jdk.test.lib that gives provide > methods to introspect the runtime, whatever is simplest and doesn't > depend on the WB API as it's independent of HotSpot. > > -Alan
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
On 04/09/2020 10:00, Alan Bateman wrote: On 04/09/2020 08:55, Aleksei Voitylov wrote: : 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. I really dislike the changes to the launcher tests and I don't think the WB API is the right place for this. I think we need to find something cleaner and maybe expose it as a method on jdk.test.lib.Platform. or alternatively, a new class in jdk.test.lib that gives provide methods to introspect the runtime, whatever is simplest and doesn't depend on the WB API as it's independent of HotSpot. -Alan
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
On 04/09/2020 08:55, Aleksei Voitylov wrote: : 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. I really dislike the changes to the launcher tests and I don't think the WB API is the right place for this. I think we need to find something cleaner and maybe expose it as a method on jdk.test.lib.Platform. -Alan
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
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
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
On 01/09/2020 12:41, 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/ .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. I agree with David that the comment in java_md.c is excessive and unnecessary. 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. -Alan
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
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
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
Hi Aleksei, On 09/01/20 19:41 pm, Aleksei Voitylov wrote: > > 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/ > 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: --- a/src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp +++ b/src/hotspot/os_cpu/linux_aarch64/os_linux_aarch64.cpp @@ -74,7 +74,6 @@ # include # include # include -# include #define REG_FP 29 #define REG_LR 30 -- Thanks, Nick
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
Great to see this - thank you for all the great work you’re putting into it! The changes are in line with what I’m expecting given that I’ve looked at them before, so looks good to me! That said, I’ve looked at this so many times now - and after all even authored some of the original changes - so I would very much appreciate some other hotspot and core libs eyes on it. :) One very minor thing I realized: 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. Cheers, Mikael > On Sep 1, 2020, at 4:41 AM, 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/ > > 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 > >
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
On 2020-09-01 13:41, 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/ Looks good to me. /Magnus 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
Re: RFC: 8229469 JEP 386: Alpine Linux/x64 Port
Build changes look ok. /Erik On 2020-09-01 04:41, 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/ 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
RFC: 8229469 JEP 386: Alpine Linux/x64 Port
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/ 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
