RFR [XS]: 8233013: deallocate memory after using NEW_C_HEAP_ARRAY

[David Holmes]

On 26/10/2019 10:57 am, Kim Barrett wrote:
>> On Oct 25, 2019, at 9:46 AM, Baesken, Matthias <matthias.baesken at sap.com> wrote:
>>
>> Hello, please review this small  change . It adds  some missing  FREE_C_HEAP_ARRAY  calls  to os::init_system_properties_values   .
>>
>> bug/webrev :
>>
>> https://bugs.openjdk.java.net/browse/JDK-8233013
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8233013.1/
> 
> I don’t think any of the proposed additional FREE_C_HEAP_ARRAY uses are needed.

Not essential but also not harmful. See below ...

>> (but in case you think we do not need the freeing because of the calls are followed  by   " vm_exit_during_initialization"   calls,
>> then probably the already in-place freeing before those calls  could be removed  too  for consistency see
>>
>> http://cr.openjdk.java.net/~mbaesken/webrevs/8233013.1/src/hotspot/os/solaris/os_solaris.cpp.frames.html
>>
>> lines 456 / 466)
> 
> I don’t think those are needed either.

These stem from a time when there was still the view that 
vm_exit_during_initialization could/should unload the VM rather than 
blow away the hosting process (which may not be the launcher). I think 
that is a battle we have abandoned now.

I think this is going to be messy of we try to fight the consistency 
battle in this area. But I can see an argument for putting in the free 
call if we wanted to allow the code to be more easily examined by code 
checkers that were looking at correct usage here. So I don't object to 
this cleanup.

> It’s not clear that it’s worth the effort of removing them, given JEP 362.
>
>> Additionally I was unsure where the freeing of "char* s" from ...
>>
>> src/hotspot/share/compiler/directivesParser.cpp , line 313
>>
>>     case JSON_STRING:
>>       if (option_key->flag_type != ccstrFlag && option_key->flag_type != ccstrlistFlag) {
>>          . . .
>>       } else {
>>         char* s = NEW_C_HEAP_ARRAY(char, v->str.length+1,  mtCompiler);
>>        strncpy(s, v->str.start, v->str.length + 1);
>>         s[v->str.length] = '\0';
>>         (set->*test)((void *)&s);
>>       }
>>       break;
>>
>> occurs, maybe someone can tell ?
> 
> That array seems to be getting stored in the directive set, although I haven’t fully traced
> through the code.  It is presumably no longer the responsibility of the allocating code to
> free it.

I would agree with that.

Thanks,
David