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

[Matias Saavedra Silva]

On Wed, 22 Mar 2023 13:36:02 GMT, Andrew Dinn <adinn at openjdk.org> wrote:

>> The nulll_check() method is perfectly fine, and complements the work done in the signal handler for segmentation fault in the first page. But my point was not about the method itself, it was about cases where this method is used when it is provable that there's no need to insert an explicit null check because the signal handler is able to handle the case. It happens when the offset is statically known at VM compilation time, and this offset is smaller than a page size: the klass pointer offset and the array's length.
>> But if you consider that the code is safer as written in its current form, I'll close the bug as not-an-issue. Anyway, it doesn't change the code being generated.
>
>> But my point was not about the method itself, it was about cases where this method is used when it is provable that there's no need to insert an explicit null check because the signal handler is able to handle the case.
> 
> Ah, ok, apologies for any confusion. I see your point.
> 
> I have always understood that the presence of, for example, both `load_klass` and `load_klass_null_check` arises because they predate employment of the signal handler/read-protection for page 0. Without that protection it is necessary to do a `load_klass_null_check` on a path where an oop has not previously been null checked but a `load_klass` will suffice on a path where it is known to be non-null. I am not sure that is the actual history but I believe it does characterize the intended use of the two method variants.
> 
> This distinction may be irrelevant given the current state of affairs but it would matter if for some reason we had to decommission the SEGV handler, for example if OpenJDK were ported to an OS which does not support read-protection for page 0.
> 
> I don't suppose we are likely to encounter such a circumstance so it might be reasonable to collapse all calls to `load_klass_null_check` into plain calls to `load_klass`.
> 
> That said, there is no good reason for this PR to modify `load_klass_null_check` to always perform a null check (i.e. the call to `null_check(Register)`). Arguably, the existing call to `null_check(Register, int)` could be removed.

Thank you for the clarification and reviews @adinn @coleenp @dholmes-ora and @fparain!

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

PR Comment: https://git.openjdk.org/jdk/pull/13026#issuecomment-1494274324