Skip to content

ci: clang-17 for fuzz and tsan #28465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 14, 2023
Merged

ci: clang-17 for fuzz and tsan #28465

merged 1 commit into from
Sep 14, 2023

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Sep 13, 2023

Bump clang in CI from 16 to 17, to:

  • Bump the CI "EOL" from Jan 2024 to July 2024, by bumping from Ubuntu lunar to mantic
  • Test, ensure compatibility, and make use of any new sanitizer features in clang-17

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 13, 2023 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK dergoegge
Concept ACK fanquake, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Sep 13, 2023
@maflcko maflcko added this to the 26.0 milestone Sep 13, 2023
@maflcko
Copy link
Member Author

maflcko commented Sep 13, 2023

Looks like tidy fails with an error:

clang-tidy-17 -p=/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz.cpp
test/fuzz/fuzz.cpp:94:34: error: do not use 'std::endl' with streams; use '\n' instead [performance-avoid-endl,-warnings-as-errors]
   94 |             std::cout << name << std::endl;
      |                                  ^~~~~~~~~
      |                                  '\n'

So I'll postpone that for now.

@fanquake
Copy link
Member

Concept ACK. Could probably just disable performance-avoid-endl.

@hebasto
Copy link
Member

hebasto commented Sep 13, 2023

Concept ACK.

@maflcko
Copy link
Member Author

maflcko commented Sep 13, 2023

Looks like USDT is broken as well on Mantic, regardless of the clang version: #28467 (comment)

Going to drop that as well, for now.

@dergoegge
Copy link
Member

Concept ACK

maflcko
maflcko commented Sep 13, 2023
View reviewed changes
ci/test/01_base_install.sh Outdated
@@ -42,7 +42,7 @@ if [ -n "$PIP_PACKAGES" ]; then
fi

if [[ ${USE_MEMORY_SANITIZER} == "true" ]]; then
git clone --depth=1 https://github.com/llvm/llvm-project -b llvmorg-16.0.6 /msan/llvm-project
git clone --depth=1 https://github.com/llvm/llvm-project -b "llvmorg-17.0.0-rc4" /msan/llvm-project

cmake -G Ninja -B /msan/clang_build/ \
-DLLVM_ENABLE_PROJECTS="clang" \
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unrelated: MAKEJOBS does not work in this file, when building the docker image. Not sure what the best fix is for this.

@maflcko
Copy link
Member Author

maflcko commented Sep 13, 2023

Looks like msan also fails:

-- Performing Test CXX_SUPPORTS_GLINE_TABLES_ONLY_FLAG - Success
CMake Error at /msan/llvm-project/libcxx/CMakeLists.txt:763 (message):
  LIBCXX_ENABLE_ASSERTIONS has been replaced by
  LIBCXX_HARDENING_MODE=hardened


-- Configuring incomplete, errors occurred!

So I'll drop that as well for now.

@maflcko maflcko changed the title ci: clang-17 ci: clang-17 for fuzz and tsan Sep 13, 2023
@maflcko
Copy link
Member Author

maflcko commented Sep 13, 2023

This is ready for review. Follow-ups that need more work:

fanquake
fanquake approved these changes Sep 13, 2023
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm fa23c9a - I can followup with MSAN & tidy

@maflcko
Copy link
Member Author

maflcko commented Sep 13, 2023

iwyu doesn't have a branch, so I think tidy will need to wait either way. Though, msan can be done, and shouldn't conflict with this pull in any way.

dergoegge
dergoegge approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fa23c9a

@fanquake fanquake merged commit 9e9206f into bitcoin:master Sep 14, 2023
@maflcko maflcko deleted the 2309-ci-clang-17- branch September 14, 2023 10:42
@fanquake fanquake mentioned this pull request Sep 14, 2023
Merged
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2023
@fanquake
Merge bitcoin#28465: ci: clang-17 for fuzz and tsan
9a1fa9a
fanquake added a commit that referenced this pull request Oct 11, 2023
@fanquake
Merge #28482: ci: use LLVM/Clang 17 in tidy job
bf32ae3 
8735e2c ci: use LLVM/Clang 17 in tidy job (fanquake)
ce46b68 ci: use LLVM 17.0.2 in MSAN jobs (fanquake)

Pull request description:

  Also update MSAN to use 17.0.2.

  Related to #28465.

ACKs for top commit:
  maflcko:
    lgtm ACK 8735e2c

Tree-SHA512: 74452b95326cf065afe8332dc1b5b8e5ac12c8fe05c278a1cee017f87a7f7e0cdb8cac5e39d718c8ef587c8ee229bbaadd847df9f191313d41c5cdcab45e7c76
@bitcoin bitcoin locked and limited conversation to collaborators Sep 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fanquake fanquake fanquake approved these changes

+1 more reviewer

@dergoegge dergoegge dergoegge approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
26.0
Development

Successfully merging this pull request may close these issues.
5 participants
@maflcko @DrahtBot @fanquake @hebasto @dergoegge