RFR: 8240110: Improve NULL

[Leo Korinth]

Thanks for taking time to look through this!

On 31/03/2020 12:04, David Holmes wrote:
> 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?

I agree that it could be compared against NULL_WORD, do you want me to 
change it? (I have no opinion on this issue)

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

Yes, with my original NULL two conversions would not compile. Assigning 
is adding one extra step from direct construction. The new style is IMO 
easier to read.

> ---
> 
> 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
The first argument (to the constructor) can be address (u_char*), 
jobject (_jobject*), or intptr_t (not a pointer). Surely if we send a 
pointer (NULL), the compiler would have a hard time choosing between the 
first two pointer constructors, right? Therefore we send a non-pointer!

The reason this worked is because I believe integer /promotion/ has 
precedence over 0-to-pointer /implicit conversion/, but I am not too 
knowledgable about these things... Thus the old NULL prefer to be 
promoted to integral type! Do you agree or am I misunderstanding something?

This is /highly/ non-intuitive and one /strong/ argument to change NULL 
to be of the future c++11 nullptr_t type! (or my failed null_t type).

> 
> Same applies to other AddressLiteral changes.

After above reasoning, I should keep my changes right?

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

No, _termString is mostly a string literal and not a "const char*" and 
/can not/ in the instances of literal be NULL. Thus the (windows) 
compiler (rightly) will complain if you use a "proper" pointer-type 
NULL. The "technical" reason is that the array will not decay to a 
pointer and my original NULL will thus not convert to the array. Which 
makes sense.

The reason I did not remove the test is that I was afraid that I either 
missed some invocation of the macro where _termString really was of 
pointer type, or that someone in the future would use it as such. The 
real problem here is macros (and c strings).

It is a pity that neither gcc nor clang complains (I am not sure about 
the standard), I am bit unsure about nullptr behaviour in the future in 
this regard, but I hope it will complain as good as my failed NULL!

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

I believe this to be a temporary limitation, my understanding is that 
when we get c++11 nullptr will convert to method pointers as well, I 
could and think I should revert these changes as I did revert my 
implementation of NULL, would you like that? My null_t type could, to my 
knowledge, not auto convert to method pointers at least not without 
template varargs stuff that comes in later versions of the standard.

> 
> ---
> 
> 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");

Because _storage can not be NULL (it is an array and not a pointer). I 
feel the new assert is strictly superior! Also an example of the power 
of static typing and more precise types.

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

This is the same issue as before with string literals and macros, my 
arguing is that I will not try to prove that value is (or in the future 
always will be) a string literal, instead I will make the code work for 
both string literals and pointers. I hope you do not mind the extra 
parentheses I added around "value".

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

I feel NULL_WORD act as a more restricted 0 integral literal than the 
zero literal itself (it does not auto converts to pointers). The size of 
the zero constant is of less interest to me.

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

I agree it is not as distinct as NULL. Now for example, JVMCIObjectArray 
is not a pointer (strictly at least, it is a class). With my first NULL 
it would need to go through two conversions to become a JVMCIObjectArray 
(NULL converted to any pointer and any pointer converted to 
JVMCIObjectArray), thus it would not compile.

There are several solutions to this including:
1) use 0 literal as I did.
2) change JVMCIObjectArray constructor to take a null(ptr)_t instead of 
a pointer (this is a nice example of how a specific null type can make 
that constructor /total/ and not require the assert(v == NULL, "must be 
NULL");). Although this is a tempting solution, I had to abandon my own 
NULL and this solution is no longer possible until c++11.
3) use NULL as before, and fix it when we get nullptr_t.
4) create another macro in some way?

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

No, the same is not true for CHECK_NULL. I have mostly changed stuff 
that does not compile. This still compiles.

I could change CHECK_0 to use NULL_WORD though and if I do, I get 
compiler errors. CHECK_0 is used for both pointers and integral zeroes 
and types that implicitly converts from either. What the original 
thought was is unclear to me; is CHECK_0 to be used to return things 
convertible from pointers? If so, what is CHECK_NULL for?

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

Maybe all this should be cleaned up separately? Maybe this macro 
solution could be improved? And preferably not in this change?

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

Sorry, my comment is unclear, this was from my original failed NULL 
implementation. I will revert this to NULL as I expect this to work with 
nullptr_t in the future.


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

I guess tier1-3 would fail if not? Maybe that is a false assumption of mine?

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

I am not concerned :-) but I will revert this as it is not necessary now 
or in the future as I reverted my original implementation and c++11 will 
fix this.

> 
> ----
> 
> That's all from me.
> 
> Thanks,
> David
> -----

Before ending this mail, I also want to raise one question. Maybe I 
ought to #ifdef a #define NULL __null for the gcc tool chain to give 
better typing support until the real nullptr? I would hate to wait, but 
maybe changing to c++11 will take longer than I hope for.


Many thanks!
Leo
> 
> 
> 
> 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