RFR(s): 8161539: 8159666 breaks minimal VM

[Calvin Cheung]

Thanks Coleen.
I'll check-in the fix soon.

Calvin

On 7/21/16, 6:26 PM, Coleen Phillimore wrote:
> Agree. This looks good.  Thanks for bearing with us!
>
> Coleen
>
> Sent from my iPhone
>
>> On Jul 21, 2016, at 9:13 PM, David Holmes<david.holmes at oracle.com>  wrote:
>>
>> Looks good! Thanks Calvin.
>>
>> David
>>
>>> On 22/07/2016 9:54 AM, Calvin Cheung wrote:
>>> Based on discussions in the bug report.
>>> Here's an updated webrev:
>>> http://cr.openjdk.java.net/~ccheung/8161539/webrev.02/
>>>
>>> It replaces the shouldNotReachHere() with an assert.
>>>
>>> thanks,
>>> Calvin
>>>
>>>> On 7/21/16, 3:00 PM, David Holmes wrote:
>>>> I'm going to wait to see what form this takes before additional
>>>> commenting beyond what is in the bug report. But a
>>>> shouldNotReachHere() implementation is/was not the intended way to
>>>> handle these conditionals. Either dummy functions are present and
>>>> called, or else no functions and the calls are guarded - not some
>>>> hybrid mix.
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>> On 22/07/2016 7:05 AM, Coleen Phillimore wrote:
>>>>>
>>>>>
>>>>>> On 7/21/16 4:49 PM, Calvin Cheung wrote:
>>>>>>
>>>>>>
>>>>>>> On 7/21/16, 1:19 PM, Coleen Phillimore wrote:
>>>>>>>
>>>>>>>
>>>>>>>> On 7/21/16 3:44 PM, Calvin Cheung wrote:
>>>>>>>> Hi Coleen,
>>>>>>>>
>>>>>>>>> On 7/21/16, 12:32 PM, Coleen Phillimore wrote:
>>>>>>>>> Hi Calvin,
>>>>>>>>>
>>>>>>>>> I really don't like these conditionals.
>>>>>>>>>
>>>>>>>>> Why take the functions out of the #else in instanceKlass.hpp?
>>>>>>>> The idea is to catch this kind of problem during build time, not
>>>>>>>> during runtime.
>>>>>>> If you want to catch this as buildtime, then take
>>>>>>> set_cached_class_file() out of the #else part and do Chris's
>>>>>>> suggested fix (which is a smaller version of Ioi's fix).
>>>>>> If we're leaving the other functions in the #else part, I think we
>>>>>> should leave the set_cached_class_file() there as well.
>>>>>> I think Chris' suggestion on using the JVMTI_ONLY macro is sufficient.
>>>>>> What do you think?
>>>>> Yes, looking at this again, I think Chris's change is fine.  You have to
>>>>> leave the definition of this in and cannot get a compile time error.
>>>>>
>>>>> Coleen
>>>>>
>>>>>> thanks,
>>>>>> Calvin
>>>>>>
>>>>>>
>>>>>>
>>>>>>> We really don't want #ifdefs sprinkled around the vm for the minimal
>>>>>>> vm.  Especially not in ClassLoaderData.cpp.
>>>>>>>
>>>>>>> Coleen
>>>>>>>
>>>>>>>> thanks,
>>>>>>>> Calvin
>>>>>>>>> thanks,
>>>>>>>>> Coleen
>>>>>>>>>
>>>>>>>>>> On 7/21/16 3:23 PM, Gerard Ziemski wrote:
>>>>>>>>>> hi Calvin.
>>>>>>>>>>
>>>>>>>>>> Looks good, (r)eviewed.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> cheers
>>>>>>>>>>
>>>>>>>>>>> On Jul 21, 2016, at 2:02 PM, Calvin Cheung
>>>>>>>>>>> <calvin.cheung at oracle.com>  wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Summary of change:
>>>>>>>>>>> - removes the dummy jvmti function bodies in instanceKlass.hpp;
>>>>>>>>>>> - put "#if INCLUDE_JVMTI" guards at the call sites of those
>>>>>>>>>>> functions.
>>>>>>>>>>>
>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8161539
>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~ccheung/8161539/webrev.00/
>>>>>>>>>>>
>>>>>>>>>>> It passed JPRT and "tier-2" testing.
>>>>>>>>>>> It also passed minimal vm build.
>>>>>>>>>>>
>>>>>>>>>>> thanks,
>>>>>>>>>>> Calvin