RFC: JEP 386 patch - minor cleanup

Wed Jul 8 18:52:37 UTC 2020

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

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

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

set_glibc_version -> set_libc_version?

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

* ps_proc.c

Hm.. What makes you say that the original code is not thread-safe?

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

* 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).

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

* childproc.c

Remind me: what happens without the fix? I’m thinking there was some reason I made it to start with..?

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

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

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

* 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).

* 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? 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)

Finally a question: What level of testing have you done of this and on which versions of Alpine/…?

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