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

[David Holmes]

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