RFC: JEP 386 patch - minor cleanup

Thu Aug 13 13:59:08 UTC 2020

Hi Magnus,

you are right: 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://www.openwall.com/lists/musl/2020/05/28/5
>>>> [2] https://www.openwall.com/lists/musl/2020/05/25/3
>>>> [3] https://www.openwall.com/lists/musl/2020/02/12/9
>>>> [4] http://hg.openjdk.java.net/jdk/jdk/rev/57e7f837e6d9
>>>>> Cheers,
>>>>> Mikael
>>>>>
>>>>>> Thanks,
>>>>>> -Aleksei
>>>>>>
>>>>>> [1] https://openjdk.java.net/jeps/386
>>>>>> [2] http://cr.openjdk.java.net/~avoitylov/webrev.8247589
>>>>>> (https://bugs.openjdk.java.net/browse/JDK-8247589)
>>>>>> [3] http://cr.openjdk.java.net/~avoitylov/webrev.portola
>>>>>> [4] https://bugs.openjdk.java.net/browse/JDK-8239450
>>>>>>
>>>>>>
>