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.)
---
First of all, I notice that you have not updated any copyright years. Do
you plan to do that by a script before pushing?
---
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.
* 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.
---
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), \
Also, the documentation of SetupNativeCompilation has not been updated
to match the implementation. Please remove the the "REORDER" line there
as well.
---
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.)
----
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! :-))
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.
---
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.
---
In RunTestPrebuilt.gmk, line 191:
# ... all others use uname -m
This comment is not relevant anymore.
---
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. :)
/Magnus
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/
