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

[Robbin Ehn]

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.

> 
>> - 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.

> 
>> - 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.
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.

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