Set Certain Warnings as Errors Unconditionally (#890)

Some constructs that are allowed in C are well-established to be virtually always erroneous, e.g. calling a function that hasn't been declared, implicitly binding a pointer to a different type, and implicitly converting an integral to a pointer. Implicitly converting a pointer-to-const to a pointer-to-mutable is not always an error, but is problematic enough that an explicit cast should be required.

Many of these errors will be caught at a later time, but only due to test failures, linker failures, or on a build on CI that enables some warnings-as-errors (currently never on Windows).

This change instead makes such constructs always-errors, thus adding a barrier to any changes that produce such errors that might otherwise have been missed. The selected warnings are well-established and detected by all modern compilers, and are very unlikely to appear anew do to compiler upgrades.

Additional warnings that are not so definitely-wrong (etc. sign conversion/compare, unused variables) are not addressed by these changes.

* MSVC fixes

* More warnings, and fixes for C-only warnings

* Only enable warnings in certain subdirectories

* Include mongoc-warnings in dist

* Disable some warnings on GCC < 5.0

* Use an OBJECT library for bundled zlib

* Disable compiler warnings for zlib

* Document MaintainerFlags versus MongoC-Warnings

Author: vector-of-bool

CMakeLists.txt

@@ -130,6 +130,8 @@ endif ()
 include (LoadVersion)
 LoadVersion (${PROJECT_SOURCE_DIR}/VERSION_CURRENT MONGOC)
 
+# Enable "maintainer flags," which are supplementary but not mandatory.
+# (As opposed to MongoC-Warnings.cmake, which are required warnings)
 include (MaintainerFlags)
 
 if ( (ENABLE_BUILD_DEPENDECIES STREQUAL OFF) AND (NOT CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) )
@@ -285,6 +287,10 @@ if (ENABLE_MONGOC)
       ${SOURCE_DIR}/src/zlib-1.2.11/gzread.c
       ${SOURCE_DIR}/src/zlib-1.2.11/gzwrite.c
    )
+   add_library (zlib_obj OBJECT "${ZLIB_SOURCES}")
+   set_property (TARGET zlib_obj PROPERTY POSITION_INDEPENDENT_CODE TRUE)
+   # Disable all warnings for compiling Zlib
+   target_compile_options (zlib_obj PRIVATE -w)
 
    set (MONGOC_ENABLE_ICU 0)
 

build/cmake/CMakeLists.txt

