Skip to content

Conversation

@hzongaro
Copy link
Member

TR_UnsafeFastPath::perform was explicitly marking the Object argument Node to Unsafe methods as non-null, but that is not correct in general.

This change ensures that the Object argument is set as non-null only for the various JITHelpers.get and JITHelpers.put methods that are used to get or put values from Object fields or array elements.

Note the comment that appears in JITHelpers.java indicates that those methods are to be used in situations where the kind of object that's being operated upon is known and that the JIT expects the object to be non-null.

Fixes: Issue #20106

TR_UnsafeFastPath::perform was explicitly marking the Object argument
Node to Unsafe methods as non-null, but that is not correct in general.

This change ensures that the Object argument is set as non-null only for
the various JITHelpers.get and JITHelpers.put methods that are used to
get or put values from Object fields or array elements.

Signed-off-by:  Henry Zongaro <[email protected]>
@hzongaro
Copy link
Member Author

@jdmpapin, may I ask you to review this fix?

@jdmpapin
Copy link
Contributor

jdmpapin commented Sep 12, 2025

This change is definitely safe, since it just arranges to setIsNonNull(true) in a strict subset of the cases where we used to. The remaining cases are pretty clearly justified. I do wonder if we can't set isObjectPresumedNonNull more often than this, though. Examples follow.

In the other cases where the same switch sets isArrayOperation, I think we could probably be confident that the object will be non-null, though it probably wouldn't matter for StringUTF16.getChar() since we already specially know that String.value is non-null anyway. I didn't see any similar handling for ConcurrentHashMap.table, but maybe it would be better to add such handling than to deal with it in unsafe fast path.

Some of the methods satisfying isKnownUnsafeCaller() could probably still justify isObjectPresumedNonNull. In particular I think the following ones are most likely safe, because they only access fields:

  • java_lang_invoke_StaticFieldVarHandle_StaticFieldVarHandleOperations_OpMethod
  • java_lang_invoke_InstanceFieldVarHandle_InstanceFieldVarHandleOperations_OpMethod
  • java_lang_invoke_StaticFieldGetterHandle_invokeExact
  • java_lang_invoke_StaticFieldSetterHandle_invokeExact
  • java_lang_invoke_FieldGetterHandle_invokeExact
  • java_lang_invoke_FieldSetterHandle_invokeExact

(At the very least, accesses to fields of type boolean, byte, short, and char can be inferred not to be accessing native memory, because if they were, then the access in the generated code would be incorrectly sized to 32-bit.)

We might also want to set the base as being non-null for any reference-typed access, since native memory doesn't contain any reference slots (of which Java code is allowed to be privy to the address).

@hzongaro
Copy link
Member Author

I do wonder if we can't set isObjectPresumedNonNull more often than this, though.

Thanks! I'll mark this as draft for now while I take another look through.

@hzongaro hzongaro marked this pull request as draft September 12, 2025 19:10
@jdmpapin
Copy link
Contributor

jdmpapin commented Sep 12, 2025

We might also want to set the base as being non-null for any reference-typed access, since native memory doesn't contain any reference slots (of which Java code is allowed to be privy to the address).

As evidence for this, observe that the Unsafe API provides a get*(long) for primitive types (except boolean, for some reason), but not for references.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants