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

[Coleen Phillimore]

Dmitry, Thank you for reviewing and providing comments.  The second 
version is better because of these things that I didn't notice until you 
pointed it out.

The malloc won't be frequent.  I think it'll be okay and follows other 
coding patterns in the jvm.

Thanks!
Coleen

On 10/28/15 12:09 PM, Dmitry Samersoff wrote:
> Coleen,
>
> OK. I'm leaving it in good hands ;)
>
> -Dmitry
>
> On 2015-10-28 18:54, Coleen Phillimore wrote:
>> 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
>