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

[Harold Seigel]

Hi David,

Thanks for the explanation.  I agree that it makes sense to keep the 
function as is.

Thanks, Harold

On 9/19/2019 5:39 PM, 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