RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Vladimir Kozlov
vladimir.kozlov at oracle.com
Tue Apr 8 21:43:38 UTC 2014
On 4/8/14 2:19 PM, Dmitry Samersoff wrote:
> Vladimir,
>
> On 2014-04-09 00:32, Vladimir Kozlov wrote:
>
>> Could you tell when you are planning to backport these changes into 8u20?
>
> This week. Do I need any additional approvals?
No need for approval but, please, send new [8u20] RFR to hotspot-dev
with links to jbs and jdk9 changeset and say that it applies cleanly (if
it is). I will review it and you can push.
It is to let people know that you backport it.
Thanks,
Vladimir
>
> -Dmitry
>
>>
>> Thanks,
>> Vladimir
>>
>> On 4/1/14 5:50 AM, Dmitry Samersoff wrote:
>>> Goetz,
>>>
>>> Push done.
>>>
>>> Let it bakes for some times in JDK9 (at least two nightlies) than I'll
>>> push it to 8u20
>>>
>>> -Dmitry
>>>
>>> On 2014-03-27 23:52, Lindenmaier, Goetz wrote:
>>>> Thanks Dmitry and Vladimir!
>>>>
>>>> Best regards,
>>>> Goetz.
>>>>
>>>> -----Original Message-----
>>>> From: Vladimir Kozlov [mailto:vladimir.kozlov at oracle.com]
>>>> Sent: Thursday, March 27, 2014 8:18 PM
>>>> To: Dmitry Samersoff; Lindenmaier, Goetz;
>>>> hotspot-dev at openjdk.java.net; ppc-aix-port-dev at 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 at oracle.com]
>>>>>>>> Sent: Donnerstag, 27. März 2014 10:49
>>>>>>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net;
>>>>>>>> ppc-aix-port-dev at 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 at oracle.com]
>>>>>>>>>>> Sent: Dienstag, 25. März 2014 11:12
>>>>>>>>>>> To: Lindenmaier, Goetz; hotspot-dev at openjdk.java.net;
>>>>>>>>>>> ppc-aix-port-dev at 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.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>
More information about the ppc-aix-port-dev
mailing list