Jens Geyer created THRIFT-6213:
----------------------------------
Summary: C++: TConfiguration's default constants are odr-used
without a definition, so unoptimised builds of UnitTests and
TTransportFactoryConfigTest do not link
Key: THRIFT-6213
URL: https://issues.apache.org/jira/browse/THRIFT-6213
Project: Thrift
Issue Type: Bug
Components: C++ - Library
Reporter: Jens Geyer
h2. What happens
With CMake's defaults -- no build type given, so a Debug build without
optimisation, and C++11 -- two C++ test executables fail to link:
{noformat}
lib/cpp/test/TTransportFactoryConfigTest.cpp:108: undefined reference to
`apache::thrift::TConfiguration::DEFAULT_MAX_MESSAGE_SIZE'
lib/cpp/test/TWebSocketServerTest.cpp:164: undefined reference to
`apache::thrift::TConfiguration::DEFAULT_MAX_MESSAGE_SIZE'
collect2: error: ld returned 1 exit status
{noformat}
{{TWebSocketServerTest.cpp}} is built into {{UnitTests}}, so that whole
executable is lost, together with {{TServerSocketTest}} and every other suite
it carries.
h2. Why
{{lib/cpp/src/thrift/TConfiguration.h}} declares its defaults as {{static const
int}} members with in-class initialisers, and nothing defines them -- there is
no {{TConfiguration.cpp}}:
{code:cpp}
const static int DEFAULT_MAX_MESSAGE_SIZE = 100 * 1024 * 1024;
const static int DEFAULT_MAX_FRAME_SIZE = 16384000;
const static int DEFAULT_RECURSION_DEPTH = 64;
{code}
Before C++17 such a member still needs a namespace-scope definition once it is
odr-used, and binding it to a reference is an odr-use.
{{std::make_shared<TConfiguration>(TConfiguration::DEFAULT_MAX_MESSAGE_SIZE,
...)}} does exactly that, since {{make_shared}} takes its arguments by
forwarding reference. The tests do it in five places:
* {{lib/cpp/test/TTransportFactoryConfigTest.cpp}}, lines 108, 150, 207 and 272
* {{lib/cpp/test/TWebSocketServerTest.cpp}}, line 164
The definitions have been missing since THRIFT-5237 introduced
{{TConfiguration}}. The tests only started odr-using the constant with
0187cf606 (July 2026) and THRIFT-6177, neither of which has been released yet.
With optimisation the compiler folds the reference away and never asks for the
symbol, so an {{-O2}} build links. Compiling the two objects both ways:
||Object||Undefined references to DEFAULT_MAX_MESSAGE_SIZE at -O0||at -O2||
|{{TTransportFactoryConfigTest.cpp.o}}|1|0|
|{{TWebSocketServerTest.cpp.o}}|1|0|
The header is public, so an application that passes one of these constants to
{{make_shared}}, {{std::min}} or anything else taking a reference runs into the
same link error in an unoptimised build.
h2. Suggested fix
Define the three constants once in the library, e.g. in a new
{{lib/cpp/src/thrift/TConfiguration.cpp}} added to both the CMake and the
autotools source lists:
{code:cpp}
const int TConfiguration::DEFAULT_MAX_MESSAGE_SIZE;
const int TConfiguration::DEFAULT_MAX_FRAME_SIZE;
const int TConfiguration::DEFAULT_RECURSION_DEPTH;
{code}
That fixes the tests and applications alike. In the meantime, linking an object
that carries these definitions into the test executables (via
{{-DCMAKE_EXE_LINKER_FLAGS}}) is enough to get the suite running.
h2. Seen with
Current master -- built from 2f75006ca; nothing in {{lib/cpp}} or
{{build/cmake}} has changed since, up to e1176aff9 -- in the {{thrift:jammy}}
build image: Ubuntu 22.04, GCC 11.4.0, CMake 3.22.1, {{cmake -GNinja}} with no
{{CMAKE_BUILD_TYPE}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
