[IOT-1561] Improve vs12_snprintf
authorDan Mihai <Daniel.Mihai@microsoft.com>
Tue, 15 Nov 2016 08:23:04 +0000 (00:23 -0800)
committerDave Thaler <dthaler@microsoft.com>
Tue, 22 Nov 2016 00:22:02 +0000 (00:22 +0000)
Improve the snprintf surrogate used to build for Windows using Visual
Studio 2013 or older.

Also remove the dependency on cmake for building gtest for Windows, to
fix a build break for the new snprintf_test.cpp.

Change-Id: I70c9d0766a9a316a19cbd283671137e61ab88dbf
Signed-off-by: Dan Mihai <Daniel.Mihai@microsoft.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/14387
Reviewed-by: Kevin Kane <kkane@microsoft.com>
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>
Reviewed-by: Dave Thaler <dthaler@microsoft.com>
extlibs/gtest/SConscript
resource/c_common/windows/src/snprintf.c
resource/c_common/windows/test/SConscript [new file with mode: 0644]
resource/c_common/windows/test/snprintf_test.cpp [new file with mode: 0644]
resource/unit_tests.scons

index 9e17973..d127ccf 100644 (file)
@@ -97,71 +97,18 @@ elif target_os == 'msys_nt':
                        gtest_env.Configure(gtest_dir, 'mv libgtest_main.a lib/.libs')
 
 elif target_os == 'windows':
-       if os.path.exists(gtest_dir):
-               if gtest_env.get('RELEASE'):
-                       output_dir = os.path.join(gtest_dir, 'Release') + os.sep
-               else:
-                       output_dir = os.path.join(gtest_dir, 'Debug') + os.sep
-
-               # Three parts to the gtest config string...
-               # 1. "Visual Studio" toolchain name.
-               # 2. VS Version + Year ("14 2015", "12 2013").
-               # 3. Target Architecture ("Win64", "Win32").
-               vs_version_year = ""
-               vs_target_arch  = ""
-               vs_num = env['MSVC_VERSION']
-               if "12.0" in vs_num:
-                       vs_version_year = "Visual Studio 12 2013"
-               elif "14.0" in vs_num:
-                       vs_version_year = "Visual Studio 14 2015"
-               else:
-                       print "Error: unknown Visual Studio version %s" % vs_num
-
-               vs_arch = env['TARGET_ARCH']
-               if "amd64" in vs_arch:
-                       vs_target_arch = "Win64"
-               elif "x86" in vs_arch:
-                       vs_target_arch = ""
-               else:
-                       print "Error: unknown Visual Studio target arch %s" % vs_arch
-
-               vs_target_string = vs_version_year + " " + vs_target_arch
-
-               # Exit if we didn't get a match for one of the above.
-               if not vs_version_year or not vs_target_arch:
-                       Exit(1)
-
-               if not gtest_env.WhereIs('cmake', gtest_env.get('PATH')):
-                       print '''*********************** Error ************************
-*                                                    *
-* Please make sure that CMake is in your System PATH *
-*                                                    *
-* You can download CMake from:                       *
-*            https://cmake.org/download/             *
-*                                                    *
-******************************************************
-'''
-                       Exit(1)
-
-               if not os.path.exists(gtest_lib_dir):
-                       # Create lib dir
-                       os.mkdir(gtest_lib_dir)
-                       os.mkdir(gtest_dotlib_dir)
-
-                       # Run configure on gtest
-                       print 'Configuring google unit test for compilation'
-                       gtest_env.Configure(gtest_dir, 'cmake . -G"' + vs_target_string + '" -Dgtest_force_shared_crt=ON')
-
-                       # Run make on gtest
-                       print 'Making google unit test'
-                       gtest_env.Configure(gtest_dir, 'msbuild gtest.vcxproj')
-                       gtest_env.Configure(gtest_dir, 'msbuild gtest_main.vcxproj')
-
-                       print 'Moving libraries to lib folder'
-                       gtest_env.Configure(gtest_dir, 'copy '+output_dir+'gtest.lib %s' % gtest_lib_dir)
-                       gtest_env.Configure(gtest_dir, 'move '+output_dir+'gtest.lib %s' % gtest_dotlib_dir )
-                       gtest_env.Configure(gtest_dir, 'copy '+output_dir+'gtest_main.lib %s' % gtest_lib_dir)
-                       gtest_env.Configure(gtest_dir, 'move '+output_dir+'gtest_main.lib %s' % gtest_dotlib_dir)
+       # Avoid building the same StaticLibrary in more than one environment, by using the
+       # IOTIVITY_GTEST_HAS_BEEN_BUILT environment variable
+       if not env.has_key('IOTIVITY_GTEST_HAS_BEEN_BUILT'):
+               gtest_env.Append(CPPPATH = [ gtest_dir ])
+               gtest = gtest_env.StaticLibrary(target = 'gtest', source = [ '%s/src/gtest-all.cc' % gtest_dir ])
+               gtest_main = gtest_env.StaticLibrary(target = 'gtest_main', source = [ '%s/src/gtest_main.cc' % gtest_dir ])
+               gtest_env.InstallTarget(gtest, 'gtest')
+               gtest_env.InstallTarget(gtest_main, 'gtest_main')
+
+               vars = Variables();
+               vars.AddVariables(('IOTIVITY_GTEST_HAS_BEEN_BUILT', '', '1'))
+               vars.Update(env)
 
 # Export flags once for all
 if target_os in targets_need_gtest:
