[8u] RFR: 8196882: VS2017 Hotspot Defined vsnprintf Function Causes C2084 Already Defined Compilation Error

[Kevin Walls]

Great, thanks Kim,

Not being an 8 Reviewer is fine, I'm very happy to get your comments 
here, and when we're happy with it I do a specific 8u approval email.

src/os/windows/vm/os_windows.cpp / os::vsnprintf()  oops yes, should not 
check the result and call _vscprintf().

src/share/vm/prims/jvm.cpp / jio_vsnprintf() should not force the 
null-terminator in the result.

Making Windows retain its different behaviour also means the tests need 
to be different for Windows, so in ostream.cpp  in test_snprintf() I 
change the boolean passed to the test depending on the platform.  On 
Windows, expect_count is false.

Updated webrev:

http://cr.openjdk.java.net/~kevinw/8196882/webrev.01/

Thanks
Kevin



On 26/07/2018 19:28, Kim Barrett wrote:
>> On Jul 13, 2018, at 3:12 PM, Kevin Walls <kevin.walls at oracle.com> wrote:
>>
>> Hi,
>>
>> I'd like to request a review of a backport from 11 to 8u:
>>
>> 8196882: VS2017 Hotspot Defined vsnprintf Function Causes C2084 Already Defined Compilation Error
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8196882
>>
>> 11 changeset:
>> http://hg.openjdk.java.net/jdk/hs/rev/eebf559c9e0d
>>
>> 11 review thread:
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-February/030252.html
>> ...to...
>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2018-February/030279.html
>>
>>
>> 8u webrev: http://cr.openjdk.java.net/~kevinw/8196882/webrev.00/
>>
>>
>>  From VS2015, vsnprintf is provided, and is C99 conforming.  This is new, and
>> conflicts with the hotspot-provided definition.  8196882 introduces os::vsnprintf()
>> so we can choose what we call in there.
>>
>> We don't necessarily want to change behaviour in jdk8u, so we call _vsnprintf()
>> on Windows to get the old behaviour.
>>
>> I didn't plan to move everything to a C99 behaviour here for jdk8u as we did in jdk11.
>> We just want to solve the symbol clash, so outputStream::do_vsnprintf() does
>> not need to change as it does in 11.
>>
>> In jvm.cpp, the change in jdk11 changes jio_vsnprintf behaviour, but we wouldn't
>> normally choose to change the behaviour at this point in 8u.
>>
>> (But ensuring the buffer is null-terminated looks like a positive change.)
>>
>> The original has GTests, here in 8u it's a debug-mode test at runtime.  I have edited some of the
>> asserts in check_snprintf_result() as not changing jio_vsnprintf.
>>
>> I've been building and testing with VS2017 and the current default compiler VS2010,
>> plus the other platforms.
>>
>> Many thanks!
>> Kevin
> Note that I am not a Reviewer for jdk8.
>
> ------------------------------------------------------------------------------
> src/os/windows/vm/os_windows.cpp
>
> According to the RFR:
>
>    "We don't necessarily want to change behaviour in jdk8u, so we call
>    _vsnprintf() on Windows to get the old behaviour."
>
> Yet this:
> 1859   if (result < 0) {
> 1860     result = _vscprintf(fmt, args);
> 1861   }
>
> doesn't retain the old behavior, which was to return -1 (on Windows
> only) if the output would have exceeded the buffer size.
>
> This matters in the call from outputStream::do_vsnprintf.  The old
> code there would never report truncation for POSIX, only for Windows.
> With this changeset the DEBUG_ONLY truncation warning there is
> incorrect / dead, since a negative result only occurs for encoding
> errors.
>
> ------------------------------------------------------------------------------
> src/share/vm/prims/jvm.cpp
> 2917 int jio_vsnprintf(char *str, size_t count, const char *fmt, va_list args) {
> [...]
> 2922   int result = os::vsnprintf(str, count, fmt, args);
> 2923   // Note: on truncation vsnprintf(3) on Unix returns number of
> 2924   // characters which would have been written had the buffer been large
> 2925   // enough; on Windows, it returns -1. We handle both cases here and
> 2926   // always return -1, and perform null termination.
> 2927   if ((result > 0 && (size_t)result >= count) || result == -1) {
> 2928     str[count - 1] = '\0';
> 2929     result = -1;
> 2930   }
>
> Since os::vsnprintf *does* guarantee null termination of the result,
> it's no longer necessary to ensure that here.  I think the jdk11
> version of this function should be fine.
>
> ------------------------------------------------------------------------------
>