RFR (XXS): 8075288: malloc without free in VM_PopulateDumpSharedSpace::doit()

[David Holmes]

On 1/05/2015 12:35 AM, Jungwoo Ha wrote:
> Can anyone approve this change?

Sorry this got neglected. I have Reviewed the change and will sponsor it 
for you. Even if this code gets changed in the future this is a simple 
fix to do now.

David

> https://bugs.openjdk.java.net/browse/JDK-8075288
> http://cr.openjdk.java.net/~jwha/8075288/webrev.00
>
> Or at least tell me what to do.
>
> On Mon, Apr 20, 2015 at 5:23 PM, Jungwoo Ha <jwha at google.com> wrote:
>
>> Any update on this bug?
>> I think it is an obvious fix.
>>
>> On Fri, Apr 10, 2015 at 9:25 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>
>>> There's this comment right above the code:
>>>
>>>    // dunno what this is for.
>>>
>>> I also don't understand why we need to allocate a saved_vtbl, clear
>>> vtbl_list, and then vtbl_list later.  I think I tried removing these lines
>>> in the past and got into problems. So I'd suggest leaving them alone until
>>> you understand what really is going on.
>>>
>>> Thanks
>>> - Ioi
>>>
>>>
>>> On 4/10/15 8:00 AM, Mikael Gerdin wrote:
>>>
>>>> PopulateDumpSharedSpace is CDS, redirecting to hotspot-runtime-dev at ...
>>>>
>>>> On 2015-04-10 16:49, Carsten Varming wrote:
>>>>
>>>>> Dear Jungwoo,
>>>>>
>>>>> Why is this array not resource allocated?
>>>>>
>>>>
>>>> Actually, none of this really matters since the VM terminates after
>>>> executing the CDS dump. This has probably been done to satisfy some static
>>>> analysis tool that Google runs on the sources.
>>>>
>>>> /Mikael
>>>>
>>>>
>>>>> Carsten
>>>>>
>>>>> On Fri, Apr 10, 2015 at 9:31 AM, Jungwoo Ha <jwha at google.com
>>>>> <mailto:jwha at google.com>> wrote:
>>>>>
>>>>>      What's the conclusion? Should I fix it or discuss it separately?
>>>>>
>>>>>      On Thu, Apr 9, 2015 at 2:43 PM, Kim Barrett <kim.barrett at oracle.com
>>>>>      <mailto:kim.barrett at oracle.com>> wrote:
>>>>>
>>>>>          On Apr 9, 2015, at 1:25 PM, Jungwoo Ha <jwha at google.com
>>>>>          <mailto:jwha at google.com>> wrote:
>>>>>           >
>>>>>           > Can someone sponsor this change?
>>>>>           >
>>>>>           > https://bugs.openjdk.java.net/browse/JDK-8075288
>>>>>           > http://cr.openjdk.java.net/~jwha/8075288/webrev.00
>>>>>
>>>>>          I think the change is correct.  Good find!
>>>>>
>>>>>          However, it’s not clear to me why this is using malloc (and now
>>>>>          free), rather than stack allocation.
>>>>>          The size is a declared variable at the allocation point, but
>>>>>          it’s initialized from a constant with a
>>>>>          not big value (17) and never modified, so could be changed to be
>>>>>          a constant.
>>>>>
>>>>>          That is, instead of
>>>>>
>>>>>            592   char* saved_vtbl = (char*)os::malloc(vtbl_list_size *
>>>>>          sizeof(void*), mtClass);
>>>>>            593   memmove(saved_vtbl, vtbl_list, vtbl_list_size *
>>>>>          sizeof(void*));
>>>>>
>>>>>          use
>>>>>
>>>>>             void* saved_vtbl[vtbl_list_size];
>>>>>             memmove(saved_vtbl, vtbl_list, ARRAY_SIZE(saved_vtbl));
>>>>>
>>>>>          Maybe the worry is that vtbl_list_size might not always be a
>>>>>          small constant?
>>>>>
>>>>>          Probably this question of whether to use malloc/free here at all
>>>>>          should be a separate CR.
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>