RFR: 8336042: Caller/callee param size mismatch in deoptimization causes crash [v2]

[Richard Reingruber]

On Mon, 17 Feb 2025 11:27:17 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

>>> I think you can make the assertion a little stricter like this [reinrich at 9c3c8a3](https://github.com/reinrich/jdk/commit/9c3c8a33a29b9ae6c4c703992b306dc0cbbcd2f0).
>> 
>> Regarding this stricter version, why are you using is_bottom_frame instead of is_top_frame?  The deoptimization code seems to name the most recent leaf frame "top".  That sounds like what frame::top_ijava_frame_abi_size is for too.
>
>> > I think you can make the assertion a little stricter like this [reinrich at 9c3c8a3](https://github.com/reinrich/jdk/commit/9c3c8a33a29b9ae6c4c703992b306dc0cbbcd2f0).
>> 
>> Regarding this stricter version, why are you using is_bottom_frame instead of is_top_frame? The deoptimization code seems to name the most recent leaf frame "top". That sounds like what frame::top_ijava_frame_abi_size is for too.
> 
> Correct, the top frame has a frame::top_ijava_frame_abi but the assertion is about the abi section in the current frame's caller and the the bottom frame's caller also has a top_ijava_frame_abi because i2c doesn't modify it.
> 
> Continue reading if you're interested in more details...
> 
> As said the i2c adapter does *not* trimm the caller frame as the interpreter would,
> replacing its large `top_ijava_frame_abi` with a smaller
> `parent_ijava_frame_abi`.
> 
> 
> 
> Example: compiled frame DEOPTEE is replaced with 3 interpreted frames
> 
> Stack before deoptimization
> 
> |                        |
> | Interpreted CALLER     |
> | of DEOPTEE frame       |
> |                        |
> +------------------------+
> |                        |
> | top_ijava_frame_abi    |
> |                        |
> +========================+
> |                        |
> | Compiled               |
> | DEOPTEE                |
> |                        |
> +------------------------+
> | java_abi               |
> +========================+
> 
> 
> Stack when assertion is checked
> (i.e. after DEOPTEE was replaced by corresponding inter. frames)
> 
> |                        |
> | Interpreted CALLER     |
> | of DEOPTEE frame       |
> |                        |
> +------------------------+
> |                        |
> | top_ijava_frame_abi    | <- i2c keeps large abi
> |                        |
> +========================+
> |                        | <- bottom frame
> | Interpreted Frame 0    |
> | corresp. to DEOPTEE    |
> |                        |
> +------------------------+
> | parent_ijava_frame_abi |
> +========================+
> |                        |
> | Interpreted Frame 1    |
> | (inlined by DEOPTEE)   |
> |                        |
> +------------------------+
> | parent_ijava_frame_abi |
> +========================+
> |                        | <- top frame
> | Interpreted Frame 2    |
> | (inlined by DEOPTEE)   |
> |                        |
> +------------------------+
> |                        |
> | top_ijava_frame_abi    |
> |                        |
> +========================+
> 
> Notes:
> (refering to the frame sections rather than the C++ types)
> 
> - top_ijava_frame_abi comp...

> @reinrich OK, got it! I pushed your change.

Thanks.

> Could you also comment on if we could use the value of sender_sp here instead?

You mean for the calculation of `l2` at L135? sender_sp has room for `Method::max_stack()`. Using it would be less strict.

-------------

PR Comment: https://git.openjdk.org/jdk/pull/23557#issuecomment-2668028520