Make the cmake & CI builds use the same settings (ridiculousfish#82)

* CI & CMake builds: use same warning level

Turn on all warnings & warnings as errors
CI build will build debug as well as release (to get assertions).

* Add sanitize options to CMake

And replace build only sanitize flags with the
cmake option.

* CI build fixes

* Install VS 2019 ASAN component

* Docs

* Use a seperate build config - Sanitize

Instead of applying flags to existing build configs.

* Code review feedback

Author: adbancroft

.gitignore

@@ -16,4 +16,5 @@ CTestTestfile.cmake
 .DS_Store
 .pio
 .vscode/*
-!.vscode/extensions.json
+!.vscode/extensions.json
+!.vscode/cmake-variants.json

.vscode/cmake-variants.json

@@ -0,0 +1,27 @@
+{
+    "buildType": {
+      "default": "debug",
+        "choices": { 
+            "debug": {
+              "short": "Debug",
+              "long": "Disable optimizations - include debug information.",
+              "buildType": "Debug"
+            },
+            "release": {
+              "short": "Release",
+              "long": "Optimize for speed - exclude debug information.",
+              "buildType": "Release"
+            },            
+            "reldebuginfo": {
+              "short": "Release with debug Info",
+              "long": "Optimize for speed - include debug information.",
+              "buildType": "RelWithDebInfo"
+            },
+            "sanitize": {
+              "short": "Release with sanitizers",
+              "long": "Fast sanitization checks.",
+              "buildType": "Sanitize"
+          }          
+        }
+    }
+}

CMakeLists.txt

@@ -9,6 +9,13 @@ include(CheckCXXSourceRuns)
 include(GNUInstallDirs)
 include(CMakePackageConfigHelpers)
 include(CMakePushCheckState)
+include(CMakeSanitize)
+
+# Compile options ################################################
+
+# Maximum warnings level & warnings as error
+add_compile_options("$<$<CXX_COMPILER_ID:MSVC>:/W4;/WX>")
+add_compile_options("$<$<CXX_COMPILER_ID:GNU,Clang,AppleClang>:-Wall;-Wextra;-pedantic;-Werror>")
 
 # Build options ################################################
 

CMakeSanitize

@@ -0,0 +1,48 @@
+set(BUILD_NAME "Sanitize")
+string(TOUPPER "${BUILD_NAME}" BUILD_NAME_U)
+
+set(BASE_BUILD_NAME "RelWithDebInfo")
+string(TOUPPER "${BASE_BUILD_NAME}" BASE_BUILD_NAME_U)
+
+if(CMAKE_CONFIGURATION_TYPES)
+  list(APPEND CMAKE_CONFIGURATION_TYPES "${BUILD_NAME}")
+  list(REMOVE_DUPLICATES CMAKE_CONFIGURATION_TYPES)
+  set(CMAKE_CONFIGURATION_TYPES "${CMAKE_CONFIGURATION_TYPES}" CACHE STRING
+    "Add the configurations that we need"
+    FORCE)
+endif()
+
+if (CMAKE_C_COMPILER_ID STREQUAL "MSVC")
+  set(COMPILER_FLAGS "/fsanitize=address /MTd")
+  set(LINKER_FLAGS "/INCREMENTAL:NO")
+elseif(CMAKE_C_COMPILER_ID MATCHES "Clang")
+  set(COMPILER_FLAGS "-fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer -U_DLL -U_MT -DDEBUG -O1 -g")
+  set(LINKER_FLAGS "-fsanitize=address")
+elseif(CMAKE_C_COMPILER_ID STREQUAL "GNU")
+  set(COMPILER_FLAGS "-fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer -DDEBUG -O1 -g")
+  set(LINKER_FLAGS "-fsanitize=address -fsanitize=undefined")
+endif()
+
+set("CMAKE_CXX_FLAGS_${BUILD_NAME_U}" "${CMAKE_CXX_FLAGS_${BASE_BUILD_NAME_U}} ${COMPILER_FLAGS}" CACHE STRING
+ "Flags used by the C++ compiler during ${BUILD_NAME} builds."
+  FORCE)
+set("CMAKE_C_FLAGS_${BUILD_NAME_U}" "${CMAKE_C_FLAGS_${BASE_BUILD_NAME_U}} ${COMPILER_FLAGS}" CACHE STRING
+  "Flags used by the C compiler during ${BUILD_NAME} builds."
+  FORCE)
+set("CMAKE_EXE_LINKER_FLAGS_${BUILD_NAME_U}" "${CMAKE_EXE_LINKER_FLAGS_${BASE_BUILD_NAME_U}} ${LINKER_FLAGS}" CACHE STRING
+  "Flags used for linking binaries during ${BUILD_NAME} builds."
+  FORCE)
+set("CMAKE_SHARED_LINKER_FLAGS_${BUILD_NAME_U}" "${CMAKE_SHARED_LINKER_FLAGS_${BASE_BUILD_NAME_U}} ${LINKER_FLAGS}" CACHE STRING
+  "Flags used by the shared libraries linker during ${BUILD_NAME} builds."
+  FORCE)
+
+mark_as_advanced(
+  "${CMAKE_CXX_FLAGS_${BUILD_NAME_U}}" 
+  "${CMAKE_C_FLAGS_${BUILD_NAME_U}}" 
+  "${CMAKE_EXE_LINKER_FLAGS_${BUILD_NAME_U}}" 
+  "${CMAKE_SHARED_LINKER_FLAGS_${BUILD_NAME_U}}")
+
+# Update the documentation string of CMAKE_BUILD_TYPE for GUIs
+set(CMAKE_BUILD_TYPE "${CMAKE_BUILD_TYPE}" CACHE STRING
+  "Choose the type of build, options are: None Debug Release RelWithDebInfo MinSizeRel ${BUILD_NAME}."
+  FORCE)

README.md

@@ -187,7 +187,7 @@ discrepancy.
 
 # Benchmark program
 
-You can pass the **benchmark** program one or more of the following arguments: ```u32```,
+You can pass the **benchmark** program one or more of the following arguments: ```u16```, ```s16```, ```u32```,
 ```s32```, ```u64```, ```s64``` to compare libdivide's speed against hardware division.
 **benchmark** tests a simple function that inputs an array of random numerators and a single
 divisor, and returns the sum of their quotients. It tests this using both hardware division, and
@@ -210,7 +210,7 @@ It will keep going as long as you let it, so it's best to stop it when you are h
 denominators tested. These columns have the following significance. All times are in
 nanoseconds, lower is better.
 
-```bash
+```
      #:  The divisor that is tested
 system:  Hardware divide time
 scalar:  libdivide time, using scalar division
@@ -226,9 +226,14 @@ so benchmark is valuable for its verification as well.
 
 # Contributing
 
-We currently do not have automated testing! Hence, before sending in patches, it would be nice
-if you compiled your new code at high warning levels using at least MSVC and GCC (or Clang).
-Also run the tester program to verify correctness and the benchmark programs to ensure you have
-not introduced any performance regressions.
+Although there are no individual unit tests, the supplied ```cmake``` builds do include several safety nets:
+
+* They compile with:
+  * All warnings on and;
+  * Warnings as errors
+* The CI build will build and run with sanitizers on ; these are available as part of the cmake build: ```-DCMAKE_BUILD_TYPE=Sanitize```
+* The ```cmake``` and CI builds will compile and run both ```C``` and ```C++``` test programs.
+
+Before sending in patches, build and run at least the ```tester``` and ```benchmark``` using the supplied ```cmake``` scripts on at least ```MSVC``` and ```GCC``` (or ```Clang```).
 
 ### Happy hacking!

appveyor.yml

@@ -17,13 +17,10 @@ for:
       only:
         - image: Ubuntu1804
           platform: x86
-    environment:
-      CFLAGS: "-Wall -Wextra -pedantic -Werror -O1 -g -fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer"
-      CXXFLAGS: "-Wall -Wextra -pedantic -Werror -O1 -g -fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer"
     install:
       - sudo apt install --yes cppcheck
     build_script:
-      - cmake . -DCMAKE_BUILD_TYPE=Debug
+      - cmake . -DCMAKE_BUILD_TYPE=Sanitize
       - make VERBOSE=1
     test_script:
       - cppcheck . --error-exitcode=1 --force -i doc
@@ -35,13 +32,10 @@ for:
       only:
         - image: Ubuntu1804
           platform: x64
-    environment:
-      CFLAGS: "-Wall -Wextra -pedantic -Werror -O1 -g -fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer"
-      CXXFLAGS: "-Wall -Wextra -pedantic -Werror -O1 -g -fsanitize=address -fsanitize=undefined -fno-sanitize-recover=all -fno-omit-frame-pointer"
     install:
       - sudo apt install --yes cppcheck
     build_script:
-      - CC=clang CXX=clang++ cmake . -DCMAKE_BUILD_TYPE=Debug
+      - CC=clang CXX=clang++ cmake . -DCMAKE_BUILD_TYPE=Sanitize
       - make VERBOSE=1
     test_script:
       - cppcheck . --error-exitcode=1 --force -i doc
@@ -66,11 +60,9 @@ for:
       only:
         - image: Visual Studio 2017
           platform: x64
-    environment:
-      CFLAGS: "/W3 /WX /DLIBDIVIDE_ASSERTIONS_ON"
-      CXXFLAGS: "/W3 /WX /DLIBDIVIDE_ASSERTIONS_ON"
     build_script:
       - cmake . -G "Visual Studio 15 2017 Win64"
+      - cmake --build . --config Debug
       - cmake --build . --config Release
     test_script:
       - cd Release
@@ -82,11 +74,14 @@ for:
       only:
         - image: Visual Studio 2019
           platform: x86
+    install:
+      - cmd: '"C:/Program Files (x86)/Microsoft Visual Studio/Installer/vs_installer.exe" modify --quiet --installpath "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community" --channelid VisualStudio.16.Release --productid Microsoft.VisualStudio.Product.Community --add Microsoft.VisualStudio.Component.VC.ASAN'
     build_script:
       - cmake . -G "Visual Studio 16 2019"
-      - cmake --build . --config Release
+      - cmake --build . --config Debug
+      - cmake --build . --config Sanitize
     test_script:
-      - cd Release
+      - cd Sanitize
       - tester.exe
       - benchmark_branchfree.exe
       - test_c99.exe
@@ -95,23 +90,22 @@ for:
       only:
         - image: Visual Studio 2019
           platform: x64
-    environment:
-      CFLAGS: "/W3 /WX /DLIBDIVIDE_ASSERTIONS_ON"
-      CXXFLAGS: "/W3 /WX /DLIBDIVIDE_ASSERTIONS_ON"
     install:
+      - cmd: '"C:/Program Files (x86)/Microsoft Visual Studio/Installer/vs_installer.exe" modify --quiet --installpath "C:\Program Files (x86)\Microsoft Visual Studio\2019\Community" --channelid VisualStudio.16.Release --productid Microsoft.VisualStudio.Product.Community --add Microsoft.VisualStudio.Component.VC.ASAN'
       - cmd: SET PATH=C:\Python39;C:\Python39\Scripts;%PATH%
       - cmd: pip install -U platformio
       - cmd: pio platform install atmelavr --with-package=tool-simavr
     build_script:
       - cmake . -G "Visual Studio 16 2019"
-      - cmake --build . --config Release
+      - cmake --build . --config Debug
+      - cmake --build . --config Sanitize
       - cd test/avr
       - ps: Get-Content -Path .\platformio.ini | Where-Object { $_.StartsWith("[env:") } | ForEach-Object { & pio run -e $_.SubString(5, $_.Length-6) }
       - cd ../..
     test_script:
       # Important check for AVR - static + inline
       - ps: Select-String -Path "libdivide.h" "static [^L]" -AllMatches | ForEach-Object { Write-Error ("Static but not line - " + $_) }
-      - cd Release
+      - cd Sanitize
       - tester.exe
       - benchmark_branchfree.exe
       - test_c99.exe

libdivide.h

@@ -33,9 +33,15 @@
 
 #if defined(_MSC_VER)
 #include <intrin.h>
+#pragma warning(push)
 // disable warning C4146: unary minus operator applied
 // to unsigned type, result still unsigned
 #pragma warning(disable : 4146)
+// disable warning C4204: nonstandard extension used : non-constant aggregate 
+// initializer
+//
+// It's valid C99
+#pragma warning(disable : 4204)
 #define LIBDIVIDE_VC
 #endif
 
@@ -869,7 +875,7 @@ static LIBDIVIDE_INLINE struct libdivide_u32_t libdivide_internal_u32_gen(
         // This power works if e < 2**floor_log_2_d.
         if (!branchfree && (e < ((uint32_t)1 << floor_log_2_d))) {
             // This power works
-            more = floor_log_2_d;
+            more = (uint8_t)floor_log_2_d;
         } else {
             // We have to use the general 33-bit algorithm.  We need to compute
             // (2**power) / d. However, we already have (2**(power-1))/d and
@@ -879,7 +885,7 @@ static LIBDIVIDE_INLINE struct libdivide_u32_t libdivide_internal_u32_gen(
             proposed_m += proposed_m;
             const uint32_t twice_rem = rem + rem;
             if (twice_rem >= d || twice_rem < rem) proposed_m += 1;
-            more = floor_log_2_d | LIBDIVIDE_ADD_MARKER;
+            more = (uint8_t)(floor_log_2_d | LIBDIVIDE_ADD_MARKER);
         }
         result.magic = 1 + proposed_m;
         result.more = more;
@@ -1028,7 +1034,7 @@ static LIBDIVIDE_INLINE struct libdivide_u64_t libdivide_internal_u64_gen(
         // This power works if e < 2**floor_log_2_d.
         if (!branchfree && e < ((uint64_t)1 << floor_log_2_d)) {
             // This power works
-            more = floor_log_2_d;
+            more = (uint8_t)floor_log_2_d;
         } else {
             // We have to use the general 65-bit algorithm.  We need to compute
             // (2**power) / d. However, we already have (2**(power-1))/d and
@@ -1038,7 +1044,7 @@ static LIBDIVIDE_INLINE struct libdivide_u64_t libdivide_internal_u64_gen(
             proposed_m += proposed_m;
             const uint64_t twice_rem = rem + rem;
             if (twice_rem >= d || twice_rem < rem) proposed_m += 1;
-            more = floor_log_2_d | LIBDIVIDE_ADD_MARKER;
+            more = (uint8_t)(floor_log_2_d | LIBDIVIDE_ADD_MARKER);
         }
         result.magic = 1 + proposed_m;
         result.more = more;
@@ -1368,7 +1374,7 @@ static LIBDIVIDE_INLINE struct libdivide_s32_t libdivide_internal_s32_gen(
     if ((absD & (absD - 1)) == 0) {
         // Branchfree and normal paths are exactly the same
         result.magic = 0;
-        result.more = floor_log_2_d | (d < 0 ? LIBDIVIDE_NEGATIVE_DIVISOR : 0);
+        result.more = (uint8_t)(floor_log_2_d | (d < 0 ? LIBDIVIDE_NEGATIVE_DIVISOR : 0));
     } else {
         LIBDIVIDE_ASSERT(floor_log_2_d >= 1);
 
@@ -1383,15 +1389,15 @@ static LIBDIVIDE_INLINE struct libdivide_s32_t libdivide_internal_s32_gen(
         // This works if works if e < 2**floor_log_2_d.
         if (!branchfree && e < ((uint32_t)1 << floor_log_2_d)) {
             // This power works
-            more = floor_log_2_d - 1;
+            more = (uint8_t)(floor_log_2_d - 1);
         } else {
             // We need to go one higher. This should not make proposed_m
             // overflow, but it will make it negative when interpreted as an
             // int32_t.
             proposed_m += proposed_m;
             const uint32_t twice_rem = rem + rem;
             if (twice_rem >= absD || twice_rem < rem) proposed_m += 1;
-            more = floor_log_2_d | LIBDIVIDE_ADD_MARKER;
+            more = (uint8_t)(floor_log_2_d | LIBDIVIDE_ADD_MARKER);
         }
 
         proposed_m += 1;
@@ -1537,7 +1543,7 @@ static LIBDIVIDE_INLINE struct libdivide_s64_t libdivide_internal_s64_gen(
     if ((absD & (absD - 1)) == 0) {
         // Branchfree and non-branchfree cases are the same
         result.magic = 0;
-        result.more = floor_log_2_d | (d < 0 ? LIBDIVIDE_NEGATIVE_DIVISOR : 0);
+        result.more = (uint8_t)(floor_log_2_d | (d < 0 ? LIBDIVIDE_NEGATIVE_DIVISOR : 0));
     } else {
         // the dividend here is 2**(floor_log_2_d + 63), so the low 64 bit word
         // is 0 and the high word is floor_log_2_d - 1
@@ -1550,7 +1556,7 @@ static LIBDIVIDE_INLINE struct libdivide_s64_t libdivide_internal_s64_gen(
         // This works if works if e < 2**floor_log_2_d.
         if (!branchfree && e < ((uint64_t)1 << floor_log_2_d)) {
             // This power works
-            more = floor_log_2_d - 1;
+            more = (uint8_t)(floor_log_2_d - 1);
         } else {
             // We need to go one higher. This should not make proposed_m
             // overflow, but it will make it negative when interpreted as an
@@ -1562,7 +1568,7 @@ static LIBDIVIDE_INLINE struct libdivide_s64_t libdivide_internal_s64_gen(
             // also set ADD_MARKER this is an annoying optimization that
             // enables algorithm #4 to avoid the mask. However we always set it
             // in the branchfree case
-            more = floor_log_2_d | LIBDIVIDE_ADD_MARKER;
+            more = (uint8_t)(floor_log_2_d | LIBDIVIDE_ADD_MARKER);
         }
         proposed_m += 1;
         int64_t magic = (int64_t)proposed_m;
@@ -3113,4 +3119,8 @@ using branchfree_divider = divider<T, BRANCHFREE>;
 
 #endif  // __cplusplus
 
+#if defined(_MSC_VER)
+#pragma warning(pop)
+#endif
+
 #endif  // LIBDIVIDE_H