clickhouse-cpp icon indicating copy to clipboard operation
clickhouse-cpp copied to clipboard

Published 20 hours ago • verified publisher icon ClickHouse

Current setup with absl is dangerous

Open Dlougach opened this issue 3 years ago • 3 comments

The way this library currently uses absl can mess up a lot of things. In my opinion it should be reconsidered or at least (as a temporary workaround measure) a switch must be added to CMakeLists.txt to switch off int128 support (and any absl dependency coming with it).

Scenario 1:

  1. User decides to use link with both clickhouse-cpp and absl from master branch and in their project
  2. int128.cc is compiled twice - once as part of clickhouse build, once as part of absl build.
  3. Build likely fails because the same functions will be exported twice from different translation units

Scenario 2:

  1. User decides to use link with both clickhouse-cpp and absl from lts_* branch and in their project. Let's assume neither of those are installed in the system, but are rather plugged into the project via FetchContent/ExternalProject in CMake.
  2. lts_ branch puts all code into something like absl::lts_20210324:: inline namespace. So from linker point of view all absl symbols will be there, even though from compiler point of view they will be visible in absl::.
  3. columns/numeric.cpp will then use absl from contrib.
  4. Any user code that directly or indirectly includes columns/numeric.h will probably use the other absl.
  5. This will lead to mysterious linker failures because now declaration from columns/numeric.h and definition in columns/numeric.cpp are ABI-incompatible.

My understanding is that scenario 2 will only occur if the user attempts to actually use Int128 columns. Scenario 1 may occur if the the project uses int128 somewhere else, even if it doesn't need Int128 columns. There is a way to convert scenario 1 to scenario 2 by enabling inline namespace (and giving it unique name) inside absl/base/options.h. The disadvantage is that scenario 2 will likely produce more incomprehensible diagnostic messages from the linker.

Dlougach avatar Aug 09 '21 13:08 Dlougach

Hi @Dlougach ! Thanks for pointing this out, it is less likely that this issue will get any attention soon. So you are welcome to submit a PR or a proposal on how to fix it.

Enmk avatar Feb 02 '22 09:02 Enmk

Hello @Enmk !

Would you be kind enough to approve my MR or at least allow the CI to run in order to see if there is any issue :-)

DavidKeller avatar Apr 08 '22 12:04 DavidKeller

Sure, no problem, please sign the CLA and address the issues mentioned in the comments.

Enmk avatar Apr 13 '22 08:04 Enmk