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

[David Holmes]

(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