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

[Marcus Larsson]

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:
>
>> 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)

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.

Thanks,
Marcus

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