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

[Kim Barrett]

> On Feb 21, 2018, at 2:30 AM, Thomas Stüfe <thomas.stuefe at gmail.com> wrote:
> 
> Hi Kim,
> 
> this is good. Please find comments inline / below.

Thanks for looking at it.  Responses inline.

> On Wed, Feb 21, 2018 at 1:08 AM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> (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?

os is where HotSpot generally deals with such platform variances, so I
think it's the right place for this. jvm.h provides access to some VM
facilities by non-VM code. I think it's somewhat strange that HotSpot
C++ code is also a user of this *C* API; especially considering the
noted deficiencies.

That os.hpp drags in a bunch of stuff is a separate issue. I
personally liked the suggestion of making os a namespace, allowing it
to be broken down into more fine-grained components. That's a
discussion that is well out of scope for the problem at hand though.
(BTW, it's not clear why os.hpp includes jvm.h.)


>> We still provide os::log_vsnprintf […]
>> 
> 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)

Renaming requires a better name.  I'm open to suggestions.

Moving it to the logging component means logging then needs to deal
with the platform variances, or defer back to os somehow, in which
case we're pretty much back here and looking at the other options.

Providing a portable wrapper for _vscprintf instead of
os::log_vsnprintf seems like it just makes things harder for callers.
It probably doesn't matter much, but it also adds overhead on
platforms where log_vsnprintf can be implemented directly in terms
of ::vsnprintf and _vscprintf functionality would also use that; on
truncation a caller would typically end up making 3 calls to
vsnprintf, rather than two.

> src/hotspot/os/posix/os_posix.cpp
> 
> +  if (len > 0) buf[len - 1] = '\0';
> Style nit: brackets ?

I think it was okay as written, according to the style guide, but I changed it anyway.

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

You are right, that’s a change.  Well spotted.

I think the old behavior here, which matches the documentation, is
probably preferable; it indicates we didn't even write a terminating
NUL, so the buffer may not be terminated.  I'm going to change
os::vsnprintf accordingly.  Of course, this is very much a corner case,
since an empty format string is pretty rare.  But it could also arise with,
for example, a format string of “%s” with an empty argument string.

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

Agreed.  Updated based on your suggestion, with adjustment for the previous item.

> test/hotspot/gtest/runtime/test_os.cpp:
> 
> +// Test os::vmsprintf and friends
> 
> Typo. 

Fixed.

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

Agreed the test can be improved along the lines you suggested.  I’ve done so, though
not exactly using the proposed replacement.

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

I ended up doing that anyway.

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

Not by much, as it turns out.