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

[David Holmes]

On Thu, 30 Mar 2023 16:24:16 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:
> 
>   Added global assert

One nit but otherwise thumbs up from me. Thanks for your patience on this one.

src/hotspot/share/memory/universe.cpp line 321:

> 319:   HandleMark   hm(THREAD);
> 320: 
> 321:   // Explicit null checks are needed if these offsets are larger than the page size

Nit: the condition checks smaller, but the comment says larger, which leaves "equal to the page size" unclear leading the reader to wonder "Is the comment wrong or the condition check?".

Suggestion: s/larger/not smaller/

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

Marked as reviewed by dholmes (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/13026#pullrequestreview-1366183638
PR Review Comment: https://git.openjdk.org/jdk/pull/13026#discussion_r1153963122