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

[Dmitry Samersoff]

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 ...