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

Add os::vsnprintf and os::snprintf.

Reviewed-by: lfoltan, stuefe, mlarsson

src/hotspot/os/posix/os_posix.cpp

@@ -331,8 +331,15 @@ char* os::reserve_memory_aligned(size_t size, size_t alignment, int file_desc) {
   return aligned_base;
 }
 
-int os::log_vsnprintf(char* buf, size_t len, const char* fmt, va_list args) {
-    return vsnprintf(buf, len, fmt, args);
+int os::vsnprintf(char* buf, size_t len, const char* fmt, va_list args) {
+  // All supported POSIX platforms provide C99 semantics.
+  int result = ::vsnprintf(buf, len, fmt, args);
+  // If an encoding error occurred (result < 0) then it's not clear
+  // whether the buffer is NUL terminated, so ensure it is.
+  if ((result < 0) && (len > 0)) {
+    buf[len - 1] = '\0';
+  }
+  return result;
 }
 
 int os::get_fileno(FILE* fp) {

src/hotspot/os/windows/os_windows.cpp

@@ -1494,13 +1494,39 @@ void os::get_summary_os_info(char* buf, size_t buflen) {
   if (nl != NULL) *nl = '\0';
 }
 
-int os::log_vsnprintf(char* buf, size_t len, const char* fmt, va_list args) {
-  int ret = vsnprintf(buf, len, fmt, args);
-  // Get the correct buffer size if buf is too small
-  if (ret < 0) {
-    return _vscprintf(fmt, args);
+int os::vsnprintf(char* buf, size_t len, const char* fmt, va_list args) {
+#if _MSC_VER >= 1900
+  // Starting with Visual Studio 2015, vsnprint is C99 compliant.
+  int result = ::vsnprintf(buf, len, fmt, args);
+  // If an encoding error occurred (result < 0) then it's not clear
+  // whether the buffer is NUL terminated, so ensure it is.
+  if ((result < 0) && (len > 0)) {
+    buf[len - 1] = '\0';
   }
-  return ret;
+  return result;
+#else
+  // Before Visual Studio 2015, vsnprintf is not C99 compliant, so use
+  // _vsnprintf, whose behavior seems to be *mostly* consistent across
+  // versions.  However, when len == 0, avoid _vsnprintf too, and just
+  // go straight to _vscprintf.  The output is going to be truncated in
+  // that case, except in the unusual case of empty output.  More
+  // importantly, the documentation for various versions of Visual Studio
+  // are inconsistent about the behavior of _vsnprintf when len == 0,
+  // including it possibly being an error.
+  int result = -1;
+  if (len > 0) {
+    result = _vsnprintf(buf, len, fmt, args);
+    // If output (including NUL terminator) is truncated, the buffer
+    // won't be NUL terminated.  Add the trailing NUL specified by C99.
+    if ((result < 0) || (result >= len)) {
+      buf[len - 1] = '\0';
+    }
+  }
+  if (result < 0) {
+    result = _vscprintf(fmt, args);
+  }
+  return result;
+#endif // _MSC_VER dispatch
 }
 
 static inline time_t get_mtime(const char* filename) {

src/hotspot/share/logging/logMessageBuffer.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -110,7 +110,7 @@ void LogMessageBuffer::vwrite(LogLevelType level, const char* fmt, va_list args)
 
     va_list copy;
     va_copy(copy, args);
-    written += (size_t)os::log_vsnprintf(current_buffer_position, remaining_buffer_length, fmt, copy) + 1;
+    written += (size_t)os::vsnprintf(current_buffer_position, remaining_buffer_length, fmt, copy) + 1;
     va_end(copy);
     if (written > _message_buffer_capacity - _message_buffer_size) {
       assert(attempts == 0, "Second attempt should always have a sufficiently large buffer (resized to fit).");

src/hotspot/share/logging/logTagSet.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -118,17 +118,17 @@ void LogTagSet::vwrite(LogLevelType level, const char* fmt, va_list args) {
   // Check that string fits in buffer; resize buffer if necessary
   int ret;
   if (prefix_len < vwrite_buffer_size) {
-    ret = os::log_vsnprintf(buf + prefix_len, sizeof(buf) - prefix_len, fmt, args);
+    ret = os::vsnprintf(buf + prefix_len, sizeof(buf) - prefix_len, fmt, args);
   } else {
     // Buffer too small. Just call printf to find out the length for realloc below.
-    ret = os::log_vsnprintf(buf, sizeof(buf), fmt, args);
+    ret = os::vsnprintf(buf, sizeof(buf), fmt, args);
   }
   assert(ret >= 0, "Log message buffer issue");
   if ((size_t)ret >= sizeof(buf)) {
     size_t newbuf_len = prefix_len + ret + 1;
     char* newbuf = NEW_C_HEAP_ARRAY(char, newbuf_len, mtLogging);
     prefix_len = _write_prefix(newbuf, newbuf_len);
-    ret = os::log_vsnprintf(newbuf + prefix_len, newbuf_len - prefix_len, fmt, saved_args);
+    ret = os::vsnprintf(newbuf + prefix_len, newbuf_len - prefix_len, fmt, saved_args);
     assert(ret >= 0, "Log message buffer issue");
     log(level, newbuf);
     FREE_C_HEAP_ARRAY(char, newbuf);

src/hotspot/share/oops/generateOopMap.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1997, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -23,7 +23,6 @@
  */
 
 #include "precompiled.hpp"
-#include "jvm.h"
 #include "interpreter/bytecodeStream.hpp"
 #include "logging/log.hpp"
 #include "logging/logStream.hpp"
@@ -33,6 +32,7 @@
 #include "oops/symbol.hpp"
 #include "runtime/handles.inline.hpp"
 #include "runtime/java.hpp"
+#include "runtime/os.hpp"
 #include "runtime/relocator.hpp"
 #include "runtime/timerTrace.hpp"
 #include "utilities/bitMap.inline.hpp"
@@ -2151,10 +2151,10 @@ void GenerateOopMap::compute_map(TRAPS) {
 void GenerateOopMap::error_work(const char *format, va_list ap) {
   _got_error = true;
   char msg_buffer[512];
-  vsnprintf(msg_buffer, sizeof(msg_buffer), format, ap);
+  os::vsnprintf(msg_buffer, sizeof(msg_buffer), format, ap);
   // Append method name
   char msg_buffer2[512];
-  jio_snprintf(msg_buffer2, sizeof(msg_buffer2), "%s in method %s", msg_buffer, method()->name()->as_C_string());
+  os::snprintf(msg_buffer2, sizeof(msg_buffer2), "%s in method %s", msg_buffer, method()->name()->as_C_string());
   if (Thread::current()->can_call_java()) {
     _exception = Exceptions::new_exception(Thread::current(),
                   vmSymbols::java_lang_LinkageError(), msg_buffer2);

src/hotspot/share/prims/jvm.cpp

@@ -2670,23 +2670,19 @@ extern "C" {
 
 ATTRIBUTE_PRINTF(3, 0)
 int jio_vsnprintf(char *str, size_t count, const char *fmt, va_list args) {
-  // see bug 4399518, 4417214
+  // Reject count values that are negative signed values converted to
+  // unsigned; see bug 4399518, 4417214
   if ((intptr_t)count <= 0) return -1;
 
-  int result = vsnprintf(str, count, fmt, args);
-  // Note: on truncation vsnprintf(3) on Unix returns numbers of
-  // characters which would have been written had the buffer been large
-  // enough; on Windows, it returns -1. We handle both cases here and
-  // always return -1, and perform null termination.
-  if ((result > 0 && (size_t)result >= count) || result == -1) {
-    str[count - 1] = '\0';
+  int result = os::vsnprintf(str, count, fmt, args);
+  if (result > 0 && (size_t)result >= count) {
     result = -1;
   }
 
   return result;
 }
 
-ATTRIBUTE_PRINTF(3, 0)
+ATTRIBUTE_PRINTF(3, 4)
 int jio_snprintf(char *str, size_t count, const char *fmt, ...) {
   va_list args;
   int len;
@@ -2696,7 +2692,7 @@ int jio_snprintf(char *str, size_t count, const char *fmt, ...) {
   return len;
 }
 
-ATTRIBUTE_PRINTF(2,3)
+ATTRIBUTE_PRINTF(2, 3)
 int jio_fprintf(FILE* f, const char *fmt, ...) {
   int len;
   va_list args;

src/hotspot/share/runtime/os.cpp

@@ -105,6 +105,14 @@ static time_t get_timezone(const struct tm* time_struct) {
 #endif
 }
 
+int os::snprintf(char* buf, size_t len, const char* fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  int result = os::vsnprintf(buf, len, fmt, args);
+  va_end(args);
+  return result;
+}
+
 // Fill in buffer with current local time as an ISO-8601 string.
 // E.g., yyyy-mm-ddThh:mm:ss-zzzz.
 // Returns buffer, or NULL if it failed.

src/hotspot/share/runtime/os.hpp

@@ -639,8 +639,10 @@ class os: AllStatic {
   static void *find_agent_function(AgentLibrary *agent_lib, bool check_lib,
                                    const char *syms[], size_t syms_len);
 
-  // Write to stream
-  static int log_vsnprintf(char* buf, size_t len, const char* fmt, va_list args) ATTRIBUTE_PRINTF(3, 0);
+  // Provide C99 compliant versions of these functions, since some versions
+  // of some platforms don't.
+  static int vsnprintf(char* buf, size_t len, const char* fmt, va_list args) ATTRIBUTE_PRINTF(3, 0);
+  static int snprintf(char* buf, size_t len, const char* fmt, ...) ATTRIBUTE_PRINTF(3, 4);
 
   // Get host name in buffer provided
   static bool get_host_name(char* buf, size_t buflen);

src/hotspot/share/utilities/exceptions.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 1998, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1998, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -33,6 +33,7 @@
 #include "runtime/init.hpp"
 #include "runtime/java.hpp"
 #include "runtime/javaCalls.hpp"
+#include "runtime/os.hpp"
 #include "runtime/thread.inline.hpp"
 #include "runtime/threadCritical.hpp"
 #include "utilities/events.hpp"
@@ -239,8 +240,7 @@ void Exceptions::fthrow(Thread* thread, const char* file, int line, Symbol* h_na
   va_list ap;
   va_start(ap, format);
   char msg[max_msg_size];
-  vsnprintf(msg, max_msg_size, format, ap);
-  msg[max_msg_size-1] = '\0';
+  os::vsnprintf(msg, max_msg_size, format, ap);
   va_end(ap);
   _throw_msg(thread, file, line, h_name, msg);
 }

src/hotspot/share/utilities/globalDefinitions_visCPP.hpp

@@ -147,14 +147,6 @@ inline int g_isfinite(jdouble f)                 { return _finite(f); }
 #pragma warning( disable : 4996 ) // unsafe string functions. Same as define _CRT_SECURE_NO_WARNINGS/_CRT_SECURE_NO_DEPRICATE
 #endif
 
-inline int vsnprintf(char* buf, size_t count, const char* fmt, va_list argptr) {
-  // If number of characters written == count, Windows doesn't write a
-  // terminating NULL, so we do it ourselves.
-  int ret = _vsnprintf(buf, count, fmt, argptr);
-  if (count > 0) buf[count-1] = '\0';
-  return ret;
-}
-
 // Portability macros
 #define PRAGMA_INTERFACE
 #define PRAGMA_IMPLEMENTATION

src/hotspot/share/utilities/ostream.cpp

@@ -96,19 +96,14 @@ const char* outputStream::do_vsnprintf(char* buffer, size_t buflen,
     result_len = strlen(result);
     if (add_cr && result_len >= buflen)  result_len = buflen-1;  // truncate
   } else {
-    // Handle truncation:
-    // posix: upon truncation, vsnprintf returns number of bytes which
-    //   would have been written (excluding terminating zero) had the buffer
-    //   been large enough
-    // windows: upon truncation, vsnprintf returns -1
-    const int written = vsnprintf(buffer, buflen, format, ap);
+    int written = os::vsnprintf(buffer, buflen, format, ap);
+    assert(written >= 0, "vsnprintf encoding error");
     result = buffer;
-    if (written < (int) buflen && written >= 0) {
+    if ((size_t)written < buflen) {
       result_len = written;
     } else {
       DEBUG_ONLY(warning("increase O_BUFLEN in ostream.hpp -- output truncated");)
       result_len = buflen - 1;
-      buffer[result_len] = 0;
     }
   }
   if (add_cr) {

test/hotspot/gtest/runtime/test_os.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2016, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2016, 2018, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -22,7 +22,9 @@
  */
 
 #include "precompiled.hpp"
+#include "memory/resourceArea.hpp"
 #include "runtime/os.hpp"
+#include "utilities/ostream.hpp"
 #include "unittest.hpp"
 
 static size_t small_page_size() {
@@ -150,3 +152,118 @@ TEST_VM_ASSERT_MSG(os, page_size_for_region_with_zero_min_pages, "sanity") {
   os::page_size_for_region_aligned(region_size, 0); // should assert
 }
 #endif
+
+//////////////////////////////////////////////////////////////////////////////
+// Test os::vsnprintf and friends.
+
+static void check_snprintf_result(int expected, size_t limit, int actual, bool expect_count) {
+  if (expect_count || ((size_t)expected < limit)) {
+    ASSERT_EQ(expected, actual);
+  } else {
+    ASSERT_GT(0, actual);
+  }
+}
+
+// PrintFn is expected to be int (*)(char*, size_t, const char*, ...).
+// But jio_snprintf is a C-linkage function with that signature, which
+// has a different type on some platforms (like Solaris).
+template<typename PrintFn>
+static void test_snprintf(PrintFn pf, bool expect_count) {
+  const char expected[] = "abcdefghijklmnopqrstuvwxyz";
+  const int expected_len = sizeof(expected) - 1;
+  const size_t padding_size = 10;
+  char buffer[2 * (sizeof(expected) + padding_size)];
+  char check_buffer[sizeof(buffer)];
+  const char check_char = '1';  // Something not in expected.
+  memset(check_buffer, check_char, sizeof(check_buffer));
+  const size_t sizes_to_test[] = {
+    sizeof(buffer) - padding_size,       // Fits, with plenty of space to spare.
+    sizeof(buffer)/2,                    // Fits, with space to spare.
+    sizeof(buffer)/4,                    // Doesn't fit.
+    sizeof(expected) + padding_size + 1, // Fits, with a little room to spare
+    sizeof(expected) + padding_size,     // Fits exactly.
+    sizeof(expected) + padding_size - 1, // Doesn't quite fit.
+    2,                                   // One char + terminating NUL.
+    1,                                   // Only space for terminating NUL.
+    0 };                                 // No space at all.
+  for (unsigned i = 0; i < ARRAY_SIZE(sizes_to_test); ++i) {
+    memset(buffer, check_char, sizeof(buffer)); // To catch stray writes.
+    size_t test_size = sizes_to_test[i];
+    ResourceMark rm;
+    stringStream s;
+    s.print("test_size: " SIZE_FORMAT, test_size);
+    SCOPED_TRACE(s.as_string());
+    size_t prefix_size = padding_size;
+    guarantee(test_size <= (sizeof(buffer) - prefix_size), "invariant");
+    size_t write_size = MIN2(sizeof(expected), test_size);
+    size_t suffix_size = sizeof(buffer) - prefix_size - write_size;
+    char* write_start = buffer + prefix_size;
+    char* write_end = write_start + write_size;
+
+    int result = pf(write_start, test_size, "%s", expected);
+
+    check_snprintf_result(expected_len, test_size, result, expect_count);
+
+    // Verify expected output.
+    if (test_size > 0) {
+      ASSERT_EQ(0, strncmp(write_start, expected, write_size - 1));
+      // Verify terminating NUL of output.
+      ASSERT_EQ('\0', write_start[write_size - 1]);
+    } else {
+      guarantee(test_size == 0, "invariant");
+      guarantee(write_size == 0, "invariant");
+      guarantee(prefix_size + suffix_size == sizeof(buffer), "invariant");
+      guarantee(write_start == write_end, "invariant");
+    }
+
+    // Verify no scribbling on prefix or suffix.
+    ASSERT_EQ(0, strncmp(buffer, check_buffer, prefix_size));
+    ASSERT_EQ(0, strncmp(write_end, check_buffer, suffix_size));
+  }
+
+  // Special case of 0-length buffer with empty (except for terminator) output.
+  check_snprintf_result(0, 0, pf(NULL, 0, "%s", ""), expect_count);
+  check_snprintf_result(0, 0, pf(NULL, 0, ""), expect_count);
+}
+
+// This is probably equivalent to os::snprintf, but we're being
+// explicit about what we're testing here.
+static int vsnprintf_wrapper(char* buf, size_t len, const char* fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  int result = os::vsnprintf(buf, len, fmt, args);
+  va_end(args);
+  return result;
+}
+
+TEST(os, vsnprintf) {
+  test_snprintf(vsnprintf_wrapper, true);
+}
+
+TEST(os, snprintf) {
+  test_snprintf(os::snprintf, true);
+}
+
+// These are declared in jvm.h; test here, with related functions.
+extern "C" {
+int jio_vsnprintf(char*, size_t, const char*, va_list);
+int jio_snprintf(char*, size_t, const char*, ...);
+}
+
+// This is probably equivalent to jio_snprintf, but we're being
+// explicit about what we're testing here.
+static int jio_vsnprintf_wrapper(char* buf, size_t len, const char* fmt, ...) {
+  va_list args;
+  va_start(args, fmt);
+  int result = jio_vsnprintf(buf, len, fmt, args);
+  va_end(args);
+  return result;
+}
+
+TEST(os, jio_vsnprintf) {
+  test_snprintf(jio_vsnprintf_wrapper, false);
+}
+
+TEST(os, jio_snprintf) {
+  test_snprintf(jio_snprintf, false);
+}