RFR (xs) 8075967: Zero interpreter asserts for SafeFetch<32, N> calls in ObjectMonitor
David Holmes
david.holmes at oracle.com
Thu Mar 26 10:42:46 UTC 2015
Hi Thomas,
On 26/03/2015 6:35 PM, Thomas Stüfe wrote:
> 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.
That contract was already implicit - this API is for use after the stubs
have been generated. There's no reason to expand the use of this API to
situations before stubs are generated. So the assert captures that and
CanUseSafeFetch is really unnecessary.
> 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.
No the problem is that SafeFetch doesn't work on ZERO - if you give it a
bad address it won't recover it will crash. The whole point is for it
not to crash. The objectMonitor code is saying "here is a load that we
know may be invalid, but if it is then we don't want to crash, so we use
SafeFetch". The expectation is that SafeFetch actually is safe no matter
what is fetched. The ZERO implementation just does a load and hopes it
isn't asked for anything bad. The objectMonitor code will rarely
actually trigger a bad load. But the newly introduced test does.
So in my opinion to return to the status-quo where ZERO really doesn't
implement SafeFetch safely but it "works okay in practice", we should
disable those explicit tests when running on ZERO.
Whether CanUseSafeFetch (aka is SafeFetchStubAvailable) is implemented
explicitly or implicitly as an assert is a side-issue.
David
> Kind Regards, Thomas
>
>
>
>
>
>
>
> On Thu, Mar 26, 2015 at 8:40 AM, Lindenmaier, Goetz
> <goetz.lindenmaier at sap.com <mailto: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
> <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
> >>>>
> >>
>
>
More information about the hotspot-dev
mailing list