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

[Doerr, Martin]

Yes, I'm fine with 8219459.02/webrev.

Best regards,
Martin


-----Original Message-----
From: hotspot-runtime-dev <hotspot-runtime-dev-bounces at openjdk.java.net> On Behalf Of coleen.phillimore at oracle.com
Sent: Montag, 13. Mai 2019 21:43
To: Thomas Stüfe <thomas.stuefe at gmail.com>
Cc: Hotspot dev runtime <hotspot-runtime-dev at openjdk.java.net>
Subject: Re: RFR (S) 8219459: oopDesc::is_valid() is broken



On 5/13/19 3:41 PM, Thomas Stüfe wrote:
> I am fine with it if Martin is. Less code is good.

Thanks, Thomas!
Coleen

>
> Thanks, Thomas
>
> On Mon, May 13, 2019, 21:32 <coleen.phillimore at oracle.com 
> <mailto: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
>     <mailto: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>
>     >> <mailto: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
>     >>
>     >
>