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

[Frederic Parain]

On Thu, 16 Mar 2023 20:02:56 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:
> 
>   load_klass now emits null check and other platforms supported

Regarding the motivation of this CR: I filled this RFE because I found the code confusing. When null_check() is called with an offset argument being klass_offset_in_bytes() and length_offset_in_bytes(), it *never* emits an explicit null check. And using this method, or methods calling it might give a false sense of security. For instance, on x86, MacroAssembler::load_klass() and MacroAssembler::load_klass_check_null() emit exactly the same code. So why having two methods, one sounding safer than the other, when in fact, they are doing the same job.
I don't think null checks are missing in the code, implicit null checks are doing a really good job, but when I filled the bug, I didn't have time to check that it was the case for all locations where it was statically known that null_check() would not produce an explicit null check.
So the motivation is a code cleanup, and not fixing some missing null checks.

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

PR: https://git.openjdk.org/jdk/pull/13026