@@ -9,6 +9,7 @@ set (build_cmake_MODULES
    LoadVersion.cmake
    MaintainerFlags.cmake
    MongoCPackage.cmake
+   MongoC-Warnings.cmake
    ParseVersion.cmake
    SphinxBuild.cmake
    CCache.cmake

build/cmake/MongoC-Warnings.cmake

@@ -0,0 +1,74 @@
+#[[
+   This file sets warning options for the directories in which it is include()'d
+
+   These warnings are intended to be ported to each supported platform, and
+   especially for high-value warnings that are very likely to catch latent bugs
+   early in the process before the code is even run.
+]]
+
+set (__is_gnu "$<C_COMPILER_ID:GNU>")
+set (__is_clang "$<OR:$<C_COMPILER_ID:Clang>,$<C_COMPILER_ID:AppleClang>>")
+set (__is_gnu_like "$<OR:${__is_gnu},${__is_clang}>")
+set (__is_msvc "$<C_COMPILER_ID:MSVC>")
+
+# "Old" GNU is GCC < 5, which is missing several warning options
+set (__is_old_gnu "$<AND:${__is_gnu},$<VERSION_LESS:$<C_COMPILER_VERSION>,5>>")
+set (__not_old_gnu "$<NOT:${__is_old_gnu}>")
+
+#[[
+   Define additional compile options, conditional on the compiler being used.
+   Each option should be prefixed by `gnu:`, `clang:`, `msvc:`, or `gnu-like:`.
+   Those options will be conditionally enabled for GCC, Clang, or MSVC.
+
+   These options are attached to the source directory and its children.
+]]
+function (mongoc_add_platform_compile_options)
+   foreach (opt IN LISTS ARGV)
+      if (NOT opt MATCHES "^(gnu-like|gnu|clang|msvc):(.*)")
+         message (SEND_ERROR "Invalid option '${opt}' (Should be prefixed by 'msvc:', 'gnu:', 'clang:', or 'gnu-like:'")
+         continue ()
+      endif ()
+      if (CMAKE_MATCH_1 STREQUAL "gnu-like")
+         add_compile_options ("$<${__is_gnu_like}:${CMAKE_MATCH_2}>")
+      elseif (CMAKE_MATCH_1 STREQUAL gnu)
+         add_compile_options ("$<${__is_gnu}:${CMAKE_MATCH_2}>")
+      elseif (CMAKE_MATCH_1 STREQUAL clang)
+         add_compile_options ("$<${__is_clang}:${CMAKE_MATCH_2}>")
+      elseif (CMAKE_MATCH_1 STREQUAL "msvc")
+         add_compile_options ("$<${__is_msvc}:${CMAKE_MATCH_2}>")
+      else ()
+         message (SEND_ERROR "Invalid option to mongoc_add_platform_compile_options(): '${opt}'")
+      endif ()
+   endforeach ()
+endfunction ()
+
+if (CMAKE_VERSION VERSION_LESS 3.3)
+   # On older CMake versions, we'll just always pass the warning options, even
+   # if the generate warnings for the C++ check file
+   set (is_c_lang "1")
+else ()
+   # $<COMPILE_LANGUAGE> is only valid in CMake 3.3+
+   set (is_c_lang "$<COMPILE_LANGUAGE:C>")
+endif ()
+
+# These below warnings should always be unconditional hard errors, as the code is
+# almost definitely broken
+mongoc_add_platform_compile_options (
+     # Implicit function or variable declarations
+     gnu-like:$<${is_c_lang}:-Werror=implicit> msvc:/we4013 msvc:/we4431
+     # Missing return types/statements
+     gnu-like:-Werror=return-type msvc:/we4716
+     # Incompatible pointer types
+     gnu-like:$<$<AND:${is_c_lang},${__not_old_gnu}>:-Werror=incompatible-pointer-types> msvc:/we4113
+     # Integral/pointer conversions
+     gnu-like:$<$<AND:${is_c_lang},${__not_old_gnu}>:-Werror=int-conversion> msvc:/we4047
+     # Discarding qualifiers
+     gnu:$<$<AND:${is_c_lang},${__not_old_gnu}>:-Werror=discarded-qualifiers>
+     clang:$<${is_c_lang}:-Werror=ignored-qualifiers>
+     msvc:/we4090
+     # Definite use of uninitialized value
+     gnu-like:-Werror=uninitialized msvc:/we4700
+
+     # Aside: Disable CRT insecurity warnings
+     msvc:/D_CRT_SECURE_NO_WARNINGS
+     )

build/maintainer-flags.txt

@@ -10,7 +10,6 @@
 -Wno-strict-aliasing
 -Wno-uninitialized
 -Wredundant-decls
--Wreturn-type
 -Wshadow
 -Wswitch-default
 -Wswitch-enum

src/common/CMakeLists.txt

@@ -5,6 +5,8 @@ if (ENABLE_DEBUG_ASSERTIONS)
     set (MONGOC_ENABLE_DEBUG_ASSERTIONS 1)
 endif ()
 
