Request for review (S, UPDATED-3) 7017193: Small memory leak in get_stack_bounds // os::create_stack_guard_pages

[Dmitry Samersoff]

Coleen,

Thank you! I'm going to commit changes.

Longest possible address line (on x86_64bit) is 103 bytes long
(see below), so 128 bytes gives us enough reserve.

-Dmitry


e.g.:

ffffffffff600000-ffffffffff601000 r-xp
00000000 00:00 0                  [vsyscall]





On 2011-02-28 19:07, Coleen Phillimore wrote:
> Dmitry,
> This looks good to me. Only one question, can the [stack] line be 128
> characters and that includes the 128th as the newline and/or null
> terminating char, or do you need 129 to return a null terminated string?
>
> thanks,
> Coleen
>
> On 2/25/2011 5:09 PM, Dmitry Samersoff wrote:
>> Dan,
>>
>> Updated webrev (spacing changes):
>>
>> http://cr.openjdk.java.net/~dsamersoff/7017193/webrev.05/
>>
>> -Dmitry
>>
>>
>>
>> On 2011-02-25 23:57, Daniel D. Daugherty wrote:
>>> On 2/25/2011 11:57 AM, Dmitry Samersoff wrote:
>>>> Hi Everyone,
>>>>
>>>> Updated webrev:
>>>>
>>>> http://cr.openjdk.java.net/~dsamersoff/7017193/webrev.04/
>>>
>>> src/os/linux/vm/os_linux.cpp
>>> nit on line 2656: add space after ','
>>> nit on line 2666: HotSpot style is 'while (('
>>>
>>> src/share/vm/runtime/os.cpp
>>> nit on line 1299: add space after ','
>>> nit on line 1302: HotSpot style is 'while (('
>>> nit on line 1302: add spaces around '=' => ' = '
>>> nit on line 1324: HotSpot style is 'while ('
>>>
>>> src/share/vm/runtime/os.hpp
>>> No comments.
>>>
>>>
>>>>
>>>> Returned sscanf and addressed other comments.
>>>>
>>>> -Dmitry
>>>>
>>>>
>>>>
>>>> On 2011-02-25 01:43, Daniel D. Daugherty wrote:
>>>>> On 2/24/2011 2:24 PM, Dmitry Samersoff wrote:
>>>>>> [got mailer error for openjdk alias, so resending]
>>>>>>
>>>>>> Hi Everyone,
>>>>>>
>>>>>> Here is updated fix:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~dsamersoff/7017193/webrev.03/
>>>>>
>>>>> src/os/linux/vm/os_linux.cpp
>>>>> line 2662: should be /proc/self/maps
>>>>>
>>>>> line 2663: literal '128' should be 'sizeof(buf)'
>>>>>
>>>>> line 2664: original logic looks for line ending with "[stack]"
>>>>> and your new logic matches a line with "[stack]" anywhere.
>>>>>
>>>>> line 2666: You assume that you got a number here. The original
>>>>> code would have returned false.
>>>>>
>>>>> line 2667: You assert if '-' is not after the first unsigned
>>>>> number translated. The orignal code would have returned
>>>>> false in case of such a mis-format.
>>>>>
>>>>> line 2668: You assume that 'np' is valid here (in product).
>>>>> The original code would have returned false.
>>>>> The second param to the second strtoul() call should be
>>>>> NULL or (char*) NULL.
>>>>>
>>>>>
>>>>> src/share/vm/runtime/os.cpp
>>>>> lines 1296-7 - space after '//'
>>>>>
>>>>> "const" on bsize parameter?
>>>>>
>>>>> line 1300: please consider adding this comment:
>>>>> // read until EOF, EOL or buf is full
>>>>>
>>>>> line 1305: please consider the following comment between lines
>>>>> 1306 and 1307 instead:
>>>>> // EOL reached so ignore EOL character and return
>>>>>
>>>>> lines 1313-4: please consider the following comment between lines
>>>>> 1315 and 1316 instead:
>>>>> // EOF reached. If we read chars before EOF, return them and
>>>>> // return EOF on next call. Otherwise, return EOF.
>>>>>
>>>>> line 1321: please move trailing ";" to next line, e.g.:
>>>>>
>>>>> while( read(fd, &ch, 1) == 1 && ch != '\n' )
>>>>> /* do nothing */ ;
>>>>>
>>>>> line 1323: please consider this comment instead:
>>>>>
>>>>> // Return initial part of line that fits in buf.
>>>>> // If we reached EOF, it will be returned on next call.
>>>>>
>>>>>
>>>>> src/share/vm/runtime/os.hpp
>>>>> "const" on bsize parameter?
>>>>>
>>>>> Why add blank lines on 725-6?
>>>>>
>>>>>
>>>>>>
>>>>>> I introduce a new convenience function primary designed to read
>>>>>> variety of /proc files. It reads first N characters of line and then
>>>>>> skip until eol.
>>>>>>
>>>>>> Than get_stack_bounds() is changed to use it instead of stdio
>>>>>> getline();
>>>>>>
>>>>>> -Dmitry
>>>>>>
>>>>
>>>>
>>
>>
>


-- 
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...