Skip to content

ci: LLVM 17 for MSAN jobs #28476

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
fanquake merged 1 commit into from
Sep 15, 2023
Merged

ci: LLVM 17 for MSAN jobs #28476

fanquake merged 1 commit into from
Sep 15, 2023

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Sep 14, 2023 •
edited
Loading

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 14, 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 MarcoFalke

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 14, 2023
@maflcko
Copy link
Member

maflcko commented Sep 14, 2023 •
edited
Loading

review ACK a241d60

  • red GHA can be ignored.

  • Seems fine to merge rc4, if CI likes it. We can do another bump in a few months to 17.0.1 (or similar).

  • Unrelated: May be good to also fixup the depends and oss-fuzz flags, in separate pull requests?

depends/hosts/linux.mk:linux_debug_CPPFLAGS=-D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_PEDANTIC -D_LIBCPP_ENABLE_ASSERTIONS=1

@maflcko
Copy link
Member

maflcko commented Sep 14, 2023

Will also fixup the issue with $MAKEJOBS currently being a no-op during the Clang & libc++ builds.

Not sure, this also seems unrelated and hard to fix. You can either pass in the variable into the dockerfile, but then changing the variable requires a re-build of the (same) dockerfile, which doesn't make sense. Or you can completely ignore it and just use nproc? I guess if someone wants to modify it, we'll let them modify the script by hand?

@fanquake fanquake marked this pull request as ready for review September 14, 2023 12:21
@fanquake
Copy link
Member Author

fanquake commented Sep 14, 2023

Seems fine to merge rc4, if CI likes it. We can do another bump in a few months to 17.0.1 (or similar).

Ok, this is fine with me.

Unrelated: May be good to also fixup the depends and oss-fuzz flags, in separate pull requests?

Will open a separate PR.

Not sure, this also seems unrelated and hard to fix.

Will punt this for now. Updated the PR description.

@fanquake
Copy link
Member Author

fanquake commented Sep 14, 2023

Seems fine to merge rc4, if CI likes it.

MSan job is now green here.

