velox
velox copied to clipboard
facebookincubator
Linking fbthrift with version v2024.02.26.00 unresolved symbols with gcc11 or later
Problem description
The following link errors (excerpt) occur when linking fbthrift (unresolved symbols from within the fbthrift library) when building with gcc11 and building the prestissimo presto_server executable.
/usr/bin/ld: /usr/local/lib/libthriftcpp2.a(RocketStreamServerCallback.cpp.o): in function `apache::thrift::rocket::RocketSinkServerCallback::onSinkError(folly::exception_wrapper)::{lambda(apache::thrift::detail::EncodedStreamError&)#2}::operator()(apache::thrift::detail::EncodedStreamError&) const':
RocketStreamServerCallback.cpp:(.text+0x11fa): undefined reference to `apache::thrift::CompressionConfig::CompressionConfig(apache::thrift::CompressionConfig const&)'
/usr/bin/ld: /usr/local/lib/libthriftcpp2.a(RocketServerConnection.cpp.o): in function `apache::thrift::rocket::RocketServerConnection::closeIfNeeded() [clone .localalias]':
RocketServerConnection.cpp:(.text+0x2b74): undefined reference to `apache::thrift::ServerPushMetadata::~ServerPushMetadata()'
/usr/bin/ld: RocketServerConnection.cpp:(.text+0x31a1): undefined reference to `apache::thrift::ServerPushMetadata::~ServerPushMetadata()'
/usr/bin/ld: /usr/local/lib/libthriftcpp2.a(RocketServerConnection.cpp.o): in function `apache::thrift::rocket::RocketServerConnection::handleUntrackedFrame(std::unique_ptr<folly::IOBuf, std::default_delete<folly::IOBuf> >, apache::thrift::rocket::StreamId, apache::thrift::rocket::FrameType, apache::thrift::rocket::Flags, folly::io::Cursor)::{lambda(std::unique_ptr<apache::thrift::rocket::RocketStreamClientCallback, std::default_delete<apache::thrift::rocket::RocketStreamClientCallback> > const&)#2}::operator()(std::unique_ptr<apache::thrift::rocket::RocketStreamClientCallback, std::default_delete<apache::thrift::rocket::RocketStreamClientCallback> > const&) const':
RocketServerConnection.cpp:(.text+0x468d): undefined reference to `apache::thrift::HeadersPayloadContent::HeadersPayloadContent()'
There is a long list.
The root cause is the change of the CXX_STANDARD from 17 to 20 in version v2024.02.19.00.
However, the Velox setup scripts attempt to force CXX_STANDARD 17 which for unknown reason does not override the hard coded value.
In setup-helper-functions.sh for the cmake command:
-DCMAKE_CXX_STANDARD=17 \
and for explicit flags:
-std=c++17
However, setting -DCMAKE_CXX_STANDARD=17 does not override the standard back to 17. Instead, the flags used are
FLAGS = -mavx2 -mfma -mavx -mf16c -mlzcnt -std=c++17 -mbmi2 -std=c++20 -fPIC
Hard coding the CXX standard back to 17 resolves the link issue (version v2024.02.19.00 can be built with that). GCC appears to be using the latest value for -std. The documentation doesn't specify what happens if it is specified multiple times but the behavior changes when the last occurrence is changed.
Hard-coding CXX standard to 17 in the fbthrift main CMakeLists.txt results in
FLAGS = -mavx2 -mfma -mavx -mf16c -mlzcnt -std=c++17 -mbmi2 -std=c++17 -fPIC
The reason this works for gcc9 is that -std=c++20 is ignored because the value is not supported.
The proposal is to fall back to version v2024.02.12.00 for all facebook dependencies (fbthrift, fizz, wangle, etc). This likely also requires a prestissimo proxygen update to match this version when the velox submodule is updated.
System information
n/a
CMake log
No response
@czentgr thanks for investigating this! I agree that we should downgrade the library versions until we fix fbthrift to allow custom CMAKE_CXX_STANDARD.
@czentgr thank you for the investigation and detailed explanation!
The problem with downgrading is that we need to have a consistent version of FBOS across all facebook dependencies, and we had to recently upgrade folly because some headers we use got moved to a different location. So I suppose simply pinning to an older version may not even build.
What needs to be fixed in fbthrift? Is it only that its build system need to be able to allow library users to override the C++ standard version?
Downgrading the c++ standard version could be problematic as well. I have to assume there was a reason it changed. New c++20 features would prevent future compilation. I also may not have built the piece that requires the new header location though I did do the standard build with the older version and found no issues.
Something changed with the exported symbols that are provided by the libthriftcpp2.a that it needs to resolve for the executable. Perhaps @assignUser has an idea? There shouldn't be a problem linking a c++20 library to c++17 executable.
I can have a look.
The documentation doesn't specify what happens if it is specified multiple times
AFAIK this is the expected behavior, the last flag wins. This helps in cases like this or when re2 added --std=c++11 to it's pc file. It doesn't seem to be documented explicitly for -std but is for -O If you use multiple -O options, with or without level numbers, the last such option is the one that is effective.
It seems we can just upgrade to a newer version (>= v2024.03.04.00) :grinning:
Back out "move fbthrift to use c++20" https://github.com/facebook/fbthrift/commit/9c891e4b879b2db7a7a211a92ef21217d6396909
Nice. Also now: https://github.com/facebook/fbthrift/commit/8926ea4ddc24ac25662543e4fc6e8a753df34412 Sets C++17 as standard but also allows override for future but who knows if that is helping. So must have caused more issues elsewhere.
It seems we can just upgrade to a newer version (>= v2024.03.04.00)
@pedroerp something we can open a PR for or it needs to be looked at by someone else first (for Meta internal processes?)
@pedroerp something we can open a PR for or it needs to be looked at by someone else first (for Meta internal processes?)
Please go ahead and open a PR; we can get it merged. Our monorepo internal build system always use latest from these libraries. Changing the cmake build system does not really affect it.
@assignUser Yes, I did a local build with gcc11 and the new version of the dependencies. I'm closing the issue. Note: still need to update proxygen in prestissimo.