+include (MongoC-Warnings)
+
 configure_file (
         "${PROJECT_SOURCE_DIR}/src/common/common-config.h.in"
         "${PROJECT_BINARY_DIR}/src/common/common-config.h"

src/common/common-thread-private.h

@@ -107,7 +107,7 @@ typedef struct {
 #define BSON_THREAD_FUN(_function_name, _arg_name) \
    unsigned (__stdcall _function_name) (void *(_arg_name))
 #define BSON_THREAD_FUN_TYPE(_function_name) \
-   unsigned (__stdcall * _function_name) (void *)
+   unsigned(__stdcall * _function_name) (void *)
 #define BSON_THREAD_RETURN return 0
 #endif
 

src/kms-message/src/kms_port.c

@@ -15,8 +15,13 @@
  */
 
 #include "kms_port.h"
+
+#include <stdlib.h>
+#include <string.h>
+
 #if defined(_WIN32)
-char * kms_strndup (const char *src, size_t len)
+char *
+kms_strndup (const char *src, size_t len)
 {
    char *dst = (char *) malloc (len + 1);
    if (!dst) {

src/libbson/CMakeLists.txt

@@ -10,6 +10,8 @@ if (ENABLE_TESTS AND NOT MONGOC_ENABLE_STATIC_BUILD)
    )
 endif ()
 
+include (MongoC-Warnings)
+
 set (BSON_OUTPUT_BASENAME "bson" CACHE STRING "Output bson library base name")
 
 include (CheckFunctionExists)
@@ -98,9 +100,10 @@ else ()
    set (BSON_HAVE_STRLCPY 1)
 endif ()
 
+CHECK_INCLUDE_FILE (stdbool.h BSON_HAVE_STDBOOL_H)
+
 if (MSVC)
    set (BSON_HAVE_CLOCK_GETTIME 0)
-   set (BSON_HAVE_STDBOOL_H 0)
    set (BSON_HAVE_STRNLEN 0)
    set (BSON_HAVE_SYSCALL_TID 0)
 else ()
@@ -112,10 +115,6 @@ else ()
    if (NOT BSON_HAVE_STRNLEN)
       set (BSON_HAVE_STRNLEN 0)
    endif ()
-   CHECK_INCLUDE_FILE (stdbool.h BSON_HAVE_STDBOOL_H)
-   if (NOT BSON_HAVE_STDBOOL_H)
-      set (BSON_HAVE_STDBOOL_H 0)
-   endif ()
    CHECK_SYMBOL_EXISTS (SYS_gettid sys/syscall.h BSON_HAVE_SYSCALL_TID)
    check_symbol_exists (syscall unistd.h _TMP_HAVE_SYSCALL)
    if (NOT BSON_HAVE_SYSCALL_TID OR NOT _TMP_HAVE_SYSCALL OR APPLE OR ANDROID)

src/libmongoc/CMakeLists.txt

@@ -2,7 +2,7 @@ cmake_minimum_required (VERSION 3.1)
 
 project (libmongoc C)
 
-set (CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} ${PROJECT_SOURCE_DIR}/../../build/cmake)
+include (MongoC-Warnings)
 
 include (InstallRequiredSystemLibraries)
 include (CheckStructHasMember)
@@ -45,9 +45,6 @@ if (NOT ENABLE_ZSTD MATCHES "ON|AUTO|OFF")
    message (FATAL_ERROR "ENABLE_ZSTD option must be ON, AUTO, or OFF")
 endif ()
 
-# Disable warnings on bundled zlib source files.
-set_source_files_properties (${ZLIB_SOURCES} PROPERTIES COMPILE_FLAGS -w)
-
 # Copy zconf.h.in to zconf.h; even when using system zlib, the 'dist' target
 # will look for zconf.h in that location.
 configure_file (
@@ -78,7 +75,7 @@ endif ()
 if ( (ENABLE_ZLIB STREQUAL "BUNDLED")
    OR (ENABLE_ZLIB STREQUAL "AUTO" AND NOT ZLIB_FOUND) )
    message (STATUS "Enabling zlib compression (bundled)")
-   set (SOURCES ${SOURCES} ${ZLIB_SOURCES})
+   set (SOURCES ${SOURCES} $<TARGET_OBJECTS:zlib_obj>)
    set (
       ZLIB_INCLUDE_DIRS
       "${SOURCE_DIR}/src/zlib-1.2.11"

src/libmongoc/tests/TestSuite.c

@@ -446,7 +446,8 @@ _print_getlasterror_win (const char *msg)
                   NULL,
                   GetLastError (),
                   MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
-                  &err_msg,
+                  /* FormatMessage is weird about this param. */
+                  (LPTSTR) &err_msg,
                   0,
                   NULL);
 

src/libmongoc/tests/mock_server/request.c

@@ -613,22 +613,22 @@ request_matches_msg (const request_t *request,
 bool
 request_matches_msgv (const request_t *request, uint32_t flags, va_list *args)
 {
-   const bson_t **docs;
+   bson_t **docs;
    size_t n_docs, allocated;
    bool r;
 
    n_docs = 0;
    allocated = 1;
    docs = bson_malloc (allocated * sizeof (bson_t *));
-   while ((docs[n_docs] = va_arg (*args, const bson_t *))) {
+   while ((docs[n_docs] = va_arg (*args, bson_t *))) {
       n_docs++;
       if (n_docs == allocated) {
          allocated = bson_next_power_of_two (allocated + 1);
          docs = bson_realloc (docs, allocated * sizeof (bson_t *));
       }
    }
 
-   r = request_matches_msg (request, flags, docs, n_docs);
+   r = request_matches_msg (request, flags, (const bson_t **) docs, n_docs);
    bson_free (docs);
    return r;
 }

src/libmongoc/tests/test-conveniences.c

@@ -60,7 +60,7 @@ test_conveniences_init ()
 
 
 void
-test_conveniences_cleanup ()
+test_conveniences_cleanup (void)
 {
    int i;
    bson_t *doc;

src/libmongoc/tests/test-conveniences.h

@@ -39,7 +39,7 @@ test_conveniences_init ();
  * Called automatically at process exit.
  */
 void
-test_conveniences_cleanup ();
+test_conveniences_cleanup (void);
 
 /* Return a bson_t representation from a single-quoted JSON string, with
  * possible printf format directives.

src/libmongoc/tests/test-mongoc-async.c

@@ -326,7 +326,7 @@ test_hello_delay_initializer (mongoc_async_cmd_t *acmd)
 }
 
 static void
-test_hello_delay ()
+test_hello_delay (void)
 {
    /* test that a delayed cmd works. */
    mock_server_t *server = mock_server_with_auto_hello (WIRE_VERSION_MAX);

src/libmongoc/tests/test-mongoc-bulk.c

@@ -3476,7 +3476,8 @@ _test_numerous (bool ordered)
    TEST_NUMEROUS (remove_one (bulk, doc), "{'q': {'_id': 1}, 'limit': 1}");
    TEST_NUMEROUS (replace_one (bulk, doc, tmp_bson ("{}"), false),
                   "{'q': {'_id': 1}, 'u': {}}");
-   TEST_NUMEROUS (update_one (bulk, doc, tmp_bson ("{'$set': {'x': 1}}"), NULL),
+   TEST_NUMEROUS (
+      update_one (bulk, doc, tmp_bson ("{'$set': {'x': 1}}"), false),
                   "{'q': {'_id': 1}, 'u': {'$set': {'x': 1}}}");
    TEST_NUMEROUS (update_many_with_opts (
                      bulk, doc, tmp_bson ("{'$set': {'x': 1}}"), NULL, NULL),

src/libmongoc/tests/test-mongoc-client-side-encryption.c

@@ -2218,7 +2218,7 @@ _test_multi_threaded (bool external_key_vault)
 }
 
 static void
-test_multi_threaded ()
+test_multi_threaded (void *ctx_unused)
 {
    _test_multi_threaded (true);
    _test_multi_threaded (false);

src/libmongoc/tests/test-mongoc-cluster.c

@@ -1554,13 +1554,13 @@ _test_cluster_command_error (bool use_op_msg)
 }
 
 static void
-test_cluster_command_error_op_msg ()
+test_cluster_command_error_op_msg (void)
 {
    _test_cluster_command_error (true);
 }
 
 static void
-test_cluster_command_error_op_query ()
+test_cluster_command_error_op_query (void)
 {
    _test_cluster_command_error (false);
 }

src/libmongoc/tests/test-mongoc-collection.c

@@ -282,7 +282,7 @@ _batch_size_test (bson_t *pipeline,
 }
 
 static void
-test_aggregate_with_batch_size ()
+test_aggregate_with_batch_size (void)
 {
    bson_t *pipeline_dollar_out;
    bson_t *pipeline_dollar_merge;
@@ -1687,7 +1687,7 @@ test_index (void)
 }
 
 static void
-test_index_w_write_concern ()
+test_index_w_write_concern (void)
 {
    mongoc_collection_t *collection;
    mongoc_database_t *database;

src/libmongoc/tests/test-mongoc-handshake.c

@@ -740,7 +740,7 @@ _get_bit (char *config_str, uint32_t bit)
 }
 
 void
-test_handshake_platform_config ()
+test_handshake_platform_config (void)
 {
    /* Parse the config string, and check that it matches the defined flags. */
    char *config_str = _mongoc_handshake_get_config_hex_string ();

src/libmongoc/tests/test-mongoc-mongohouse.c

@@ -151,7 +151,6 @@ cmd_succeeded_cb (const mongoc_apm_command_succeeded_t *event)
 
    /* Store cursor information from our initial find. */
    if (strcmp (cmd, "find") == 0) {
-
       BSON_ASSERT (!test->parsed_cursor);
 
       bson_iter_init (&iter, reply);
@@ -163,7 +162,7 @@ cmd_succeeded_cb (const mongoc_apm_command_succeeded_t *event)
       BSON_ASSERT (bson_iter_find_descendant (&iter, "cursor.ns", &child_iter));
       BSON_ASSERT (BSON_ITER_HOLDS_UTF8 (&child_iter));
 
-      test->cursor_ns = bson_strdup(bson_iter_utf8(&child_iter, NULL));
+      test->cursor_ns = bson_strdup (bson_iter_utf8 (&child_iter, NULL));
       BSON_ASSERT (NULL != test->cursor_ns);
 
       test->parsed_cursor = true;
@@ -194,7 +193,7 @@ cmd_succeeded_cb (const mongoc_apm_command_succeeded_t *event)
 /* Test that the driver properly constructs and issues a killCursors command to
  * ADL. */
 static void
-test_mongohouse_kill_cursors ()
+test_mongohouse_kill_cursors (void *ctx_unused)
 {
    mongoc_apm_callbacks_t *callbacks;
    mongoc_collection_t *coll;
@@ -273,7 +272,7 @@ _run_ping_test (const char *connection_string)
 /* Test that the driver can establish a connection to ADL with authentication.
    Test both SCRAM-SHA-1 and SCRAM-SHA-256. */
 static void
-test_mongohouse_auth ()
+test_mongohouse_auth (void *ctx_unused)
 {
    /* SCRAM-SHA-1 */
    _run_ping_test (
@@ -286,7 +285,7 @@ test_mongohouse_auth ()
 
 /* Test that the driver can connect to ADL without authentication. */
 static void
-test_mongohouse_no_auth ()
+test_mongohouse_no_auth (void *ctx_unused)
 {
    _run_ping_test ("mongodb://localhost:27017");
 }

src/libmongoc/tests/test-mongoc-read-prefs.c

@@ -1053,7 +1053,7 @@ aggregate (mongoc_collection_t *collection, mongoc_read_prefs_t *prefs)
 /* direct connection to a secondary requires read pref primaryPreferred to
  * avoid "not primary" error from server */
 static void
-test_op_msg_direct_secondary ()
+test_op_msg_direct_secondary (void)
 {
    _test_op_msg_direct_connection (
       false /* is_mongos */,
@@ -1075,7 +1075,7 @@ test_op_msg_direct_secondary ()
 
 /* direct connection to mongos must not auto-add read pref primaryPreferred */
 static void
-test_op_msg_direct_mongos ()
+test_op_msg_direct_mongos (void)
 {
    _test_op_msg_direct_connection (true /* is_mongos */,
                                    find,

src/libmongoc/tests/test-mongoc-shared.c

@@ -34,7 +34,7 @@ my_value_free_v (void *ptr)
 }
 
 static void
-test_simple ()
+test_simple (void)
 {
    int destroyed_value = 0;
    mongoc_shared_ptr ptr = MONGOC_SHARED_PTR_NULL;