RFR: 8240110: Improve NULL
Kim Barrett
kim.barrett at oracle.com
Fri Apr 17 07:46:33 UTC 2020
> On Apr 14, 2020, at 9:53 AM, Leo Korinth <leo.korinth at oracle.com> wrote:
> On 06/04/2020 01:37, David Holmes wrote:
>>> 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).
>
> It might be intended for it, but it is not used in such a way (therefore my remark on compile failures when using NULL_WORD)
>
> I will do no change here.
It sounds like there are some places using CHECK_0 that should be
using CHECK_NULL, and they should be fixed. But that can be a separate
change. It might be aided (and backsliding prevented) by doing
something similar to what I suggest for JVMCI_CHECK_0/NULL below.
> total combined change:
> http://cr.openjdk.java.net/~lkorinth/8240110/1
I've gone through all the changes in the "total combined change", with
comments below.
------------------------------------------------------------------------------
In the end you seem to have backed away from fixing the definition of
NULL_WORD, and I'm not sure why. Though I think the definition that
was proposed is at least unnecessarily baroque, possibly wrong. It
seems to me that a good and sufficient change for that would be to
delete all the existing platform definitions and put the following in
globalDefinitions.hpp:
const intptr_t NULL_WORD = 0;
Then it's no longer a null pointer constant, so can't be misused in
contexts where a pointer is required, will be treated as an integral
value by templates, &etc.
I wouldn't worry about this comment in globalDefinitions_solstudio.hpp:
"On some platforms, sizeof(intptr_t) > sizeof(void*)"
or you can be really paranoid and STATIC_ASSERT that they are the same
size. Though I really wonder what platform(s) that's talking about.
That comment comes from 6452081: 3/4 Allow for Linux builds with Sun
Studio Linux compilers. So maybe 32bit Solaris Studio Linux is weird?
------------------------------------------------------------------------------
There is a pattern to some of the changes where I'd prefer something
different.
We have classes with a pointer member that is initialized to NULL when
default constructed, and initialized from the argument by an implicit
conversion constructor with an appropriately typed pointer parameter.
Clearly this pattern permits implicit conversion construction from
NULL, but the proposed changes suggest one should avoid doing so. But
nothing is being done to prevent it.
Implicit conversion constructors, like implicit conversion operators,
can be problematic. I think a better change for these would be to make
the conversion from pointer constructor explict and use default
construction for the NULL pointer case. But whether a change from
implicit to explicit conversion is desirable might not be obvious in
all cases, and ought to be discussed with people who work with the
relevant parts of the code a lot.
Rather than repeat all that, I've used the shorthand "PATTERN CHANGE"
to indicate these in the comments below.
------------------------------------------------------------------------------
src/hotspot/cpu/sparc/c1_CodeStubs_sparc.cpp
303 AddressLiteral addrlit(NULL_WORD, metadata_Relocation::spec(_index));
318 AddressLiteral addrlit(NULL_WORD, oop_Relocation::spec(_index));
I think these should be left as NULL rather than being changed to
NULL_WORD. The expected type is the address type, and other types are
converted to that via casts in constructor overloads.
Similarly here:
src/hotspot/cpu/sparc/c1_LIRAssembler_sparc.cpp
426 AddressLiteral addrlit(NULL_WORD, oop_Relocation::spec(oop_index));
445 AddressLiteral addrlit(NULL_WORD, metadata_Relocation::spec(index));
------------------------------------------------------------------------------
src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp
[old code]
100 CachingCgroupController* _memory = NULL;
101 CgroupV1Controller* _cpuset = NULL;
102 CachingCgroupController* _cpu = NULL;
103 CgroupV1Controller* _cpuacct = NULL;
How does this even compile? Non-static member initializers are a
C++11 feature. Weirdly, g++ with -std=gnu++98 warns about this for
some types/initializers but not for others.
The proposed change is better, even with C++11.
Similarly here:
src/hotspot/os/linux/cgroupV2Subsystem_linux.hpp
------------------------------------------------------------------------------
src/hotspot/os/solaris/attachListener_solaris.cpp
461 status = ::sema_init(&_wakeup, 0, NULL, NULL);
=>
461 status = ::sema_init(&_wakeup, 0, 0, NULL);
The old code appears to have been cribbed from the documentation:
https://docs.oracle.com/cd/E36784_01/html/E36874/sema-init-3c.html
See the example of "Default semaphore initialization (intra-process)".
Rather than a literal 0, better would be USYNC_THREAD. Presumably the
value of USYNC_THREAD is 0.
------------------------------------------------------------------------------
src/hotspot/os/windows/os_windows.cpp
1659 if (!GetFileVersionInfo(kernel32_path, 0, version_size, version_info)) {
Given that the second argument, which was changed from NULL to 0, is a
"handle" (though unused), I think using NULL was appropriate and this
change shouldn't be made.
------------------------------------------------------------------------------
src/hotspot/share/classfile/javaClasses.cpp
851 k->set_java_mirror_handle(NULL);
=>
851 k->set_java_mirror_handle(OopHandle(NULL));
PATTERN CHANGE
Similarly here:
src/hotspot/share/classfile/moduleEntry.hpp
82 _module = OopHandle(NULL);
84 _pd = OopHandle(NULL);
And here:
src/hotspot/share/classfile/modules.cpp
156 version_symbol = TempNewSymbol(NULL);
161 TempNewSymbol location_symbol(NULL);
368 version_symbol = TempNewSymbol(NULL);
373 TempNewSymbol location_symbol(NULL);
Although I'm not sure why the assignments on line 156 and line 368 is
even being done, since each version_symbol was already default
initialized.
------------------------------------------------------------------------------
src/hotspot/share/code/codeHeapState.cpp
140 if (((_termString) != 0) && (strlen(_termString) > 0)){ \
154 if (((_termString) != 0) && (strlen(_termString) > 0)){ \
I think these changes to compare _termstring with 0 rather than NULL
should not be made. _termString is supposed to be a `[const] char*`,
so comparing to NULL is preferable to comparing to 0.
Though at a brief skim it looks like _termString is always provided
and never NULL, so it might be better to assert that _termString is
non-NULL. But that's a completely different change.
------------------------------------------------------------------------------
src/hotspot/share/gc/parallel/psAdaptiveSizePolicy.cpp
src/hotspot/share/gc/parallel/psParallelCompact.cpp
src/hotspot/share/gc/shared/memset_with_concurrent_readers.hpp
src/hotspot/share/gc/z/zErrno.cpp
These #include changes seem unrelated to NULL.
------------------------------------------------------------------------------
src/hotspot/share/jfr/jni/jfrGetAllEventClasses.cpp
172 assert(add_method_signature != NULL, "invariant");
=>
172 assert(add_method_sig_sym != NULL, "invariant");
This seems to be an unrelated bug fix; the wrong value was being
compared to NULL.
------------------------------------------------------------------------------
src/hotspot/share/jfr/jni/jfrJavaCall.cpp
57 assert(_storage != NULL, "invariant");
=>
57 assert(_storage_index < (int) ARRAY_SIZE(_storage), "invariant");
_storage is an array, so the original assert is definitionally true,
but it's harmless to verify, in case future maintenance makes it an
allocated array rather than inline.
The size of _storage is definitionally SIZE, and _storage_index is
checked against SIZE a couple lines later, making the proposed
replacement just a duplicative check.
So I think the proposed change should not be made.
Similarly at lines 65, 73, and 91.
------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
1562 JVMCIENV->set_HotSpotStackFrameReference_localIsVirtual(hs_frame, 0);
This change to use literal 0 rather than NULL as a null pointer
constant does not seem like an improvement to me. I think this change
should not be made.
------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
I looked at the changes from JVMCI_CHECK_0 to JVMCI_CHECK_NULL and
think they are correct, assuming a suitable definition of the latter.
(Though see below regarding that.)
------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
2572 JVMCIObjectArray current_array = NULL;
=>
2572 JVMCIObjectArray current_array(NULL);
PATTERN CHANGE
------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciCompilerToVMInit.cpp
267 if ((value) != 0) { \
Value is a char*; I don't think this change should be made.
------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciExceptions.hpp
50 #define JVMCI_CHECK_NULL JVMCI_CHECK_(NULL)
=>
50 #define JVMCI_CHECK_NULL JVMCI_CHECK_(0)
For platforms that warn about converting NULL to an integral value
this is a type safety regression. I think this change should not be
made.
------------------------------------------------------------------------------
src/hotspot/share/jvmci/jvmciExceptions.hpp
49 #define JVMCI_CHECK_0 JVMCI_CHECK_(0)
=>
49 #define JVMCI_CHECK_0 JVMCI_CHECK_(NULL_WORD)
I kind of think this is an abuse of NULL_WORD. It also serves no real
purpose with the current definitions of NULL_WORD (though would with
the suggestion far above). If the goal is to have something that isn't
a null pointer constant so it can't be used in inappropriate
contexts (which is a reasonable goal), then replace "0" with "int(0)"
(and comment appropriately).
------------------------------------------------------------------------------
src/hotspot/share/oops/access.hpp
296 class ArrayAccess ...
How about adding the following private functions to this class, and
use them in the obvious places:
template<typename T> static const T* const_raw_null() { return NULL; }
template<typename T> static T* raw_null() { return NULL; }
------------------------------------------------------------------------------
src/hotspot/share/oops/constantPool.cpp
270 set_resolved_references(OopHandle(NULL));
302 set_resolved_references(OopHandle(NULL));
376 set_resolved_references(OopHandle(NULL));
PATTERN CHANGE
Similarly here:
src/hotspot/share/oops/cpCache.cpp
766 set_resolved_references(OopHandle(NULL));
src/hotspot/share/oops/klass.cpp
549 _java_mirror = OopHandle(NULL);
src/hotspot/share/prims/jni.cpp
319 TempNewSymbol class_name(NULL);
src/hotspot/share/prims/jvm.cpp
954 TempNewSymbol class_name(NULL);
src/hotspot/share/prims/methodHandles.cpp
1311 TempNewSymbol name(NULL);
1312 TempNewSymbol sig(NULL);
------------------------------------------------------------------------------
src/hotspot/share/runtime/os.cpp
1324 if (path == NULL || strlen(path) == 0 || file_name_length == 0) {
`(size_t)NULL` ?? Good catch!
------------------------------------------------------------------------------
More information about the hotspot-dev
mailing list