[jdk18] RFR: 8272058: 25 Null pointer dereference defect groups in 4 files

[Daniel D.Daugherty]

On Wed, 22 Dec 2021 17:45:48 GMT, Gerard Ziemski <gziemski at openjdk.org> wrote:

>> A small refactoring to resolve a Parfait complaint about the return value from
>> `MacroAssembler::target_addr_for_insn(address insn_addr, unsigned insn)`
>> on AARCH64. The logic that supports returning `nullptr` as the target addr for
>> a particular instruction is moved from
>> `MacroAssembler::target_addr_for_insn(address insn_addr, unsigned insn)` to
>> `MacroAssembler::target_addr_for_insn_allow_null_ret(address insn_addr, unsigned insn)`.
>> A couple of `target_addr_for_insn()` call sites that can tolerate a `nullptr` are
>> converted to use `target_addr_for_insn_allow_null_ret()`.
>> 
>> This fix has been tested with Mach5 Tier[1-3].
>
> I don't understand the underlying code, but from a big picture point of view, splitting the call into "null not allowed" and "null allowed" makes sense. If the testing came back fine, then that's the best we can do, without asking someone who wrote this code to chime in.
> 
> Just one question: why do we need the "null allowed" version here?
> 
> void NativeMovRegMem::verify() {
>  #ifdef ASSERT
>    address dest = MacroAssembler::target_addr_for_insn_allow_null_ret(instruction_address());
>  #endif
>  }

@gerard-ziemski - Thanks for the review.

According to "git blame", the original author is:

9c458decf5f1 hotspot/src/cpu/aarch64/vm/macroAssembler_aarch64.cpp (Andrew Haley           2015-01-20 11:34:17 -0800  211) address MacroAssembler::target_addr_for_insn(address insn_addr, unsigned insn) {

which I believe is why @dholmes-ora pinged @theRealAPH on this PR...

As for this function:


void NativeMovRegMem::verify() {
#ifdef ASSERT
  address dest = MacroAssembler::target_addr_for_insn_allow_null_ret(instruction_address());
#endif
}


I did not see any harm in converting to `target_addr_for_insn_allow_null_ret`.
The baseline code worked just fine before I refactored it so I'm giving
the function equivalence. In other words, if `NativeMovRegMem::verify()`
was previous called with an `instruction_address()` value that leads to
a nullptr, then baseline `verify()` would work without error. I wanted the
new `verify()` to also work the same.

-------------

PR: https://git.openjdk.java.net/jdk18/pull/51