RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Dmitry Samersoff
dmitry.samersoff at oracle.com
Tue Apr 8 21:19:07 UTC 2014
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?
-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.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
--
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.
More information about the ppc-aix-port-dev
mailing list