libc++ 13 compatibility by Algunenano · Pull Request #32484 · ClickHouse/ClickHouse

Algunenano · 2021-12-09T19:50:48Z

Changelog category (leave one):

Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
PENDING (Should mention submodule updates and versions)

Detailed description / Documentation draft:

Still missing (to be done in another PR):

Update libcxx and libcxxabi submodule.

References #32018

Algunenano · 2021-12-10T15:29:03Z

Temporarily switching the boost submodule to my fork to test if upgrading everything to boost 1.78 is the best way to go. That way I don't need to keep pushing PRs that might or might not work when they reach the CI

ClickHouse#32018 (comment)

Algunenano · 2021-12-15T16:07:46Z

Seems like the pending failures are either found in master too (test_s3_zero_copy_replication) or issues when downloading docker images (#32772) so I believe it should be ready for review. I'll create PRs to the remaining submodules first.

Algunenano · 2021-12-16T09:52:29Z

PRs with the submodule changes:

NuRaft: Support Boost 1.78 NuRaft#34
Boost: Use boost 1.78 as base boost#28
Arrow: I can't create a new branch in a PR. @alexey-milovidov Could you please push the release-6.0.1 branch from upstream (https://github.com/apache/arrow) into ClickHouse-Extras (https://github.com/ClickHouse-Extras/arrow) and add a commit removing the submodules? They aren't necessary and that way we keep things flat. Thanks a lot!

AFAICS, with that plus the changes here everything should be ready to use libc++13.

Algunenano · 2021-12-20T17:45:28Z

Updated with master/HEAD. Changes boost to use ClickHouse-Extras after the changes were merged. Add a missing include after a recent change.

Still missing: Nuraft PR and Arrow 6.0.1 branch in ClickHouse-Extras.

Algunenano · 2021-12-22T09:43:30Z

Removing the Draft tag since all this need now is to accept the changes to submodules and review the changes in this PR

alexey-milovidov · 2021-12-23T05:16:47Z

Ok.

milovidov@milovidov-desktop:~/work/ClickHouse/contrib/arrow$ git fetch [email protected]:apache/arrow.git release-6.0.1
milovidov@milovidov-desktop:~/work/ClickHouse/contrib/arrow$ git checkout -b release-6.0.1 FETCH_HEAD
milovidov@milovidov-desktop:~/work/ClickHouse/contrib/arrow$ git remote add origin-ssh [email protected]:ClickHouse-Extras/arrow.git
milovidov@milovidov-desktop:~/work/ClickHouse/contrib/arrow$ git push origin-ssh

alexey-milovidov · 2021-12-23T05:18:06Z

contrib/arrow-cmake/CMakeLists.txt

@@ -1,5 +1,22 @@
 set (CMAKE_CXX_STANDARD 17)

+set(ARROW_VERSION "6.0.1")


We completely don't need it as we don't install Arrow .so.

It is needed, or something equivalent, to generate the config.h with proper values. Otherwise it will fail to compile with errors like:

/mnt/ch/ClickHouse/contrib/arrow/cpp/src/arrow/config.cc:33:5: error: indirection requires pointer operand ('int' invalid) ARROW_VERSION,

alexey-milovidov · 2021-12-23T05:19:37Z

src/Core/Field.h

 {
+    assert(which != Types::String); // See specialization for char
    using ValueType = std::decay_t<T>;
    ValueType * MAY_ALIAS ptr = reinterpret_cast<ValueType *>(&storage);


Rewrite with unalignedLoad?

unalignedLoad returns a new object and then the compiler might decide it doesn't need to create it, and reinterpret always returns a reference to an object built on top of the existing memory. I'm not sure if they are equivalent.

alexey-milovidov · 2021-12-23T05:20:00Z

src/Core/Field.h

+template <>
+inline char & Field::reinterpret<char>()
+{
+    using ValueType = std::decay_t<char>;


Looks weird.

alexey-milovidov · 2021-12-23T05:20:18Z

src/Core/Field.h

+    {
+        // For String we want to return a pointer to the data, not the start of the class
+        // as the layout of std::string depends on the STD version and options
+        ValueType * MAY_ALIAS ptr = reinterpret_cast<ValueType *>(reinterpret_cast<String *>(&storage)->data());


This is unneeded, as aliasing of char is permitted by C++ standard.

alexey-milovidov · 2021-12-23T05:20:36Z

src/Core/Field.h

+// Specialize reinterpreting to char (used in ColumnUnique) to make sure Strings are reinterpreted correctly
+// inline to avoid multiple definitions
+template <>
+inline char & Field::reinterpret<char>()


Please simplify the code of this function.

alexey-milovidov

Mostly LGTM.

robot-clickhouse added the pr-build Pull request with build/testing/packaging improvement label Dec 9, 2021

robot-ch-test-poll4 added the submodule changed At least one submodule changed in this PR. label Dec 9, 2021

Algunenano force-pushed the libcxx13_take2 branch from c7b4711 to 12fd3b6 Compare December 10, 2021 15:28

Algunenano force-pushed the libcxx13_take2 branch 2 times, most recently from 6f187d4 to 6457baf Compare December 10, 2021 17:28

Algunenano added 14 commits December 13, 2021 16:49

Adapt to arrow 6

d9e4544

Fix arrow build

61d959d

Adapt parquet reference

1a0e0db

Fix String reinterpret

cc6f7a7

ClickHouse#32018 (comment)

Update contrib/s2geometry-cmake/CMakeLists.txt

1c26033

Abseil: Use upstream lts_2021_11_02 branch

fe9a2c7

s2 cleanup

229b865

arrow: Point to upstream instead of fork

352c7b1

Update submodules for libc++13 compatibility

f959380

Arrow missing deps

9bd50eb

Shut up abseil

f249a35

Prepare for boost 1.78

13dd539

TEMPORAL: Try updating everything to boost 1.78

04471d7

TEMPORAL: patch NuRaft SSL mock to support boost 1.78.0

e659ef6

Algunenano mentioned this pull request Dec 13, 2021

Silence MSAN warning when running protoc ClickHouse/protobuf#4

Merged

Update boost asio with msan disabling commit

af4beb4

Algunenano force-pushed the libcxx13_take2 branch from 6457baf to af4beb4 Compare December 13, 2021 16:52

Algunenano added 3 commits December 13, 2021 18:54

Update protobuf to ignore protoc MSAN errors

8208c3d

Merge remote-tracking branch 'blessed/master' into libcxx13_take2

44f3b1c

Cherry-pick other previous changes to boost libs

b0638b0

Algunenano mentioned this pull request Dec 15, 2021

Incorrect status of ClickHouse Build Check #32799

Closed

Algunenano added 3 commits December 15, 2021 10:55

Merge remote-tracking branch 'blessed/master' into libcxx13_take2

cd465ca

Cherry-pick old fix for m1

f600b9f

Merge remote-tracking branch 'blessed/master' into libcxx13_take2

3de002c

This was referenced Dec 16, 2021

Support Boost 1.78 ClickHouse/NuRaft#34

Merged

Use boost 1.78 as base ClickHouse/boost#28

Merged

Algunenano added 3 commits December 20, 2021 17:47

Merge remote-tracking branch 'blessed/master' into libcxx13_take2

b553e51

Use ClickHouse-Extras for boost·

c42f03b

Missing include

36164a7

Algunenano changed the title ~~Draft: libc++ 13 compatibility~~ libc++ 13 compatibility Dec 22, 2021

Algunenano marked this pull request as ready for review December 22, 2021 09:43

alexey-milovidov self-assigned this Dec 23, 2021

alexey-milovidov reviewed Dec 23, 2021

View reviewed changes

Algunenano and others added 5 commits December 23, 2021 09:16

Merge remote-tracking branch 'blessed/master' into libcxx13_take2

88b8fd8

Use ClickHouse-Extras for the submodules

e5a0fba

Improvements based on PR review

b94dbb6

Merge remote-tracking branch 'blessed/master' into libcxx13_take2

77db850

Update Field.h

dea2543

alexey-milovidov merged commit c583ea7 into ClickHouse:master Dec 25, 2021

		@@ -1,5 +1,22 @@
		set (CMAKE_CXX_STANDARD 17)

		set(ARROW_VERSION "6.0.1")

Conversation

Algunenano commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Algunenano commented Dec 10, 2021

Uh oh!

Algunenano commented Dec 15, 2021

Uh oh!

Algunenano commented Dec 16, 2021

Uh oh!

Algunenano commented Dec 20, 2021

Uh oh!

Algunenano commented Dec 22, 2021

Uh oh!

alexey-milovidov commented Dec 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-milovidov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Algunenano commented Dec 9, 2021 •

edited

Loading