RFR: 8240110: Improve NULL
David Holmes
david.holmes at oracle.com
Tue Mar 31 10:04:46 UTC 2020
Hi Leo,
I looked at the "combined change". Overall I like the cleanup, but I did
find a few questionable items:
src/hotspot/cpu/arm/interpreterRT_arm.cpp
- _toGP[_last_gp++] = (*(intptr_t*)from_addr == 0) ? NULL : from_addr;
+ _toGP[_last_gp++] = (*(intptr_t*)from_addr == 0) ? NULL_WORD :
from_addr;
} else {
- *_to++ = (*(intptr_t*)from_addr == 0) ? NULL : from_addr;
+ *_to++ = (*(intptr_t*)from_addr == 0) ? NULL_WORD : from_addr;
If from_addr is being treated as intptr_t* then shouldn't it be compared
against NULL_WORD rather than 0?
---
General query: are changes like:
Foo f = NULL;
to
Foo f(NULL);
just making the implicit constructor selection explicit? (I have to say
it would nice if whenever you saw " x = NULL" that you knew x was a
pointer type.)
---
src/hotspot/cpu/sparc/c1_CodeStubs_sparc.cpp
- AddressLiteral addrlit(NULL, metadata_Relocation::spec(_index));
+ AddressLiteral addrlit(NULL_WORD, metadata_Relocation::spec(_index));
If the first constructor arg is an "address" which seems to be the
pointer type "uchar*" then surely NULL is correct here, not NULL_WORD?
Same applies to other AddressLiteral changes.
---
src/hotspot/share/code/codeHeapState.cpp
This doesn't look right:
- if (((_termString) != NULL) && (strlen(_termString) > 0)){\
+ if (((_termString) != 0) && (strlen(_termString) > 0)){ \
_termString should be a "const char*" so comparison against NULL is correct.
---
share/compiler/directivesParser.cpp
> new NULL value does unfortunately not convert to method pointers
Is that a temporary limitation? I'm not sure what progress were are
making if some pointers can use NULL and others must use 0.
---
src/hotspot/share/jfr/jni/jfrJavaCall.cpp
I don't understand why the NULL check has been removed:
- assert(_storage != NULL, "invariant");
+ assert(_storage_index < (int) ARRAY_SIZE(_storage), "invariant");
---
src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp
- if (value != NULL) { \
+ if ((value) != 0) { \
Unclear what type value is - in some cases char* in others I can't
determine.
---
src/hotspot/share/jvmci/jvmciExceptions.hpp
-#define JVMCI_CHECK_0 JVMCI_CHECK_(0)
-#define JVMCI_CHECK_NULL JVMCI_CHECK_(NULL)
+#define JVMCI_CHECK_0
JVMCI_CHECK_(NULL_WORD)
+#define JVMCI_CHECK_NULL JVMCI_CHECK_(0)
These just seem plain wrong. CHECK_0 is for returning a integral zero
value (e.g. jint, jbtye etc), nothing to do with intptr_t and NULL_WORD.
Having CHECK_NULL become CHECK_(0) seems completely wrong when you
consider that if you were expanding by hand in a function returning a
pointer then you would write "return NULL". Further if this chasnge is
necessary/desirable for JVMCI_CHECK_NULL then surely the same is true
for the basic CHECK_NULL in exceptions.hpp ?
Aside:
./share/jvmci/jvmciCompilerToVM.cpp
C2V_VMENTRY_0(jboolean, isInternedString, (JNIEnv* env, jobject, jobject
object))
Handle str = JVMCIENV->asConstant(JVMCIENV->wrap(object), JVMCI_CHECK_0);
if (!java_lang_String::is_instance(str())) {
return false;
CHECK_0 should be CHECK_false. Ditto in isInstance,
isCurrentThreadAttached, attachCurrentThread.
---
src/hotspot/share/runtime/sweeper.hpp
> gcc does unfortunately not know that our NULL is a safe format string
- static void log_sweep(const char* msg, const char* format = NULL,
...) ATTRIBUTE_PRINTF(2, 3);
+ static void log_sweep(const char* msg, const char* format = 0, ...)
ATTRIBUTE_PRINTF(2, 3);
I don't follow. This isn't "our NULL" we're using the toolchain's NULL,
and format is a pointer type so this should be fine. Is gcc broken?
---
src/hotspot/share/utilities/globalDefinitions_solstudio.hpp
It isn't clear that the issue of passing 0 as NULL in a varargs
situation is now fixed in Sol Studio.
---
src/hotspot/share/utilities/linkedlist.hpp
- return equal<E>(_data, t, NULL);
+ return equal<E>(_data, t, 0);
Again concerned about the method/function pointer situation.
----
That's all from me.
Thanks,
David
-----
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.
> 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
More information about the hotspot-dev
mailing list