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

[Roman Kennke]

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

I am pretty sure that the changes are not quite right. needs_explicit_null_check() checks whether we must emit an *explicit* null-check, or else if we could go with an *implicit* null-check. Implicit null checks plant some debug info (iirc) and don't emit any actual code to do the null-check. So what you see here, no code being emitted for null-check, is not necessarily wrong. Instead, it lets the SIGSEGV happen and then the fault-handler will kick in and determine what to do with it, usually throwing an NPE. What the change does is basically turn a bunch of implicit null checks into explicit null-checks. At the very least, this is less efficient than doing implicit null-checks. And this being in load_klass() I suspect the impact could be quite significant.

The part that I am missing is the motivation for the changes. Have you observed situations where we don't throw an NPE where we should, and crash with a SEGV instead? This would be what I expect when a null-check is really missing in the sense that not even implicit null-checks work.

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

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