RFR: 8240110: Improve NULL

Mon Mar 30 00:24:51 UTC 2020

Hi, could I have a review on this change to improve NULL?

I try to:
1) make NULL only convert to pointer types for better type safety, 
__null on gcc achieves some of this, but can be casted away.
2) define NULL in one place, in a platform independent way, no need to 
define it for every tool chain.
3) make NULL be of a special type (null_t) so that one can template 
specialise against it. This is for the future!

I fail with this, but continue to read...

I try to achieve this by defining NULL as a variable of type null_t in 
the header null.hpp using ODR. null_t converts to (almost) any pointer type.

Minor negative issues:
1) No conversion to method pointers, they are only used in three places 
though, I use a literal 0 instead.
2) gcc does unfortunately not know that our NULL is a safe format 
string, only needed to change one place.
3) No constexpr yet, so I needed to use 0 to initialise a thread variable.

IMO positive issues:
1) I have fixed many bad usages of NULL for integer types and instead 
used NULL_WORD or 0.
2) Too many implicit conversions in certain places makes the code not 
compile, the changed code does look better and more idiomatic IMO.
3) Possible benign bug found where fields are initialised to NULL (not 
in constructor), this should not be possible right?
4) NULL does not convert to array types, on Windows this results in 
compile time error as the windows compiler does not decay arrays when 
comparing to null_t, this did locate one bug, and possibly some 
unnecessary comparisons in xmacros that I kept by changing NULL to 0.
5) Found a reinterpret_cast on NULL that is not allowed by the standard.

I also made a new NULL_WORD in the same way as I did NULL, but instead 
converting to a intptr_t of value 0. I was about to release NULL_WORD as 
a separate patch, but due to the current practice of defining NULL_WORD 
as NULL on many platforms, it was easier to merge these two changes into 
one. Only small issues in JVMCI code for this change, and one fix in 
sparc assembler. I find NULL_WORD questionable, but it is widely used.

Something to notice is the strange comment in the solstudio header about 
intptr_t being wider than int on some systems that is to my 
understanding false (for systems we support). I have a static assert to 
check that intptr_t is indeed of (char) pointer size. I did remove an 
assert that checks the size of NULL.

Now, the sad thing is that my change does not work fully, I can not make 
NULL constexpr before c++ 11, and thus several initialisation order bugs 
appear on global variables set to NULL. I could set them to zero or not 
set them at all, and they would work, but it feels brittle and could 
cause future bugs when people expect NULLs on globals to be initialised 
directly. Thus instead, I propose that I revert somewhat and define NULL 
as 0 and change NULL_WORD to a constant 0 of type intptr_t.

When we get c++>=11 we can define NULL to nullptr of type nullptr_t, and 
hopefully that will be a simple fix after this change.

What do you think?

Below I have given a short description of the changes:
src/hotspot/cpu/aarch64/interpreterRT_aarch64.cpp
correct the usage of integer type from NULL to NULL_WORD

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp
too many implicit conversions

src/hotspot/cpu/arm/interpreterRT_arm.cpp
correct the usage of integer type from NULL to NULL_WORD

src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp
too many implicit conversions

src/hotspot/cpu/sparc/c1_CodeStubs_sparc.cpp
correct the usage of integer type from NULL to NULL_WORD

src/hotspot/cpu/sparc/c1_LIRAssembler_sparc.cpp
correct the usage of integer type from NULL to NULL_WORD, and the
opposite way as well!

src/hotspot/cpu/sparc/interpreterRT_sparc.cpp
correct the usage of integer type from NULL to NULL_WORD

src/hotspot/cpu/x86/interp_masm_x86.cpp
correct the usage of integer type from NULL to NULL_WORD

src/hotspot/cpu/x86/interpreterRT_x86_64.cpp
correct the usage of integer type from NULL to NULL_WORD

src/hotspot/cpu/x86/stubGenerator_x86_64.cpp
correct the usage of integer type from NULL to NULL_WORD

src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp
correct member initialisation to be in initialise list, gcc bug?

src/hotspot/os/linux/cgroupV2Subsystem_linux.hpp
correct member initialisation to be in initialise list, gcc bug?

src/hotspot/os/solaris/attachListener_solaris.cpp
correct the usage of integer type from NULL to 0

src/hotspot/os/windows/osThread_windows.cpp
correct the usage of integer type from NULL to 0

src/hotspot/os/windows/os_windows.cpp
correct the usage of integer type from NULL to 0

src/hotspot/os/windows/pdh_interface.cpp
new NULL value does unfortunately not convert to method pointers

