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

[Lindenmaier, Goetz]

Hi,

sorry I only comment on this now ...
I think it's rather strange that you can not use SafeFetch on zero, but it's there, 
and it's actually used ... which will be the situation after this change.

Why not just change objectMonitor as the SafeFetch api is meant to be used?
Obviously zero takes care that this access always succeeds:

diff -r a35854c84d9d src/share/vm/runtime/objectMonitor.cpp
--- a/src/share/vm/runtime/objectMonitor.cpp    Wed Mar 25 12:36:28 2015 +0100
+++ b/src/share/vm/runtime/objectMonitor.cpp    Thu Mar 26 08:19:53 2015 +0100
@@ -2241,7 +2241,12 @@
   }

   assert(sizeof(((JavaThread *)ox)->_thread_state == sizeof(int)), "invariant");
-  int jst = SafeFetch32((int *) &((JavaThread *) ox)->_thread_state, -1);;
+  int jst;
+  if (CanUseSafeFetch32()) {
+    jst = SafeFetch32((int *) &((JavaThread *) ox)->_thread_state, -1);
+  } else {
+    jst = ((JavaThread *)ox)->_thread_state;
+  }
   // consider also: jst != _thread_in_Java -- but that's overspecific.
   return jst == _thread_blocked || jst == _thread_in_native;
 }

It's handled similarly in vmError.cpp.  And no #ifdefs ;)

But I'm also fine with the other change ... to get zero working again.

Best regards,
  Goetz.



-----Original Message-----
From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
Sent: Thursday, March 26, 2015 3:30 AM
To: David Holmes; hotspot-dev developers
Subject: Re: RFR (xs) 8075967: Zero interpreter asserts for SafeFetch<32, N> calls in ObjectMonitor


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