[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR (XXS): 8075288: malloc without free in VM_PopulateDumpSharedSpace::doit()
From:       Jungwoo Ha <jwha () google ! com>
Date:       2015-04-30 14:35:45
Message-ID: CA+n_jhigJfQdgaZ7xdDPP7iGCCOVRGYJNRZEfNm0TG+3pq77-w () mail ! gmail ! com
[Download RAW message or body]

Can anyone approve this change?

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@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@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@...
>>>
>>> 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@google.com
>>>> <mailto:jwha@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@oracle.com
>>>>     <mailto:kim.barrett@oracle.com>> wrote:
>>>>
>>>>         On Apr 9, 2015, at 1:25 PM, Jungwoo Ha <jwha@google.com
>>>>         <mailto:jwha@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.
>>>>
>>>>
>>>>
>>>>
>>
>
[prev in list] [next in list] [prev in thread] [next in thread]