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

[Jungwoo Ha]

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