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

Thu Mar 26 08:35:18 UTC 2015

Hi,

I agree with Goetz.

SafeFetch() was meant to be guarded with CanUseSafeFetch(), this is part of
the contract, so using it without this guard is actually incorrect and
would crash were the calling code ever called before Stub routine
initialization.

So now
- CanUseSafeFetch really now means "SafeFetch will behave as excpected"
- SafeFetch is softened to "will safely fetch content of address unless on
zero, there it may crash the VM".

I think this is more confusing than before. In objectMonitor, the logic we
want is "use SafeFetch wherever possible but if not, just access the memory
directly". This logic now is hidden behind a seemingly incorrect use of
SafeFetch without CanUseSafeFetch, and I think Goetz proposal would have
been far more clearer.

Kind Regards, Thomas

On Thu, Mar 26, 2015 at 8:40 AM, Lindenmaier, Goetz <
goetz.lindenmaier at sap.com> wrote:

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