RFC: JEP 386 patch - minor cleanup

Thu Aug 20 20:26:41 UTC 2020

This all looks really good, thanks for making the changes! Some very minor nit-picking:

* make/ReleaseFile.gmk 

How about introducing a RELEASE_FILE_OS_LIBC for consistency?

* src/hotspot/os/linux/os_linux.hpp

Looks like the indentation is slightly off now for the code blocks where the ‘g’ got dropped.

* src/hotspot/share/prims/whitebox.cpp

Indent the "(void*)&WB_GetLibcName” part to match the other entries.

The changes related to WAKEUP_SIGNAL and the isnan change are really not specific to musl, so it would certainly be possible and to me even _preferable_ to upstream them to mainline right away. Thoughts?

I’d also like to see all of this pushed to the portola repo, more or less replacing the changes there today with some exceptions (jib-profiles.js being the key one, but there may be others). What do you think about that?

Cheers,
Mikael

> On Aug 13, 2020, at 6:59 AM, Aleksei Voitylov <aleksei.voitylov at bell-sw.com> wrote:
> 
> Hi Magnus,
> 
> you are right: http://cr.openjdk.java.net/~avoitylov/webrev.8247589.03/ <http://cr.openjdk.java.net/~avoitylov/webrev.8247589.03/>
> 
> -Aleksei
> 
> On 13/08/2020 13:05, Magnus Ihse Bursie wrote:
>> On 2020-08-12 18:58, Aleksei Voitylov wrote:
>>> Hi Magnus,
>>> 
>>> thank you for looking into it. Here is a webrev that addresses your
>>> comments:
>>> 
>>> http://cr.openjdk.java.net/~avoitylov/webrev.8247589.02/
>> Thank you, looks much better! (If I may ask of anything more, it is to
>> keep a consistent ordering OS -> CPU -> LIBC  where they all occur
>> together. It helps keep your sanity up when reading the code)
>> 
>> /Magnus
>> 
>>> 
>>> As far as code changes are concerned, I'd still like to seek consensus
>>> within this project if the patch is overall good and has reached a point
>>> to submit for pre-review upstream. Meanwhile, it was concluded that
>>> JDK-8250630 is a generic issue that will be submitted upstream
>>> separately - it has to do with linux networking stack configuration.
>>> 
>>> -Aleksei
>>> 
>>> On 10/08/2020 15:52, Magnus Ihse Bursie wrote:
>>>> On 2020-07-28 19:20, Aleksei Voitylov wrote:
>>>>> Hi Mikael,
>>>>> 
>>>>> thank you for looking into it. Here is an updated version of the
>>>>> webrev
>>>>> with your comments addressed:
>>>>> 
>>>>> http://cr.openjdk.java.net/~avoitylov/webrev.8247589.01/
>>>> Some build comments:
>>>> 
>>>> make/autoconf/build-aux/config.sub:
>>>> I think you could match on just "linux-musl", to facilitate future
>>>> ports to other architectures.
>>>> 
>>>> make/autoconf/platform.m4:
>>>> The new clib matching should move to a separate
>>>> PLATFORM_EXTRACT_VARS_FROM_CLIB function. (Maybe these should have
>>>> been named "FOR" instead of "FROM"...)
>>>> 
>>>> Apart from this, it looks good from a build perspective.
>>>> 
>>>> /Magnus
>>>>> Please see inline for details.
>>>>> 
>>>>> On 08/07/2020 21:52, Mikael Vidstedt wrote:
>>>>>>> On Jun 26, 2020, at 5:00 AM, Aleksei Voitylov
>>>>>>> <aleksei.voitylov at bell-sw.com> wrote:
>>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> As JEP 386 is now Candidate [1] I'd like to request comments on the
>>>>>>> patch [2] before moving the JEP to PTT and submitting for review
>>>>>>> upstream. The patch is rebased to 59791:a22af2c3d969 and minor
>>>>>>> cleanup
>>>>>>> of the changes from the original portola branch [3] was made.
>>>>>>> 
>>>>>>> Webrev [2]: http://cr.openjdk.java.net/~avoitylov/webrev.8247589
>>>>>>> Original [3]: http://cr.openjdk.java.net/~avoitylov/webrev.portola
>>>>>> So glad to see that this is making progress, thanks a lot for
>>>>>> picking it up! I’ve had a quick look at the changes and have a few
>>>>>> initial comments, inlined below. I’ll also mention that I recently
>>>>>> started the work of bringing the portola repo up-to-date again after
>>>>>> the move to GitHub earlier this year. You’ll see that some of the
>>>>>> changes I’m making there are not the same as the ones you have in
>>>>>> this patch. Don’t read anything into that please - I started making
>>>>>> those changes before I realized that you had this patch out for
>>>>>> review and it wouldn’t surprise me if your version is preferable in
>>>>>> many/most cases. Let’s figure out how to resolve that in the portola
>>>>>> repo a bit later.
>>>>>> 
>>>>>> Disclaimer: this is *not* a complete review, and should not be
>>>>>> considered as blessing the changes for upstream/mainline. For that
>>>>>> you’ll need to get the changes sent out to the respective lists
>>>>>> (build-dev, hotspot-dev, …).
>>>>>> 
>>>>>> High level comment: There are a handful of JBS issues with Fix
>>>>>> Version=repo-portola which would probably be good to have a look at
>>>>>> and possibly close if they’re no longer relevant.
>>>>>> 
>>>>> I did some bug scrubbing, here is the list of open issues:
>>>>> 
>>>>> JDK-8241092 make test-bundles fails on Alpine Linux because of
>>>>> TestTLS.java test
>>>>>     is fixed as part of the proposed patch.
>>>>> JDK-8240187 Test
>>>>> hotspot/jtreg/runtime/jni/terminatedThread/TestTerminatedThread.java
>>>>> fails on Alpine Linux
>>>>>     the test is excluded as part of the proposed patch.
>>>>> JDK-8241053 Hotspot
>>>>> runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java test
>>>>> fails on Alpine Linux with debug build
>>>>>     - runtime/CommandLine/OptionsValidation/TestOptionsWithRanges.java
>>>>>     - runtime/Thread/TestThreadStackSizes.java
>>>>>     tests are fixed as part of the proposed patch, David suggests
>>>>> to fix
>>>>> it in mainline as it does not seem to be specific to the port. I
>>>>> tend to
>>>>> agree with the evaluation.
>>>>> JDK-8178692 Clean up RequiresSetenv / LD_LIBRARY_PATH
>>>>>     please see below
>>>>> JDK-8250630 test/jdk/com/sun/jdi/JdwpListenTest.java fails on Alpine
>>>>> Linux
>>>>>     - test/jdk/com/sun/jdi/JdwpAllowTest.java
>>>>>     - test/jdk/com/sun/jdi/JdwpListenTest.java
>>>>>     Alex is looking into the reason of the issue. It could be
>>>>> because of
>>>>> the network settings on the host or the way the getaddrinfo(3) returns
>>>>> addresses in musl. Currently it looks to be not very specific to the
>>>>> port, though.
>>>>> 
>>>>> These issues reflect the current testing status of the port.
>>>>>>> Build System, Toolchains:
>>>>>>> 
>>>>>>> - make/autoconf/build-aux/config.guess - replaced the "head -1 | cut
>>>>>>> -f1" construct with sed to be slightly more robust
>>>>>>> - make/autoconf/build-aux/config.sub - style brought in line with
>>>>>>> the
>>>>>>> current file style
>>>>>>> - make/autoconf/hotspot.m4 - HOTSPOT_SETUP_SA is no longer required
>>>>>>> after JDK-8239450 [4]
>>>>>>> - make/modules/jdk.jdwp.agent/Lib.gmk, make/test/JtregNativeJdk.gmk,
>>>>>>> hotspot/share/gc/shared/genCollectedHeap.cpp - disabling GCC
>>>>>>> warnings
>>>>>>> and modifying initialization is no longer required, Alpine Linux
>>>>>>> toolchain is quite recent
>>>>>> Great cleanups! Happy to see that the SA special casing is no longer
>>>>>> needed - https://bugs.openjdk.java.net/browse/JDK-8181313 is also to
>>>>>> thank for that.
>>>>>> 
>>>>>> * ReleaseFile.gmk
>>>>>> 
>>>>>> The choice to use “LIBC” as the key name in the releases file was
>>>>>> not something I/we spent a lot of time on, so we’ll want to revisit
>>>>>> that and make sure it’s the right name or come up with a better one.
>>>>> For ReleaseFile.gmk I think LIBC is good, as it is used as a handle
>>>>> for
>>>>> the actual OPENJDK_TARGET_LIBC. For source code, MUSL_LIBC is what
>>>>> came
>>>>> up as the least intrusive indication that musl is a kind of libc.
>>>>> Leaving it just as MUSL could be confusing.
>>>>>>> Desktop:
>>>>>>> 
>>>>>>> - src/java.desktop/unix/native/libawt_xawt/xawt/XToolkit.c -
>>>>>>> changes are
>>>>>>> present upstream and no longer required
>>>>>> Yay!
>>>>>> 
>>>>>>> HotSpot:
>>>>>>> 
>>>>>>> - src/hotspot/os/linux/os_linux.cpp - checks for MUSL can be made
>>>>>>> static. Defined stub dlvsym for MUSL instead of dlvsym_if_available
>>>>>>> logic
>>>>>>> - src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c - using
>>>>>>> strerror is cleaner and more portable, the code originally present
>>>>>>> there
>>>>>>> is not thread-safe so it's ok to use it
>>>>>>> - src/jdk.jdwp.agent/share/native/libjdwp/util.h - including a
>>>>>>> header
>>>>>>> instead
>>>>>> * os_linux.cpp
>>>>>> 
>>>>>> Out of curiosity, have you tested with/without the various libnuma
>>>>>> library versions?
>>>>> Please see the updated version of the patch which is tested to work
>>>>> with
>>>>> (and without) libnuma v1.2 on Alpine Linux. Note that musl can't
>>>>> recognize library versions, and it only allows to install v1.2 in the
>>>>> distribution. It's good there is a single place where a change was
>>>>> required.
>>>>>> set_glibc_version -> set_libc_version?
>>>>> Fixed, thanks.
>>>>>> * globalDefinitions_gcc.hpp
>>>>>> 
>>>>>> Did you verify that the change to use isnan instead of isnanf is
>>>>>> correct for all versions of linux/bsd and the respective (supported)
>>>>>> toolchain versions? See:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8178689. I linked it to the
>>>>>> JEP in JBS.
>>>>> I updated JDK-8178689 with proof that such a replacement is safe
>>>>> for all
>>>>> practical combinations of gcc/glibc and llvm versions.
>>>>>> * ps_proc.c
>>>>>> 
>>>>>> Hm.. What makes you say that the original code is not thread-safe?
>>>>> I checked LinuxDebuggerLocal.java and all attach0 calls 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 completely unpredictable
>>>>> results. If
>>>>> we run ptrace_attach from different threads but with the same PID, it
>>>>> either 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 using strerror instead of strerror_r to mitigate compatibility
>>>>> issues
>>>>> should not be a problem.
>>>>>>> java.base:
>>>>>>> 
>>>>>>> - src/java.base/linux/native/libnet/linux_close.c - made (SIGRTMAX
>>>>>>> - 2)
>>>>>>> a define
>>>>>>> - src/java.base/unix/native/libjava/childproc.c - suggest to skip
>>>>>>> this
>>>>>>> for now. Also, if the OS does not handle scripts without shebang it
>>>>>>> should not be emulated.
>>>>>> * jdk_util_md.h
>>>>>> 
>>>>>> The same isnan comment as above.
>>>>> This is no longer relevant, fixed upstream.
>>>>>> * INTERRUPT_SIGNAL
>>>>>> 
>>>>>> Can the definition of INTERRUPT_SIGNAL (perhaps then with a better
>>>>>> name) be moved to something like net_util_md.h so that it is shared
>>>>>> for both files using it? I’d also remove the spaces around the
>>>>>> arguments to pthread_kill in linux_close.c (pre-existing).
>>>>> Done, renamed INTERRUPT_SIGNAL to WAKEUP_SIGNAL.
>>>>>> * java_md.c
>>>>>> 
>>>>>> Did you look into alternatives ways to resolve the issue? As covered
>>>>>> by https://bugs.openjdk.java.net/browse/JDK-8178692 I was hoping we
>>>>>> could find some other way of dealing with that to avoid the the
>>>>>> LD_LIBRARY_PATH dance and the ugly/fragile fallout from it.
>>>>> Yes, but neither of them looked better [1, 2].
>>>>>> * childproc.c
>>>>>> 
>>>>>> Remind me: what happens without the fix? I’m thinking there was some
>>>>>> reason I made it to start with..?
>>>>> Without the fix we rely on the OS execve mechanism. On Alpine it does
>>>>> not currently handle files without shebang [3].
>>>>>>> Tests:
>>>>>>> 
>>>>>>> - added method isBusyBox() to test/lib/jdk/test/lib/Platform.java
>>>>>>> - added WB_InternalVMInfo method to
>>>>>>> hotspot/share/prims/whitebox.cpp for
>>>>>>> better test handling
>>>>>>> - refactored affected tests to cater for the above
>>>>>> * VMProps.java
>>>>>> 
>>>>>> If I read it correctly I think WB.internalVMInfo().contains("musl”)
>>>>>> may incorrectly return true if the VM info strings happens to
>>>>>> contain “musl” in, say, HOTSPOT_BUILD_USER or whatever, so you’ll
>>>>>> probably want something more targeted for the check. Also, the
>>>>>> pattern used in other places in the file to convert the boolean to a
>>>>>> String seems to be “" + p.
>>>>> Yes, converted the check into
>>>>> WhiteBox.getWhiteBox().getLibcName().contains("musl") - this should be
>>>>> more robust.
>>>>>> * exeinvoke.c
>>>>>> 
>>>>>> I’m don’t know the "complicated interaction of signal handlers” well
>>>>>> enough to understand if the changes are correct, but assuming they
>>>>>> are:
>>>>>> 
>>>>>> I’d feel better having the whole JDK1_1InitArgs jdk_args struct
>>>>>> initialized (memset).
>>>>>> 
>>>>>> size_t stacksize seems to be unused. Perhaps get_java_stacksize
>>>>>> should return a size_t?
>>>>>> 
>>>>>> I’d also remove the spaces between pthread_ and the argument list
>>>>>> while your at it (also pre-existing), not exactly obvious why only
>>>>>> the pthread_ calls have them..
>>>>> This is addressed in the updated webrev.
>>>>>> * exestack-tls.c
>>>>>> 
>>>>>> As a potential alternative, base the decision on a define set by the
>>>>>> build system (MUSL_LIBC). Just something to consider. What you have
>>>>>> should work.
>>>>> I'd suggest to keep it as is - there is no other precedent of this in
>>>>> tests to support altering the build system.
>>>>>> * TestTerminatedThread.java
>>>>>> 
>>>>>> I don’t know where we stand on adding comments, but personally I’d
>>>>>> prefer a comment explaining why the test is excluded when running
>>>>>> with musl (not unlike the one you have in NativeLibraryTest.java).
>>>>> Fixed in the updated webrev.
>>>>>> * Test7029048.java
>>>>>> 
>>>>>> Can VMProps::isMusl be reused? I’m assuming that moving the
>>>>>> predicate definition to TestHelper would require a slew of tests to
>>>>>> use -XX:+WhiteBox?
>>>>> Yes, this is exactly the problem we try to avoid here. Another
>>>>> option is
>>>>> to introduce another helper class just for these couple of tests, but
>>>>> there would be only a couple of places where it would be used.
>>>>>> Also, I know I looked at this before but don’t remember the
>>>>>> conclusion: why is the check at the end “passed < expected” rather
>>>>>> than exact match - passed == expected? (pre-existing)
>>>>> The test was refactored upstream and does not have any magic
>>>>> numbers any
>>>>> more [4]. The webrev is updated.
>>>>>> Finally a question: What level of testing have you done of this and
>>>>>> on which versions of Alpine/…?
>>>>> 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 3.8 and 3.11 with JTReg, VM TestBase.
>>>>> With
>>>>> the exception of the issues listed above and SA which is not supported
>>>>> as per the JEP, there are no differences with Linux x86_64. A downport
>>>>> of these changes to 14 passes JCK 14 on Alpine Linux.
>>>>> 
>>>>> -Aleksei
>>>>> 
>>>>> [1] https://urldefense.com/v3/__https://www.openwall.com/lists/musl/2020/05/28/5__;!!GqivPVa7Brio!K8peFNtteharrQnXwV5YhpYsiJtL-HbWoMKdVWr-YmE3act6PtOfcOS_ALXdsgKGnV3w$ <https://urldefense.com/v3/__https://www.openwall.com/lists/musl/2020/05/28/5__;!!GqivPVa7Brio!K8peFNtteharrQnXwV5YhpYsiJtL-HbWoMKdVWr-YmE3act6PtOfcOS_ALXdsgKGnV3w$> 
>>>>> [2] https://urldefense.com/v3/__https://www.openwall.com/lists/musl/2020/05/25/3__;!!GqivPVa7Brio!K8peFNtteharrQnXwV5YhpYsiJtL-HbWoMKdVWr-YmE3act6PtOfcOS_ALXdsp8FgoM5$ <https://urldefense.com/v3/__https://www.openwall.com/lists/musl/2020/05/25/3__;!!GqivPVa7Brio!K8peFNtteharrQnXwV5YhpYsiJtL-HbWoMKdVWr-YmE3act6PtOfcOS_ALXdsp8FgoM5$> 
>>>>> [3] https://urldefense.com/v3/__https://www.openwall.com/lists/musl/2020/02/12/9__;!!GqivPVa7Brio!K8peFNtteharrQnXwV5YhpYsiJtL-HbWoMKdVWr-YmE3act6PtOfcOS_ALXdstJH5-RH$ <https://urldefense.com/v3/__https://www.openwall.com/lists/musl/2020/02/12/9__;!!GqivPVa7Brio!K8peFNtteharrQnXwV5YhpYsiJtL-HbWoMKdVWr-YmE3act6PtOfcOS_ALXdstJH5-RH$> 
>>>>> [4] http://hg.openjdk.java.net/jdk/jdk/rev/57e7f837e6d9 <http://hg.openjdk.java.net/jdk/jdk/rev/57e7f837e6d9>
>>>>>> Cheers,
>>>>>> Mikael
>>>>>> 
>>>>>>> Thanks,
>>>>>>> -Aleksei
>>>>>>> 
>>>>>>> [1] https://openjdk.java.net/jeps/386 <https://openjdk.java.net/jeps/386>
>>>>>>> [2] http://cr.openjdk.java.net/~avoitylov/webrev.8247589 <http://cr.openjdk.java.net/~avoitylov/webrev.8247589>
>>>>>>> (https://bugs.openjdk.java.net/browse/JDK-8247589 <https://bugs.openjdk.java.net/browse/JDK-8247589>)
>>>>>>> [3] http://cr.openjdk.java.net/~avoitylov/webrev.portola <http://cr.openjdk.java.net/~avoitylov/webrev.portola>
>>>>>>> [4] https://bugs.openjdk.java.net/browse/JDK-8239450 <https://bugs.openjdk.java.net/browse/JDK-8239450>