@@ -177,5 +124,7 @@ if target_os in targets_need_gtest:
                        gtest_env.AppendUnique(CXXFLAGS = ['-pthread'])
                        gtest_env.PrependUnique(LIBS = ['pthread'])
        gtest_env.PrependUnique(LIBS = ['gtest', 'gtest_main'])
+       if target_os in ['windows']:
+               gtest_env.AppendUnique(LINKFLAGS = ['/subsystem:CONSOLE'])
 
 Return('gtest_env')
index c9c7de5..1a65930 100644 (file)
 
 int vs12_snprintf(char *buffer, size_t count, const char *format, ...)
 {
-    va_list args;
+    // These three values are invalid when calling _vsnprintf
+    if ((buffer == NULL) || (count == 0) || (format == NULL))
+    {
+        return -1;
+    }
 
+    // _vsnprintf behaves similarly to snprintf, but:
+    //
+    // - It doesn't always add a zero terminator to the output buffer
+    // - When the output buffer is too small, it returns -1 instead of the
+    //   required buffer size
+    va_list args;
     va_start(args, format);
-
     int length = _vsnprintf(buffer, count, format, args);
-
     va_end(args);
 
-    if (length > count)
+    if ((length == -1) || (length == count))
     {
+        // Add the missing zero character terminator.
         buffer[count - 1] = '\0';
+
+        // When length == -1, it would be good to compute the required output
+        // buffer size, and return that computed value instead of -1. That would
+        // bring the behavior of vs12_snprintf closer to the C99 version of
+        // snprintf. However, IoTivity doesn't have an easy way to compute the
+        // required output buffer size. Also, there are no known callers of
+        // snprintf in IoTivity that are relying on a compliant return value,
+        // when that compliant return value would be larger than the 'count'
+        // input parameter.
+        //
+        // @todo Compute and return the output buffer size, instead of -1,
+        //       when the output buffer was too small.
     }
 
     return length;
 }
