RFR (S) 8219459: oopDesc::is_valid() is broken

[Thomas Stüfe]

I am fine with it if Martin is. Less code is good.

Thanks, Thomas

On Mon, May 13, 2019, 21:32 <coleen.phillimore at oracle.com> wrote:

>
> http://cr.openjdk.java.net/~coleenp/2019/8219459.02/webrev/index.html
>
> I think this is simplest.  If we want to reintroduce
> is_range_in_committed to conditionally lock for other uses, we should do
> it when there's code that needs it.
>
> thanks,
> Coleen
>
> On 5/13/19 3:13 PM, coleen.phillimore at oracle.com wrote:
> >
> > Hi Thomas,
> >
> > On 5/13/19 2:07 PM, Thomas Stüfe wrote:
> >> Hi Coleen, Martin,
> >>
> >> I'm also fine with this patch. I agree this function is a bit
> >> overkillish.
> >>
> >> Also, in Klass::is_valid(), we do:
> >>
> >> if (!MetaspaceUtils::is_range_in_committed(k, k + 1)) return false;
> >>
> >> which is strict but then again not really correct, since we only do
> >> test range of the base Klass structure, excluding v/itable. We should
> >> probably check with k->size() range instead.
> >
> > I had a tested version that looked like this:
> >
> >   // Should only call unlocked Metaspace::contains when error is
> > reported or Debugging
> >   // but Shenandoah calls this before asserting.
> >   // Lots of verifications call Metaspace::contains without the
> > MetaspaceExpand_lock.
> >   if (!Metaspace::contains(k) ||
> > !Metaspace::contains(k+(k->size()-1))) return false;
> >
> >
> >>
> >> Or, just do a Metaspace::contains() with the Klass pointer. I think
> >> the cases where it points to valid Metaspace but its end does not are
> >> probably rare.
> >
> > In the end, I wanted to go with the minimal change to get is_valid()
> > working again, so that was what I changed.
> >
> > I could just take out is_range_in_committed() and have the line be if
> > (!Metaspace::contains(k)) return false;
> >
> > for extra simplicity.  This oopDesc::oop_or_null (vs. is_oop_or_null)
> > is only called during error reporting, so it doesn't need the
> > MetaspaceExpand_lock, unless very rarely during crashing we're doing
> > Full GC and shrinking the metaspace.
> >
> > Thanks,
> > Coleen
> >
> >
> >
> > Coleen
> >
> >>
> >> Cheers, Thomas
> >>
> >>
> >> On Wed, May 8, 2019 at 11:45 PM <coleen.phillimore at oracle.com
> >> <mailto:coleen.phillimore at oracle.com>> wrote:
> >>
> >>     Summary: Use Metaspace::contains() to test address ranges.
> >>
> >>     Tested with hs-tier1-3, and all tier1 on linux-x64-debug. Tested
> >>     manually in gdb.
> >>
> >>     open webrev at
> >>     http://cr.openjdk.java.net/~coleenp/2019/8219459.01/webrev
> >>     bug link https://bugs.openjdk.java.net/browse/JDK-8219459
> >>
> >>     Thanks,
> >>     Coleen
> >>
> >
>
>