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

[coleen.phillimore at oracle.com]

Thanks, Martin!
Coleen

On 5/14/19 7:57 AM, Doerr, Martin wrote:
> 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
>>      >>
>>      >
>>