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

[Thomas Stüfe]

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:

> Please review this change to how HotSpot handles platform variations
> of vsnprintf and snprintf.
>
> We propose to provide os::vsnprintf and os::snprintf, which have the
> agreed behavior for such functions for use in HotSpot; see
> jio_vsnprintf and friends. Specifically, (1) always NUL-terminate the
> target buffer unless its indicated size is zero, and (2) return -1 on
> output truncation.  This places the platform dependent code in
> OS-specific files where it belongs (os_windows.cpp and os_posix.cpp),
> while giving the rest of HotSpot a consistent API to use.
>
> An additional benefit is that these new os functions are decorated
> with format attributes, so will produce warnings for incorrect calls
> (on platforms which support the decorations and associated
> checking). The jio_ variants have the attributes in jvm.cpp rather
> than jvm.h, making them largely useless. (Maybe that's just a bug? But
> jvm.h doesn't have the infrastructure for platform-dependent
> configuration that is available for in-HotSpot code.)  However, to
> really get the benefit of this, we will need to change HotSpot code to
> consistently use the os functions, rather than the jio_ equivalents.
> (That would have the additional benefit of not needing to include
> jvm.h all over the place just to have access to the jio_ functions.)
>

os.hpp is no lightweight alternative though. It includes quite a lot of
other headers, including jvm.h :)

Also system headers like <setjmp.h>. And then, whatever comes with the
os_xxx_xxx.h files.

So, this is the part I do not like much about this change, it forces us to
include a lot of stuff where before we would just include jvm.h or just
roll with raw ::snprintf(). Can we disentangle the header dep better?


> That's a change for later, not part of this change.
>
> We still provide os::log_vsnprintf, which differs from the new
> os::vsnprintf in the return value when output truncation occurs.  (It
> returns the size the output would have been had it not been
> truncated.)  It has been changed to always NUL-terminate the output,
> and documented as such.  None of the current uses care, and this makes
> it consistent with os::vsnprintf.  Note that os::log_vsnprintf was
> added as part of UL and is presently only used by it.  Maybe it could
> have a better name?
>

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)


> This change leaves no direct calls to vsnprintf in HotSpot, outside of
> the relevant parts of the os implementation. However, there are a lot
> of direct calls to snprintf, potentially resulting in strings that are
> not NUL-terminated. Those should perhaps be calling os::snprintf (or
> previously, jio_snprintf), but that's another change for later.
>
> CR:
> https://bugs.openjdk.java.net/browse/JDK-8196882
>
> Webrev:
> http://cr.openjdk.java.net/~kbarrett/8196882/open.00/
>
> Testing:
> Mach5 {hs,jdk}-tier{1,2,3} (used VS2013 for Windows testing)
> Also built with VS2017.
>
>
The changes:

src/hotspot/os/posix/os_posix.cpp

+  if (len > 0) buf[len - 1] = '\0';
Style nit: brackets ?

src/hotspot/share/prims/jvm.cpp

Small behaviour change for input buffers of 0 and format strings of "".
Before we returned -1, now (I guess) 0. Does any existing caller care? I
looked but could not find anyone even checking the return code, so it is
probably nothing.

src/hotspot/share/runtime/os.hpp

Comment to os::(v)snprintf: "These functions return -1 if the
+  // output has been truncated, rather than returning the number of
characters
+  // that would have been written (exclusive of the terminating NUL) if the
+  // output had not been truncated."

I would not describe what the functions do NOT. Only what they do - would
be a bit clearer.

Proposal for a more concise version (feel free to reformulate, I am no
native speaker):

"os::snprintf and os::vsnprintf are identical to snprintf(3) and
vsnprintf(3) except in the following points:
- On truncation they will return -1.
- On truncation, the output string will be zero terminated, unless the
input buffer length is 0.
"

test/hotspot/gtest/runtime/test_os.cpp:

+// Test os::vmsprintf and friends

Typo.

--
Thinking about it I think the test could be made a bit denser and simpler
and have more coverage. How about this instead:

static void test_snprintf(int (*pf)(char*, size_t, const char*, ...), bool
expect_count) {

  const char expected[] = "0123456789012345678901234567890123456789";
  char buffer[sizeof(expected) + 4];
  const int lengths_to_test[] = { sizeof(buffer), ......,  1, 0, -1 };
  for (int i = 0; lengths_to_test[i] != -1; i ++) {
    int length = lengths_to_test[i];
    memset(buffer, 'x', sizeof(buffer)); // to catch overwriters
    int result = pf(buffer, length, "%s", expected);
    if (length > 0) {
      ASSERT_EQ('\0', buffer[length - 1]); // expect terminating zero
      if (length > sizeof(expected)) {
        ASSERT_EQ(0, strcmp(buffer, expected)); // expect the whole string
to fit
ASSERT_EQ(result, strlen(expected))
      } else {
        ASSERT_EQ(0, strncmp(buffer, expected, length)); // expect
truncation
ASSERT_EQ(result, expect_count ? strlen(expected) : -1)
      }
    }
    ASSERT_EQ('x', buffer[lengths[i]]); // canary
  }
}

(for real paranoia one could check also for leading overwriters by writing
to buffer[1] and checking buffer[0] for 'x', but I do not think this is
necessary.)

And should we move log_snprintf to logging, the tests would get a bit
simpler too.

Thanks and Kind Regards, Thomas