Skip to content

Commit

Permalink
Support compile-time assert configuration in release build (TileDB-In…
Browse files Browse the repository at this point in the history
…c#2962)

* Add support for compile-time assertion configuration.

This commit adds support for configuring the behavior of `assert` at compile time.
In particular, assertions may now be enabled even if building in release mode.
CMake unconditionally adds the `-DNDEBUG` to compile definitions in a
release build, so a CMake snippet is added from LLVM which strips the
definition from CMake default flags.

Modifying this paramater after the first cmake initialization is
unlikely to have any effect.

Also add `bootstrap` support for --enable-assertions

---
TYPE: IMPROVEMENT
DESC: Add support for compile-time assertion configuration

* Add test for assertions build.

The test strategy here is:
- auxiliary executable try_assert which should assert out if
  TILEDB_ASSERTIONS=ON
- test executable test_assert which runs try_assert and checks the
  error code. Test failure is conditional on compile-time configuration:
  - if assertions enabled, we expect the try_assert process to exit
    to SIGABRT.

---
TYPE: NO_HISTORY

* Address review comments

* DEBUG -> Debug
  • Loading branch information
ihnorton authored Mar 17, 2022
1 parent c6d0c36 commit 2e68437
Show file tree
Hide file tree
Showing 9 changed files with 160 additions and 9 deletions.
10 changes: 9 additions & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ endif()
############################################################
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake")
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Modules")
list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/Options")

set(TILEDB_CMAKE_INPUTS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/cmake/inputs")

Expand Down Expand Up @@ -113,6 +114,7 @@ option(TILEDB_AZURE "Enables Azure Storage support using azure-storage-cpp" OFF)
option(TILEDB_GCS "Enables GCS Storage support using google-cloud-cpp" OFF)
option(TILEDB_HDFS "Enables HDFS support using the official Hadoop JNI bindings" OFF)
option(TILEDB_WERROR "Enables the -Werror flag during compilation." ON)
option(TILEDB_ASSERTIONS "Build with assertions enabled (default off for release, on for debug build)." OFF)
option(TILEDB_CPP_API "Enables building of the TileDB C++ API." ON)
option(TILEDB_CMAKE_IDE "(Used for CLion builds). Disables superbuild and sets the EP install dir." OFF)
option(TILEDB_STATS "Enables internal TileDB statistics gathering." ON)
Expand Down Expand Up @@ -142,6 +144,12 @@ if (WIN32 AND NOT TILEDB_SKIP_S3AWSSDK_DIR_LENGTH_CHECK)
endif()
endif()

# enable assertions by default for debug builds
if (CMAKE_BUILD_TYPE EQUAL "Debug")
set(TILEDB_ASSERTIONS TRUE)
endif()
include(TileDBAssertions)

############################################################
# Superbuild setup
############################################################
Expand Down Expand Up @@ -225,7 +233,7 @@ else()
if (CMAKE_BUILD_TYPE MATCHES "Debug")
add_compile_options(-DDEBUG -O0 -g3 -ggdb3 -gdwarf-3)
elseif (CMAKE_BUILD_TYPE MATCHES "Release")
add_compile_options(-DNDEBUG -O3)
add_compile_options(-O3)
elseif (CMAKE_BUILD_TYPE MATCHES "RelWithDebInfo")
add_compile_options(-DNDEBUG -O3 -g3 -ggdb3 -gdwarf-3)
elseif (CMAKE_BUILD_TYPE MATCHES "Coverage")
Expand Down
11 changes: 8 additions & 3 deletions bootstrap
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ Configuration:
--enable-sanitizer=SAN enable sanitizer (clang only)
(address|memory|leak|thread|undefined)
--enable-debug enable debug build
--enable-assertions enable assertions in compiled code
--enable-release-symbols enable create symbols for release build
--enable-coverage enable build with code coverage support
--enable-verbose enable verbose status messages
Expand Down Expand Up @@ -94,6 +95,7 @@ tiledb_force_all_deps="OFF"
tiledb_stats="ON"
tiledb_static="OFF"
tiledb_disable_avx2=""
tiledb_enable_assertions="OFF"
tiledb_serialization="OFF"
tiledb_tools="OFF"
tiledb_ccache="OFF"
Expand All @@ -111,9 +113,7 @@ while test $# != 0; do
--disable-cpp-api) tiledb_cpp_api="OFF";;
--disable-stats) tiledb_stats="OFF";;
--disable-avx2) tiledb_disable_avx2="-DCOMPILER_SUPPORTS_AVX2=FALSE";;
--enable-static-tiledb) tiledb_static="ON";;
--enable-sanitizer=*) san=`arg "$1"`
sanitizer="$san";;
--enable-assertions) tiledb_assertions="ON";;
--enable-debug) build_type="Debug";;
--enable-release-symbols) build_type="RelWithDebInfo";;
--enable-coverage) build_type="Coverage";;
Expand All @@ -123,6 +123,9 @@ while test $# != 0; do
--enable-azure) tiledb_azure="ON";;
--enable-gcs) tiledb_gcs="ON";;
--enable-serialization) tiledb_serialization="ON";;
--enable-static-tiledb) tiledb_static="ON";;
--enable-sanitizer=*) san=`arg "$1"`
sanitizer="$san";;
--enable-tools) tiledb_tools="ON";;
--enable-ccache) tiledb_ccache="ON";;
--enable-arrow-tests) tiledb_arrow_tests="ON";;
Expand All @@ -139,6 +142,7 @@ done
IFS=',' read -ra enables <<< "$enable_multiple"
for en in "${enables[@]}"; do
case "$en" in
assertions) tiledb_assertions="ON";;
debug) build_type="Debug";;
release-symbols) build_type="RelWithDebInfo";;
coverage) build_type="Coverage";;
Expand Down Expand Up @@ -191,6 +195,7 @@ ${cmake} -DCMAKE_BUILD_TYPE=${build_type} \
-DCMAKE_C_COMPILER="${c_compiler}" \
-DCMAKE_CXX_COMPILER="${cxx_compiler}" \
-DCMAKE_PREFIX_PATH="${dependency_dir}" \
-DTILEDB_ASSERTIONS=${tiledb_assertions} \
-DTILEDB_VERBOSE=${tiledb_verbose} \
-DTILEDB_HDFS=${tiledb_hdfs} \
-DTILEDB_S3=${tiledb_s3} \
Expand Down
16 changes: 14 additions & 2 deletions bootstrap.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ Optionally specify the CMake generator string, e.g. "Visual Studio 15
.PARAMETER EnableDebug
Enable Debug build.
.PARAMETER EnableAssert
Enable Assertions in compiled code (always on for debug build;
default off in release).
.PARAMETER EnableReleaseSymbols
Enable symbols with Release build.
Expand Down Expand Up @@ -75,6 +79,7 @@ Param(
[string]$Prefix,
[string]$Dependency,
[string]$CMakeGenerator,
[switch]$EnableAssert,
[switch]$EnableDebug,
[switch]$EnableReleaseSymbols,
[switch]$EnableCoverage,
Expand Down Expand Up @@ -109,11 +114,18 @@ $DefaultPrefix = Join-Path $SourceDirectory "dist"
# Choose the default dependency install prefix.
$DefaultDependency = $DefaultPrefix

# Set assertion mode
# No-op for a debug build.
$AssertionMode = "OFF"
if ($EnableAssert.IsPresent) {
$AssertionMode = "ON"
}

# Set TileDB build type
$BuildType = "Release"
if ($EnableDebug.IsPresent) {
$BuildType = "Debug"
}
}
if ($EnableReleaseSymbols.IsPresent) {
$BuildType = "RelWithDebInfo"
}
Expand Down Expand Up @@ -213,7 +225,7 @@ if ($CMakeGenerator -eq $null) {

# Run CMake.
# We use Invoke-Expression so we can echo the command to the user.
$CommandString = "cmake -A X64 -DCMAKE_BUILD_TYPE=$BuildType -DCMAKE_INSTALL_PREFIX=""$InstallPrefix"" -DCMAKE_PREFIX_PATH=""$DependencyDir"" -DMSVC_MP_FLAG=""/MP$BuildProcesses"" -DTILEDB_VERBOSE=$Verbosity -DTILEDB_AZURE=$UseAzure -DTILEDB_S3=$UseS3 -DTILEDB_SERIALIZATION=$UseSerialization -DTILEDB_WERROR=$Werror -DTILEDB_CPP_API=$CppApi -DTILEDB_TESTS=$Tests -DTILEDB_STATS=$Stats -DTILEDB_STATIC=$TileDBStatic -DTILEDB_FORCE_ALL_DEPS=$TileDBBuildDeps -DTILEDB_TOOLS=$TileDBTools $GeneratorFlag ""$SourceDirectory"""
$CommandString = "cmake -A X64 -DCMAKE_BUILD_TYPE=$BuildType -DCMAKE_INSTALL_PREFIX=""$InstallPrefix"" -DCMAKE_PREFIX_PATH=""$DependencyDir"" -DMSVC_MP_FLAG=""/MP$BuildProcesses"" -DTILEDB_ASSERTIONS=$AssertionMode -DTILEDB_VERBOSE=$Verbosity -DTILEDB_AZURE=$UseAzure -DTILEDB_S3=$UseS3 -DTILEDB_SERIALIZATION=$UseSerialization -DTILEDB_WERROR=$Werror -DTILEDB_CPP_API=$CppApi -DTILEDB_TESTS=$Tests -DTILEDB_STATS=$Stats -DTILEDB_STATIC=$TileDBStatic -DTILEDB_FORCE_ALL_DEPS=$TileDBBuildDeps -DTILEDB_TOOLS=$TileDBTools $GeneratorFlag ""$SourceDirectory"""
Write-Host $CommandString
Write-Host
Invoke-Expression "$CommandString"
Expand Down
29 changes: 29 additions & 0 deletions cmake/Options/TileDBAssertions.cmake
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# Lightly modified from LLVM, used under "Apache 2.0 License with LLVM exceptions”
# https://github.com/llvm/llvm-project/blob/93d1a623cecb6f732db7900baf230a13e6ac6c6a/llvm/cmake/modules/HandleLLVMOptions.cmake#L60-L85

if(TILEDB_ASSERTIONS)
# MSVC doesn't like _DEBUG on release builds. See LLVM PR 4379.
if( NOT MSVC )
add_definitions( -D_DEBUG )
endif()
# On non-Debug builds cmake automatically defines NDEBUG, so we
# explicitly undefine it:
if( NOT CMAKE_BUILD_TYPE STREQUAL "Debug" )
# NOTE: use `add_compile_options` rather than `add_definitions` since
# `add_definitions` does not support generator expressions.
add_compile_options($<$<OR:$<COMPILE_LANGUAGE:C>,$<COMPILE_LANGUAGE:CXX>>:-UNDEBUG>)
if (MSVC)
# Also remove /D NDEBUG to avoid MSVC warnings about conflicting defines.
foreach (flags_var_to_scrub
CMAKE_CXX_FLAGS_RELEASE
CMAKE_CXX_FLAGS_RELWITHDEBINFO
CMAKE_CXX_FLAGS_MINSIZEREL
CMAKE_C_FLAGS_RELEASE
CMAKE_C_FLAGS_RELWITHDEBINFO
CMAKE_C_FLAGS_MINSIZEREL)
string (REGEX REPLACE "(^| )[/-]D *NDEBUG($| )" " "
"${flags_var_to_scrub}" "${${flags_var_to_scrub}}")
endforeach()
endif()
endif()
endif()
1 change: 1 addition & 0 deletions cmake/TileDB-Superbuild.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ set(INHERITED_CMAKE_ARGS
-DCMAKE_CXX_FLAGS=${CMAKE_CXX_FLAGS}
-DCOMPILER_SUPPORTS_AVX2=${COMPILER_SUPPORTS_AVX2}
-DTILEDB_VERBOSE=${TILEDB_VERBOSE}
-DTILEDB_ASSERTIONS=${TILEDB_ASSERTIONS}
-DTILEDB_S3=${TILEDB_S3}
-DTILEDB_AZURE=${TILEDB_AZURE}
-DTILEDB_GCS=${TILEDB_GCS}
Expand Down
8 changes: 5 additions & 3 deletions test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ set(TILEDB_TEST_SUPPORT_SOURCES
src/helpers-dimension.h
src/vfs_helpers.cc
)

set(TILEDB_UNIT_TEST_SOURCES
src/helpers.h
src/helpers.cc
Expand Down Expand Up @@ -157,7 +157,7 @@ set(TILEDB_UNIT_TEST_SOURCES
src/unit-dimension.cc
src/unit-duplicates.cc
src/unit-empty-var-length.cc
src/unit-filter-buffer.cc
src/unit-filter-buffer.cc
src/unit-filter-pipeline.cc
src/unit-gcs.cc
src/unit-gs.cc
Expand Down Expand Up @@ -399,4 +399,6 @@ cmake_print_properties(
TARGETS Catch2::Catch2
PROPERTIES INCLUDE_DIRECTORIES INTERFACE_INCLUDE_DIRECTORIES
)


# CI tests
add_subdirectory(ci)
30 changes: 30 additions & 0 deletions test/ci/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
include(TileDBAssertions)

find_package(Catch_EP REQUIRED)

add_executable(
test_assert
test_assert.cc
)

target_link_libraries(test_assert PUBLIC Catch2::Catch2)

if (TILEDB_ASSERTIONS)
target_compile_definitions(
test_assert
PRIVATE
-DTILEDB_ASSERTIONS
)
endif()

add_executable(
try_assert
try_assert.cc
)

add_dependencies(test_assert try_assert)

add_test(
NAME "test_ci_asserts"
COMMAND test_assert
)
55 changes: 55 additions & 0 deletions test/ci/test_assert.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/**
* @file test/ci/test_assert
*
* @section LICENSE
*
* The MIT License
*
* @copyright Copyright (c) 2022 TileDB, Inc.
*
* Permission is hereby granted, free of charge, to any person obtaining a copy
* of this software and associated documentation files (the "Software"), to deal
* in the Software without restriction, including without limitation the rights
* to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
* copies of the Software, and to permit persons to whom the Software is
* furnished to do so, subject to the following conditions:
*
* The above copyright notice and this permission notice shall be included in
* all copies or substantial portions of the Software.
*
* THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
* IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
* FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
* AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
* LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
* OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
* THE SOFTWARE.
*
* @section DESCRIPTION
*
* This file defines a test which calls executable `try_assert` to determine
* whether assertions are correctly enabled in build tree.
*/

#define CATCH_CONFIG_MAIN
#include <catch.hpp>

#include <cassert>
#include <iostream>

#ifdef _WIN32
constexpr int assert_exit_code = 3;
#else
constexpr int assert_exit_code = 6;
#endif

TEST_CASE("CI: Test assertions configuration", "[ci][assertions]") {
int retval = system("./try_assert");

#ifdef TILEDB_ASSERTIONS
REQUIRE(retval == assert_exit_code);
#else
(void)assert_exit_code;
REQUIRE(retval == 0);
#endif
}
9 changes: 9 additions & 0 deletions test/ci/try_assert.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#include <cassert>
#include <iostream>

int main(int, char**) {
assert(false);

std::cout << "Assert did not exit!" << std::endl;
return 0;
}

0 comments on commit 2e68437

Please sign in to comment.