src/hotspot/share/classfile/javaClasses.cpp
too many implicit conversions

src/hotspot/share/classfile/moduleEntry.hpp
too many implicit conversions

src/hotspot/share/classfile/modules.cpp
too many implicit conversions

src/hotspot/share/code/codeHeapState.cpp
new NULL does not compare with arrays witch is great, maybe (xmacros) 
the NULL check can be removed, this change is safe though

src/hotspot/share/compiler/directivesParser.cpp
new NULL value does unfortunately not convert to method pointers

src/hotspot/share/gc/g1/g1ConcurrentRefine.cpp
remove unnecessary system include, might redefine NULL

src/hotspot/share/gc/parallel/psAdaptiveSizePolicy.cpp
remove unnecessary system include, might redefine NULL

src/hotspot/share/gc/parallel/psParallelCompact.cpp
remove unnecessary system include, might redefine NULL

src/hotspot/share/gc/shared/memset_with_concurrent_readers.hpp
remove unnecessary system include, might redefine NULL

src/hotspot/share/gc/z/zErrno.cpp
remove unnecessary system include, might redefine NULL

src/hotspot/share/jfr/jni/jfrGetAllEventClasses.cpp
fix bad assert found due to new NULL

src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
too many implicit conversions, and misc changes

src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp
new NULL does not compare with arrays witch is great, maybe (xmacros) 
the NULL check can be removed, this change is safe though

src/hotspot/share/jvmci/jvmciExceptions.hpp
too many implicit conversions

src/hotspot/share/oops/access.hpp 
reinterpret_cast is not safe on NULL, rewrite

src/hotspot/share/oops/constantPool.cpp
too many implicit conversions

src/hotspot/share/oops/cpCache.cpp
too many implicit conversions

src/hotspot/share/oops/klass.cpp
too many implicit conversions

src/hotspot/share/oops/oopsHierarchy.hpp
add null_t overload to oop class

src/hotspot/share/prims/jni.cpp
too many implicit conversions

src/hotspot/share/prims/jvm.cpp
too many implicit conversions

src/hotspot/share/prims/methodHandles.cpp
too many implicit conversions

src/hotspot/share/runtime/os.cpp
correct the usage of integer type from NULL to 0

src/hotspot/share/runtime/os.inline.hpp
#undef NULL set by system headers, use the new NULL instead

src/hotspot/share/runtime/sweeper.hpp
gcc does unfortunately not know that our NULL is a safe format string

src/hotspot/share/runtime/thread.cpp
our conversion can not be a constexpr before c++11, use 0 instead

src/hotspot/share/utilities/debug.hpp
stop using system headers for NULL

src/hotspot/share/utilities/globalDefinitions.cpp
change to static asserts, and remove one sizeof(NULL) assert.
I will change the assert on sizeof(uintptr_t) to sizeof(NULL_WORD) in 
the next webrev as it is both wrong and not clear (I defined NULL_WORD 
to be of type intptr_t and not uintptr_t)

src/hotspot/share/utilities/globalDefinitions.hpp
include the new definition of NULL *after* system includes, also do 
redefinition after include guard end

src/hotspot/share/utilities/globalDefinitions_gcc.hpp
remove old definition of NULL

src/hotspot/share/utilities/globalDefinitions_solstudio.hpp
remove old definition of NULL

src/hotspot/share/utilities/globalDefinitions_visCPP.hpp
remove old definition of NULL

src/hotspot/share/utilities/globalDefinitions_xlc.hpp
remove old definition of NULL

src/hotspot/share/utilities/linkedlist.hpp
new NULL value does unfortunately not convert to method pointers

src/hotspot/share/utilities/null.hpp
new NULL

test/hotspot/gtest/gc/z/test_zAddress.cpp
correct the usage of integer type from NULL to NULL_WORD

test/hotspot/gtest/unittest.hpp
do not use system NULL

I have a follow up patch that tries to (totally) remove usage of system 
header NULL, but that is orthogonal to this patch so I did not want to 
mix them if the approach is not liked.

Bug:
https://bugs.openjdk.java.net/browse/JDK-8240110

Webrev:
http://cr.openjdk.java.net/~lkorinth/8240110/part1 (NULL)
http://cr.openjdk.java.net/~lkorinth/8240110/part2 (NULL_WORD)
http://cr.openjdk.java.net/~lkorinth/8240110/part3 (reverse somewhat)
http://cr.openjdk.java.net/~lkorinth/8240110/0     (combined change that 
I will create incremental changes against)

Testing:
tier 1-3

Thanks,
Leo