Add ability to build with Clang #507

p-senichenkov · 2024-12-23T16:23:31Z

Make Desbordante be able to be built with Clang on Linux and LLVM/Apple CLang on macOS:

Make some changes to build.sh and CMakeLists.txt (bump versions of some libraries and workaround some Clang bugs)
Fix warnings, UB and address issues, that GCC hasn't found and Clang has
Update "build" section in README.md
Get rid of GCC intrinsics
Fix some implementation-defined behaviour
Add corresponding tests to CI
See commit descriptions for more information

Vdaleke

Thank you for your great work. I'm not completely done with the review yet and there will probably be more comments soon, but I'll post these so you can start fixing them.

I will leave a few comments that cannot be attached to individual files:

There are no binding-tests for Clang and macOS Apple Clang. And there may be problems similar to those that caused the Fix CMakeLists.txt, macOS guide and refactor .gitignore #492, I mean DYLD_PATH on macOS.
We keep a short commit history. Pls, squash commits to some parts. I propose squash into 4 like this: CI, src-fixing, CMake, README.
You made great descriptions for all commits, pls copy them in some way to PR description. If someone else looks at the PR, they don't look for commit descriptions, and it's also a good way to present the result.

.github/composite-actions/download-libraries/action.yml

.github/workflows/core-tests.yml

build.sh

.github/composite-actions/download-libraries/action.yml

README.md

.github/workflows/core-tests.yml

Vdaleke · 2025-01-04T19:07:53Z

Interesting checks, maybe somewhere into project settings these names are inlined to block merge without them?

p-senichenkov · 2025-01-04T19:12:42Z

Interesting checks, maybe somewhere into project settings these names are inlined to block merge without them?

Yes, it's branch protection rules. I left a comment about it in PR#499

Vdaleke · 2025-01-04T19:17:03Z

Interesting checks, maybe somewhere into project settings these names are inlined to block merge without them?

Yes, it's branch protection rules. I left a comment about it in PR#499

Oh, I didn't notice there are 2 pull request, is 499 needed at all if this one exists? I suggest closing it and continuing only in this one.

p-senichenkov · 2025-01-04T19:22:36Z

Oh, I didn't notice there are 2 pull request, is 499 needed at all if this one exists? I suggest closing it and continuing only in this one.

There's 3 pull requests. 499 addresses Clang on Linux, 500 -- LLVM Clang on macOS, and this PR addresses Apple Clang.
499 is a part of 500, and 500 is a part of 507. I recommend to review them in this order.
I think, it's better to have 3 different PRs and try to merge as many of them as possible before the release

Vdaleke · 2025-01-04T21:46:19Z

Oh, I didn't notice there are 2 pull request, is 499 needed at all if this one exists? I suggest closing it and continuing only in this one.

There's 3 pull requests. 499 addresses Clang on Linux, 500 -- LLVM Clang on macOS, and this PR addresses Apple Clang. 499 is a part of 500, and 500 is a part of 507. I recommend to review them in this order. I think, it's better to have 3 different PRs and try to merge as many of them as possible before the release

I don't like that they are tightly coupled in CI. It seems to me that when all the fixes are already in place, it's easier to do and review everything together, rather than merge one by one, changing the architecture in the process. And do we really need a release without a pip-package on MacOS?

Vdaleke

The second part of review.

As for constructors which is in a lot of comments. I heard about the performance of the aggregates from colleagues at work, and constantly. For example, that's why it is not recommended to use a std::pair, for some reason they added a constructor there and a regular handwritten structure will be faster. But I can't find normal proofs on the Internet.

src/core/util/bitset_extensions.cpp

src/core/util/bitset_extensions.h

src/core/util/maybe_unused.h

src/tests/test_algo_interfaces.cpp

src/tests/test_pfdtane.cpp

src/tests/test_tane_afd_measures.cpp

src/core/util/auto_join_thread.h

p-senichenkov · 2025-01-05T09:17:24Z

