RFR: 8240110: Improve NULL

David Holmes david.holmes at oracle.com
Mon Mar 30 01:05:21 UTC 2020 

Hi Leo,

A couple of initial responses before looking at the code ...

On 30/03/2020 10:24 am, Leo Korinth wrote:
> 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.

It will be good if that is the case. The reason it was defined for each 
tool chain was because differnet toolchains had different expectation 
and behaviours and quirks when it came to defining NULL. Of course that 
is mainly ancient history.

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

We have some special classes that we do not allocate and construct using 
the C++ facilities. For example, hastable entry types like 
ResolutionErrorEntry. These do not have a constructor run and the fields 
are initialised directly following the allocation. ObjectMonitors are 
another special case where we block allocate and then run an init function.

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

This refers to 32-bit systems: int == 32bits; ptr == 64-bits; intptr_t 
== 64-bits

Cheers,
David
-----

> 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

More information about the hotspot-dev mailing list