RFR: 8241613: Suspicious calls to MacroAssembler::null_check(Register, offset) [v5]

[David Holmes]

On Fri, 24 Mar 2023 18:50:00 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> In several places in HotSpot, the method MacroAssembler::null_check(Register, offset) is called in a way that never produces any null check in the assembly code. The method null_check(Register, offset) calls needs_explicit_null_check(offset) to determine if it must emit a null check in the assembly code or not.
>> 
>> needs_explicit_null_check(offset) returns true only if the offset is negative or bigger than the os page size. 
>> the offset being passed is the offset of a field in the header of Java object or a Java array. In both cases, the offset is always positive and smaller than an os page size. A null_check() call with a single parameter will always produce a null check in assembly.
>> 
>> The cases suggested in the issue have been addressed by either removing or preserving the null_check. Verified with tier 1-3 tests.
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Removed accidental change

Changes requested by dholmes (Reviewer).

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 4319:

> 4317: void MacroAssembler::load_klass(Register dst, Register src) {
> 4318:   assert(oopDesc::klass_offset_in_bytes() < static_cast<intptr_t>(os::vm_page_size()),
> 4319:          "Doesn't need explicit null check");

Sorry I meant for this to be a "global" assert somewhere, executed once per VM startup, not done on each call like this.

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 3581:

> 3579: 
> 3580: void TemplateTable::arraylength() {
> 3581:   assert(arrayOopDesc::length_offset_in_bytes() < static_cast<intptr_t>(os::vm_page_size()),

Ditto: "global" assertion please.

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

PR Review: https://git.openjdk.org/jdk/pull/13026#pullrequestreview-1358241400
PR Review Comment: https://git.openjdk.org/jdk/pull/13026#discussion_r1148728658
PR Review Comment: https://git.openjdk.org/jdk/pull/13026#discussion_r1148728814