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

[coleen.phillimore at oracle.com]

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
>