RFR: 8138916: Logging write function does not allow for long enough messages

[Coleen Phillimore]

Hi, I'm also reviewing this.   I think this looks good.   I don't think 
the solution to allocate a static buffer is very nice because it still 
has limits and we don't want to allocate something large on the stack.   
The malloc case is the infrequent case.  If it becomes frequent we can 
increase LogBufferSize from 512.

I think 111 the assert is good because if the windows code is wrong (or 
the vscprintf does something wrong an unexpected) we can catch the error 
with the assert before writing over memory.

I agree 116 shouldn't redeclare 'ret'.

Otherwise, I think it looks really good and I don't need to see another 
revision.

Thanks!
Coleen


On 10/28/15 11:40 AM, Dmitry Samersoff wrote:
> Rachel,
>
> On 2015-10-28 17:48, Rachel Protacio wrote:
>> Thank you for the comments, both Dmitry and Mikael. Replies inline.
>>
>> Modified webrev: http://cr.openjdk.java.net/~rprotacio/8138916.01/
> 111 Assert is never happens on Windows, so it's better to move it to
> os_posix
>
> 113 newbuf_len doesn't account prefix_len
>
> 115 you may consider to use pre-calculated at ll. 108 prefix rather than
> calculate it again.
>
> 116 int ret redefined it causes a warning.
>
> -Dmitry
>
>
>> (Retested and works fine.)
>>
>> On 10/27/2015 7:52 AM, Dmitry Samersoff wrote:
>>> Rachel,
>>>
>>> In worst case (windows, overflow) the code would parse format string
>>> four times.
>> You're right. I've added a check. New amount of times vnsprintf() is
>> called:
>> posix, fits => 1
>> posix, overflow => 2
>> windows, fits => 1
>> windows, overflow => 3 (the best performance we can get)
>>
>>> Also NEW_C_HEAP_ARRAY takes a lock and might become a
>>> performance bottleneck.
>>>
>>> POSIX equivalent of _vscprintf(fmt, args) is vsnprintf(NULL, 0, fmt,
>>> args)
>>>
>>> So it might be better to create os::log_vcprintf(fmt, args), then write
>>> code at log.hpp as:
>>>
>>> int buf_len = os::log_vcprintf(fmt, args);
>>> assert(buf_len < 2*page_size, "Stack overflow in logging");
>>> char buf[buf_len];
>>> vsprintf(buf, fmt, args);
>> I don't think this would work anyway. A char[] needs a static size.
>> Thanks,
>> Rachel
>>> -Dmitry
>>>
>>>
>>>
>>> On 2015-10-26 23:58, Rachel Protacio wrote:
>>>> Hello, all,
>>>>
>>>> Please review this fix to the faulty write function from the Unified
>>>> Logging framework.
>>>>
>>>> Summary: In logging/log.hpp, the logging vwrite function previously
>>>> asserted that the buffer remains within 512 characters, which is too
>>>> short for logging message of non-pre-determined length, e.g. for vtable
>>>> and itable function names. Now, the function resizes the buffer to the
>>>> appropriate length, asserting only that the resulting buffer is valid.
>>>> Includes tag "logtest" to test that logging can print an output of 518
>>>> characters.
>>>>
>>>> Open webrev:http://cr.openjdk.java.net/~rprotacio/8138916/
>>>> Bug:https://bugs.openjdk.java.net/browse/JDK-8138916
>>>>
>>>> Includes own acceptance test, and passes JPRT and remote hotspot/test
>>>> tests.
>>>>
>>>> Thank you,
>>>> Rachel
>