Skip to content

Sanitize thirdparty libraries for public flags#43275

Merged
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
azat:build/sanitize-targets
Nov 22, 2022
Merged

Sanitize thirdparty libraries for public flags#43275
alexey-milovidov merged 1 commit intoClickHouse:masterfrom
azat:build/sanitize-targets

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Nov 16, 2022

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)

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Fixes: #12447 (cc @alexey-milovidov )

@azat
Sanitize thirdparty libraries for public flags
c3f3078 
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 d5ea69d..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: ClickHouse#12447
Signed-off-by: Azat Khuzhin <[email protected]>
@azat azat mentioned this pull request Nov 16, 2022
Merged
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-not-for-changelog This PR should not be mentioned in the changelog label Nov 16, 2022
@azat
Copy link
Copy Markdown
Member Author

azat commented Nov 18, 2022

Failures are unrelated:

AST fuzzer (ubsan) — ../src/Analyzer/Passes/QueryAnalysisPass.cpp:1523:53: runtime error: member call on null pointer of type 'DB::IQueryTreeNode'

Stress test (asan) — Backward compatibility check: Error message in clickhouse-server.log (see bc_check_error_messages.txt)

2022.11.16 14:19:58.746429 [ 251275 ] {} <Error> virtual void DB::MetadataStorageFromDiskTransaction::commit(): Code: 566. DB::ErrnoException: Cannot rmdir /var/lib/clickhouse/disks/s3/data/system/session_log/delete_tmp_202211_133_133_0, errno: 2, strerror: No such file or directory. (CANNOT_RMDIR), Stack trace (when copying this message, always include the lines below):
2022.11.16 14:19:58.747495 [ 251275 ] {} <Error> system.session_log: Cannot quickly remove directory /var/lib/clickhouse/disks/s3/data/system/session_log/delete_tmp_202211_133_133_0 by removing files; fallback to recursive removal. Reason: std::exception. Code: 1001, type: std::__1::__fs::filesystem::filesystem_error, e.what() = filesystem error: in create_directory: No such file or directory ["/var/lib/clickhouse/disks/s3/data/system/session_log/delete_tmp_202211_133_133_0"]
2022.11.16 14:19:58.889230 [ 251275 ] {} <Error> virtual void DB::MetadataStorageFromDiskTransaction::commit(): Code: 566. DB::ErrnoException: Cannot rmdir /var/lib/clickhouse/disks/s3/data/system/session_log/delete_tmp_202211_133_133_0/, errno: 2, strerror: No such file or directory. (CANNOT_RMDIR), Stack trace (when copying this message, always include the lines below):
2022.11.16 14:20:20.897346 [ 250991 ] {} <Error> Application: Caught exception while loading metadata: Code: 566. DB::ErrnoException: Cannot rmdir /var/lib/clickhouse/disks/s3/data/system/session_log/delete_tmp_202211_133_133_0/, errno: 2, strerror: No such file or directory: While committing metadata operation #0. (CANNOT_RMDIR), Stack trace (when copying this message, always include the lines below):
2022.11.16 14:20:24.912323 [ 250991 ] {} <Error> Application: DB::ErrnoException: Cannot rmdir /var/lib/clickhouse/disks/s3/data/system/session_log/delete_tmp_202211_133_133_0/, errno: 2, strerror: No such file or directory: While committing metadata operation #0

Stress test (msan) — OOM killer (or signal 9) in clickhouse-server.log

Real OOM and:

2022.11.16 14:38:22.131458 [ 279161 ] {} <Error> f711cad1-3050-496f-9d11-5c911ca0e030::all_0_52_2 (MergeFromLogEntryTask): virtual bool DB::ReplicatedMergeMutateTaskBase::executeStep(): Code: 84. DB::Exception: Directory /var/lib/clickhouse/disks/s3/store/f71/f711cad1-3050-496f-9d11-5c911ca0e030/tmp_merge_all_0_52_2/ already exists. (DIRECTORY_ALREADY_EXISTS), Stack trace (when copying this message, always include the lines below):

@alexey-milovidov alexey-milovidov merged commit f42be5f into ClickHouse:master Nov 22, 2022
@alexey-milovidov alexey-milovidov self-assigned this Nov 22, 2022
@azat azat deleted the build/sanitize-targets branch November 22, 2022 18:29
@Tiaonmmn
Copy link
Copy Markdown
Contributor

Tiaonmmn commented Nov 26, 2022

This breaks Intel QPL Build.
Using cmake command line:

cmake -D ENABLE_AVX2=ON -D ENABLE_AVX512=ON -D ENABLE_RUST=ON -D ENABLE_BMI=ON -D ENABLE_AVX512_VBMI=OFF -D ENABLE_AVX512_FOR_SPEC_OP=ON -D ENABLE_EXAMPLES=OFF -D ENABLE_AVX=ON -D WITH_JEMALLOC=ON -D ENABLE_MULTITARGET_CODE=OFF -D ENABLE_AZURE_BLOB_STORAGE=OFF -D ENABLE_SSL=ON -D ENABLE_S3=OFF -D ENABLE_CLICKHOUSE_TEST=OFF -D ENABLE_AMQPCPP=OFF -D ENABLE_CASSANDRA=OFF -D ENABLE_FUZZING=OFF -D ENABLE_NATS=OFF -D ENABLE_QPL=ON ..

Error occured:
CMake Error at cmake/sanitize_targets.cmake:69 (message):
isal_asm set INTERFACE_COMPILE_OPTIONS to
-I/root/ClickHouse/contrib/qpl/sources/isal/include/;-I/root/ClickHouse/contrib/qpl/sources/isal/igzip/;-I/root/ClickHouse/contrib/qpl/sources/isal/crc/;-DQPL_LIB;-DHAVE_AS_KNOWS_AVX512;-DAS_FEATURE_LEVEL=10.
This is forbidden.
Call Stack (most recent call first):
cmake/sanitize_targets.cmake:79 (sanitize_interface_flags)
CMakeLists.txt:596 (include)

@azat
Copy link
Copy Markdown
Member Author

azat commented Nov 26, 2022

@Tiaonmmn we can do this checks under options, or disallow only -Wxxx flags. But how about changing your build instead and set them only for the library itself (i.e. PRIVATE), and maybe if (TARGET contrib::qpl) do_something() in cmake rules (if you need something)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexey-milovidov alexey-milovidov alexey-milovidov approved these changes

Assignees

@alexey-milovidov alexey-milovidov

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Ensure that no -Wno arguments suddenly appear.

4 participants

@azat @Tiaonmmn @alexey-milovidov @robot-ch-test-poll1