clang-tidy now also checks code in header files. Because the analyzer
finds tons of issues, activate the check only for directory "base/" (see
file ".clang-tidy"). All other directories, in particular "src/" are
left to future work.
While many findings were fixed, some were not (and suppressed instead).
Reasons for this include: a) the file is 1:1 copypaste of a 3rd-party
lib (e.g. pcg_extras.h) and fixing stuff would make upgrades/fixes more
difficult b) a fix would have broken lots of using code
getauxval() from glibc-compatibility did not work always correctly:
- It does not work after setenv(), and this breaks vsyscalls,
like sched_getcpu() [1] (and BaseDaemon.cpp always set TZ if timezone
is defined, which is true for CI [2]).
Also note, that fixing setenv() will not fix LSan,
since the culprit is getauxval()
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1163404
[2]: ClickHouse#32928 (comment)
- Another think that is definitely broken is LSan (Leak Sanitizer), it
relies on worked getauxval() but it does not work if __environ is not
initialized yet (there is even a commit about this).
And because of, at least, one leak had been introduced [3]:
[3]: ClickHouse#33840
Fix this by using /proc/self/auxv with fallback to environ solution to
make it compatible with environment that does not allow reading from
auxv (or no procfs).
v2: add fallback to environ solution
v3: fix return value for __auxv_init_procfs()
(cherry picked from commit f187c3499a)
v4: more verbose message on errors, CI founds [1]:
AUXV already has value (529267711)
[1]: https://s3.amazonaws.com/clickhouse-test-reports/39103/2325f7e8442d1672ce5fb43b11039b6a8937e298/stress_test__memory__actions_.html
v5: break at AT_NULL
v6: ignore AT_IGNORE
v7: suppress TSan and remove superior check to avoid abort() in case of race
v8: proper suppressions (not inner function but itself)
Refs: #33957
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
getauxval() from glibc-compatibility did not work always correctly:
- It does not work after setenv(), and this breaks vsyscalls,
like sched_getcpu() [1] (and BaseDaemon.cpp always set TZ if timezone
is defined, which is true for CI [2]).
Also note, that fixing setenv() will not fix LSan,
since the culprit is getauxval()
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1163404
[2]: ClickHouse#32928 (comment)
- Another think that is definitely broken is LSan (Leak Sanitizer), it
relies on worked getauxval() but it does not work if __environ is not
initialized yet (there is even a commit about this).
And because of, at least, one leak had been introduced [3]:
[3]: ClickHouse#33840
Fix this by using /proc/self/auxv with fallback to environ solution to
make it compatible with environment that does not allow reading from
auxv (or no procfs).
v2: add fallback to environ solution
v3: fix return value for __auxv_init_procfs()
Refs: #33957
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
A simple HelloWorld program with zero includes except iostream triggers
a build of ca. 2000 source files. The reason is that ClickHouse's
top-level CMakeLists.txt overrides "add_executable()" to link all
binaries against "clickhouse_new_delete". This links against
"clickhouse_common_io", which in turn has lots of 3rd party library
dependencies ... Without linking "clickhouse_new_delete", the number of
compiled files for "HelloWorld" goes down to ca. 70.
As an example, the self-extracting-executable needs none of its current
dependencies but other programs may also benefit.
In order to restore access to the original "add_executable()", the
overriding version is now prefixed. There is precedence for a
"clickhouse_" prefix (as opposed to "ch_"), for example
"clickhouse_split_debug_symbols". In general prefixing makes sense also
because overriding CMake commands relies on undocumented behavior and is
considered not-so-great practice (*).
(*) https://crascit.com/2018/09/14/do-not-redefine-cmake-commands/
- 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
cmake/target.cmake defines macros for the supported platforms, this
commit changes predefined system macros to our own macros.
__linux__ --> OS_LINUX
__APPLE__ --> OS_DARWIN
__FreeBSD__ --> OS_FREEBSD
Replxx: When disabled via -DENABLE_LIBRARIES=0 or -DENABLE_REPLXX (the
latter was undocumented) the build broke because replxx symbols were
used since [0] in header LineReader.h. This header should in theory
stay clean of replxx but doesn't for efficiency reasons.
This change makes compilation of replxx mandatory. As replxx is quite
small, I guess this is okay. (The alternative is to litter the code
with ifdefs for non-replxx and a replxx paths.)
[0] https://github.com/ClickHouse/ClickHouse/pull/33201
Enable:
- bugprone-lambda-function-name: "Checks for attempts to get the name of
a function from within a lambda expression. The name of a lambda is
always something like operator(), which is almost never what was
intended."
- bugprone-unhandled-self-assignment: "Finds user-defined copy
assignment operators which do not protect the code against
self-assignment either by checking self-assignment explicitly or using
the copy-and-swap or the copy-and-move method.""
- hicpp-invalid-access-moved: "Warns if an object is used after it has
been moved."
- hicpp-use-noexcept: "This check replaces deprecated dynamic exception
specifications with the appropriate noexcept specification (introduced
in C++11)"
- hicpp-use-override: "Adds override (introduced in C++11) to overridden
virtual functions and removes virtual from those functions as it is
not required."
- performance-type-promotion-in-math-fn: "Finds calls to C math library
functions (from math.h or, in C++, cmath) with implicit float to
double promotions."
Split up:
- cppcoreguidelines-*. Some of them may be useful (haven't checked in
detail), therefore allow to toggle them individually.
Disable:
- linuxkernel-*. Obvious.
Official docs:
Some headers from C library were deprecated in C++ and are no longer
welcome in C++ codebases. Some have no effect in C++. For more details
refer to the C++ 14 Standard [depr.c.headers] section. This check
replaces C standard library headers with their C++ alternatives and
removes redundant ones.
Official docs:
This check replaces deprecated dynamic exception specifications with
the appropriate noexcept specification (introduced in C++11). By
default this check will replace throw() with noexcept, and
throw(<exception>[,...]) or throw(...) with noexcept(false).
The original motivation for this commit was that shared_ptr_helper used
std::shared_ptr<>() which does two heap allocations instead of
make_shared<>() which does a single allocation. Turned out that
1. the affected code (--> Storages/) is not on a hot path (rendering the
performance argument moot ...)
2. yet copying Storage objects is potentially dangerous and was
previously allowed.
Hence, this change
- removes shared_ptr_helper and as a result all inherited create() methods,
- instead, Storage objects are now created using make_shared<>() by the
caller (for that to work, many constructors had to be made public), and
- all Storage classes were marked as noncopyable using boost::noncopyable.
In sum, we are (likely) not making things faster but the code becomes
cleaner and harder to misuse.
When I tried to add cool new clang-tidy 14 warnings, I noticed that the
current clang-tidy settings already produce a ton of warnings. This
commit addresses many of these. Almost all of them were non-critical,
i.e. C vs. C++ style casts.
Here is oneliner:
$ gg 'LOG_\(DEBUG\|TRACE\|INFO\|TEST\|WARNING\|ERROR\|FATAL\)([^,]*, [a-zA-Z]' -- :*.cpp :*.h | cut -d: -f1 | sort -u | xargs -r sed -E -i 's#(LOG_[A-Z]*)\(([^,]*), ([A-Za-z][^,)]*)#\1(\2, fmt::runtime(\3)#'
Note, that I tried to do this with coccinelle (tool for semantic
patchin), but it cannot parse C++:
$ cat fmt.cocci
@@
expression log;
expression var;
@@
-LOG_DEBUG(log, var)
+LOG_DEBUG(log, fmt::runtime(var))
I've also tried to use some macros/templates magic to do this implicitly
in logger_useful.h, but I failed to do so, and apparently it is not
possible for now.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
v2: manual fixes
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
getauxval() from glibc-compatibility did not work always correctly:
- it does not work after setenv(), and this breaks vsyscalls,
like sched_getcpu() [1] (and BaseDaemon.cpp always set TZ if timezone
is defined, which is true for CI [2]).
[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1163404
[2]: https://github.com/ClickHouse/ClickHouse/pull/32928#issuecomment-1015762717
- another think that is definitely broken is LSan (Leak Sanitizer), it
relies on worked getauxval() but it does not work if __environ is not
initialized yet (there is even a commit about this).
And because of, at least, one leak had been introduced [3]:
[3]: https://github.com/ClickHouse/ClickHouse/pull/33840
Fix this by using /proc/self/auxv.
And let's see how many issues will LSan find...
I've verified this patch manually by printing AT_BASE and compared it
with output of LD_SHOW_AUXV.
Signed-off-by: Azat Khuzhin <a.khuzhin@semrush.com>
This will allow:
- use addWords() everywhere
- remove that optimization for empty words case (initial load)
- and now we remove duplicates according to comparator
v2: replace parameter pack in addWords() with explicit Compare for both
cases (just pass default)
- use plain mutex over atomic
- use addWords() for initial suggestion filling
- return vector<> from getCompletions() over iterator, to avoid possible
questions (even though it was safe, since addWords() could not be
called in parallel with getCompletions() before)
- Move some code into module part to avoid dependency from IStorage in SystemLog
- Remove extra headers from SystemLog.h
- Rewrite some code that was relying on headers that was included by SystemLog.h
v2: rebase
v3: squash move into module part with explicit template instantiation
(to make each commit self compilable after rebase)
Make ClickHouse compilable and runnable on risc-v 64
So far only basic functionality was tested (on real hw),
clickhouse server runs, exceptions works, client works,
simple tests works.
What doesn't work:
1. traces - they are always empty
2. system.stack_trace only have first frame
... after obtaining the memory amount, in order to return not greater
than the physical memory from `getMemoryAmount()`
Additionally reading the memory limit as a singned int64_t, since
there is no guarantie the setting file contains non-negative value.
See issue #25662
In glibc 2.32 new version of some symbols had been added [1]:
$ nm -D clickhouse | fgrep -e @GLIBC_2.32
U pthread_getattr_np@GLIBC_2.32
U pthread_sigmask@GLIBC_2.32
[1]: https://www.spinics.net/lists/fedora-devel/msg273044.html
Right now ubuntu 20.04 is used as official image for building
ClickHouse, however once it will be switched someone may not be happy
with that fact that he/she cannot use official binaries anymore because
they have glibc < 2.32.
To avoid this dependency, let's force previous version of those
symbols from glibc.
Note, that I've tested this by compiling with glibc 2.32 and verifying
that output ELF does not have @GLIBC_2.32 symbols and also running that
binary inside ubuntu:20.04 image (that has glibc 2.31).
v1: -Wl,--wrap
v2: -Wl,--defsym
v3: -include
v4: fix versioning for aarch64
This will allow to avoid superfluous sleep during query execution, since
this not only not desired behavoiur, but also may hang the server, since
if you will execute enough queries that will use MySQL database but will
not allow enough connections (or your MySQL server is too slow) then you
may run out of threads in the global thread pool.
Also note that right now it is possible to get deadlock when the mysql
pool is full, consider the following scenario:
- you have m1 and m2 mysql tables
- you have q1 and q2 queries, bot queries join m1 and m2
- q1 allocated connection for m1 but cannot allocate connection for m2
- q2 allocated connection for m2 but cannot allocate connection for m1
- but to resolve the lock one should give up on the locking while it is not possible right now...
And then you got no free threads and this:
# grep -h ^202 /proc/$(pgrep clickhouse-serv)/task/*/syscall | cut -d' ' -f2 | sort | uniq -c | sort -nr | head
1554 0x7ffb60b92fe8 # mutex in mysqlxx::PoolWithFailover::get
1375 0x7ffb9f1c4748 # mutex in ::PoolEntryHelper::~PoolEntryHelper from DB::MultiplexedConnections::invalidateReplica
1160 0x7ffb612918b8 # mutex in mysqlxx::PoolWithFailover::get
42 0x7ffb9f057984 # mutex in ThreadPoolImpl<std::__1::thread>::worker
*NOTE: 202 is a `futex` with WAIT*
(Went with `syscall` because debugging 10k+ threads is not easy, and
eventually it may TRAP)
replxx requires each history line to prepended with time line:
### YYYY-MM-DD HH:MM:SS.SSS
select 1
And w/o those service lines it will load all lines from history file as
one history line for suggestion. And if there are lots of lines in file it
will take lots of time (getline() + tons of reallocations).
This is the function that should take into account TLS block, and 1MB is
too high, since it will be used for sigaltstack() on SIGSEGV
v0: copy-paste glibc __pthread_get_minstack()
v2: return static 16K instead of 1MB
- Uses a small assembly file to include binary resources, rather than
objcopy
- Updates `base/common/getResource.cpp` for this new method of inclusion
- Removes linux-only guards in CMake files, as this solution is
cross-platform.
The resulting binary resources are available in the ClickHouse server
binary on Linux, macOS, and illumos platforms. FreeBSD has not been
tested, but will likely work as well.
Some code does not expect from the logger code to throw, and this can
lead to the problems like:
- marking distributed batch as broken (in
StorageDistributedDirectoryMonitor)
- some MergeTree code may also be affected.
So instead of throwing the exception, it will be logged to the stderr
(if it is possible).
