On Mon, 26 Jan 2026 10:10:07 GMT, Stefan Karlsson <[email protected]> wrote:
>> Joel Sikström has updated the pull request incrementally with four
>> additional commits since the last revision:
>>
>> - Clear exception before setting inline klass
>> - Split assert in field sorting methods
>> - IOOB assert message
>> - InstanceKlass instead of Klass for set_inline_layout_info_klass
>
> src/hotspot/share/classfile/classFileParser.cpp line 6298:
>
>> 6296: if (klass != nullptr) {
>> 6297: if (klass->is_inline_klass()) {
>> 6298: set_inline_layout_info_klass(fieldinfo.index(), klass,
>> CHECK);
>
> I see that this can throw a Metaspace OOME (hence the CHECK) and that we
> later have this comment:
>
>
> // Loads triggered by the LoadableDescriptors attribute are
> speculative, failures must not impact loading of current class
> if (HAS_PENDING_EXCEPTION) {
> CLEAR_PENDING_EXCEPTION;
> }
>
> Can you verify that using CHECK here is really appropriate? The
> `resolve_super_or_fail` uses THREAD, and I think that's what the comment
> refer to. But with the current patch there's a "returning" call in-between
> those two parts of the function.
No, I didn't consider any pending exceptions when I added the CHECK. I see that
we have an `assert(false, ...)` down in the Metaspace allocation if there's a
pending exception, so we really shouldn't try to allocate the array of
InlineLayoutInfo if there's a pending exception, which makes sense.
Looking at the code, I think we should re-arrange it so that we clear any
pending exception before calling `set_inline_layout_info_klass`, while keeping
the `CHECK` so that we exit from `ClassFileParser::post_process_parsed_stream`
if we fail to allocate in Metaspace. This seems to me to be consistent with the
old behavior, but it would be nice for someone to fill in here.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/1966#discussion_r2727341149
