RFR(s): 8161539: 8159666 breaks minimal VM

[Calvin Cheung]

David,

Thanks for your review.

Calvin

On 7/21/16, 6:13 PM, David Holmes 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
>>>>>>>>
>>>>>>
>>>>