As for constructors which is in a lot of comments. I heard about the performance of the aggregates from colleagues at work, and constantly. For example, that's why it is not recommended to use a std::pair, for some reason they added a constructor there and a regular handwritten structure will be faster. But I can't find normal proofs on the Internet.

The only thing I can do is to define constructors conditionnally -- only when aggregate initialization isn't availible.

Vdaleke · 2025-01-05T13:29:39Z

As for constructors which is in a lot of comments. I heard about the performance of the aggregates from colleagues at work, and constantly. For example, that's why it is not recommended to use a std::pair, for some reason they added a constructor there and a regular handwritten structure will be faster. But I can't find normal proofs on the Internet.

The only thing I can do is to define constructors conditionnally -- only when aggregate initialization isn't availible.

I don't quite understand why you can't just use brace-initialization everywhere?

p-senichenkov · 2025-01-05T14:06:29Z

I don't quite understand why you can't just use brace-initialization everywhere?

Brace-initialization calls constructor. It won't help if we want to get rid of constructors.

Vdaleke · 2025-01-05T14:31:00Z

Brace-initialization calls constructor.

It's not true?

https://learn.microsoft.com/en-us/cpp/cpp/initializing-classes-and-structs-without-constructors-cpp?view=msvc-170#:~:text=When%20a%20class%20or%20struct%20has%20no%20constructor%2C%20you%20provide%20the%20list%20elements%20in%20the%20order%20that%20the%20members%20are%20declared%20in%20the%20class.

p-senichenkov · 2025-01-06T09:56:30Z

In terms of this page on cppreference, apparently, you suggest to use direct-list-initialization (2 variant) instead of copy-list-initialization (7 variant). Maybe this will work -- I'll check it later. But in most cases we have emplace_back, that cannot be replaced with direct-list-initialization.
So, there are still two possible ways:

conditionally define constructor
conditionally replace emplace_back(arg1, arg2) with push_back(T{arg1, arg2})
(by "conditionally", I mean using of feature-test macros). As for me, the second way is very bad.

Vdaleke · 2025-01-06T11:49:33Z

In terms of this page on cppreference,

Not working link

But in most cases we have emplace_back, that cannot be replaced with direct-list-initialization.

Use push_back instead

p-senichenkov · 2025-01-06T11:51:37Z

Not working link

https://en.cppreference.com/w/cpp/language/list_initialization

.github/composite-actions/install-dependencies/action.yml

github-actions

clang-tidy made some suggestions

src/core/util/bitset_extensions.h

Vdaleke · 2025-01-06T21:20:55Z

Due to ac2ad57.
Let me remind you that at the meeting we agreed to leave in the readme only gcc for Linux and only apple clang for macOS, everything else will move to the wiki.

github-actions

clang-tidy made some suggestions

src/core/algorithms/md/hymd/md_lhs.h

github-actions

clang-tidy made some suggestions

src/core/algorithms/md/hymd/md_lhs.h

github-actions

clang-tidy made some suggestions

src/core/algorithms/dd/split/split.cpp

