Implement snprintf for MSVC with correct return value.

[zturner]

I hadn't thought to write to /dev/null to get the length. Good idea. But why do you check for !errno? The docs just say that if _vsnprintf fails it returns -1, it doesn't say anything about errno.

[chaoren]

https://msdn.microsoft.com/en-us/library/1kt27hek.aspx

If buffer or format is NULL, or if count is less than or equal to zero,
these functions invoke the invalid parameter handler, as described in Parameter
Validation https://msdn.microsoft.com/en-us/library/ksazx244.aspx. If
execution is allowed to continue, these functions return -1 *and set *
*errno to EINVAL*.

[zturner]

Yea, I saw that, but it only says that it sets errno in the case that buffer==nullptr or count==0 (not sure why it says count<=0 since count is an unsigned type). So as far as I can tell the value of errno is undefined in the case that buffer, format, and count are all valid, but count is just not big enough (which is really the case we're trying to handle anyway).

Even in the case the MSDN doc talks about, you either won't return from the function at all (i.e. it will abort) or it will return -1. So I think it's best to remove the errno check. Otherwise lgtm.

[chaoren]

So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).

Yeah, errno should be 0 only in the case that count is not big enough,
which is exactly what I'm checking for. (It's the caller's responsibility
to clear errno before calling snprintf). Otherwise, there's an error, and
it should be returned as is, and the fprintf wouldn't be necessary.

[chaoren]

So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).

FWIW, I think it's not defined in the MSDN docs because it's already
defined in the C standard for all system/library calls.

[zturner]

In D10048#179404, @chaoren wrote:

So as far as I can tell the value of errno is undefined in the case that
buffer, format, and count are all valid, but count is just not big
enough (which is really the case we're trying to handle anyway).

FWIW, I think it's not defined in the MSDN docs because it's already
defined in the C standard for all system/library calls.

Well, the whole reason we have to write this function is because Microsoft's implementation doesn't conform :) So I don't think we should assume they conform for anything else either.

In any case, I just tested it with this code:

int main(int argc, char* argv[])
{
    char buffer[10];

    errno = 0;
    snprintf(buffer, 10, "This is a test of the snprintf function and buffer size");

return 0;

}

When snprintf calls _vsnprintf, _vsnprintf returns -1 as expected, and sets errno to 34. So the check is definitely wrong currently.

[chaoren]

Well, the whole reason we have to write this function is because
Microsoft's implementation doesn't conform :) So I don't think we should
assume they conform for anything else either.

https://memegen.googleplex.com/templateimage?template=burn

Should we just check for ERANGE (34) then? We need to save and restore
errno in that case to conform with the C standards. But I'm not sure about
using an undocumented behavior.

[amccarth]

source/Host/windows/Windows.cpp

228

I think there's still a problem here. _vsnprintf will not terminate if the intended content exactly fills count. My understanding on other platforms is that vsnprintf will always terminate.

I'm reading between these lines from MSDN:
"vsnprintf ,_vsnprintf, and _vsnwprintf return the number of characters written if the number of characters to write is less than or equal to count; if the number of characters to write is greater than count, these functions return -1 indicating that output has been truncated. The return value does not include the terminating null, if one is written."

"If there is room at the end (that is, if the number of characters to write is less than count), the buffer will be null-terminated."

[zturner]

In D10048#179475, @chaoren wrote:

Well, the whole reason we have to write this function is because
Microsoft's implementation doesn't conform :) So I don't think we should
assume they conform for anything else either.

https://memegen.googleplex.com/templateimage?template=burn

Should we just check for ERANGE (34) then? We need to save and restore
errno in that case to conform with the C standards. But I'm not sure about
using an undocumented behavior.

I don't think we should check for ERANGE because the documentation doesn't even say that it returns ERANGE. I think we should just check for -1 and ignore errno. If you want to save and restore errno you can do that by saving it to a temporary variable first, and restoring it if _vsnprintf returns >= 0.

I'm not sure if that will be 100% standards conforming, but it will at least work correctly :)

[chaoren]

• Correct handling of errno for when count <= bytes (would be) written.

[zturner]

looks good.

[amccarth]

I still think there's a bug where the string can be unterminated.

[chaoren]

I still think there's a bug where the string can be unterminated.

Do you mean the case where the number of characters written (no terminating
'\0') is the same as count? buffer[count-1] = '\0' should handle that.

[amccarth]

Oh, I missed that line. Yes, that's what I meant and it looks like this handles it. Thanks.

include/lldb/Host/windows/win32.h

 
 #include <inttypes.h>
 #include <stdint.h>
 #include <io.h>
 typedef unsigned short mode_t;
 
 #ifdef LLDB_DISABLE_PYTHON
 typedef uint32_t pid_t;
-#endif
+#endif // LLDB_DISABLE_PYTHON
 
 int usleep(uint32_t useconds);
 
 char* getcwd(char* path, int max);
 int chdir(const char* path);
 char* basename(char *path);
 char *dirname(char *path);
 
 int strcasecmp(const char* s1, const char* s2);
 int strncasecmp(const char* s1, const char* s2, size_t n);
+int snprintf(char *buffer, size_t count, const char *format, ...);
 
 #define STDIN_FILENO  0
 #define STDOUT_FILENO 1
 #define STDERR_FILENO 2
 
 #define __PRETTY_FUNCTION__ __FUNCSIG__
 
 #define S_IFDIR  _S_IFDIR
 #define S_ISDIR(mode)  (((mode) & S_IFMT) == S_IFDIR)
 
-#define snprintf _snprintf
-#endif
+#endif // _MSC_VER
 
 // timespec
 struct timespec
 {
     time_t tv_sec;
     long   tv_nsec;
 };
 
 
 #endif  // LLDB_lldb_win32_h_

source/Host/windows/Windows.cpp

 
 int strncasecmp(const char* s1, const char* s2, size_t n)
 {
     return strnicmp(s1, s2, n);
 }
 
 int usleep(uint32_t useconds)
 {
-  Sleep(useconds / 1000);
-  return 0;
+    Sleep(useconds / 1000);
+    return 0;
 }
 
+int snprintf(char *buffer, size_t count, const char *format, ...)
+{
+    int old_errno = errno;
+    va_list argptr;
+    va_start(argptr, format);
+    int r = vsnprintf(buffer, count, format, argptr);
+    int new_errno = errno;
+    buffer[count-1] = '\0';
+    if (r == -1 || r == count)
+    {
+        FILE *nul = fopen("nul", "w");
+        r = vfprintf(nul, format, argptr);
+        fclose(nul);
+        errno = r < count ? new_errno : old_errno;
+    }
+    va_end(argptr);
+    return errno ? -1 : r;
+}
+
 #endif // _MSC_VER