RFR (S): 8230395: Code checks for NULL value returned from NEW_C_HEAP_ARRAY which can not happen

[David Holmes]

Adding hotspot-jfr-dev list as I just realized this os_perf code belongs 
to JFR.

I filed:

https://bugs.openjdk.java.net/browse/JDK-8231271

as a follow up, but the logic was changed to always return true in JDK 11.

David

On 20/09/2019 7:39 am, David Holmes wrote:
> (adding back hotspot-compiler-dev)
> 
> Hi Harold,
> 
> On 20/09/2019 12:14 am, Harold Seigel wrote:
>> Hi David,
>>
>> The change looks good.
> 
> Thanks for taking a look.
> 
>> Can CPUInformationInterface::initialize() be changed to return void?  
>> It looks like it will now always returns TRUE.
> 
> It currently appears that way, however:
> 
> 1. There is a common pattern for all these XXXInterface classes to 
> include a "bool initialize()" method. This allows them to all be handled 
> consistently e.g. the JFR code has this:
> 
> template <typename T>
> static T* create_interface() {
>    ResourceMark rm;
>    T* iface = new T();
>    if (iface != NULL) {
>      if (!iface->initialize()) {
>        delete iface;
>        iface = NULL;
>      }
>    }
>    return iface;
> }
> 
> which would break if we changed the signature for one.
> 
> 2. I think the Windows version is broken and returning true when it 
> should be returning false! Certainly this logic makes no sense to me:
> 
> bool CPUPerformanceInterface::CPUPerformance::initialize() {
>    if (!pdh_acquire()) {
>      return true;
>    }
>    _context_switches = create_counter_query(PDH_SYSTEM_IDX, 
> PDH_CONTEXT_SWITCH_RATE_IDX);
>    _process_cpu_load = create_process_query();
>    if (_process_cpu_load == NULL) {
>      return true;
>    }
>    allocate_counters(_process_cpu_load, 2);
>    if (initialize_process_counter(_process_cpu_load, 0, 
> PDH_PROCESSOR_TIME_IDX) != OS_OK) {
>      return true;
>    }
>    if (initialize_process_counter(_process_cpu_load, 1, 
> PDH_PRIV_PROCESSOR_TIME_IDX) != OS_OK) {
>      return true;
>    }
>    _process_cpu_load->set.initialized = true;
>    _machine_cpu_load = create_multi_counter_query();
>    if (_machine_cpu_load == NULL) {
>      return true;
>    }
>    initialize_cpu_query(_machine_cpu_load, PDH_PROCESSOR_TIME_IDX);
>    return true;
> }
> 
> I'll file a bug for Windows. :)
> 
> Thanks,
> David
> -----
> 
>> Thanks, Harold
>>
>> On 9/19/2019 1:57 AM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8230395
>>> webrev: http://cr.openjdk.java.net/~dholmes/8230395/webrev/
>>>
>>> First, thanks to Leo for going through and finding the places that 
>>> needed to be fixed. I didn't go looking for others.
>>>
>>> For the most part I have deleted the NULL checks. A few notes:
>>>
>>> - changed to NEW_C_HEAP_ARRAY_RETURN_NULL in a couple of places where 
>>> it was more consistent with the general recoverability approach of 
>>> the code in question (and did the opposite change in one place)
>>> - for the "perf" changes I also noticed that we were checking for 
>>> NULL after doing "new SomeCHeapObj()" which also cannot return NULL, 
>>> so removed those NULL checks too
>>> - the JVMCI thread counter changes became a bit more extensive due to 
>>> the fact a boolean method could now only every return true. So it was 
>>> changed to void and that had to filter all the way back up to the 
>>> JVMCI Java API. The JVMCI/Graal folk need to approve this
>>>
>>> Testing: tier 1-3 sanity tests (in progress)
>>>
>>> Thanks,
>>> David