RFR (xs) 8075967: Zero interpreter asserts for SafeFetch<32, N> calls in ObjectMonitor

[Coleen Phillimore]

On 3/25/15, 10:15 PM, David Holmes wrote:
> On 26/03/2015 11:00 AM, Coleen Phillimore wrote:
>>
>> On 3/25/15, 8:21 PM, David Holmes wrote:
>>> On 26/03/2015 10:04 AM, Coleen Phillimore wrote:
>>>>
>>>> On 3/25/15, 7:59 PM, David Holmes wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> Why generate stubs that can't be used and then add zero-specific 
>>>>> logic
>>>>> to the shared CanUseSafeFetchN instead of not generating the stubs so
>>>>> that CanUseSafeFetchN will return false anyway ??
>>>>
>>>> Because there is platform independent code in objectMonitor.cpp that
>>>> uses SafeFetchX (both).  I'd rather not burden the caller of this with
>>>> testing CanUseSafeFetchX for each call.   This code existed before
>>>> SafeFetch was implemented, so I'm restoring previous behavior for 
>>>> Zero.
>>>
>>> Sorry Coleen I thought this was deal to with 8075533, I hadn't
>>> realized 8075533 broke the objectMonitor code and this was a follow up.
>>>
>>> What a mess. :(
>>
>> Yes, it is.  Is this a code Review?
>
> Let me work through this :) The original change for 8074552:
>
> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/3eb61269f421
>
> added the extra error handling stuff related to SafeFetch and a new 
> ErrorHandler test that was predicated on CanUseSafeFetch32 which 
> returns true if the stubs exist. It also added testing of SafeFetch in 
> StubRoutines::initialize2 which was excluded for windows-32-bit and 
> which did not check CanUseSafeFetch32.
>
> That change broke zero as reported in 8075533 because you can't 
> actually use the SafeFetch routines with an unsafe address on Zero, 
> and the solution:
>
> http://hg.openjdk.java.net/jdk9/hs-rt/hotspot/rev/5c2bc6278fc4
>
> was to remove the stubs so that CanUseSafeFetch32 would return false, 
> and to include a check of CanUseSafeFetch32 in the tests used with 
> StubRoutines::initialize2.
>
> And now we find that the SafeFetch routines are used unconditionally 
> by the objectMonitor code, so we need to restore the stubs that were 
> previously removed, but force CanUseSafeFetch32 to return false on zero.

Yes, exactly.
>
> Ok. Only nit is that:
>
> return NOT_ZERO(true) ZERO_ONLY(false);

Yes, you're right.  I'll make that change.

Thanks!
Coleen

>
> would be more readable than the ifdefs.
>
> Thanks,
> David
>
>> thanks!
>> Coleen
>>>
>>> Thanks,
>>> David
>>>
>>>
>>>
>>>> We could file an RFE to either implement SafeFetch for Zero or rewrite
>>>> this objectMonitor code to not need SafeFetch.  I didn't want to do
>>>> either of these things with this patch.
>>>>
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>>
>>>>> On 26/03/2015 3:53 AM, Coleen Phillimore wrote:
>>>>>> Summary: Implement SafeFetchX unsafely and make CanUseSafeFetchX 
>>>>>> false
>>>>>> for Zero
>>>>>>
>>>>>> Also, this fixes a couple other minor issues.
>>>>>>
>>>>>> Ran jdk9 jck tests with one timeout.  hotspot/runtime jtreg tests
>>>>>> don't
>>>>>> run because Zero doesn't support UseCompressedOops (not sure why) 
>>>>>> and
>>>>>> CDS (know why).
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8075967/
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8075967
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>