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

[Kim Barrett]

> 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.

------------------------------------------------------------------------------