RFR: 8196882: VS2017 Hotspot Defined vsnprintf Function Causes C2084 Already Defined Compilation Error

[Marcus Larsson]

Hi,


On 2018-02-22 05:20, Kim Barrett wrote:
>> On Feb 21, 2018, at 3:23 AM, Marcus Larsson <marcus.larsson at oracle.com> wrote:
>>
>> Hi,
>>
>>
>> On 2018-02-21 08:30, Thomas Stüfe wrote:
>>> Hi Kim,
>>>
>>> this is good. Please find comments inline / below.
>>>
>>> On Wed, Feb 21, 2018 at 1:08 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>>>> We still provide os::log_vsnprintf, which differs from the new
>>>> os::vsnprintf in the return value when output truncation occurs. […]
>>>>
>>> I totally agree. Possible alternatives:
>>>
>>> 1 rename it
>>> 2 move it into the log sub project as an internal implementation detail
>>> 3 Or, provide a platform independent version of _vcsprintf (
>>> https://msdn.microsoft.com/en-us/library/w05tbk72.aspx) instead. So whoever
>>> really wants to count characters in resolved format string should first use
>>> that function, then alloc the appropiate buffer, then do the real printing.
>>> Posix variant for  _vcsprintf could just be vsnprintf with a zero byte
>>> output buffer.
>>>
>>> I personally like (3) best, followed by (2)
>> The best alternative, IMHO, would be to make os::vsnprintf behave just like log_vsnprintf (C99 standard vsnprintf), thus removing the need for log_vsnprintf completely. Also, that behavior is strictly better than always returning -1 on error.
> There was discussion of the behavior in this area around JDK-8062370
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-November/015779.html
>
> The consensus then was that a guarantee of NUL-termination was
> desirable. That's already at variance with C99 vsnprintf.

 From what I can tell, C99 guarantees NUL-termination (for n > 0, 
naturally).

>
> It was noted at the time that most calls to these functions in HotSpot
> ignored the return value. That's still true. I looked at the 57 I
> found via egrep "[=,(]\s*(jio_|::|)v?snprintf\(", and only found 2
> that seem to want the C99 vsnprintf return value behavior. (Also found
> a handful that appear to be mishandling the result in one way or
> another; will be filing some more bugs...) I didn't look at the
> roughly 350 calls that are ignoring the result.
>
> So being consistent with C99 vsnprintf doesn't appear to be useful in
> this code base, would add some cost to converting away from using the
> jio_ functions in order to get the -Wformat warnings, and might add
> some (likely not really measurable) performance cost for Windows.

Just for the record: Starting with VS2015, (v)snprintf implementations 
are C99 conforming.

>
> So I'm not inclined toward making the return value from os::vsnprintf
> consistent with C99 vsnprintf. If there's a groundswell of support, it
> can be done, but I'd rather keep such an effort separate from this
> change, which is intended to address a problem with building using
> recent versions of Visual Studio, doing a little bit of cleanup in the
> process.

That's fine with me, I just wanted to voice my opinion on the API I 
would prefer. Also that keeping two implementations of essentially the 
same thing seems weird to me (vsnprintf vs log_vsnprintf).

Thanks,
Marcus