This is an automated email from the ASF dual-hosted git repository.
lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git
The following commit(s) were added to refs/heads/main by this push:
new ed58fe8 Enable ASan/UBSan/cpplint (#26)
ed58fe8 is described below
commit ed58fe82a078a4a6c4ec0559b5ca2b67ac7658b8
Author: David Li <[email protected]>
AuthorDate: Fri Jun 24 13:49:14 2022 -0400
Enable ASan/UBSan/cpplint (#26)
---
.github/workflows/cpp.yml | 4 +-
.pre-commit-config.yaml | 9 ++
adbc_driver_manager/adbc_driver_manager_test.cc | 9 ++
ci/build_support/sanitizer-disallowed-entries.txt | 16 +++
cmake_modules/AdbcDefines.cmake | 7 +-
cmake_modules/san-config.cmake | 124 ++++++++++++++++++++++
drivers/flight_sql/flight_sql.cc | 4 +-
drivers/flight_sql/flight_sql_test.cc | 4 +-
drivers/sqlite/sqlite.cc | 23 ++--
drivers/sqlite/sqlite_test.cc | 10 ++
drivers/test_util.h | 2 +
11 files changed, 187 insertions(+), 25 deletions(-)
diff --git a/.github/workflows/cpp.yml b/.github/workflows/cpp.yml
index 8558505..1c7458b 100644
--- a/.github/workflows/cpp.yml
+++ b/.github/workflows/cpp.yml
@@ -78,7 +78,7 @@ jobs:
run: |
mkdir -p build/driver_sqlite
pushd build/driver_sqlite
- cmake ../../drivers/sqlite -DADBC_BUILD_TESTS=ON
-DCMAKE_BUILD_TYPE=Debug -DCMAKE_PREFIX_PATH=$CONDA_PREFIX
-DADBC_BUILD_SHARED=ON -DADBC_BUILD_STATIC=OFF
+ cmake ../../drivers/sqlite -DADBC_BUILD_TESTS=ON
-DCMAKE_BUILD_TYPE=Debug -DCMAKE_PREFIX_PATH=$CONDA_PREFIX
-DADBC_BUILD_SHARED=ON -DADBC_BUILD_STATIC=OFF -DADBC_USE_ASAN=ON
-DADBC_USE_UBSAN=ON
cmake --build .
export DYLD_LIBRARY_PATH=$(pwd)
# MacOS doesn't allow DYLD_LIBRARY_PATH to propagate through
@@ -90,7 +90,7 @@ jobs:
run: |
mkdir -p build/driver_manager
pushd build/driver_manager
- cmake ../../adbc_driver_manager -DADBC_BUILD_TESTS=ON
-DCMAKE_BUILD_TYPE=Debug -DCMAKE_PREFIX_PATH=$CONDA_PREFIX
-DADBC_BUILD_SHARED=ON -DADBC_BUILD_STATIC=OFF
+ cmake ../../adbc_driver_manager -DADBC_BUILD_TESTS=ON
-DCMAKE_BUILD_TYPE=Debug -DCMAKE_PREFIX_PATH=$CONDA_PREFIX
-DADBC_BUILD_SHARED=ON -DADBC_BUILD_STATIC=OFF -DADBC_USE_ASAN=ON
-DADBC_USE_UBSAN=ON
cmake --build .
export DYLD_LIBRARY_PATH=$(pwd):../driver_sqlite
export LD_LIBRARY_PATH=$(pwd):../driver_sqlite
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index ae1de97..cf02f14 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -39,6 +39,15 @@ repos:
hooks:
- id: cmake-format
args: [--in-place]
+ - repo: https://github.com/cpplint/cpplint
+ rev: 1.6.0
+ hooks:
+ - id: cpplint
+ args:
+ # From Arrow's config
+ -
"--filter=-whitespace/comments,-readability/casting,-readability/todo,-readability/alt_tokens,-build/header_guard,-build/c++11,-build/include_order,-build/include_subdir"
+ - "--linelength=90"
+ - "--verbose=2"
- repo: https://github.com/macisamuele/language-formatters-pre-commit-hooks
rev: v2.3.0
hooks:
diff --git a/adbc_driver_manager/adbc_driver_manager_test.cc
b/adbc_driver_manager/adbc_driver_manager_test.cc
index 0c5781f..e130621 100644
--- a/adbc_driver_manager/adbc_driver_manager_test.cc
+++ b/adbc_driver_manager/adbc_driver_manager_test.cc
@@ -23,6 +23,10 @@
#include <arrow/table.h>
#include <arrow/testing/matchers.h>
+#include <memory>
+#include <string>
+#include <vector>
+
#include "adbc.h"
#include "adbc_driver_manager.h"
#include "drivers/test_util.h"
@@ -60,6 +64,10 @@ class DriverManager : public ::testing::Test {
}
void TearDown() override {
+ if (error.message) {
+ error.release(&error);
+ }
+
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcConnectionRelease(&connection,
&error));
ASSERT_EQ(connection.private_data, nullptr);
@@ -192,6 +200,7 @@ TEST_F(DriverManager, BulkIngestStream) {
ADBC_ASSERT_OK_WITH_ERROR(
error, AdbcStatementBindStream(&statement, &export_stream, &error));
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementExecute(&statement, &error));
+ ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementRelease(&statement, &error));
}
{
diff --git a/ci/build_support/sanitizer-disallowed-entries.txt
b/ci/build_support/sanitizer-disallowed-entries.txt
new file mode 100644
index 0000000..13a8339
--- /dev/null
+++ b/ci/build_support/sanitizer-disallowed-entries.txt
@@ -0,0 +1,16 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you 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.
diff --git a/cmake_modules/AdbcDefines.cmake b/cmake_modules/AdbcDefines.cmake
index 4196e37..a4b2572 100644
--- a/cmake_modules/AdbcDefines.cmake
+++ b/cmake_modules/AdbcDefines.cmake
@@ -17,8 +17,12 @@
# Common definitions for the CMake projects in this repository.
-enable_language(C)
+enable_language(C CXX)
+
+set(BUILD_SUPPORT_DIR "${REPOSITORY_ROOT}/ci/build_support")
+
include(DefineOptions)
+include(san-config)
set(ADBC_VERSION "9.0.0-SNAPSHOT")
set(ADBC_BASE_VERSION "9.0.0")
@@ -38,5 +42,4 @@ add_custom_target(all-tests)
if(ADBC_BUILD_TESTS)
find_package(GTest)
set(ADBC_TEST_LINK_LIBS GTest::gtest_main GTest::gtest GTest::gmock)
- set(BUILD_SUPPORT_DIR "${REPOSITORY_ROOT}/ci/build_support")
endif()
diff --git a/cmake_modules/san-config.cmake b/cmake_modules/san-config.cmake
new file mode 100644
index 0000000..a678d87
--- /dev/null
+++ b/cmake_modules/san-config.cmake
@@ -0,0 +1,124 @@
+# 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. See accompanying LICENSE file.
+
+# Taken from Arrow's san-config with variables renamed
+
+# Clang does not support using ASAN and TSAN simultaneously.
+if("${ADBC_USE_ASAN}" AND "${ADBC_USE_TSAN}")
+ message(SEND_ERROR "Can only enable one of ASAN or TSAN at a time")
+endif()
+
+# Flag to enable clang address sanitizer
+# This will only build if clang or a recent enough gcc is the chosen compiler
+if(${ADBC_USE_ASAN})
+ if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
+ OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
+ OR (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION
+ VERSION_GREATER "4.8"))
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fsanitize=address
-DADDRESS_SANITIZER")
+ else()
+ message(SEND_ERROR "Cannot use ASAN without clang or gcc >= 4.8")
+ endif()
+endif()
+
+# Flag to enable clang undefined behavior sanitizer
+# We explicitly don't enable all of the sanitizer flags:
+# - disable 'vptr' because of RTTI issues across shared libraries (?)
+# - disable 'alignment' because unaligned access is really OK on Nehalem and
we do it
+# all over the place.
+# - disable 'function' because it appears to give a false positive
+# (https://github.com/google/sanitizers/issues/911)
+# - disable 'float-divide-by-zero' on clang, which considers it UB
+# (https://bugs.llvm.org/show_bug.cgi?id=17000#c1)
+# Note: GCC does not support the 'function' flag.
+if(${ADBC_USE_UBSAN})
+ if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID
STREQUAL
+ "Clang")
+ set(CMAKE_CXX_FLAGS
+ "${CMAKE_CXX_FLAGS} -fsanitize=undefined
-fno-sanitize=alignment,vptr,function,float-divide-by-zero
-fno-sanitize-recover=all"
+ )
+ elseif(CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION
+ VERSION_GREATER_EQUAL "5.1")
+ set(CMAKE_CXX_FLAGS
+ "${CMAKE_CXX_FLAGS} -fsanitize=undefined -fno-sanitize=alignment,vptr
-fno-sanitize-recover=all"
+ )
+ else()
+ message(SEND_ERROR "Cannot use UBSAN without clang or gcc >= 5.1")
+ endif()
+endif()
+
+# Flag to enable thread sanitizer (clang or gcc 4.8)
+if(${ADBC_USE_TSAN})
+ if(NOT
+ (CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang"
+ OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang"
+ OR (CMAKE_CXX_COMPILER_ID STREQUAL "GNU" AND CMAKE_CXX_COMPILER_VERSION
+ VERSION_GREATER "4.8")))
+ message(SEND_ERROR "Cannot use TSAN without clang or gcc >= 4.8")
+ endif()
+
+ add_definitions("-fsanitize=thread")
+
+ # Enables dynamic_annotations.h to actually generate code
+ add_definitions("-DDYNAMIC_ANNOTATIONS_ENABLED")
+
+ # changes atomicops to use the tsan implementations
+ add_definitions("-DTHREAD_SANITIZER")
+
+ # Disables using the precompiled template specializations for std::string,
shared_ptr, etc
+ # so that the annotations in the header actually take effect.
+ add_definitions("-D_GLIBCXX_EXTERN_TEMPLATE=0")
+
+ # Some of the above also need to be passed to the linker.
+ set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -pie
-fsanitize=thread")
+
+ # Strictly speaking, TSAN doesn't require dynamic linking. But it does
+ # require all code to be position independent, and the easiest way to
+ # guarantee that is via dynamic linking (not all 3rd party archives are
+ # compiled with -fPIC e.g. boost).
+ if("${ADBC_LINK}" STREQUAL "a")
+ message("Using dynamic linking for TSAN")
+ set(ADBC_LINK "d")
+ elseif("${ADBC_LINK}" STREQUAL "s")
+ message(SEND_ERROR "Cannot use TSAN with static linking")
+ endif()
+endif()
+
+if(${ADBC_USE_COVERAGE})
+ if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID
STREQUAL
+ "Clang")
+
add_definitions("-fsanitize-coverage=pc-table,inline-8bit-counters,edge,no-prune,trace-cmp,trace-div,trace-gep"
+ )
+
+ set(CMAKE_CXX_FLAGS
+ "${CMAKE_CXX_FLAGS}
-fsanitize-coverage=pc-table,inline-8bit-counters,edge,no-prune,trace-cmp,trace-div,trace-gep"
+ )
+ else()
+ message(SEND_ERROR "You can only enable coverage with clang")
+ endif()
+endif()
+
+if("${ADBC_USE_UBSAN}"
+ OR "${ADBC_USE_ASAN}"
+ OR "${ADBC_USE_TSAN}")
+ # GCC 4.8 and 4.9 (latest as of this writing) don't allow you to specify
+ # disallowed entries for the sanitizer.
+ if(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID
STREQUAL
+ "Clang")
+ set(CMAKE_CXX_FLAGS
+ "${CMAKE_CXX_FLAGS}
-fsanitize-blacklist=${BUILD_SUPPORT_DIR}/sanitizer-disallowed-entries.txt"
+ )
+ else()
+ message(WARNING "GCC does not support specifying a sanitizer disallowed
entries list. Known sanitizer check failures will not be suppressed."
+ )
+ endif()
+endif()
diff --git a/drivers/flight_sql/flight_sql.cc b/drivers/flight_sql/flight_sql.cc
index 370450f..50b11d3 100644
--- a/drivers/flight_sql/flight_sql.cc
+++ b/drivers/flight_sql/flight_sql.cc
@@ -61,7 +61,7 @@ void SetError(struct AdbcError* error, Args&&... args) {
class FlightSqlDatabaseImpl {
public:
- explicit FlightSqlDatabaseImpl() : client_(nullptr), connection_count_(0) {}
+ FlightSqlDatabaseImpl() : client_(nullptr), connection_count_(0) {}
flightsql::FlightSqlClient* Connect() {
std::lock_guard<std::mutex> guard(mutex_);
@@ -170,7 +170,7 @@ class FlightSqlConnectionImpl {
class FlightSqlStatementImpl : public arrow::RecordBatchReader {
public:
- FlightSqlStatementImpl(std::shared_ptr<FlightSqlConnectionImpl> connection)
+ explicit FlightSqlStatementImpl(std::shared_ptr<FlightSqlConnectionImpl>
connection)
: connection_(std::move(connection)), info_() {}
void Init(std::unique_ptr<flight::FlightInfo> info) { info_ =
std::move(info); }
diff --git a/drivers/flight_sql/flight_sql_test.cc
b/drivers/flight_sql/flight_sql_test.cc
index 499477b..ecb5a30 100644
--- a/drivers/flight_sql/flight_sql_test.cc
+++ b/drivers/flight_sql/flight_sql_test.cc
@@ -31,12 +31,12 @@ namespace adbc {
using arrow::PointeesEqual;
-static std::string kServerEnvVar = "ADBC_FLIGHT_SQL_LOCATION";
+static const char kServerEnvVar[] = "ADBC_FLIGHT_SQL_LOCATION";
class AdbcFlightSqlTest : public ::testing::Test {
public:
void SetUp() override {
- if (const char* location = std::getenv(kServerEnvVar.c_str())) {
+ if (const char* location = std::getenv(kServerEnvVar)) {
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcDatabaseNew(&database, &error));
ADBC_ASSERT_OK_WITH_ERROR(
error, AdbcDatabaseSetOption(&database, "location", location,
&error));
diff --git a/drivers/sqlite/sqlite.cc b/drivers/sqlite/sqlite.cc
index ca8ca0d..10abb8c 100644
--- a/drivers/sqlite/sqlite.cc
+++ b/drivers/sqlite/sqlite.cc
@@ -44,22 +44,6 @@ void ReleaseError(struct AdbcError* error) {
error->message = nullptr;
}
-void SetError(sqlite3* db, const std::string& source, struct AdbcError* error)
{
- if (!error) return;
- std::string message =
- arrow::util::StringBuilder("[SQLite3] ", source, ": ",
sqlite3_errmsg(db));
- if (error->message) {
- message.reserve(message.size() + 1 + std::strlen(error->message));
- message.append(1, '\n');
- message.append(error->message);
- delete[] error->message;
- }
- error->message = new char[message.size() + 1];
- message.copy(error->message, message.size());
- error->message[message.size()] = '\0';
- error->release = ReleaseError;
-}
-
template <typename... Args>
void SetError(struct AdbcError* error, Args&&... args) {
if (!error) return;
@@ -77,6 +61,10 @@ void SetError(struct AdbcError* error, Args&&... args) {
error->release = ReleaseError;
}
+void SetError(sqlite3* db, const std::string& source, struct AdbcError* error)
{
+ return SetError(error, source, ": ", sqlite3_errmsg(db));
+}
+
AdbcStatusCode CheckRc(sqlite3* db, int rc, const char* context,
struct AdbcError* error) {
if (rc != SQLITE_OK) {
@@ -154,7 +142,7 @@ std::shared_ptr<arrow::Schema>
StatementToSchema(sqlite3_stmt* stmt) {
class SqliteDatabaseImpl {
public:
- explicit SqliteDatabaseImpl() : db_(nullptr), connection_count_(0) {}
+ SqliteDatabaseImpl() : db_(nullptr), connection_count_(0) {}
AdbcStatusCode Connect(sqlite3** db, struct AdbcError* error) {
std::lock_guard<std::mutex> guard(mutex_);
@@ -271,6 +259,7 @@ class SqliteConnectionImpl {
return ADBC_STATUS_INTERNAL;
}
query += escaped;
+ sqlite3_free(escaped);
std::shared_ptr<arrow::Schema> arrow_schema;
ADBC_RETURN_NOT_OK(
diff --git a/drivers/sqlite/sqlite_test.cc b/drivers/sqlite/sqlite_test.cc
index fb7d103..6ffcdc2 100644
--- a/drivers/sqlite/sqlite_test.cc
+++ b/drivers/sqlite/sqlite_test.cc
@@ -62,6 +62,10 @@ class Sqlite : public ::testing::Test {
}
void TearDown() override {
+ if (error.message) {
+ error.release(&error);
+ }
+
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcConnectionRelease(&connection,
&error));
ASSERT_EQ(connection.private_data, nullptr);
@@ -87,6 +91,7 @@ class Sqlite : public ::testing::Test {
ADBC_ASSERT_OK_WITH_ERROR(
error, AdbcStatementBind(&statement, &export_table, &export_schema,
&error));
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementExecute(&statement, &error));
+ ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementRelease(&statement, &error));
}
AdbcDatabase database;
@@ -254,6 +259,7 @@ TEST_F(Sqlite, BulkIngestTable) {
ADBC_ASSERT_OK_WITH_ERROR(
error, AdbcStatementBind(&statement, &export_table, &export_schema,
&error));
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementExecute(&statement, &error));
+ ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementRelease(&statement, &error));
}
{
@@ -294,6 +300,7 @@ TEST_F(Sqlite, BulkIngestStream) {
ADBC_ASSERT_OK_WITH_ERROR(
error, AdbcStatementBindStream(&statement, &export_stream, &error));
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementExecute(&statement, &error));
+ ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementRelease(&statement, &error));
}
{
@@ -699,6 +706,7 @@ TEST_F(Sqlite, Transactions) {
ASSERT_NE(ADBC_STATUS_OK, AdbcStatementSetSqlQuery(&statement, query,
&error));
ASSERT_THAT(error.message, ::testing::HasSubstr("database schema is
locked"));
error.release(&error);
+ ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementRelease(&statement, &error));
}
// Rollback
@@ -710,6 +718,7 @@ TEST_F(Sqlite, Transactions) {
ASSERT_NE(ADBC_STATUS_OK, AdbcStatementSetSqlQuery(&statement, query,
&error));
ASSERT_THAT(error.message, ::testing::HasSubstr("no such table"));
error.release(&error);
+ ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementRelease(&statement, &error));
}
// Commit, should now be visible on other connection
@@ -722,6 +731,7 @@ TEST_F(Sqlite, Transactions) {
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementExecute(&statement, &error));
ArrowArrayStream stream;
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementGetStream(&statement,
&stream, &error));
+ stream.release(&stream);
ADBC_ASSERT_OK_WITH_ERROR(error, AdbcStatementRelease(&statement, &error));
}
diff --git a/drivers/test_util.h b/drivers/test_util.h
index 8cc06af..97bfa62 100644
--- a/drivers/test_util.h
+++ b/drivers/test_util.h
@@ -18,6 +18,8 @@
#pragma once
#include <memory>
+#include <string>
+#include <utility>
#include <arrow/c/bridge.h>
#include <arrow/ipc/json_simple.h>
