On Tue, 16 Dec 2025 10:35:31 GMT, Stefan Karlsson <[email protected]> wrote:
> The InlineKlassFixedBlock class has the following fields:
>
> class InlineKlassFixedBlock {
> Array<SigEntry>** _extended_sig;
> Array<VMRegPair>** _return_regs;
> address* _pack_handler;
> address* _pack_handler_jobject;
> address* _unpack_handler;
> int* _null_reset_value_offset;
>
>
> with associated functions that all look something like this:
>
> address adr_extended_sig() const {
> assert(_adr_inlineklass_fixed_block != nullptr, "Should have been
> initialized");
> return ((address)_adr_inlineklass_fixed_block) +
> in_bytes(byte_offset_of(InlineKlassFixedBlock, _extended_sig));
> }
>
>
> Given the name of the function you would expect that it returned the address
> of the `_extended_sig` field, and the implementation seems to support this
> observation. So, the type above should be `Array<SigEntry>***`. However,
> callers of this function expects the type to be `Array<SigEntry>**` and casts
> it as such.
>
> This all seems to work because we erase the type by casting to `address` and
> then it doesn't matter what types the `InlineKlassFixedBlock` fields have, as
> long as they are pointer fields.
>
> I propose that we restructure the code to reduce the casting and fix the type
> confusion.
>
> As part of this cleanup I've also:
>
> 1) Moved the initialization of the InlineKlassFixedBlock to the InlineKlass
> constructor.
>
> I think we can pull this even further and move the InlineKlassFixedBlock over
> to inlineKlass.hpp instead of having it in instanceKlass.hpp. And adding a
> proper constructor for this class instead of the explicit set of the fields.
> I've refrained from doing that change in this PR.
>
> 2) Simplified (IMHO) the address calculation of the location of the fixed
> block.
>
> 3) Moved the fixed block access functions together
>
> Alternative:
>
> One alternatives that we could consider is to skip adding the setters because
> they are all used from InlineKlass, which has direct access to the fields of
> the fixed block. Then the code would simply look like this:
>
> fixed_block()._field = value;
>
>
> I'm leaving this open for reviewers nudge what they think is cleanest.
>
> Question:
>
> Has the name "fixed block" some meaning that conveys a concept of value
> classes?
>
> Testing:
>
> tier1-3 on an earlier version, but I've made some tweaks after that and will
> rerun testing.
Thank you for this nice cleanup.
The InlineKlassFixedBlock should be declared in inlineKlass.hpp, but for some
reasons I forgot, it was not possible when initially added, this is why it
ended in instanceKlass.hpp unfortunately. Fixing this oddity in a different CR
is fine.
-------------
Marked as reviewed by fparain (Committer).
PR Review:
https://git.openjdk.org/valhalla/pull/1803#pullrequestreview-3583217768
