Request for review 8000725: NPG: method_holder() and pool_holder() and _pool_holder field should be InstanceKlass

[Coleen Phillimore]

The reason that we had InstanceKlass::cast()s all over the place is 
because with Permgen, almost everything was passed around as an "oop".  
The type klassOop was inherited from oop.    We needed these asserts 
because things, like during GC and in this other JNI bug you reference, 
would cast down to oop and back up to klassOop frequently.  Since these 
are now distinct C++ types, this casting should be completely gone in 
the repository.  If you see any casts between "oop" and "klassOop" it is 
a bug and should be fixed.   There is one case in CMS we do something 
like this but it's a special case and doesn't escape this context.

Because we are using C++ types now for metadata, the compiler can 
statically check the code.   Of course, you can write code like Klass* k 
= (Klass*)5; but having Klass::cast(k) isn't going to be useful to see 
if k->is_Klass().    Having types check that they are really the type 
declared would make for really messy code, and in fact, Harold's next 
webrev is a really nice cleanup.

Actually Klass is inherited from Metadata so is_Klass() is still useful 
for the cases where this is mixed.    If we keep Klass::cast() we could 
use it for when the argument is (Metadata*), and continue to remove it 
from places where the type is already Klass*.   I don't think this is 
worth doing because there are only a few of these cases in about 3 
places.   They already have casts and checks so it would be redundant 
there too.

Coleen

On 11/5/2012 1:37 AM, David Holmes wrote:
> Hi Harold,
>
> On 3/11/2012 4:10 AM, harold seigel wrote:
>> I don't see any value in the below assert.  The compiler would have
>> already ensured that any input argument is already of type Klass.  If
>> that isn't true then we would need to add asserts to every method
>> verifying that its input arguments are really the type that they are
>> declared to be.
>
> The assert does not check the type of the argument, it checks whether 
> the argument's is_Klass() method returns true. They are not the same 
> thing (you can pretty much cast anything to Klass* and pass it to 
> Klass::cast and the compiler will not complain).
>
> On the surface I agree with you that this seems redundant. But the 
> very fact this code is there tells me that there is more at play here 
> than meets the eye. See:
>
> https://jbs.oracle.com/bugs/browse/JDK-6992035
> https://jbs.oracle.com/bugs/browse/JDK-4802822
>
> for cases where this assert failed. And there are others.
>
> As the saying goes "never take down a fence until you understand why 
> it was put up in the first place".
>
> Cheers,
> David
> ------
>
>>
>>     static Klass* cast(Klass* k) {
>>     *  assert(k->is_klass(), "cast to Klass"); *
>>        return k;
>>     }
>>
>> For the asserts in InstanceKlass::cast(), note that the call to
>> set_pool_holder() (from method.cpp) calls InstanceKlass::cast().  So the
>> asserts will get exercised.  The C++ compiler statically checks the rest
>> of the cases.
>>
>> Thanks, Harold
>>
>> On 10/29/2012 10:55 PM, David Holmes wrote:
>>> On 29/10/2012 10:38 PM, harold seigel wrote:
>>>> Hi David,
>>>>
>>>> We will have a webrev for review later this week that will remove the
>>>> Klass::cast() function and all its uses (see bug 8001471). We 
>>>> thought it
>>>> would be easier to just remove the unnecessary InstanceKlass::cast()
>>>> calls in this webrev (along with a few Klass::cast() calls).
>>>
>>> Can you comment on the loss of asserts and the failure modes those
>>> asserts were covering?
>>>
>>> David
>>>
>>>> Thanks for reviewing it!
>>>>
>>>> Harold
>>>>
>>>> On 10/27/2012 4:02 AM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> On 27/10/2012 4:11 AM, harold seigel wrote:
>>>>>> Please review the following changes.
>>>>>>
>>>>>> Summary: Change the function return type of method_holder(),
>>>>>> field_holder(), and pool_holder() to InstanceKlass. Also, change the
>>>>>> type of field _pool_holder to InstanceKlass. These changes 
>>>>>> enabled the
>>>>>> removal of lots of InstanceKlass::cast() calls from the code.
>>>>>
>>>>> There have also been a lot of removals of Klass::cast even though the
>>>>> previous return types were Klass*. So looking at Klass::cast I see
>>>>>
>>>>> // Casting
>>>>> static Klass* cast(Klass* k) {
>>>>> assert(k->is_klass(), "cast to Klass");
>>>>> return k;
>>>>> }
>>>>>
>>>>> which begs the question: how can k->is_klass() not be true? And have
>>>>> we lost anything by getting rid of these "casts"? There are 
>>>>> additional
>>>>> checks in instanceKlass::cast too
>>>>>
>>>>> // Casting from Klass*
>>>>> static InstanceKlass* cast(Klass* k) {
>>>>> assert(k->is_klass(), "must be");
>>>>> assert(k->oop_is_instance(), "cast to InstanceKlass");
>>>>> return (InstanceKlass*) k;
>>>>> }
>>>>>
>>>>> Is the need for the asserts obsolete in a NPG world?
>>>>>
>>>>> That aside in vframe.cpp:
>>>>>
>>>>> + InstanceKlass* k = m->method_holder();
>>>>> tty->print_cr("frame( sp=" INTPTR_FORMAT ", unextended_sp="
>>>>> INTPTR_FORMAT ", fp=" INTPTR_FORMAT ", pc=" INTPTR_FORMAT ")",
>>>>> _fr.sp(), _fr.unextended_sp(), _fr.fp(), _fr.pc());
>>>>> tty->print("%s.%s", Klass::cast(k)->internal_name(),
>>>>> m->name()->as_C_string());
>>>>>
>>>>> there is a Klass::cast that can be removed.
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>>
>>>>>> This webrev has a lot of files but most of the changes are small.
>>>>>>
>>>>>> Open webrev at http://cr.openjdk.java.net/~coleenp/bug_8000725
>>>>>> <http://cr.openjdk.java.net/%7Ecoleenp/bug_8000725>
>>>>>>
>>>>>> Bug link at http://bugs.sun.com/view_bug.do?bug_id=8000725
>>>>>>
>>>>>> The changes were tested with JPRT, JCK, and other tests including 
>>>>>> ones
>>>>>> for sa-jdi.
>>>>>>
>>>>>> Thanks, Harold