RFR(s): 8161539: 8159666 breaks minimal VM

[Coleen Phillimore]

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
>>>>