RFR(s): 8228758: assert(_no_handle_mark_nesting == 0) failed: allocating handle inside NoHandleMark

[David Holmes]

Hi Robbin,

On 3/09/2019 7:46 pm, Robbin Ehn wrote:
> Hi David,
> 
> On 9/2/19 7:03 AM, David Holmes wrote:
>> Hi Robbin,
>>
>> On 30/08/2019 4:56 pm, Robbin Ehn wrote:
>>> Hi David, thanks for having a look!
>>>
>>> On 2019-08-30 03:24, David Holmes wrote:
>>>> Hi Robbin,
>>>>
>>>> On 29/08/2019 10:48 pm, Robbin Ehn wrote:
>>>>> Hi all, please review.
>>>>>
>>>>> The JNI_QUICK_ENTRY/JVM_QUICK_ENTRY skips creating the 
>>>>> HandleMarkCleaner, instead they have a NoHandleMark. Handshakes can
>>>>> be executed by the JavaThread itself, thus might require a
>>>>> HandleMark. QUICK entries allow polls for safepoint/handshakes, if a
>>>>> handshake is executed and a HandleMark is created we hit this
>>>>> assert.
>>>>>
>>>>> HandleMarkCleaner have previous been optimized for inlining, thus 
>>>>> cheap.
>>>>> Performance benchmarks shows no benefit of not allowing HandleMark.
>>>>>
>>>>> This changeset removes the the QUICK entry and avoids this issue.
>>>>>
>>>>> Note that JNI_QUICK_ENTRY also don't have WeakPreserveExceptionMark,
>>>>> so it's changed to JNI_ENTRY_NO_PRESERVE.
>>>>
>>>> Why did you extend this to the JNI_QUICK_ENTRY case when the 
>>>> impacted code was only a JVM_QUICK_ENTRY? The expanded scope of the 
>>>> change seems unnecessary given this is just a workaround for a 
>>>> particular failure mode.
>>>
>>> - If your code require you not to use any HandleMarks it most likely 
>>> require no polls, for correctness. JRT_LEAF does this correctly, sets 
>>> up both NoSafepointVerifier and NoHandleMark. (with the HandleMark in 
>>> handshake we now always require NoSafepointVerifier if you use 
>>> NoHandleMark for correctness)
>>
>> I think the distinction between QUICK and LEAF is very murky. But you 
>> seem to be suggesting an equivalence that would then lead to 
>> replacement of QUICK entry with a LEAF entry ??
> 
> No, since quick do not need NoHandleMark for correctness.

You stated that if you need to not use any handleMarks then you most 
likely don't want polls - and no polls is what a LEAF routine has to 
guarantee:

QUICK -> No handles -> no polls == LEAF

>>
>>> - There is no performance benefit, so we just have extra code which 
>>> is useless.
>>
>> I tried to get some background on the QUICK_ENTRY and why the 
>> HandleMark removal made it "quick", but this is day one code from Oct 
>> 1998 and there is no explanation I can find. I also find the comments 
>> a bit misleading:
>>
>> // QUICK_ENTRY routines behave like ENTRY but without a handle mark
>>
>> This suggests that having a HandleMark somehow slows things down so it 
>> is removed. But it doesn't just remove the HandleMark (or these days 
>> HandleMarkCleaner) from the entry, it also adds a NoHandleMark, which 
>> is asserting quite a different constraint.
> 
> Since we missing the cleaner, accidentally using a HandleMark would lead 
> to a leak. The NoHandleMark protected against this leak.

Maybe I'm misunderstanding exactly how HandleMarks nest and behave. I 
would have thought this nesting would be allowed:

entry_point
   NoHandleMark
   -> subroutine
      HandleMark  // prevents leak of Handle to outer scope
      allocate handle

whereas this would cause an error

entry_point
   NoHandleMark
   -> subroutine
      allocate handle

but IIUC a NoHandleMark prevents all and any handle use in its own scope 
or a nest scope - no?

>>
>>> - It might be affected, I have not track down every path. But even if 
>>> it's
>>> not, nothing is stopping someone from accidentally changing the code 
>>> to call
>>> something which does a poll, since we don't have NoSafepointVerifier.
>>> So we avoid getting the same failure mode in future.
>>
>> I could argue that it would be useful to see such a failure during 
>> testing of such a change to realize that it doesn't in fact introduce 
>> a poll path.
>>
>>> - Reducing the complexity of interafceSupport is always good!
>>
>> Can't argue there :)
>>
>>> - There was no reason to keep it.
>>>
>>> So if you don't a very strong opinion about it, I really like to get 
>>> rid of it.
>>
>> I don't have a strong enough opinion to fight to keep it.
> 
> Great!
> 
>>
>> But I remain very concerned that the true problem here is that 
>> handshakes can now lead to executing code that violates the 
>> expectations of code at the poll site. We used to know exactly what 
>> code could be executed in a given path, but now it will expand to 
>> include the set of in-thread handshake operations that may be 
>> possible. For now that seems restricted to biased-locking revocation, 
>> but the use of Handshakes is set to expand.
> 
> Yes, now a thread doing a polls needs to be able to execute 'any' VM code.

I'm not sure that is viable, or the right way to try and address this. 
In the extreme it would require all VM susbsystems to be fully 
re-entrant and that is not a reasonable expectation.

> This changes the polling model a bit and we have not proper look at this.
> Maybe we should add the constraints to the poll and to find any potential
> troublesome polls.

I'm not sure how we would discover the problems until we have a 
handshake that executes some code that violates a constraint. The 
constraints themselves are not obvious. Subsystems do not clearly 
identify themselves as being reentrant or not - most things are not 
reentrant by their nature.

Cheers,
David

> Thanks, Robbin
> 
>>
>> Thanks,
>> David
>> -----
>>
>>>
>>> Thanks, Robbin
>>>
>>> Some extra words about NoHandleMark:
>>> I believe NoHandleMark should be removed.
>>> In the LEAF case, there is no point in creating a Handle within a
>>> NoSafepointVerifier, since we don't safepoint.
>>> The other usecase seems to be, I'm in java don't do VM things, and 
>>> someone tried
>>> to protected that with a NoHandleMark. There many ways to create a 
>>> much more
>>> robust check for that.
>>> But a discussion for another time.
>>>
>>>
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> Issue:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8228758
>>>>>
>>>>> Changeset:
>>>>> http://cr.openjdk.java.net/~rehn/8228758/webrev/
>>>>>
>>>>> Passes t1-3 (and performance benchmarks)
>>>>>
>>>>> Thanks, Robbin