RFR(M) JDK-8031819: Remove legacy jdk checks and code

[David Holmes]

Hi Harold,

New changes look good - thanks!

One minor nit in thread.cpp:

1742     // Invoke java.lang.Thread.dispatchUncaughtException
1743     if (uncaught_exception.not_null()) {
1744       EXCEPTION_MARK;
1745       // Call method Thread.dispatchUncaughtException().

You don't need both comments. :)

Also this seems unused:

1746       KlassHandle recvrKlass(THREAD, threadObj->klass());

Thanks,
David

On 11/06/2014 10:20 AM, harold seigel wrote:
> Hi,
>
> Please review my latest version of the fix for JDK-8031819
> <https://bugs.openjdk.java.net/browse/JDK-8031819>. It contains the
> fixes suggested below by David and Coleen.
>
> The changed files from the previous webrev are:
>
>     classLoader.cpp
>     classLoader.hpp
>     systemDictionary.hpp
>     thread.cpp
>
> The new webrev is at: http://cr.openjdk.java.net/~hseigel/bug_8031819_3/
>
> The latest changes have been tested with the JCK Lang, VM, and hotspot
> jtreg tests.  Additional tests are in progress.
>
> Thanks! Harold
>
> On 6/10/2014 3:45 PM, harold seigel wrote:
>> Hi Coleen,
>>
>> Thanks for your comments.  I'll make the change that you (and Dave)
>> suggest.
>>
>> Harold
>>
>> On 6/10/2014 2:33 PM, Coleen Phillimore wrote:
>>>
>>> On 6/10/14, 1:24 PM, harold seigel wrote:
>>>> Hi David,
>>>>
>>>> Thanks for the comments.  Please see my comments inline.
>>>>
>>>> Thanks, Harold
>>>>
>>>> On 6/10/2014 2:10 AM, David Holmes wrote:
>>>>> Hi Harold,
>>>>>
>>>>> thread.cpp:
>>>>>
>>>>> You don't need the comment regarding the change in method to be
>>>>> called as again there is no context that tells you there was a
>>>>> different method in an earlier release. So this:
>>>>>
>>>>> 1742     // JSR-166: change call from ThreadGroup.uncaughtException to
>>>>> 1743     // java.lang.Thread.dispatchUncaughtException
>>>>>
>>>>> can simply be:
>>>>>
>>>>> 1742     // Invoke java.lang.Thread.dispatchUncaughtException
>>>> Done
>>>>>
>>>>> You are still checking if dispatchUncaughtException exists but that
>>>>> is not necessary so this block can go:
>>>>>
>>>>> 1748  CallInfo callinfo;
>>>>> 1749  KlassHandle
>>>>> thread_klass(THREAD,SystemDictionary::Thread_klass());
>>>>> 1750  LinkResolver::resolve_virtual_call(callinfo, threadObj,
>>>>> recvrKlass, thread_klass,
>>>>> 1751 vmSymbols::dispatchUncaughtException_name(),
>>>>> 1752             vmSymbols::throwable_void_signature(),
>>>>> 1753             KlassHandle(), false, false, THREAD);
>>>>> 1754  CLEAR_PENDING_EXCEPTION;
>>>>> 1755  methodHandle method = callinfo.selected_method();
>>>>> 1756  assert(method.not_null(), "Thread.dispatchUncaughtException()
>>>>> not found");
>>>>>
>>>> I'll remove all of the above except for line 1749. 'thread_klass' is
>>>> needed by the subsequent call to call_virtual().
>>>>> ---
>>>>>
>>>>> A few follow ups on your other responses ...
>>>>>
>>>>> On 10/06/2014 1:03 AM, harold seigel wrote:
>>>>>>>> In classLoader.* is it worth renaming/absorbing the is_rt_jar13 and
>>>>>>>> related "13" items?
>>>>>> Good suggestion, but I think that this is a lot of change without a
>>>>>> significant benefit.
>>>>>
>>>>> The benefit is code clarity, as noone will understand what the 13
>>>>> indicates. It seems to me that the real_jzentry12 and real_jzfile12
>>>>> definitions are now unused. So the "13" variants can just drop the
>>>>> 13 and is_rt_jar13 becomes is_rt_jar.
>>>> I'll rename them and post a new webrev.
>>>>>
>>>>>>>> systemDictionary.hpp:
>>>>>>>>
>>>>>>>> The check_klass_Opt_Only_JDK* functions seem to be unused and only
>>>>>>>> generated assertion failures when they were used.
>>>>>>>>
>>>>>> I was unsure about this change so I did not remove the
>>>>>> check_klass_Opt_Only_JDK* functions.
>>>>>
>>>>> Unsure in what sense - that they are dead code? They seem as dead
>>>>> as other things you are deleting :)
>>>> The check_klass_Opt_Only_JDK* functions cannot be removed because
>>>> they are still being used.  The WK_KLASSES_DO and WK_KLASS_DECLARE
>>>> macros define functions such as reflect_ConstantPool_klass() and
>>>> reflect_MethodAccessorImpl_klass() that call the
>>>> check_klass_Opt_Only_JDK* functions:
>>>>
>>>>    #define WK_KLASSES_DO(do_klass)  \
>>>>                          ...
>>>>       do_klass(reflect_MethodAccessorImpl_klass,
>>>>    sun_reflect_MethodAccessorImpl, Opt_Only_JDK14NewRef) \
>>>>       do_klass(reflect_ConstructorAccessorImpl_klass,
>>>>    sun_reflect_ConstructorAccessorImpl, Opt_Only_JDK14NewRef) \
>>>>       do_klass(reflect_ConstantPool_klass,
>>>>    sun_reflect_ConstantPool,                  Opt_Only_JDK15 ) \
>>>>       do_klass(reflect_UnsafeStaticFieldAccessorImpl_klass,
>>>>    sun_reflect_UnsafeStaticFieldAccessorImpl, Opt_Only_JDK15 ) \
>>>
>>> I think you can change OPT_Only_JDK14NewRef to "Pre" and
>>> Opt_Only_JDK15 to "Opt".  More code to remove!
>>>
>>> Coleen
>>>
>>>> ...
>>>>
>>>>       #define WK_KLASS_DECLARE(name, symbol, option) \
>>>>         static Klass* name() { return
>>>> check_klass_##option(_well_known_klasses[WK_KLASS_ENUM_NAME(name)]);
>>>> } \
>>>>         static Klass** name##_addr() { \
>>>>           return
>>>> &SystemDictionary::_well_known_klasses[SystemDictionary::WK_KLASS_ENUM_NAME(name)];
>>>>
>>>>    \
>>>>         }
>>>>       WK_KLASSES_DO(WK_KLASS_DECLARE);
>>>>
>>>>   declares:
>>>>
>>>>       static Klass* reflect_MethodAccessorImpl_klass { return
>>>>    check_klass_Opt_Only_JDK14NewRef(...);  }
>>>>       static Klass* reflect_ConstructorAccessorImpl_klass { return
>>>>    check_klass_Opt_Only_JDK14NewRef(...);  }
>>>>       static Klass* reflect_ConstantPool_klass() { return
>>>>    check_klass_Opt_Only_JDK15(...); }
>>>>       static Klass* reflect_UnsafeStaticFieldAccessorImpl_klass() {
>>>>    return check_klass_Opt_Only_JDK15(...);  }
>>>>
>>>>>
>>>>> Thanks,
>>>>> David
>>>>
>>>
>>
>