On 2020-05-07 00:52, Mikael Vidstedt wrote:
Magnus, thank you so much for the thorough review!! Will send out a
new webrev in a bit, meanwhile some comments inline..
On May 6, 2020, at 4:04 AM, Magnus Ihse Bursie
<magnus.ihse.bur...@oracle.com
<mailto:magnus.ihse.bur...@oracle.com>> wrote:
Hi Mikael,
On 2020-05-04 07:12, Mikael Vidstedt wrote:
Please review this change which implements part of JEP 381:
JBS: https://bugs.openjdk.java.net/browse/JDK-8244224
webrev:
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/build/open/webrev/
It's a massive change. So you're getting a massive review from me. :-)
In the following comments, the line numbers refer to the old files.
(I realize now that it might have been more convenient for you to
have the new numbers. I'm sorry I did not think of it before.)
Not a problem!
---
First of all, I notice that you have not updated any copyright years.
Do you plan to do that by a script before pushing?
Correct - whether I’ll end up using a script or not remains to be
seen, but I’ll fix the copyright years when the reviews/changes have
settled down a bit. I chose not to change all the copyright years
since that would distract from the “actual” changes, and also pretty
much guarantee conflicts when I pull in the latest changes.. It’s
challenging enough as it is :)
---
Several variables are not needed anymore and can be removed. These
include:
* TAR_CREATE_EXTRA_PARAM. Remove from Bundles.gmk,
autoconf/basic_tools.m4 and autoconf/spec.gmk.in.
* ELFEDIT. Remove from autoconf/spec.gmk.in.
* STLPORT_LIB. Remove from autoconf/spec.gmk.in.
* SA_LDFLAGS. Remove from modules/jdk.hotspot.agent/Lib.gmk.
* GLOBAL_LIBS (as has been noted by Kim as well). Remove from
common/NativeCompilation.gmk, autoconf/spec.gmk.in and
autoconf/libraries.m4.
* LDFLAGS_WARNINGS_ARE_ERRORS was (unfortunately!) only supported on
solstudio. No reason to keep it anymore. Remove from
common/NativeCompilation.gmk, autoconf/flags-ldflags.m4 and
autoconf/spec.gmk.in.
Fixed all of the above.
* GNM is not needed anymore, but there is certainly a need for
related cleanup (we do not need the if statement, but could use
"UTIL_CHECK_TOOLS(NM, nm gcc-nm)" for all platforms). I understand if
you do not want to mess around with it, let me know and I'll file a
separate follow-up bug.
Would definitely appreciate if you could file a follow-up, thank you!
Ok, will do.
---
In some places we used to have a variable that was passed into
Setup*Compilation, but the variable was removed. However, the
argument to the function was left in place. (Make do not warn for
unknown variables, unfortunately.)
lib/CompileJvm.gmk: EXTRA_OBJECT_FILES :=
$(DTRACE_EXTRA_OBJECT_FILES), \
Awt2dLibraries.gmk: ASFLAGS := $(LIBAWT_ASFLAGS), \
Fixed.
Also, the documentation of SetupNativeCompilation has not been
updated to match the implementation. Please remove the the "REORDER"
line there as well.
I’ve removed REORDER completely now. For completeness: In webrev.00 I
(only) removed C_FLAG_REORDER, and didn’t touch the REORDER argument
to SetupNativeCompilation. As you noted though, the REORDER
functionality is not actually used anywhere - that is true even in
mainline today. Turns out the last uses of the REORDER functionality
disappeared with https://bugs.openjdk.java.net/browse/JDK-8200178 /
http://hg.openjdk.java.net/jdk/jdk/rev/396ea30afbd5 authored by …
would you look at that - “ihse”! Wonder who that is? ;)
Ah, I see. I missed the fact that you removed C_FLAG_REORDER and not
REORDER. Thank you for cleaning up after me anyway. :-)
---
For dtrace, I want to make sure that the removal is complete (cleanse
it with fire, please! :-)).
The build tool generateJvmOffsets is not used anymore, so
generateJvmOffsets.cpp can be removed (actually, that means the
entire hotspot/src/native/).
I assume you remove src/java.base/solaris/native/libjvm_dtrace and
libjvm_db in the core-libs patch, right?
I also want to make sure that you actually remove hotspot_jni.d,
hotspot.d and hs_private.d from src/hotspot/os/posix/dtrace, and
src/hotspot/os/solaris/dtrace/jhelper.d. (Done in the hotspot patch,
I hope.)
I will not remove all traces of dtrace (no pun intended) as part of
this JEP, but (separate from this JEP) we should certainly discuss and
decide how to move forward with dtrace on the other platforms. The
stuff under src/java.base/solaris is indeed removed as part of the
core-libs patch, and the src/hotspot/os/solaris stuff is removed as
part of the hotspot patch, but AFAICT the src/hotspot/os/posix/dtrace
files still do get picked up by the logic in GensrcDtrace.gmk so I’m
going to leave those files untouched. Let me know if I’m missing
something.
Ok, I probably came across as too aggressive towards dtrace here. I do
not mean that I want all of dtrace removed. The parts that are
cross-platform are quite sane, at least from a build perspective. It's
all the messy meddling things that dtrace for solaris did that I dislike.
I double-checked now, and you are correct that the
src/hotspot/os/posix/dtrace files are still needed. I don't know how I
managed to believe they were not used before. (The power of wishful
thinking, perhaps.)
/Magnus
----
More cleanup in lib-freetype.m4:
On line 95:
# will report an error. On other platforms (Linux), they will
Remove the "(Linux)"; this applies to AIX as well so this
specification is both unnecessary and incorrect. (The best kind of
wrong! :-))
Fixed. :)
On line 170 and forwards:
# On solaris, pkg_check adds -lz to freetype libs,
which isn't
# necessary for us.
FREETYPE_LIBS=`$ECHO $FREETYPE_LIBS | $SED 's/-lz//g'`
Remove these lines, they are not needed anymore.
Fixed (I had that on my follow-up list because I wasn’t 100% sure if
it was in fact Solaris-specific).
---
Some changes I believe you need to revert to keep Zero working on
linux-sparc:
In flags.m4, line 268:
test "x$OPENJDK_TARGET_CPU_ARCH" = xsparc ||
In jvm-features.m4, line 324:
test "x$OPENJDK_TARGET_OS-$OPENJDK_TARGET_CPU" =
"xlinux-sparcv9"; then
and line 451:
if test "x$OPENJDK_TARGET_OS-$OPENJDK_TARGET_CPU" =
"xlinux-sparcv9"; then
JVM_FEATURES_PLATFORM_FILTER="$JVM_FEATURES_PLATFORM_FILTER jfr"
In platform.m4, lines 327-330:
elif test "x$OPENJDK_TARGET_CPU_ARCH" = "xsparc"; then
OPENJDK_TARGET_CPU=sparc
AC_MSG_ERROR([Reduced build (--with-target-bits=32) is only
supported on x86_64 and sparcv9])
and all the changes between lines 456-487.
In CompileJvm.gmk, line 63:
else ifeq ($(call isTargetCpu, sparcv9), true)
OPENJDK_TARGET_CPU_VM_VERSION := sparc
JvmFeatures.gmk, line 53:
This is explicitly done in a check for zero! It needs to be kept.
This also implies that at least the file
src/hotspot/cpu/sparc/memset_with_concurrent_readers_sparc.cpp will
be needed to keep for zero, as well.
I’m counting on somebody (hello Adrian! :)) to provide the list of
changes which would have to be reverted/preserved to support zero,
otherwise we’ll just be guessing..
---
In RunTestPrebuilt.gmk, line 191:
# ... all others use uname -m
This comment is not relevant anymore.
Fixed.
---
And finally, I want to draw attention to this gold nugget in
CompileDemos.gmk:
ifeq ($(call isTargetOs, solaris), true)
TARGETS += $(patsubst $(DEMO_SHARE_SRC)/nbproject/%, \
$(SUPPORT_OUTPUTDIR)/demos/image/nbproject/%, \
$(call FindFiles, $(DEMO_SHARE_SRC)/nbproject))
else
TARGETS += $(patsubst $(DEMO_SHARE_SRC)/nbproject/%, \
$(SUPPORT_OUTPUTDIR)/demos/image/nbproject/%, \
$(call FindFiles, $(DEMO_SHARE_SRC)/nbproject))
endif
LOL! :-D Good thing you cleared this up. :)
I remember staring at that one for a really long time, not seeing what
was actually different and not understanding what that meant for my
change. Had to go back in the mercurial history to double-check my
(in)sanity. :)
Cheers,
Mikael
JEP: https://bugs.openjdk.java.net/browse/JDK-8241787
Note: When reviewing this, please be aware that this exercise was
*extremely* mind-numbing, so I appreciate your help reviewing all
the individual changes carefully. You may want to get that coffee
cup filled up (or whatever keeps you awake)!
Background:
Because of the size of the total patch and wide range of areas
touched, this patch is one out of in total six partial patches which
together make up the necessary changes to remove the Solaris and
SPARC ports. The other patches are being sent out for review to
mailing lists appropriate for the respective areas the touch. An
email will be sent to jdk-dev summarizing all the patches/reviews.
To be clear: this patch is *not* in itself complete and stand-alone
- all of the (six) patches are needed to form a complete patch. Some
changes in this patch may look wrong or incomplete unless also
looking at the corresponding changes in other areas.
For convenience, I’m including a link below[1] to the full webrev,
but in case you have comments on changes in other areas, outside of
the files included in this thread, please provide those comments
directly in the thread on the appropriate mailing list for that area
if possible.
In case it helps, the changes were effectively produced by searching
for and updating any code mentioning “solaris", “sparc”,
“solstudio”, “sunos”, etc. More information about the areas impacted
can be found in the JEP itself.
Testing:
A slightly earlier version of this change successfully passed
tier1-8, as well as client tier1-2. Additional testing will be done
after the first round of reviews has been completed.
Cheers,
Mikael
[1]
http://cr.openjdk.java.net/~mikael/webrevs/8244224/webrev.00/all/open/webrev/
