RFR(M): 8038201: Clean up misleading usage of malloc() in init_system_properties_values()
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Apr 9 22:53:37 UTC 2014
Hi Goetz,
You can help :)
When Dmitry push the patch into jdk8u/hs-dev can you build and test from
it on your platforms (ppc64)? If everything is fine I will do the final
merge into ppc-aix-port/stage and we will freeze it.
thanks,
Vladimir
On 4/8/14 11:40 PM, Lindenmaier, Goetz wrote:
> Vladimir, Dmitry,
>
> Thanks for handling this!
> Sorry I can't help.
>
> Best regards,
> Goetz
>
> -----Original Message-----
> From: Dmitry Samersoff [mailto:dmitry.samersoff at oracle.com]
> Sent: Mittwoch, 9. April 2014 00:08
> To: Vladimir Kozlov; 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()
>
> Vladimir,
>
> OK. Thank you for clarification.
>
> -Dmitry
>
> On 2014-04-09 01:43, Vladimir Kozlov wrote:
>> 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