RFR: 8255452: Doing GC during JVMTI MethodExit event posting breaks return oop [v2]

[Serguei Spitsyn]

On Tue, 3 Nov 2020 13:52:24 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

>> Erik,
>> 
>> Thank you for the update! It looks more elegant.
>> 
>> One concern is that after the move of this fragment to the post_method_exit_inner:
>> 1614   if (state == NULL || !state->is_interp_only_mode()) {
>> 1615     // for any thread that actually wants method exit, interp_only_mode is set
>> 1616     return;
>> 1617   }
>> there is no guarantee that the current frame is interpreted below:
>> 1580     if (!exception_exit) {
>> 1581       oop oop_result;
>> 1582       BasicType type = current_frame.interpreter_frame_result(&oop_result, &value);
>>  . . .
>> 1597   if (result.not_null() && !mh->is_native()) {
>> 1598     // We have to restore the oop on the stack for interpreter frames
>> 1599     *(oop*)current_frame.interpreter_frame_tos_address() = result();
>> 1600   }
>> Probably, extra checks for current_frame.is_interpreted_frame() in these fragments will be sufficient.
>
>> Erik,
>> 
>> Thank you for the update! It looks more elegant.
>> 
>> One concern is that after the move of this fragment to the post_method_exit_inner:
>> 
>> ```
>> 1614   if (state == NULL || !state->is_interp_only_mode()) {
>> 1615     // for any thread that actually wants method exit, interp_only_mode is set
>> 1616     return;
>> 1617   }
>> ```
>> 
>> there is no guarantee that the current frame is interpreted below:
>> 
>> ```
>> 1580     if (!exception_exit) {
>> 1581       oop oop_result;
>> 1582       BasicType type = current_frame.interpreter_frame_result(&oop_result, &value);
>>  . . .
>> 1597   if (result.not_null() && !mh->is_native()) {
>> 1598     // We have to restore the oop on the stack for interpreter frames
>> 1599     *(oop*)current_frame.interpreter_frame_tos_address() = result();
>> 1600   }
>> ```
>> 
>> Probably, extra checks for current_frame.is_interpreted_frame() in these fragments will be sufficient.
> 
> That makes sense. Added a check in the latest version that we are in interp only mode.

Hi Erik,

I'm not sure, if this fragment is still needed:
1620   if (state == NULL || !state->is_interp_only_mode()) {
1621     // for any thread that actually wants method exit, interp_only_mode is set
1622     return;
1623   }

Also, can it be that this condition is true:
`  (state == NULL || !state->is_interp_only_mode())`
but the top frame is interpreted?
If so, then should we still safe/restore the result oop over a possible safepoint?

Thanks,
Serguei

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

PR: https://git.openjdk.java.net/jdk/pull/930