RFC: JEP 386 patch - minor cleanup

Wed Aug 12 16:58:08 UTC 2020

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/

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
>>>>
>>>>
>