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

[Gerard Ziemski]

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:
> 
> [9c458de](https://github.com/openjdk/jdk18/commit/9c458decf5f1ed468639de57f9f34adab98a593d) 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.

As long as parfait doesn't complain here - I didn't see where we checked for NULL explicitly in this case.  I assume that parfait skips the ASSERT parts?

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

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