Previously, in #39387, this protection had been added only for linux x86_64.
And use include_guard() instead of manual if()+set().
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Public flags, especially -Wxx (i.e. -Wno-XX) can hide some warnings,
that had been added in the main cmake rules of ClickHouse.
This patch had been tested manually with the following patch:
```patch
diff --git a/contrib/jemalloc-cmake/CMakeLists.txt b/contrib/jemalloc-cmake/CMakeLists.txt
index d5ea69d4926..7e79fba0c16 100644
--- a/contrib/jemalloc-cmake/CMakeLists.txt
+++ b/contrib/jemalloc-cmake/CMakeLists.txt
@@ -158,6 +158,7 @@ target_include_directories(_jemalloc SYSTEM PRIVATE
"${CMAKE_CURRENT_BINARY_DIR}/${JEMALLOC_INCLUDE_PREFIX}/jemalloc/internal")
target_compile_definitions(_jemalloc PRIVATE -DJEMALLOC_NO_PRIVATE_NAMESPACE)
+target_compile_options(_jemalloc INTERFACE -Wno-error) # also PUBLIC had been checked
if (CMAKE_BUILD_TYPE_UC STREQUAL "DEBUG")
target_compile_definitions(_jemalloc PRIVATE
```
And cmake gave:
CMake Error at cmake/sanitize_targets.cmake:69 (message):
_jemalloc set INTERFACE_COMPILE_OPTIONS to -Wno-error. This is forbidden.
Call Stack (most recent call first):
cmake/sanitize_targets.cmake:79 (sanitize_interface_flags)
CMakeLists.txt:595 (include)
Fixes: #12447
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
Note, that this is just a syntastic change, that should not makes any
difference (well the only difference is that now it supports gold and
other links, since the option is handled by the plugin itself instead of
the linker).
Refs: https://reviews.llvm.org/D133092
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
We currently only log a compiler-generated "build id" at startup which
is different for each build. That makes it useless to determine the
exact source code state in tests (e.g. BC test) and from user log files
(e.g. if someone compiled an intermediate version of ClickHouse).
Current log message:
Starting ClickHouse 22.10.1.1 with revision 54467, build id: 6F35820328F89C9F36E91C447FF9E61CAF0EF019, PID 42633
New log message:
Starting ClickHouse 22.10.1.1 (revision 54467, git hash: b6b1f7f763f94ffa12133679a6f80342dd1c3afe, build id: 47B12BE61151926FBBD230DE42F3B7A6652AC482), PID 981813
Although this increase debug symbol size from 510MB to 1.8GB, but it is
not a problem for packages, since they are compressed anyway.
Checked deb package, and size slightly increased though, 834M -> 962M.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
During first run of cmake the toolchain file will be loaded twice,
- /usr/share/cmake-3.23/Modules/CMakeDetermineSystem.cmake
- /bld/CMakeFiles/3.23.2/CMakeSystem.cmake
But once you already have non-empty cmake cache it will be loaded only
once:
- /bld/CMakeFiles/3.23.2/CMakeSystem.cmake
This has no harm except for double load of toolchain will add
--gcc-toolchain multiple times that will not allow ccache to reuse the
cache.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
- TSA is a static analyzer build by Google which finds race conditions
and deadlocks at compile time.
- It works by associating a shared member variable with a
synchronization primitive that protects it. The compiler can then
check at each access if proper locking happened before. A good
introduction are [0] and [1].
- TSA requires some help by the programmer via annotations. Luckily,
LLVM's libcxx already has annotations for std::mutex, std::lock_guard,
std::shared_mutex and std::scoped_lock. This commit enables them
(--> contrib/libcxx-cmake/CMakeLists.txt).
- Further, this commit adds convenience macros for the low-level
annotations for use in ClickHouse (--> base/defines.h). For
demonstration, they are leveraged in a few places.
- As we compile with "-Wall -Wextra -Weverything", the required compiler
flag "-Wthread-safety-analysis" was already enabled. Negative checks
are an experimental feature of TSA and disabled
(--> cmake/warnings.cmake). Compile times did not increase noticeably.
- TSA is used in a few places with simple locking. I tried TSA also
where locking is more complex. The problem was usually that it is
unclear which data is protected by which lock :-(. But there was
definitely some weird code where locking looked broken. So there is
some potential to find bugs.
*** Limitations of TSA besides the ones listed in [1]:
- The programmer needs to know which lock protects which piece of shared
data. This is not always easy for large classes.
- Two synchronization primitives used in ClickHouse are not annotated in
libcxx:
(1) std::unique_lock: A releaseable lock handle often together with
std::condition_variable, e.g. in solve producer-consumer problems.
(2) std::recursive_mutex: A re-entrant mutex variant. Its usage can be
considered a design flaw + typically it is slower than a standard
mutex. In this commit, one std::recursive_mutex was converted to
std::mutex and annotated with TSA.
- For free-standing functions (e.g. helper functions) which are passed
shared data members, it can be tricky to specify the associated lock.
This is because the annotations use the normal C++ rules for symbol
resolution.
[0] https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
[1] https://static.googleusercontent.com/media/research.google.com/en//pubs/archive/42958.pdf
- the CI builds were recently upgraded to Clang 14
- this commit bumps versions of other LLVM tools needed for the build
- this is important for people who have multiple LLVM versions installed
via their package manager
First part, updated most UTF8, hashing, memory and codecs. Except
utf8lower and upper, maybe a little later.
That includes huge amount of research with movemask dealing. Exact
details and blog post TBD.
- It was noticed that in (*), the crashstack says "There is no
information about the reference checksum."
- The binaries are pulled via docker hub and upon inspection they indeed
lack the hash embedded as ELF section ".note.ClickHouse.hash" in the
clickhouse binary. This is weird because docker hub images are
"official" builds which should trigger the hash embedding.
- Turns out that the docker hub binaries are also stripped which was too
aggressive. We now no longer remove sections ".comment" and ".note"
which are anyways only 140 bytes in size, i.e. binary size still goes
down (on my stystem) from 2.1 GB to 0.47 + 0.40 GB binary + dbg info.
(*) https://playground.lodthe.me/ba75d494-95d1-4ff6-a0ad-60c138636c9b
