[lworld] RFR: 8375441: [lworld] C2: assert(is_instance()) failed: bad cast [v2]

Mon Feb 2 16:38:57 UTC 2026

On Wed, 28 Jan 2026 09:19:18 GMT, Marc Chevalier <mchevalier at openjdk.org> 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