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

[coleen.phillimore at oracle.com]

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
>>
>