- Add `set -e` in build.sh to stop build process on error - Add SYSTEM property to all `add_subdirectory` commands that add external dependencies to avoid compiler warnings that aren't related to Desbordante - Disable alloc-dealloc-mismatch check as it's false positive on Ubuntu (see comment in CMakeLists.txt) - Limit UB sanitizer "function" check to only our code when building with Clang on macOS (see comment in CMakeLists.txt) - Update googletest version from 1.13.0 to 1.14.0, because 1.13.0 produces "narrowing conversion" warning due to a bug (issue #4206, google/googletest#4206) when building with Clang. This bug was fixed in 1.14.0 (commit 3044657, google/googletest@3044657) - Disable deprecated-declaration warning on Clang-18 (see comment in CMakeLists.txt)

Vdaleke

Thanks for the corrections, it looks much better. I left a couple of minor comments and questions. Of the possible major edits, only replacing action with a composition in CI-tests, if it is possible to catch an incorrect configuration.

.github/composite-actions/install-dependencies/action.yml

.github/workflows/bindings-tests.yml

.github/workflows/check-codestyle.yml

README.md

src/core/util/bitset_extensions.h

src/core/util/maybe_unused_on_clang.h

src/tests/test_algo_interfaces.cpp

Fix Clang warnings and UB and ADDRESS sanitizer errors: - Replace INSTANTIATE_TEST_CASE_P, which is deprecated, with INSTANTIATE_TEST_SUITE_P in NDVerifier tests - Mark unused fields as [[maybe_unused]]. Use MAYBE_UNUSED_PRIVATE_FIELD macro, because GCC produces warning when [[maybe_unused]] attribute is present, and Clang -- when it isn't. (see comment in util/maybe_unused_on_clang.h) - Convert double literals to model::DoubleType and model::IntType explicitly in tests/test_types.cpp - Check that column is numeric *before* casting it to INumericType in DataStats::GetMedianAD - Don't process empty vector in GetPartition in gfd_validation.cpp (immediately return 0) - Disable tests causing float-to-long cast when result is infinity in tests/test_types.cpp - Use std::chrono::high_resolution_clock instead of std::chrono::_V2::high_resolution_clock in fastod/util/timer.h, because symbol versioning is gcc-specific - Use &Insert instead of this->Insert in KDTree constructor - Use system_clock everywhere instead of mix of system_clock and high_resolution_clock in Cords::ExecuteInternal - Add fallback behaviour in hymd::utility::MdLessPairs if operator <=> for vectors isn't availible - Add explicit #include <string> in exceptions.h - Add template keyword in CanonicalOD::IsValid to avoid most vexing parse (see cross-platform guide in wiki) - Explicitly cast kSeed_ to result_type in des/rng.h, because result_type of mt19937 is always 32-bit and it's not bound to some built-in integral type - Use reverse iterators to traverse set in reverse order, instead of strange behaviour with direct iterators in egfd_validation.cpp/ReverseConstruction

Add information about other compilers in README. Extra build instructions (Linux & Clang, macOS & LLVM CLang, macOS & GCC) are now moved to wiki

Vdaleke

LGTM, just remove FIXME pls

src/core/util/bitset_extensions.h

- Use simple RAII wrapper of std::thread instead of std::jthread when the latter isn't implemented - Use custom implementations of std::bitset::_Find_first and _Find_next when gcc intrinsics aren't availible. Also, implement some utility functions using custom FindFirst and FindNext

Don't use features, which behaviour is implementation-defined and differ in libstdc++ and libc++. Instead use "stable" versions: - sort pairs by value if frequencies are equal in Cords' FrequencyHandler::InitFrequencyHandler - Use std::stable_sort instead of std::sort in DES::GetRandomPopulationInDomainds - Use std::set instead of std::unordered_set in differential_functions.cpp/GetRandIndices - Use std::stable_sort in PDFTane::CalculatePFDError instead of std::sort

.github/workflows/core-tests.yml

Vdaleke

Sorry for the request after approval, but it seems like an easy way to get rid of the double name in actions.

.github/workflows/bindings-tests.yml

.github/workflows/core-tests.yml

.github/workflows/bindings-tests.yml

src/python_bindings/od/bind_od.cpp

src/core/algorithms/od/order/order_utility.cpp

.github/composite-actions/install-dependencies/action.yml

Vdaleke · 2025-01-12T09:32:03Z

I managed to build a pip package for macOS under llvm-clang and now I think that llvm-clan should be the default compiler for macOS because llvm-clang has more extensions and it should be better in performance?
Then I suggest leaving such configurations:

{macos-latest, llvm-clang, Release},
{macos-latest, llvm-clang, Debug},
{macos-latest, llvm-clang, Debug, UB},
{macos-latest, llvm-clang, Debug, Address},
{macos-latest, apple-clang, Release},

The last one to check compatibility with apple clang in terms of extensions and standard support.

And then I think that in the readme we need to insert the assembly configuration under the llvm clang, since we choose it as the main one.

p-senichenkov · 2025-01-12T09:41:26Z

I managed to build a pip package for macOS under llvm-clang and now I think that llvm-clan should be the default compiler for macOS because llvm-clang has more extensions and it should be better in performance?

Why do you think LLVM Clang will be more effective? The only difference is aggregate initialization, which is corrected anyway. I think, "native" compiler can be better (and can be worse -- we should test it).

However, Apple Clang building instruction are much shorter, as there's no need in compiling boost. Since Apple Clang is pre-installed on macOS, users can build project without much preparation, like with GCC on Linux. I don't think we should place LLVM Clang in README as "main" compiler.

Vdaleke · 2025-01-12T12:08:01Z

The only difference is aggregate initialization, which is corrected anyway.

No that's not the only difference, llvm clang provides bitset and jthread extensions for which we have provided a replacement that should not be as efficient as the implementation from the clang compiler

However, Apple Clang building instruction are much shorter, as there's no need in compiling boost. Since Apple Clang is pre-installed on macOS, users can build project without much preparation, like with GCC on Linux. I don't think we should place LLVM Clang in README as "main" compiler.

Maybe it's true, but I think it's worth making some kind of annotation that we use llvm-clang as the default compiler for building the pip package and it's worth trying to build using it

p-senichenkov · 2025-01-12T12:17:19Z

No that's not the only difference, llvm clang provides bitset and jthread extensions for which we have provided a replacement that should not be as efficient as the implementation from the clang compiler

libc++, which is used in any Clang does not provide bitset extensions and thread. See https://en.cppreference.com/w/cpp/compiler_support

Add tests for Clang on Linux, LLVM Clang on macOS and Apple Clang on macOS. Also, bump boost version used in CI, as 1.8.10 produces an error on macOS with LLVM Clang. Here it's said that boost 1.85.0 works fine: bambulab/BambuStudio#3957

Aggregate initialization isn't availible on Apple Clang. Use direct-list-initialization instead wherewer possible: - Split::IndexSearchSpace - MinPickerLattice::AddGeneralizations - OneByOnePicker::AddGeneralizations - md_lattice/Specializer - MdLattice::GetAll - hymd::LhsNode - ::BatchValidator - SimilarityData::Creator - BatchValidator::MultiCardPartitionElementProvider - MD::GetDescription - order::GetIndexedByteData - python_bindings::BindOd - model::DD Define constructor elsewhere: - test_algo_interfaces/KeysTestParams - test_dc_verifier/DCTestParams - test_ind_verifier/INDVerifierTestConfig - test_pfdtane/PFDTaneValidationParams - test_tane_afd_measures/TaneValidationParams - /PdepSelfValidationParams - test_typed_column_data/TypeParsingParams

Vdaleke · 2025-01-12T13:29:41Z

libc++, which is used in any Clang does not provide bitset extensions and thread.

I don't quite understand, it was necessary to add jthread and the bitset extension only when apple-clang was added. Can't we use libstdc++?

p-senichenkov · 2025-01-12T13:34:43Z

it was necessary to add jthread and the bitset extension only when apple-clang was added

No, it was necessary even on Linux (see #499)

Can't we use libstdc++?

We can, but there's no point in compiling with Clang, if we link it with GCC's standard library. In that case we could just close this PR without merging.

Vdaleke · 2025-01-12T13:36:58Z

it was necessary to add jthread and the bitset extension only when apple-clang was added

No, it was necessary even on Linux (see #499)

Can't we use libstdc++?

We can, but there's no point in compiling with Clang, if we link it with GCC's standard library. In that case we could just close this PR without merging.

now I think that we should supply a pip package built with gcc even on macos, but that's not your concern anymore, maybe I'll add it myself

p-senichenkov force-pushed the apple-clang-buildability branch from dac18fc to 2108d8a Compare January 3, 2025 11:30

p-senichenkov marked this pull request as ready for review January 3, 2025 11:31

Vdaleke self-requested a review January 3, 2025 14:54

Vdaleke requested changes Jan 4, 2025

View reviewed changes

Vdaleke requested changes Jan 5, 2025

View reviewed changes

p-senichenkov force-pushed the apple-clang-buildability branch from be21b26 to eb10e2e Compare January 6, 2025 13:05

Vdaleke requested changes Jan 6, 2025

View reviewed changes

github-actions bot reviewed Jan 6, 2025

View reviewed changes

src/core/util/bitset_extensions.h Show resolved Hide resolved

p-senichenkov force-pushed the apple-clang-buildability branch from 6fc2ab3 to 1bd7606 Compare January 6, 2025 20:37

github-actions bot reviewed Jan 7, 2025

View reviewed changes

src/core/algorithms/md/hymd/md_lhs.h Outdated Show resolved Hide resolved

src/core/algorithms/md/hymd/md_lhs.h Outdated Show resolved Hide resolved

github-actions bot reviewed Jan 7, 2025

View reviewed changes

src/core/algorithms/md/hymd/md_lhs.h Outdated Show resolved Hide resolved

github-actions bot reviewed Jan 7, 2025

View reviewed changes

src/core/algorithms/dd/split/split.cpp Outdated Show resolved Hide resolved

p-senichenkov force-pushed the apple-clang-buildability branch from d09bbbd to 34a9520 Compare January 8, 2025 12:50

p-senichenkov force-pushed the apple-clang-buildability branch from 34a9520 to 859b5c0 Compare January 8, 2025 15:24

p-senichenkov requested a review from Vdaleke January 8, 2025 21:11

Vdaleke requested changes Jan 8, 2025

View reviewed changes

p-senichenkov added 2 commits January 9, 2025 21:28

Update "build" section of README

ce21ddb

Add information about other compilers in README. Extra build instructions (Linux & Clang, macOS & LLVM CLang, macOS & GCC) are now moved to wiki

p-senichenkov requested a review from Vdaleke January 9, 2025 19:20

p-senichenkov force-pushed the apple-clang-buildability branch from ca4b7e7 to 35fb7a7 Compare January 9, 2025 20:32

Vdaleke requested changes Jan 9, 2025

View reviewed changes

src/core/util/bitset_extensions.h Outdated Show resolved Hide resolved

src/core/util/bitset_extensions.h Outdated Show resolved Hide resolved

p-senichenkov added 2 commits January 10, 2025 11:25

p-senichenkov force-pushed the apple-clang-buildability branch from 35fb7a7 to 4a16399 Compare January 10, 2025 08:26

p-senichenkov requested a review from Vdaleke January 10, 2025 08:26

p-senichenkov mentioned this pull request Jan 10, 2025

Simplify configuration logic in CI #512

Open

2 tasks

p-senichenkov force-pushed the apple-clang-buildability branch from 9e384f4 to cfd4b2b Compare January 10, 2025 09:55

Vdaleke approved these changes Jan 10, 2025

View reviewed changes

Vdaleke reviewed Jan 10, 2025

View reviewed changes

.github/workflows/core-tests.yml Outdated Show resolved Hide resolved

p-senichenkov force-pushed the apple-clang-buildability branch from cfd4b2b to a8814af Compare January 10, 2025 12:00

Vdaleke requested changes Jan 11, 2025

View reviewed changes

.github/workflows/bindings-tests.yml Outdated Show resolved Hide resolved

.github/workflows/bindings-tests.yml Outdated Show resolved Hide resolved

.github/workflows/core-tests.yml Outdated Show resolved Hide resolved

.github/workflows/core-tests.yml Outdated Show resolved Hide resolved

Vdaleke reviewed Jan 11, 2025

View reviewed changes

.github/workflows/bindings-tests.yml Outdated Show resolved Hide resolved

Vdaleke reviewed Jan 11, 2025

View reviewed changes

src/python_bindings/od/bind_od.cpp Outdated Show resolved Hide resolved

Vdaleke reviewed Jan 11, 2025

View reviewed changes

src/core/algorithms/od/order/order_utility.cpp Outdated Show resolved Hide resolved

Vdaleke reviewed Jan 11, 2025

View reviewed changes

.github/composite-actions/install-dependencies/action.yml Outdated Show resolved Hide resolved

p-senichenkov added 2 commits January 12, 2025 15:28

Add Clang tests to CI

6b174c5

Add tests for Clang on Linux, LLVM Clang on macOS and Apple Clang on macOS. Also, bump boost version used in CI, as 1.8.10 produces an error on macOS with LLVM Clang. Here it's said that boost 1.85.0 works fine: bambulab/BambuStudio#3957

p-senichenkov force-pushed the apple-clang-buildability branch from c76af53 to 3096622 Compare January 12, 2025 12:28

Vdaleke approved these changes Jan 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to build with Clang #507

Add ability to build with Clang #507

p-senichenkov commented Dec 23, 2024 •

edited

Loading

Vdaleke left a comment •

edited

Loading

Vdaleke commented Jan 4, 2025 •

edited

Loading

p-senichenkov commented Jan 4, 2025 •

edited

Loading

Vdaleke commented Jan 4, 2025

p-senichenkov commented Jan 4, 2025

Vdaleke commented Jan 4, 2025

Vdaleke left a comment •

edited

Loading

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 6, 2025 •

edited

Loading

Vdaleke commented Jan 6, 2025 •

edited

Loading

p-senichenkov commented Jan 6, 2025

github-actions bot left a comment

Vdaleke commented Jan 6, 2025

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

Vdaleke left a comment •

edited

Loading

Vdaleke left a comment

Vdaleke left a comment

Vdaleke commented Jan 12, 2025 •

edited

Loading

p-senichenkov commented Jan 12, 2025

Vdaleke commented Jan 12, 2025

p-senichenkov commented Jan 12, 2025

Vdaleke commented Jan 12, 2025

p-senichenkov commented Jan 12, 2025

Vdaleke commented Jan 12, 2025 •

edited

Loading

Add ability to build with Clang #507

Are you sure you want to change the base?

Add ability to build with Clang #507

Conversation

p-senichenkov commented Dec 23, 2024 • edited Loading

Vdaleke left a comment • edited Loading

Choose a reason for hiding this comment

Vdaleke commented Jan 4, 2025 • edited Loading

p-senichenkov commented Jan 4, 2025 • edited Loading

Vdaleke commented Jan 4, 2025

p-senichenkov commented Jan 4, 2025

Vdaleke commented Jan 4, 2025

Vdaleke left a comment • edited Loading

Choose a reason for hiding this comment

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 5, 2025

Vdaleke commented Jan 5, 2025

p-senichenkov commented Jan 6, 2025 • edited Loading

Vdaleke commented Jan 6, 2025 • edited Loading

p-senichenkov commented Jan 6, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

Vdaleke commented Jan 6, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

Vdaleke left a comment • edited Loading

Choose a reason for hiding this comment

Vdaleke left a comment

Choose a reason for hiding this comment

Vdaleke left a comment

Choose a reason for hiding this comment

Vdaleke commented Jan 12, 2025 • edited Loading

p-senichenkov commented Jan 12, 2025

Vdaleke commented Jan 12, 2025

p-senichenkov commented Jan 12, 2025

Vdaleke commented Jan 12, 2025

p-senichenkov commented Jan 12, 2025

Vdaleke commented Jan 12, 2025 • edited Loading

p-senichenkov commented Dec 23, 2024 •

edited

Loading

Vdaleke left a comment •

edited

Loading

Vdaleke commented Jan 4, 2025 •

edited

Loading

p-senichenkov commented Jan 4, 2025 •

edited

Loading

Vdaleke left a comment •

edited

Loading

p-senichenkov commented Jan 6, 2025 •

edited

Loading

Vdaleke commented Jan 6, 2025 •

edited

Loading

Vdaleke left a comment •

edited

Loading

Vdaleke commented Jan 12, 2025 •

edited

Loading

Vdaleke commented Jan 12, 2025 •

edited

Loading