On Wed, 28 Jan 2026 09:19:18 GMT, Marc Chevalier <[email protected]> wrote:
>> Some code added by >> [JDK-8372700](https://bugs.openjdk.org/browse/JDK-8372700) can compute the >> constant value of a field of a (flatten) element in a flat array. We get a >> crash when the element of the array is known to be `null`, and so the field >> doesn't exist. >> >> So, let's just check in `ciConstant ciFlatArray::field_value(int index, >> ciField* field)` whether we get a null constant before interpreting it as a >> `ciInstance` and trying to retrieve a field from there. This should be >> enough since a `ciObject` is (directly) derived by `ciNullObject`, >> `ciInstance` and `ciArray`. Since we are looking up a value of a flat array, >> an element cannot be a `ciArray` (arrays have identities and can't be >> contained in a flat array). After looking up whether the flat array element >> is null, the `obj->as_instance()` cast acts as an assert, should we ever add >> another derived class from `ciObject`. >> >> In case of a null array element, `field_value` simply returns an invalid >> `ciConstant`. >> >> Tested with >> tier1,tier2,tier3,hs-precheckin-comp,hs-comp-stress,valhalla-comp-stress. >> Looks good. >> >> Thanks, >> Marc > > Marc Chevalier has updated the pull request incrementally with 10 additional > commits since the last revision: > > - Details > - exception handling > - oops > - Also cache correctly whole stable fields > - fix comment > - Remove debug > - ShouldNotReachHere > - explicit casts > - Clean up > - expand_constant Thanks a lot for working on this, I have a couple of comments. src/hotspot/share/ci/ciInstance.cpp line 86: > 84: InstanceKlass* holder = InstanceKlass::cast(obj->klass()); > 85: int index = -1; > 86: for (JavaFieldStream fs(holder); !fs.done(); fs.next()) { My latest PR should include a `_layout_kind` in the `ciField` object, can you use it? src/hotspot/share/ci/ciInstance.cpp line 95: > 93: InlineLayoutInfo* layout_info = > holder->inline_layout_info_adr(index); > 94: InlineKlass* vk = layout_info->klass(); > 95: oop res = vk->read_payload_from_addr(obj, offset, > layout_info->kind(), THREAD); It seems to imply that this access is required to be atomic, what if it is a non-atomic field? src/hotspot/share/ci/ciInstance.cpp line 137: > 135: // desired field. If we want a sub-field of a flat field, we then > extract the field > 136: // out of the cached copy. > 137: ciConstant ciInstance::field_value(ciField* field) { Similarly, what kind of field is expected here, is it a declared field, or a primitive subfield? src/hotspot/share/ci/ciInstance.cpp line 144: > 142: ciInstanceKlass* klass = this->klass()->as_instance_klass(); > 143: ciField* containing_field; > 144: if (field->original_holder() == nullptr) { Is this branch necessary, I thought `klass->field_index_by_offset` should give the correct result if `field` is not a subfield? src/hotspot/share/ci/ciInstance.cpp line 154: > 152: return ciConstant(); > 153: } > 154: if (field->original_holder() == nullptr) { I think simply comparing `field` and `containing_field` is enough. src/hotspot/share/ci/ciInstance.cpp line 175: > 173: // Extract a field from a value object. > 174: // This won't cache. Must be used only on cached values. > 175: ciConstant ciInstance::non_flat_field_value(ciField* field) { Can you specify which kind of `field` is required here. I would assume this is a declared field that is not a flat field. src/hotspot/share/ci/ciInstanceKlass.cpp line 440: > 438: } > 439: > 440: int ciInstanceKlass::field_index_by_offset(int offset) { Why is assert needs removing? src/hotspot/share/opto/compile.cpp line 2106: > 2104: ciInstance* loaded_from = nullptr; > 2105: if (FoldStableValues) { > 2106: const TypeOopPtr* base_type = > igvn.type(loadn->base())->isa_oopptr(); Normally, `isa_...` methods are only used if we want to null check it, otherwise, use the corresponding `is_...` method. src/hotspot/share/opto/compile.cpp line 2125: > 2123: } else if (oop != nullptr && oop->is_array()) { > 2124: ciArray* array = oop->as_array(); > 2125: ciConstant elt = array->element_value_by_offset(off); IIUC, `off` can be `Type::OffsetBot` here, I think it would be better to handle it here because it is an implementation detail that should not be visible to `ci`. ------------- PR Review: https://git.openjdk.org/valhalla/pull/1923#pullrequestreview-3740519059 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755233617 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755235862 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755208953 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755222711 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755226019 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755200089 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755239944 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755166332 PR Review Comment: https://git.openjdk.org/valhalla/pull/1923#discussion_r2755181723
