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

[David Holmes]

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.

Ok. Only nit is that:

return NOT_ZERO(true) ZERO_ONLY(false);

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