fanquake added a commit to fanquake/bitcoin that referenced this pull request Sep 14, 2023
@fanquake
build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE_ASSERTIONS
4a82503 
`_LIBCPP_ENABLE_ASSERTIONS` is deprecated, and will be removed. [See (from libc++ __config in main)](https://github.com/llvm/llvm-project/blob/b57df9fe9a1a230f277d671bfa0884bbda9fc1c5/libcxx/include/__config#L205-L209):

> TODO(hardening): remove this in LLVM 19.
> This is for backward compatibility -- make enabling `_LIBCPP_ENABLE_ASSERTIONS` (which predates hardening modes)
> equivalent to setting the safe mode.
> ifdef _LIBCPP_ENABLE_ASSERTIONS
> warning "_LIBCPP_ENABLE_ASSERTIONS is deprecated, please use _LIBCPP_ENABLE_SAFE_MODE instead."

From LLVM 17, `_LIBCPP_ENABLE_DEBUG_MODE` can be used instead.

See https://libcxx.llvm.org/Hardening.html.

Related to bitcoin#28476.
@fanquake fanquake mentioned this pull request Sep 14, 2023
Merged
@maflcko
Copy link
Member

maflcko commented Sep 15, 2023

rfm, or is something left to be done here?

@fanquake fanquake merged commit 717a4d8 into bitcoin:master Sep 15, 2023
@fanquake fanquake deleted the msan_17 branch September 15, 2023 10:50
@maflcko
Copy link
Member

maflcko commented Sep 15, 2023

Looks like this broke fuzz msan? https://cirrus-ci.com/task/5290774615556096?logs=ci#L9230

@fanquake
Copy link
Member Author

fanquake commented Sep 15, 2023

Weird. Taking a look.

Yugthakkar04
Yugthakkar04 reviewed Sep 15, 2023
View reviewed changes
Copy link

@Yugthakkar04 Yugthakkar04 left a comment

Choose a reason for hiding this comment

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

change this alibi to the converge libc++ from on to off and switch it to an internal open source internet LLVM 17

reviewing console:
https://discourse.llvm.org/t/rfc-removing-the-legacy-debug-mode-from-libc/71026

fanquake added a commit that referenced this pull request Sep 16, 2023
@fanquake
Merge #28479: build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE_ASSERT…
f01416e 
…IONS

4a82503 build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE_ASSERTIONS (fanquake)

Pull request description:

  `_LIBCPP_ENABLE_ASSERTIONS` is deprecated, and will be removed. [See (from libc++ __config in main)](https://github.com/llvm/llvm-project/blob/b57df9fe9a1a230f277d671bfa0884bbda9fc1c5/libcxx/include/__config#L205-L209):

  > TODO(hardening): remove this in LLVM 19.
  > This is for backward compatibility -- make enabling `_LIBCPP_ENABLE_ASSERTIONS` (which predates hardening modes)
  > equivalent to setting the safe mode.
  > ifdef _LIBCPP_ENABLE_ASSERTIONS
  > warning "_LIBCPP_ENABLE_ASSERTIONS is deprecated, please use _LIBCPP_ENABLE_SAFE_MODE instead."

  From LLVM 17, `_LIBCPP_ENABLE_DEBUG_MODE` can be used instead, which also performs more checks than safe mode:

  > Enables the debug mode which contains all the checks from the hardened mode and additionally more expensive checks that may affect the complexity of algorithms. The debug mode is intended to be used for testing, not in production. Mutually exclusive with `_LIBCPP_ENABLE_HARDENED_MODE` and `_LIBCPP_ENABLE_SAFE_MODE`.

  See https://libcxx.llvm.org/Hardening.html.

  Related to #28476.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 4a82503 🙏

Tree-SHA512: ca52603f86214e8e9350bd2b2baa44fbde0f72f1b186da7aecd8690256dff5b2be75fe89383158298a6f683bbd6ae0dff528d2ba4cc5ece1f56cfbdee0e1dc5d
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2023
@fanquake
Merge bitcoin#28479: build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE…
17a3555 
…_ASSERTIONS

4a82503 build: use _LIBCPP_ENABLE_DEBUG_MODE over ENABLE_ASSERTIONS (fanquake)

Pull request description:

  `_LIBCPP_ENABLE_ASSERTIONS` is deprecated, and will be removed. [See (from libc++ __config in main)](https://github.com/llvm/llvm-project/blob/b57df9fe9a1a230f277d671bfa0884bbda9fc1c5/libcxx/include/__config#L205-L209):

  > TODO(hardening): remove this in LLVM 19.
  > This is for backward compatibility -- make enabling `_LIBCPP_ENABLE_ASSERTIONS` (which predates hardening modes)
  > equivalent to setting the safe mode.
  > ifdef _LIBCPP_ENABLE_ASSERTIONS
  > warning "_LIBCPP_ENABLE_ASSERTIONS is deprecated, please use _LIBCPP_ENABLE_SAFE_MODE instead."

  From LLVM 17, `_LIBCPP_ENABLE_DEBUG_MODE` can be used instead, which also performs more checks than safe mode:

  > Enables the debug mode which contains all the checks from the hardened mode and additionally more expensive checks that may affect the complexity of algorithms. The debug mode is intended to be used for testing, not in production. Mutually exclusive with `_LIBCPP_ENABLE_HARDENED_MODE` and `_LIBCPP_ENABLE_SAFE_MODE`.

  See https://libcxx.llvm.org/Hardening.html.

  Related to bitcoin#28476.

ACKs for top commit:
  MarcoFalke:
    lgtm ACK 4a82503 🙏

Tree-SHA512: ca52603f86214e8e9350bd2b2baa44fbde0f72f1b186da7aecd8690256dff5b2be75fe89383158298a6f683bbd6ae0dff528d2ba4cc5ece1f56cfbdee0e1dc5d
@maflcko
Copy link
Member

maflcko commented Sep 19, 2023

Weird. Taking a look.

Seems to be passing on aarch64, so far

@maflcko
Copy link
Member

maflcko commented Sep 19, 2023

on x86_64:

# PRINT_ALL_FUZZ_TARGETS_AND_ABORT=1 LD_LIBRARY_PATH=/ci_container_base/depends/x86_64-pc-linux-gnu/lib /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz 
==29942==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x561e032f63ff in SetArgs(int, char**) src/test/fuzz/fuzz.cpp:48:5
    #1 0x561e032f63ff in LLVMFuzzerInitialize src/test/fuzz/fuzz.cpp:185:5
    #2 0x561e02b5f6c3 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) /msan/llvm-project/compiler-rt/lib/fuzzer/FuzzerDriver.cpp:647:5
    #3 0x561e02b8d9b2 in main /msan/llvm-project/compiler-rt/lib/fuzzer/FuzzerMain.cpp:20:10
    #4 0x7f37ebc51d8f  (/lib/x86_64-linux-gnu/libc.so.6+0x29d8f) (BuildId: 229b7dc509053fe4df5e29e8629911f0c3bc66dd)
    #5 0x7f37ebc51e3f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29e3f) (BuildId: 229b7dc509053fe4df5e29e8629911f0c3bc66dd)
    #6 0x561e02b526d4 in _start (/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz+0x8076d4)

  Member fields were destroyed
    #0 0x561e02c1d0fd in __sanitizer_dtor_callback_fields /msan/llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:982:5
    #1 0x561e04b809e8 in std::__1::unique_ptr<std::__1::__tree_node<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, void*>, std::__1::__tree_node_destructor<std::__1::allocator<std::__1::__tree_node<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, void*>>>>::~unique_ptr[abi:v170000]() /msan/cxx_build/include/c++/v1/__memory/unique_ptr.h:137:44
    #2 0x561e04b809e8 in std::__1::unique_ptr<std::__1::__tree_node<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, void*>, std::__1::__tree_node_destructor<std::__1::allocator<std::__1::__tree_node<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, void*>>>>::~unique_ptr[abi:v170000]() /msan/cxx_build/include/c++/v1/__memory/unique_ptr.h:266:84
    #3 0x561e04b809e8 in std::__1::pair<std::__1::__tree_iterator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::__tree_node<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, void*>*, long>, bool> std::__1::__tree<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::less<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>>::__emplace_unique_impl<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /msan/cxx_build/include/c++/v1/__tree:2167:1
    #4 0x561e02b52504 in std::__1::pair<std::__1::__tree_iterator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::__tree_node<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, void*>*, long>, bool> std::__1::__tree<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::less<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>>::__emplace_unique[abi:v170000]<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /msan/cxx_build/include/c++/v1/__tree:1200:16
    #5 0x561e02b52504 in std::__1::pair<std::__1::__tree_const_iterator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::__tree_node<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, void*>*, long>, bool> std::__1::set<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>, std::__1::less<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>, std::__1::allocator<std::__1::pair<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>>>>::emplace[abi:v170000]<std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&>(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) /msan/cxx_build/include/c++/v1/set:787:29
    #6 0x561e02b52504 in CRPCConvertTable::CRPCConvertTable() src/rpc/client.cpp:341:23
    #7 0x561e02b52504 in __cxx_global_var_init.222 src/rpc/client.cpp:345:25
    #8 0x561e02b52504 in _GLOBAL__sub_I_client.cpp src/rpc/client.cpp
    #9 0x7f37ebc51eba in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x29eba) (BuildId: 229b7dc509053fe4df5e29e8629911f0c3bc66dd)

SUMMARY: MemorySanitizer: use-of-uninitialized-value src/test/fuzz/fuzz.cpp:48:5 in SetArgs(int, char**)
Exiting

Yugthakkar04
Yugthakkar04 reviewed Sep 19, 2023
View reviewed changes
Copy link

@Yugthakkar04 Yugthakkar04 left a comment

Choose a reason for hiding this comment

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

All changes are done perfectly just work upon the security procedure once

@maflcko
Copy link
Member

maflcko commented Sep 20, 2023

Took another look, and this has something to do with dropping -DLIBCXX_ENABLE_DEBUG_MODE=ON

Locally, the same problem happens on 16.0.6 when dropping -DLIBCXX_ENABLE_DEBUG_MODE=ON.

🤔

@fanquake fanquake mentioned this pull request Oct 3, 2023
Closed
@maflcko maflcko mentioned this pull request Mar 19, 2024
Merged
fanquake added a commit that referenced this pull request Mar 20, 2024
@fanquake
Merge #29676: ci: Bump msan to llvm-18
5b9831a 
faecf3a ci: Bump msan to llvm-18 (MarcoFalke)

Pull request description:

  Last one: #28476

ACKs for top commit:
  fanquake:
    ACK faecf3a - There is now a 18.1.2, but given it doesn't fix the instrumenting in libunwind, we don't need that here. I've tested that both jobs are now working on both arches.

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

Reviewers

1 more reviewer

@Yugthakkar04 Yugthakkar04 Yugthakkar04 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fanquake @DrahtBot @maflcko @Yugthakkar04