You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jb...@apache.org on 2018/07/23 23:12:03 UTC
[1/2] impala git commit: IMPALA-3675: part 1: -Werror for ASAN
Repository: impala
Updated Branches:
refs/heads/master 42809cbd1 -> f7efba236
IMPALA-3675: part 1: -Werror for ASAN
This fixes some clang warnings and enables -Werror going forward for
ASAN.
It adds -Wno-return-type-c-linkage to suppress a warning that otherwise
would fail the build.
It also enables the mismatched tags check and fixes the various
instances of it (this warning is irritating for YouCompleteMe users).
Change-Id: If83ca41cde49fd6cfde479e45f9cfdd9e3a45bec
Reviewed-on: http://gerrit.cloudera.org:8080/11002
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/cb0f8a0a
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/cb0f8a0a
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/cb0f8a0a
Branch: refs/heads/master
Commit: cb0f8a0ada054cf09995b362e7dd0fe7674e535d
Parents: 42809cb
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Fri Jul 20 09:58:47 2018 -0700
Committer: Tim Armstrong <ta...@cloudera.com>
Committed: Mon Jul 23 20:17:22 2018 +0000
----------------------------------------------------------------------
.clang-tidy | 1 -
be/CMakeLists.txt | 12 +++++-------
be/src/exec/hdfs-table-writer.h | 2 +-
be/src/exprs/expr.h | 2 +-
be/src/exprs/scalar-expr.h | 2 +-
be/src/runtime/krpc-data-stream-recvr.h | 2 +-
be/src/runtime/mem-tracker.h | 2 +-
7 files changed, 10 insertions(+), 13 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/cb0f8a0a/.clang-tidy
----------------------------------------------------------------------
diff --git a/.clang-tidy b/.clang-tidy
index 54f095c..bf37d7a 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -46,7 +46,6 @@ Checks: "-*,clang*,\
-clang-diagnostic-gnu-anonymous-struct,\
-clang-diagnostic-header-hygiene,\
-clang-diagnostic-implicit-fallthrough,\
--clang-diagnostic-mismatched-tags,\
-clang-diagnostic-missing-prototypes,\
-clang-diagnostic-missing-variable-declarations,\
-clang-diagnostic-nested-anon-types,\
http://git-wip-us.apache.org/repos/asf/impala/blob/cb0f8a0a/be/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/CMakeLists.txt b/be/CMakeLists.txt
index f0915af..89b02c2 100644
--- a/be/CMakeLists.txt
+++ b/be/CMakeLists.txt
@@ -63,8 +63,6 @@ SET(CXX_COVERAGE_FLAGS "-fprofile-arcs -ftest-coverage")
# makes extra calls to clang which may have extra includes (-I) that are unused.
# -fcolor-diagnostics: ensure clang generates colorized output, which is necessary
# when using ccache as clang thinks it is not called from a terminal.
-# -Wno-mismatched-tags: ignore harmless class/struct mismatch for forward declarations.
-# Disabling to be consistent with gcc, which doesn't have this warning.
# -Wno-zero-as-null-pointer-constant: We are slowly moving towards the use of nullptr,
# but till we switch to it completely, we will ignore the warnings due to use of
# NULL as a null pointer constant.
@@ -75,10 +73,10 @@ SET(CXX_COVERAGE_FLAGS "-fprofile-arcs -ftest-coverage")
# destructors with 'override' which is enforced by clang by not recommended by c++
# core guidelines (read C.128).
SET(CXX_CLANG_FLAGS "-Qunused-arguments -fcolor-diagnostics -Wno-unused-local-typedef")
-SET(CXX_CLANG_FLAGS "${CXX_CLANG_FLAGS} -Wno-mismatched-tags")
SET(CXX_CLANG_FLAGS "${CXX_CLANG_FLAGS} -Wno-zero-as-null-pointer-constant")
SET(CXX_CLANG_FLAGS "${CXX_CLANG_FLAGS} -Wno-c++17-extensions")
SET(CXX_CLANG_FLAGS "${CXX_CLANG_FLAGS} -Wno-inconsistent-missing-destructor-override")
+SET(CXX_CLANG_FLAGS "${CXX_CLANG_FLAGS} -Wno-return-type-c-linkage")
# For any gcc builds:
# -g: Enable symbols for profiler tools
# -Wno-unused-local-typedefs: Do not warn for local typedefs that are unused.
@@ -104,15 +102,15 @@ endif()
# Debug information is stored as dwarf2 to be as compatible as possible
SET(CXX_FLAGS_DEBUG "${CXX_GCC_FLAGS} -ggdb -gdwarf-2")
# -Werror: compile warnings should be errors when using the toolchain compiler.
-# Only enable for debug builds because this is what we test in pre-commit tests.
+# Enabled for DEBUG, ASAN, TSAN and UBSAN builds which are built pre-commit.
SET(CXX_FLAGS_DEBUG "${CXX_FLAGS_DEBUG} -Werror")
SET(CXX_FLAGS_RELEASE "${CXX_GCC_FLAGS} -O3 -DNDEBUG -gdwarf-2")
SET(CXX_FLAGS_ADDRESS_SANITIZER
- "${CXX_CLANG_FLAGS} -O1 -g -fsanitize=address -fno-omit-frame-pointer -DADDRESS_SANITIZER")
+ "${CXX_CLANG_FLAGS} -Werror -O1 -g -fsanitize=address -fno-omit-frame-pointer -DADDRESS_SANITIZER")
# Set the flags to the undefined behavior sanitizer, also known as "ubsan"
# Turn on sanitizer and debug symbols to get stack traces:
-SET(CXX_FLAGS_UBSAN "${CXX_CLANG_FLAGS} -ggdb3 -fno-omit-frame-pointer -fsanitize=undefined")
+SET(CXX_FLAGS_UBSAN "${CXX_CLANG_FLAGS} -Werror -ggdb3 -fno-omit-frame-pointer -fsanitize=undefined")
# Set preprocessor macros to facilitate initialization the relevant configuration.
SET(CXX_FLAGS_UBSAN "${CXX_FLAGS_UBSAN} -DUNDEFINED_SANITIZER")
# Calling getenv() in __ubsan_default_options doesn't work, likely because of
@@ -131,7 +129,7 @@ SET(CXX_FLAGS_UBSAN "${CXX_FLAGS_UBSAN} -O0")
# Set the flags to the thread sanitizer, also known as "tsan"
# Turn on sanitizer and debug symbols to get stack traces:
-SET(CXX_FLAGS_TSAN "${CXX_CLANG_FLAGS} -O1 -ggdb3 -fno-omit-frame-pointer")
+SET(CXX_FLAGS_TSAN "${CXX_CLANG_FLAGS} -Werror -O1 -ggdb3 -fno-omit-frame-pointer")
SET(CXX_FLAGS_TSAN "${CXX_FLAGS_TSAN} -fsanitize=thread -DTHREAD_SANITIZER")
SET(CXX_FLAGS_TIDY "${CXX_CLANG_FLAGS}")
http://git-wip-us.apache.org/repos/asf/impala/blob/cb0f8a0a/be/src/exec/hdfs-table-writer.h
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-table-writer.h b/be/src/exec/hdfs-table-writer.h
index cc6c6cc..a6f06c0 100644
--- a/be/src/exec/hdfs-table-writer.h
+++ b/be/src/exec/hdfs-table-writer.h
@@ -30,7 +30,7 @@ namespace impala {
class HdfsPartitionDescriptor;
class HdfsTableDescriptor;
class HdfsTableSink;
-class OutputPartition;
+struct OutputPartition;
class RowBatch;
class RuntimeState;
class ScalarExprEvaluator;
http://git-wip-us.apache.org/repos/asf/impala/blob/cb0f8a0a/be/src/exprs/expr.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr.h b/be/src/exprs/expr.h
index 1a47db6..79f980d 100644
--- a/be/src/exprs/expr.h
+++ b/be/src/exprs/expr.h
@@ -32,7 +32,7 @@
namespace impala {
class IsNullExpr;
-class LibCacheEntry;
+struct LibCacheEntry;
class LlvmCodeGen;
class MemTracker;
class ObjectPool;
http://git-wip-us.apache.org/repos/asf/impala/blob/cb0f8a0a/be/src/exprs/scalar-expr.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-expr.h b/be/src/exprs/scalar-expr.h
index e1954b7..6eb221e 100644
--- a/be/src/exprs/scalar-expr.h
+++ b/be/src/exprs/scalar-expr.h
@@ -55,7 +55,7 @@ using impala_udf::StringVal;
using impala_udf::DecimalVal;
using impala_udf::CollectionVal;
-class LibCacheEntry;
+struct LibCacheEntry;
class LlvmCodeGen;
class MemTracker;
class ObjectPool;
http://git-wip-us.apache.org/repos/asf/impala/blob/cb0f8a0a/be/src/runtime/krpc-data-stream-recvr.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/krpc-data-stream-recvr.h b/be/src/runtime/krpc-data-stream-recvr.h
index 1435e44..18454d7 100644
--- a/be/src/runtime/krpc-data-stream-recvr.h
+++ b/be/src/runtime/krpc-data-stream-recvr.h
@@ -43,7 +43,7 @@ class MemTracker;
class RowBatch;
class RuntimeProfile;
class SortedRunMerger;
-class TransmitDataCtx;
+struct TransmitDataCtx;
class TransmitDataRequestPB;
class TransmitDataResponsePB;
http://git-wip-us.apache.org/repos/asf/impala/blob/cb0f8a0a/be/src/runtime/mem-tracker.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/mem-tracker.h b/be/src/runtime/mem-tracker.h
index 9c41693..01fd331 100644
--- a/be/src/runtime/mem-tracker.h
+++ b/be/src/runtime/mem-tracker.h
@@ -41,7 +41,7 @@ namespace impala {
class ObjectPool;
class MemTracker;
-class ReservationTrackerCounters;
+struct ReservationTrackerCounters;
class TQueryOptions;
/// A MemTracker tracks memory consumption; it contains an optional limit
[2/2] impala git commit: IMPALA-5031: Fix undefined behavior: memset
NULL
Posted by jb...@apache.org.
IMPALA-5031: Fix undefined behavior: memset NULL
memset has undefined behavior when its first argument is NULL. The
instance fixed here was found by Clang's undefined behavior
sanitizer.
It was found in the end-to-end tests. The interesting part of the
stack trace is:
/exec/data-source-scan-node.cc:152:10: runtime error: null pointer passed as argument 1, which is declared to never be null
/usr/include/string.h:62:79: note: nonnull attribute specified here
#0 0x482fd8e in DataSourceScanNode::GetNextInputBatch() /exec/data-source-scan-node.cc:152:3
#1 0x482fb40 in DataSourceScanNode::Open(RuntimeState*) /exec/data-source-scan-node.cc:124:10
#2 0x47ef854 in AggregationNode::Open(RuntimeState*) /exec/aggregation-node.cc:71:49
#3 0x23506a4 in FragmentInstanceState::Open() /runtime/fragment-instance-state.cc:266:53
#4 0x234b6a8 in FragmentInstanceState::Exec() /runtime/fragment-instance-state.cc:81:12
#5 0x236ee52 in QueryState::ExecFInstance(FragmentInstanceState*) /runtime/query-state.cc:401:24
#6 0x237093e in QueryState::StartFInstances()::$_0::operator()() const /runtime/query-state.cc:341:44
Change-Id: I18fa02dc887a42a94c6f81e4923d17568f2184f2
Reviewed-on: http://gerrit.cloudera.org:8080/10948
Reviewed-by: Jim Apple <jb...@apache.org>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/f7efba23
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/f7efba23
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/f7efba23
Branch: refs/heads/master
Commit: f7efba23607974a9c5d3da02999db531a92628c2
Parents: cb0f8a0
Author: Jim Apple <jb...@apache.org>
Authored: Sun Jul 15 18:32:52 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Mon Jul 23 22:37:16 2018 +0000
----------------------------------------------------------------------
be/src/exec/data-source-scan-node.cc | 3 ++-
be/src/util/ubsan.h | 37 +++++++++++++++++++++++++++++++
2 files changed, 39 insertions(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/f7efba23/be/src/exec/data-source-scan-node.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/data-source-scan-node.cc b/be/src/exec/data-source-scan-node.cc
index 88cf11b..fa11d05 100644
--- a/be/src/exec/data-source-scan-node.cc
+++ b/be/src/exec/data-source-scan-node.cc
@@ -33,6 +33,7 @@
#include "runtime/tuple-row.h"
#include "util/jni-util.h"
#include "util/periodic-counter-updater.h"
+#include "util/ubsan.h"
#include "util/runtime-profile-counters.h"
#include "common/names.h"
@@ -149,7 +150,7 @@ Status DataSourceScanNode::GetNextInputBatch() {
input_batch_.reset(new TGetNextResult());
next_row_idx_ = 0;
// Reset all the indexes into the column value arrays to 0
- memset(cols_next_val_idx_.data(), 0, sizeof(int) * cols_next_val_idx_.size());
+ Ubsan::MemSet(cols_next_val_idx_.data(), 0, sizeof(int) * cols_next_val_idx_.size());
TGetNextParams params;
params.__set_scan_handle(scan_handle_);
RETURN_IF_ERROR(data_source_executor_->GetNext(params, input_batch_.get()));
http://git-wip-us.apache.org/repos/asf/impala/blob/f7efba23/be/src/util/ubsan.h
----------------------------------------------------------------------
diff --git a/be/src/util/ubsan.h b/be/src/util/ubsan.h
new file mode 100644
index 0000000..78f6bc1
--- /dev/null
+++ b/be/src/util/ubsan.h
@@ -0,0 +1,37 @@
+// 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.
+
+#ifndef UTIL_UBSAN_H_
+#define UTIL_UBSAN_H_
+
+// Utilities mimicking parts of the standard prone to accidentally using in a way causeing
+// undefined behavior.
+
+#include <cstring>
+
+class Ubsan {
+ public:
+ static void* MemSet(void* s, int c, size_t n) {
+ if (s == nullptr) {
+ DCHECK_EQ(n, 0);
+ return s;
+ }
+ return std::memset(s, c, n);
+ }
+};
+
+#endif // UTIL_UBSAN_H_