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

[Jungwoo Ha]

Thank you, David!

On Mon, May 18, 2015 at 10:31 PM, David Holmes <david.holmes at oracle.com>
wrote:

> 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.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>


-- 
Jungwoo Ha | Java Platform Team | jwha at google.com