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

[Rachel Protacio]

Thank you both. I've removed the re-declaration and added prefix_len to 
newbuf_len. As for line 115, that line is not for the sake of 
recalculating prefix_len, but for writing the prefix to the buffer.

http://cr.openjdk.java.net/~rprotacio/8138916.02/

Thanks,
Rachel

On 10/28/2015 12:27 PM, Coleen Phillimore wrote:
>
> 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
>>
>