RFR(s): 8161539: 8159666 breaks minimal VM

[David Holmes]

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