-
diff --git a/resource/c_common/windows/test/SConscript b/resource/c_common/windows/test/SConscript
new file mode 100644 (file)
index 0000000..fbffdfb
--- /dev/null
@@ -0,0 +1,53 @@
+#******************************************************************
+#
+# Copyright 2016 Microsoft
+#
+#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+#-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
+
+import os.path
+
+######################################################################
+# Build the gtest framework
+######################################################################
+gtest_env = SConscript('#extlibs/gtest/SConscript')
+
+c_common_windows_test_env = gtest_env.Clone()
+
+######################################################################
+# Build flags
+######################################################################
+c_common_windows_test_env.PrependUnique(CPPPATH = ['../include'])
+
+c_common_windows_test_env.AppendUnique(LIBPATH = [os.path.join(c_common_windows_test_env.get('BUILD_DIR'), 'resource', 'c_common')])
+c_common_windows_test_env.PrependUnique(LIBS = ['win_helper'])
+
+if c_common_windows_test_env.get('LOGGING'):
+    c_common_windows_test_env.AppendUnique(CPPDEFINES = ['TB_LOG'])
+
+######################################################################
+# Source files and Targets
+######################################################################
+c_common_windows_tests = c_common_windows_test_env.Program('c_common_windows_tests', ['snprintf_test.cpp'])
+
+Alias("test", [c_common_windows_tests])
+
+c_common_windows_test_env.AppendTarget('test')
+
+from tools.scons.RunTest import *
+run_test(c_common_windows_test_env,
+         'resource_c_common_windows_test.memcheck',
+         'resource/c_common/windows/test/c_common_windows_tests')
diff --git a/resource/c_common/windows/test/snprintf_test.cpp b/resource/c_common/windows/test/snprintf_test.cpp
new file mode 100644 (file)
index 0000000..cce6276
--- /dev/null
@@ -0,0 +1,122 @@
+/* *****************************************************************
+ *
+ * Copyright 2016 Microsoft
+ *
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ *
+ * *****************************************************************/
+
+#include <windows.h>
+#include <gtest/gtest.h>
+#include "vs12_snprintf.h"
+
+void OutputBufferLargerThanNeeded(size_t outputBufferSize, size_t inputBufferSize, int callerLineNumber)
+{
+    ASSERT_GT(outputBufferSize, inputBufferSize);
+    ASSERT_GT(inputBufferSize, 1);
+
+    char *inputBuffer = new char[inputBufferSize];
+    memset(inputBuffer, 'i', inputBufferSize - 1);
+    inputBuffer[inputBufferSize - 1] = '\0';
+
+    char *outputBuffer = new char[outputBufferSize];
+    memset(outputBuffer, 'o', outputBufferSize);
+
+    EXPECT_EQ((inputBufferSize - 1),
+              vs12_snprintf(outputBuffer, outputBufferSize, "%s", inputBuffer))
+              << "caller line number: " << callerLineNumber;
+    EXPECT_STREQ(inputBuffer, outputBuffer) << "caller line number: " << callerLineNumber;
+
+    delete[] inputBuffer;
+    delete[] outputBuffer;
+}
+
+void OutputBufferSmallerThanNeeded(size_t outputBufferSize, size_t inputBufferSize, int callerLineNumber)
+{
+    ASSERT_GT(inputBufferSize, outputBufferSize);
+    ASSERT_GT(outputBufferSize, 1);
+
+    char *inputBuffer = new char[inputBufferSize];
+    memset(inputBuffer, 'i', inputBufferSize - 1);
+    inputBuffer[inputBufferSize - 1] = '\0';
+
+    char *outputBuffer = new char[outputBufferSize];
+    memset(outputBuffer, 'o', outputBufferSize);
+
+    if (outputBufferSize == (inputBufferSize - 1))
+    {
+        // When the output buffer size is just off by 1, the return value of
+        // vs12_snprintf is C99-compliant.
+        EXPECT_EQ((inputBufferSize - 1),
+                   vs12_snprintf(outputBuffer, outputBufferSize, "%s", inputBuffer))
+                   << "caller line number: " << callerLineNumber;
+    }
+    else
+    {
+        // vs12_snprintf returns -1 in this case (a non C99-compliant value).
+        EXPECT_EQ(-1,
+                  vs12_snprintf(outputBuffer, outputBufferSize, "%s", inputBuffer))
+                  << "caller line number: " << callerLineNumber;
+    }
+
+    // snprintf output should be zero-terminated, even when the output buffer was too small.
+    EXPECT_EQ('\0', outputBuffer[outputBufferSize - 1]) << "caller line number: " << callerLineNumber;
+
+    // Truncate the input buffer, then compare it with the output buffer.
+    inputBuffer[outputBufferSize - 1] = '\0';
+    EXPECT_STREQ(inputBuffer, outputBuffer) << "caller line number: " << callerLineNumber;
+
+    delete[] inputBuffer;
+    delete[] outputBuffer;
+}
+
+TEST(vs12_snprintfTest, OutputBufferLargerThanNeeded1)
+{
+    OutputBufferLargerThanNeeded(10, 10 - 1, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferLargerThanNeeded2)
+{
+    OutputBufferLargerThanNeeded(123, 123 - 2, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferAsLargeAsNeeded)
+{
+    char outputBuffer[54];
+    char inputBuffer[sizeof(outputBuffer)];
+
+    memset(inputBuffer, 'i', sizeof(inputBuffer) - 1);
+    inputBuffer[sizeof(inputBuffer) - 1] = '\0';
+
+    memset(outputBuffer, 'o', sizeof(outputBuffer));
+
+    EXPECT_EQ((sizeof(inputBuffer) - 1),
+               vs12_snprintf(outputBuffer, sizeof(outputBuffer), "%s", inputBuffer));
+    EXPECT_STREQ(inputBuffer, outputBuffer);
+}
+
+TEST(vs12_snprintfTest, OutputBufferSmallerThanNeeded1)
+{
+    OutputBufferSmallerThanNeeded(100, 100 + 1, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferSmallerThanNeeded2)
+{
+    OutputBufferSmallerThanNeeded(123, 123 + 2, __LINE__);
+}
+
+TEST(vs12_snprintfTest, OutputBufferSmallerThanNeeded3)
+{
+    OutputBufferSmallerThanNeeded(432, 2 * 432, __LINE__);
+}
index 40b1e79..fd18284 100644 (file)
@@ -66,6 +66,9 @@ elif target_os == 'windows' and env.get('TEST') == '1':
        # get it and install it
        SConscript('#extlibs/gtest/SConscript')
 
+       # Build Common unit tests
+       SConscript('c_common/windows/test/SConscript')
+
        # Build C stack's unit tests.
        SConscript('csdk/stack/test/SConscript')
        SConscript('csdk/connectivity/test/SConscript')