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

Wed Feb 21 08:30:53 UTC 2018

On Wed, Feb 21, 2018 at 9:23 AM, Marcus Larsson <marcus.larsson at oracle.com>
wrote:

> 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.
>
>
Arguably, people tend to check - if they check at all -  for -1 than for
result > sizeof input buffer.

But I also see that this argument goes both ways, because strictly speaking
you should check for both -1 and truncation and handle them differently, so
your way would be more correct.

Best Regards, Thomas

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