RFR(trivial): 8231024: Improve the debug info when the output is truncated

[David Holmes]

Hi Jie,

That seems fine to me.

Thanks,
David
-----

On 17/09/2019 11:58 am, Jie Fu wrote:
> Hi Ioi and David,
> 
> Thank you very much for your review and valuable comments.
> 
> Updated: http://cr.openjdk.java.net/~jiefu/8231024/webrev.02/
>   -1) Change the error message which is somewhat misleading
>   -2) Rename "written" to "required_len"
> 
> - Before the patch
> ------------------------------------
> [2019-09-14 16:07:55,752] Agent[1]: stderr: OpenJDK 64-Bit Server VM 
> warning: increase O_BUFLEN in ostream.hpp -- output truncated
> [2019-09-14 16:07:55,758] Agent[1]: stderr: OpenJDK 64-Bit Server VM 
> warning: increase O_BUFLEN in ostream.hpp -- output truncated
> [2019-09-14 16:07:55,763] Agent[1]: stderr: OpenJDK 64-Bit Server VM 
> warning: increase O_BUFLEN in ostream.hpp -- output truncated
> ...
> ------------------------------------
> 
> - After the patch
> ------------------------------------
> [2019-09-17 09:49:21,567] Agent[1]: stderr: OpenJDK 64-Bit Server VM 
> warning: outputStream::do_vsnprintf output truncated -- buffer length is 
> 2000 bytes but 2013 bytes are needed.
> [2019-09-17 09:49:21,571] Agent[1]: stderr: OpenJDK 64-Bit Server VM 
> warning: outputStream::do_vsnprintf output truncated -- buffer length is 
> 2000 bytes but 2031 bytes are needed.
> [2019-09-17 09:49:21,576] Agent[1]: stderr: OpenJDK 64-Bit Server VM 
> warning: outputStream::do_vsnprintf output truncated -- buffer length is 
> 2000 bytes but 2049 bytes are needed.
> ...
> ------------------------------------
> 
> Could you please review it and give me some advice?
> 
> Thanks a lot.
> Best regards,
> Jie
> 
> On 2019/9/17 上午7:20, Ioi Lam wrote:
>>
>>
>> On 9/16/19 12:44 AM, David Holmes wrote:
>>> On 16/09/2019 5:09 pm, Jie Fu wrote:
>>>> Hi David,
>>>>
>>>> Sorry for the confusion.
>>>
>>> The confusion is mine, not from you :)
>>>
>>>> On 2019/9/16 下午2:40, David Holmes wrote:
>>>>> Okay I'm confused. I had assumed that the passed in buffer size 
>>>>> must be different in each case because of the 2014/2032/2050 values 
>>>>> in the message, but in fact they are all sized at 2000. 
>>>> According to the source code, the buffer size can be either 
>>>> 2000(O_BUFLEN) [1] or 2048(BUFLEN) [2].
>>>> So it isn't always 2000.
>>>
>>> It is 2000 is all three examples of the output that you gave.
>>
>> xmlStream::va_tag() uses 2*K.
>>
>>>
>>>>> That makes no sense
>>>> I think it makes sense.
>>>> It is still unclear which one (O_BUFLEN or BUFLEN?) should be 
>>>> increased if the current buffer size wasn't printed.
>>>
>>> You need to know the original caller that passed in the buffer size.
>>>
>>> But we can't just keep increasing the buffer size. We probably need 
>>> to examine the callers to see what amount of information is trying to 
>>> be passed through. Then decide whether to adjust what the callers are 
>>> passing through, or adjust the buffer size - perhaps on a callsite by 
>>> callsite basis rather than just bumping O_BUFLEN or BUFLEN.
>>>
>>
>> I think the message should stop saying "increase O_BUFLEN" -- probably 
>> the reviewers will reject all future increase of O_BUFLEN and will 
>> suggest fixing the caller of outputStream::do_vsnprintf instead.
>>
>> Callers of outputStream::do_vsnprintf don't necessarily use O_BUFLEN. 
>> E.g., xmlStream::va_tag allocates its own buffer. 
>> VMError::report_and_die() also uses its own buffer -- it uses O_BUFLEN 
>> today, but there's no guarantee or requirement that it always does that.
>>
>> How about saying something like "outputStream::do_vsnprintf output 
>> truncated -- buffer length is 2000 bytes but 2100 bytes are needed."
>>
>>
>>>>> - how can we have written e.g. 2014 bytes to a buffer of only 2000 
>>>>> bytes? 
>>>>
>>>> The output is truncated since the current buffer size is too small 
>>>> (e.g., 2000 < 2014).
>>>
>>> Ah sorry - I'd missed that aspect of vsnprintf. We're calling:
>>>
>>>  102     int written = os::vsnprintf(buffer, buflen, format, ap);
>>>
>>> but it isn't "written" when it exceeds buflen, it's the required 
>>> buffer size.
>>>
>>
>> I think the variable "written" should be renamed to "required".
>>
>> Thanks
>> - Ioi
>>
>>> David
>>> -----
>>>
>>>>
>>>>> Do we have a bug in os::vsnprintf?
>>>>
>>>> I don't think so.
>>>>
>>>> Thanks a lot.
>>>> Best regards,
>>>> Jie
>>>>
>>>> [1] 
>>>> http://hg.openjdk.java.net/jdk/jdk/file/24df796eef3d/src/hotspot/share/utilities/ostream.cpp#l124 
>>>>
>>>> [2] 
>>>> http://hg.openjdk.java.net/jdk/jdk/file/24df796eef3d/src/hotspot/share/utilities/xmlstream.cpp#l138 
>>>>
>>>>
>>>>
>>
>