RFR: JDK-8301497: Replace NULL with nullptr in cpu/s390
Johan Sjölen
jsjolen at openjdk.org
Tue Apr 11 10:12:09 UTC 2023
On Tue, 31 Jan 2023 11:40:09 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:
> Hi, this PR changes all occurrences of NULL to nullptr for the subdirectory cpu/s390. Unfortunately the script that does the change isn't perfect, and so we
> need to comb through these manually to make sure nothing has gone wrong. I also review these changes but things slip past my eyes sometimes.
>
> Here are some typical things to look out for:
>
> 1. No changes but copyright header changed (probably because I reverted some changes but forgot the copyright).
> 2. Macros having their NULL changed to nullptr, these are added to the script when I find them. They should be NULL.
> 3. nullptr in comments and logs. We try to use lower case "null" in these cases as it reads better. An exception is made when code expressions are in a comment.
>
> An example of this:
>
> ```c++
> // This function returns null
> void* ret_null();
> // This function returns true if *x == nullptr
> bool is_nullptr(void** x);
>
>
> Note how `nullptr` participates in a code expression here, we really are talking about the specific value `nullptr`.
>
> Thanks!
Reviewed and found a decent chunk of things to fix.
Don't close yet.
src/hotspot/cpu/s390/abstractInterpreter_s390.cpp line 124:
> 122: // Parameters:
> 123: //
> 124: // interpreter_frame != null:
nullptr
src/hotspot/cpu/s390/frame_s390.cpp line 114:
> 112:
> 113: // At this point, there still is a chance that fp_safe is false.
> 114: // In particular, (fp == null) might be true. So let's check and
nullptr
src/hotspot/cpu/s390/gc/g1/g1BarrierSetAssembler_s390.cpp line 407:
> 405: __ verify_oop(value, FILE_AND_LINE);
> 406: DecoratorSet decorators = IN_NATIVE | ON_PHANTOM_OOP_REF;
> 407: g1_write_barrier_pre(masm, decorators, (const Address*)nullptr, value, noreg, tmp1, tmp2, true);
unnecessary cast?
src/hotspot/cpu/s390/interp_masm_s390.cpp line 278:
> 276: Register tmp = Z_ARG3;
> 277: load_and_test_long(jvmti_thread_state, Address(Z_thread, JavaThread::jvmti_thread_state_offset()));
> 278: z_bre(L); // if (thread->jvmti_thread_state() == null) exit;
nullptr
src/hotspot/cpu/s390/interp_masm_s390.cpp line 984:
> 982: // } else if (THREAD->is_lock_owned((address)displaced_header))
> 983: // // Simple recursive case.
> 984: // monitor->lock()->set_displaced_header(null);
nullptr
src/hotspot/cpu/s390/interp_masm_s390.cpp line 1029:
> 1027: // } else if (THREAD->is_lock_owned((address)displaced_header))
> 1028: // // Simple recursive case.
> 1029: // monitor->lock()->set_displaced_header(null);
nullptr
src/hotspot/cpu/s390/interp_masm_s390.cpp line 1085:
> 1083: // if ((displaced_header = monitor->displaced_header()) == null) {
> 1084: // // Recursive unlock. Mark the monitor unlocked by setting the object field to null.
> 1085: // monitor->set_obj(null);
nullptr
src/hotspot/cpu/s390/interp_masm_s390.cpp line 1088:
> 1086: // } else if (Atomic::cmpxchg(obj->mark_addr(), monitor, displaced_header) == monitor) {
> 1087: // // We swapped the unlocked mark in displaced_header into the object's mark word.
> 1088: // monitor->set_obj(null);
nullptr
src/hotspot/cpu/s390/interp_masm_s390.cpp line 1111:
> 1109: // if ((displaced_header = monitor->displaced_header()) == null) {
> 1110: // // Recursive unlock. Mark the monitor unlocked by setting the object field to null.
> 1111: // monitor->set_obj(null);
nullptr
src/hotspot/cpu/s390/interp_masm_s390.cpp line 1123:
> 1121: // } else if (Atomic::cmpxchg(obj->mark_addr(), monitor, displaced_header) == monitor) {
> 1122: // // We swapped the unlocked mark in displaced_header into the object's mark word.
> 1123: // monitor->set_obj(null);
nullptr
src/hotspot/cpu/s390/interp_masm_s390.cpp line 1473:
> 1471: // // degenerate decision tree, rooted at row[2]
> 1472: // if (row[2].rec == rec) { row[2].incr(); goto done; }
> 1473: // if (row[2].rec != null) { count.incr(); goto done; } // overflow
nullptr
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3188:
> 3186: Register monitor_tagged = displacedHeader; // Tagged with markWord::monitor_value.
> 3187: bind(object_has_monitor);
> 3188: // The object's monitor m is unlocked iff m->owner == null,
nullptr
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3391:
> 3389: } else {
> 3390: if (needs_explicit_null_check((intptr_t)offset)) {
> 3391: // Provoke OS null exception if reg = null by
reg is
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3397:
> 3395: // else
> 3396: // Nothing to do, (later) access of M[reg + offset]
> 3397: // will provoke OS null exception if reg = null.
reg is
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3503:
> 3501: // This function calculates the size of the code generated by
> 3502: // decode_klass_not_null(register dst, Register src)
> 3503: // when (Universe::heap() != null). Hence, if the instructions
nullptr
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3669:
> 3667: // Rbase - Base address of cKlass in memory.
> 3668: // maybenull - True if Rop1 possibly is a null.
> 3669: void MacroAssembler::compare_klass_ptr(Register Rop1, int64_t disp, Register Rbase, bool maybenullptr) {
`maybe_null` is the best name here, imho.
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3803:
> 3801: // maybenull - True if Rop1 possibly is a null.
> 3802: // maybenulltarget - Branch target for Rop1 == null, if flow control shall NOT continue with compare instruction.
> 3803: void MacroAssembler::compare_heap_oop(Register Rop1, Address mem, bool maybenullptr) {
make sure these are consistent also
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3917:
> 3915: // only32bitValid is set, if later code only uses the lower 32 bits. In this
> 3916: // case we must not fix the upper 32 bits.
> 3917: void MacroAssembler::oop_encoder(Register Rdst, Register Rsrc, bool maybenullptr,
Consistency
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 3952:
> 3950: assert_different_registers(Rdst, Z_R1);
> 3951: assert_different_registers(Rsrc, Rbase);
> 3952: if (maybenullptr) {
Consistency
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 4053:
> 4051: // - keep Rdst and Rsrc distinct from Rbase. Rdst == Rsrc is ok for performance.
> 4052: // - avoid Z_R1 for Rdst if Rdst == Rbase.
> 4053: void MacroAssembler::oop_decoder(Register Rdst, Register Rsrc, bool maybenullptr, Register Rbase, int pow2_offset) {
Consistency
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 4077:
> 4075:
> 4076: // Rsrc contains a narrow oop. Thus we are sure the leftmost <oop_shift> bits will never be set.
> 4077: if (maybenullptr) { // null ptr must be preserved!
Consistency
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 4139:
> 4137: // Scale oop and check for null.
> 4138: // Rsrc contains a narrow oop. Thus we are sure the leftmost <oop_shift> bits will never be set.
> 4139: if (maybenullptr) { // null ptr must be preserved!
Consistency
src/hotspot/cpu/s390/macroAssembler_s390.cpp line 5529:
> 5527: // Generated code must not undergo any transformation, e.g. ShortenBranches, to be safe.
> 5528: address MacroAssembler::stop_chain(address reentry, int type, const char* msg, int id, bool allow_relocation) {
> 5529: BLOCK_COMMENT(err_msg("stop_chain(%s,%s): %s {", reentry==null?"init":"cont", allow_relocation?"reloc ":"static", msg));
Glitched patch
src/hotspot/cpu/s390/macroAssembler_s390.hpp line 774:
> 772: // This function calculates the size of the code generated by
> 773: // decode_klass_not_null(register dst)
> 774: // when (Universe::heap() != null). Hence, if the instructions
nullptr
src/hotspot/cpu/s390/macroAssembler_s390.hpp line 785:
> 783: int get_oop_base_complement(Register Rbase, uint64_t oop_base);
> 784: void compare_heap_oop(Register Rop1, Address mem, bool maybenullptr);
> 785: void compare_klass_ptr(Register Rop1, int64_t disp, Register Rbase, bool maybenullptr);
Consistency
src/hotspot/cpu/s390/macroAssembler_s390.hpp line 808:
> 806: Register Rbase = Z_R1, int pow2_offset = -1, bool only32bitValid = false);
> 807: void oop_decoder(Register Rdst, Register Rsrc, bool maybenullptr,
> 808: Register Rbase = Z_R1, int pow2_offset = -1);
Consistency
src/hotspot/cpu/s390/nativeInst_s390.cpp line 388:
> 386: ShouldNotReachHere();
> 387: #endif
> 388: return *(intptr_t *)nullptr;
Unnecessary cast? Also, this is de-referencing a null pointer, so we might have to do this differently after the conversion.
src/hotspot/cpu/s390/runtime_s390.cpp line 118:
> 116: __ z_lgr(Z_SP, saved_sp);
> 117:
> 118: // [Z_RET]!=null was possible in hotspot5 but not in sapjvm6.
nullptr
src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 1046:
> 1044: __ add2reg(rHandle, oop_slot_offset, Z_SP);
> 1045:
> 1046: // If Oop == null, use a null handle.
is
src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 1327:
> 1325: in_ByteSize(-1),
> 1326: in_ByteSize(-1),
> 1327: (OopMapSet *) nullptr);
Unnecessary cast?
src/hotspot/cpu/s390/sharedRuntime_s390.cpp line 2079:
> 2077: __ load_and_test_long(Z_R0_scratch, method_(code));
> 2078: __ z_lg(ientry, method_(interpreter_entry)); // Preload interpreter entry (also if patching).
> 2079: __ z_brne(patch_callsite); // Patch required if code != null (compiled target exists).
nullptr
src/hotspot/cpu/s390/templateInterpreterGenerator_s390.cpp line 1133:
> 1131: // Initialize z_ijava_state->mdx.
> 1132: Register Rmdp = Z_bcp;
> 1133: // native_call: assert that mdo == null
nullptr
src/hotspot/cpu/s390/templateTable_s390.cpp line 4015:
> 4013: __ bind(done);
> 4014: // tos = 0: obj == null or obj is not an instanceof the specified klass
> 4015: // tos = 1: obj != null and obj is an instanceof the specified klass
nullptr
src/hotspot/cpu/s390/templateTable_s390.cpp line 4143:
> 4141: }
> 4142:
> 4143: // Rfree_slot != null -> found one
nullptr
src/hotspot/cpu/s390/vtableStubs_s390.cpp line 85:
> 83:
> 84: const Register rcvr_klass = Z_R1_scratch;
> 85: address npe_addr = __ pc(); // npe == null ptr exception
This reads a bit strange.
src/hotspot/cpu/s390/vtableStubs_s390.cpp line 198:
> 196: // Get receiver klass.
> 197: // Must do an explicit check if offset too large or implicit checks are disabled.
> 198: address npe_addr = __ pc(); // npe == null ptr exception
This also reads a bit strange
-------------
PR Review: https://git.openjdk.org/jdk/pull/12325#pullrequestreview-1295242200
PR Comment: https://git.openjdk.org/jdk/pull/12325#issuecomment-1477543951
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104199478
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104235222
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104235828
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104236459
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104236650
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104236732
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104236820
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104236901
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104237008
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104237153
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104237391
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104238486
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104238696
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104238845
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104239028
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104240066
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104240637
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104240952
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104241157
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104241372
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104241453
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104241530
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104241920
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104242237
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104242375
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104242548
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104245997
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104247150
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104247507
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104247736
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104247948
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104248993
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104249718
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104249975
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104250871
PR Review Comment: https://git.openjdk.org/jdk/pull/12325#discussion_r1104251134
More information about the hotspot-dev
mailing list