[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-ppc-aix-port-dev
Subject: RE: RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
From: "Lindenmaier, Goetz" <goetz.lindenmaier () sap ! com>
Date: 2014-03-27 19:52:32
Message-ID: 4295855A5C1DE049A61835A1887419CC2CEAB48B () DEWDFEMB12A ! global ! corp ! sap
[Download RAW message or body]
Thanks Dmitry and Vladimir!
Best regards,
Goetz.
-----Original Message-----
From: Vladimir Kozlov [mailto:vladimir.kozlov@oracle.com]
Sent: Thursday, March 27, 2014 8:18 PM
To: Dmitry Samersoff; Lindenmaier, Goetz; hotspot-dev@openjdk.java.net; ppc-aix-port-dev@openjdk.java.net
Subject: Re: RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Thank you, Dmitry
We need to backport it into 8u20 too.
Regards,
Vladimir
On 3/27/14 12:16 PM, Dmitry Samersoff wrote:
> Vladimir,
>
> I can push it to RT next week.
>
> -Dmitry
>
> On 2014-03-27 23:01, Vladimir Kozlov wrote:
>> On 3/27/14 11:53 AM, Dmitry Samersoff wrote:
>>> Goetz,
>>>
>>> Looks good for me!
>>
>> +1.
>>
>> Dmitry,
>>
>> Do you want me to push it into Comp repo or you will do it into RT repo?
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> -Dmitry
>>>
>>>
>>> On 2014-03-27 18:56, Lindenmaier, Goetz wrote:
>>>> Hi Dmitry,
>>>>
>>>> thanks for the thorough review and catching the forgotten FREEs!
>>>> I had a look at set_boot_path(). That will never return false,
>>>> as it also calls NEW_C_HEAP_ARRAY that will abort the VM.
>>>> In addition it would be wrong to return, there should be a
>>>> call to vm_exit_...
>>>> So I removed the test and return statement.
>>>> I added the other missing FREEs.
>>>>
>>>> I also implemented your proposal with only one sprintf
>>>> for the ld_library_path, and did so for bsd, aix and linux
>>>> to get it more similar. Solaris seems too different to
>>>> adapt to that scheme.
>>>>
>>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.03/
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Dmitry Samersoff [mailto:dmitry.samersoff@oracle.com]
>>>> Sent: Donnerstag, 27. März 2014 10:49
>>>> To: Lindenmaier, Goetz; hotspot-dev@openjdk.java.net;
>>>> ppc-aix-port-dev@openjdk.java.net
>>>> Subject: Re: RFR(M): 8038201: Clean up misleading usage of malloc()
>>>> in init_system_properties_values()
>>>>
>>>> Goetz,
>>>>
>>>> Thank you for doing it - code is much cleaner now.
>>>>
>>>> os_aix.cpp:
>>>> Did you forget FREE_C_HEAP_ARRAY() at ll.548 and ll.576?
>>>>
>>>> os_bsd.cpp:
>>>> missed FREE_C_HEAP_ARRAY() at ll. 389, ll. 477
>>>>
>>>> ll.407 sprintf could be moved to ll.420 under else
>>>>
>>>>
>>>> ll.497 (and below) a trick like one below:
>>>>
>>>> char *l = ::getenv("JAVA_LIBRARY_PATH");
>>>> char *v = ::getenv("DYLD_LIBRARY_PATH");
>>>> char *v_colon = ":";
>>>>
>>>> if (l != NULL || v != NULL) {
>>>>
>>>> if (v == NULL){ v = "", v_colon = ""; };
>>>> if (l == NULL){ l = "", v_colon = ""; };
>>>>
>>>> t = (char *) ... /* allocate memory for all strings */
>>>>
>>>> sprintf(t, "%s%s%s:%s", v,v_colon, l, ld_library_path);
>>>> if (ld_library_path != buf){ ... /* free ld_library_path */ }
>>>> ld_library_path = t;
>>>> }
>>>>
>>>> might be useful to assemble the path in one shot,
>>>> without extra copying
>>>>
>>>>
>>>> ll.520 As you appending constant value ":.", you can just
>>>> take it into account at ll. 495
>>>>
>>>>
>>>> os_linux.cpp:
>>>> missed FREE_C_HEAP_ARRAY() at ll. 400
>>>>
>>>> os_solaris.cpp:
>>>> missed FREE_C_HEAP_ARRAY() at ll. 714
>>>>
>>>>
>>>> -Dmitry
>>>>
>>>> On 2014-03-27 12:54, Lindenmaier, Goetz wrote:
>>>>> Hi,
>>>>>
>>>>> please have a look at this new webrev:
>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.02/
>>>>>
>>>>> Dmitry, sorry I forgot to reply to all in my last mail. I copied
>>>>> the missing parts in here, below.
>>>>>
>>>>> Avoiding big stack allocations I wanted to remove the buf[MAXPATHLEN]
>>>>> at the beginning of the function, too. That's why I said I can't
>>>>> get along
>>>>> with a single allocation. strlen(v) is only known later.
>>>>>
>>>>> I could redesign the whole code, computing sizes first, and then
>>>>> doing a single allocation, but I think that doesn't help the
>>>>> readability of the code.
>>>>>
>>>>> So now I compute the max size of all buffers I can estimate at the
>>>>> beginning
>>>>> and do a single allocation for them.
>>>>> Also, bsd contains an extra section for __APPLE__.
>>>>>
>>>>> Best regards,
>>>>> Goetz.
>>>>>
>>>>>
>>>>>
>>>>> Goetz,
>>>>>
>>>>>> The allocation in 556 must know about the length of the
>>>>>> getenv("LIBPATH") result, which may contain several paths.
>>>>>
>>>>> You can use
>>>>>
>>>>> MAX(
>>>>> strlen(v) + 1 + sizeof(DEFAULT_LIBPATH) + 1,
>>>>> MAXPATHLEN + sizeof(EXTENSIONS_DIR) + sizeof(ENDORSED_DIR)
>>>>> )
>>>>>
>>>>> as the size for a buffer at ll. 556 and have one allocation.
>>>>>
>>>>> But I'm fine with two different allocations.
>>>>>
>>>>> Thank you for addressing other concerns.
>>>>>
>>>>> -Dmitry
>>>>>
>>>>> On 2014-03-25 17:55, Lindenmaier, Goetz wrote:
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> see my comments inline.
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Dmitry Samersoff [mailto:dmitry.samersoff@oracle.com]
>>>>>>> Sent: Dienstag, 25. März 2014 11:12
>>>>>>> To: Lindenmaier, Goetz; hotspot-dev@openjdk.java.net;
>>>>>>> ppc-aix-port-dev@openjdk.java.net
>>>>>>> Subject: Re: RFR(M): 8038201: Clean up misleading usage of
>>>>>>> malloc() in init_system_properties_values()
>>>>>>>
>>>>>>> Goetz,
>>>>>>>
>>>>>>> os_aix.cpp: 565
>>>>>>>
>>>>>>> It might be better to allocate one buffer that satisfy all three
>>>>>>> sprintf() calls at ll. 556 and free it at ll. 574
>>>>>> I need at least two allocations. The first one has a statically
>>>>>> known size. At
>>>>>> least more or less, as I know there will be only one path of
>>>>>> unknown size in the buffer.
>>>>>> The allocation in 556 must know about the length of the
>>>>>> getenv("LIBPATH") result,
>>>>>> which may contain several paths.
>>>>>> Also, in the other files, paths are concatenated, so at least two
>>>>>> buffers are needed.
>>>>>>
>>>>>>> os_bsd.cpp: 350
>>>>>>>
>>>>>>> I would prefer to have the single #ifdef __APPLE__ block, it makes
>>>>>>> code much more readable.
>>>>>> I'll fix this.
>>>>>>
>>>>>>> os_bsd.cpp: 485
>>>>>>>
>>>>>>> you replace malloc to on-stack allocation of more than 4096 bytes. I
>>>>>>> think this allocation should be done through NEW_C_HEAP_ARRAY.
>>>>>> > see also comments to os_aix above - you can allocate large enough
>>>>>>> buffer once and than reuse it.
>>>>>> Is there a boundary at 4k? There was a char buf[MAXPATHLEN] in the
>>>>>> code anyways, so I thought a few bytes more would not matter.
>>>>>> The space required by the existing buf can be reused by the C
>>>>>> compiler.
>>>>>>
>>>>>> But I'll fix that and only use allocated memory.
>>>>>> I'll remove the buf that was already there and replace all spaces
>>>>>> I can estimate by MAXPATHLEN + some offset by a single, allocated
>>>>>> buffer.
>>>>>> I'll do this the same on all four platforms.
>>>>>>
>>>>>>> os_linux.cpp: 434
>>>>>>> the same comments as above.
>>>>>>>
>>>>>>> os_solaris.cpp: 728
>>>>>>>
>>>>>>> As soon as you doing cleanup, I would love to have this code
>>>>>>> changed
>>>>>>> a bit. It comes from solaris man page long time ago and doesn't
>>>>>>> respect
>>>>>>> hotspot convention of using underscore to indicate class global
>>>>>>> variables.
>>>>>>>
>>>>>>> Could you change it to something like:
>>>>>>> Dl_serinfo info_sz, *info;
>>>>>> Yes, I'll fix that.
>>>>>>
>>>>>> Best regards,
>>>>>> Goetz.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>> if (dlinfo(RTLD_SELF, RTLD_DI_SERINFOSIZE, (void *)&info_sz) == -1) {
>>>>> ...
>>>>>
>>>>> info = (Dl_serinfo*)NEW_C_HEAP_ARRAY(char, info_sz.dls_size,
>>>>> mtInternal);
>>>>> ...
>>>>>
>>>>> os_solaris.cpp: 829
>>>>> The same comment about on-stack allocation as for os_bsd.cpp
>>>>>
>>>>> -Dmitry
>>>>>
>>>>> On 2014-03-25 01:50, Lindenmaier, Goetz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> please review and test this change. I please need a sponsor.
>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8038201-sec/webrev.00/
>>>>>>
>>>>>> This change addresses the implementation of
>>>>>> init_system_properties_values
>>>>>> on aix, linux, solaris and bsd.
>>>>>> In init_system_properties_values a macro was defined mapping malloc to
>>>>>> NEW_C_HEAP_ARRAY. NEW_C_HEAP_ARRAY checks for successful allocation
>>>>>> or exits the VM. The code of this method handles the allocated
>>>>>> pointers as
>>>>>> allocated with real malloc, i.e., there were checks for NULL and
>>>>>> calls to free().
>>>>>>
>>>>>> This change replaces the macro malloc with NEW_C_HEAP_ARRAY and
>>>>>> removes
>>>>>> the unnecessary checks making the code clearer. Also, it uses a
>>>>>> local array where
>>>>>> possible.
>>>>>>
>>>>>> The allocated memory is passed to calls that end up at
>>>>>> SystemProperty::set_value().
>>>>>> set_value copies the strings passed. Thus the memory allocated in
>>>>>> init_system_properties_values must be freed after these calls.
>>>>>> Most of these
>>>>>> frees were missing.
>>>>>>
>>>>>> This change adds the missing frees.
>>>>>>
>>>>>> Testing this change I ran into a warning in ppc code which I fixed,
>>>>>> too.
>>>>>> I did some local test. I'll get broader tests by Wednesday.
>>>>>>
>>>>>> Best regards,
>>>>>> Goetz.
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>
>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic