RFR: 8240110: Improve NULL

[David Holmes]

Hi Leo,

On 1/04/2020 5:10 am, Leo Korinth wrote:
> 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)

If we're trying to clean up the use of 0, NULL, NULL_WORD then I think 
it should be changed.

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

I agree.

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

Ah I looked at the wrong platform definition. sparc is a bit of mess 
here. Under the covers they all get cast to address anyway. But yes 
using NULL_WORD to explicitly select the intptr_t variant seems fine.

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

I'm not sure how the compiler handles this.

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

Yes - fine for sparc.

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

Sorry yes, I was thinking of this as if it were a parameter but of 
course this is a macro with direct substitution.

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

Agreed.

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

This would look better to me as NULL.

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

Oops! Yep missed that.

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

Normal style rules for macros say to use the macro arg in parentheses - 
but there are three other uses in the same macro.

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

My point is that semantically the bare CHECK_ macros and the 
JVMCI_CHECK_ macros are supposed to be doing exactly the same thing 
(which makes me wonder why they defined the JVMCI variants?). It seems 
the exact usage contexts are determining whether you need to use NULL or 
NULL_WORD and that seems wrong to me.

IMO the functions that return JVMCIObjectArray should not be returning 
"null" unless it does return an actual "null" instance of the class. 
Otherwise it should be returning JVMCIObjectArray*

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

No CHECK_0 is intended for int-returning functions (which would also 
extend to intptr_t).

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

This misuse just jumped out at me when looking at the code.

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

Ok.

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

I would consider that a necessary but not sufficient condition.

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

Ok.

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

That sounds like a good intermediate step until we have C++11 support.

My main concern here is that this looks clean from a developer 
perspective: use NULL for pointers, NULL_WORD for integers and hopefully 
avoid needing to use 0 where the type system can't distinguish the two.

Thanks,
David

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