Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical active findings show that unsupported or incompletely verified aligned allocation can leave production and installed C++14 consumers under-aligned or fail at link time, while the configuration and test paths do not reject or safely handle those toolchains.
6 open issues
What changed in this PR
This pull request addresses cacheline misalignment of pooled objects in C++14 builds by enabling aligned allocation support and adding regression coverage.
Changes:
- Detect and enable
-faligned-newin CMake andconfig_brpc.sh. - Add a test verifying that plain
newhonors 64-byte alignment. - Preserve existing C++17 and Bazel behavior while requiring compatible C++14 runtimes.
The current configuration can still silently permit unsafe C++14 builds, fail to validate aligned allocation at link time, and omit the requirement from installed consumer metadata. The regression test also fails on unsupported toolchains without providing a safe fallback.
This pull request requires changes.
| File | Summary |
|---|---|
test/resource_pool_unittest.cpp |
Adds regression coverage for cacheline-aligned allocations with plain new; unsupported C++14 toolchains require an actionable configuration failure or alignment fallback. |
config_brpc.sh |
Detects and enables -faligned-new; the probe should validate the linked runtime, and configuration must not proceed unsafely when support is unavailable. |
CMakeLists.txt |
Detects and propagates aligned allocation support; the probe should allocate an over-aligned object, unsupported configurations should not proceed silently, and installed consumer metadata must propagate the requirement. |
Review effort: Lite
| include(CheckCXXCompilerFlag) | ||
| check_cxx_compiler_flag(-faligned-new BRPC_HAS_ALIGNED_NEW) | ||
| if(BRPC_HAS_ALIGNED_NEW) | ||
| list(APPEND BRPC_COMMON_COMPILE_OPTIONS $<$<COMPILE_LANGUAGE:CXX>:-faligned-new>) |
There was a problem hiding this comment.
brpc.pc now emits -faligned-new in Cflags for C++14 builds. bRPC does not install exported CMake targets, so pkg-config is the only consumer metadata. Consumers that pass include paths by hand and build as C++14 still need to add the flag, or build with C++17.
| #if !defined(__cpp_aligned_new) | ||
| FAIL() << "Build with C++17 or -faligned-new"; |
There was a problem hiding this comment.
Configuration now stops first on such toolchains, so this test only runs where aligned new is expected to be in effect, and it fails if it isn't.
ResourcePool, ObjectPool, their LocalPool and SocketPool are declared with BAIDU_CACHELINE_ALIGNMENT and created with plain new. C++17 uses aligned operator new for them, but C++14 builds (the CMake and Make default with protobuf < 22) do not, and glibc returns only 16-byte aligned storage. With Clang 14 this corrupts sockets created during a client connect and crashes in Socket::ResetFileDescriptor. For C++14 builds, CMake and config_brpc.sh now link an over-aligned allocation with -faligned-new, so both the compiler and the C++ runtime are checked, and fail configuration when that does not work. The flag is added to the build, to brpc.pc for pkg-config consumers and, as before, to config.mk. C++17 builds are unchanged. Add a test that fails when aligned new is not in effect.
59702d1 to
ed58548
Compare
What problem does this PR solve?
ResourcePool,ObjectPool, theirLocalPoolandSocketPoolare declared withBAIDU_CACHELINE_ALIGNMENTand created with plainnew. C++17 uses the alignedoperator newfor them. C++14 builds don't, and glibc then returns only 16-byte aligned storage. CMake andconfig_brpc.shboth pick C++14 when protobuf < 22.On Ubuntu 22.04 with Clang 14, this crashes client connects in
Socket::ResetFileDescriptor(_transportis null). It reproduces 3/3 with the FlatBuffers protocol tests in #3559. Rebuilding onlysocket.cppwith-faligned-newor-std=gnu++17fixes it, and-std=gnu++17 -fno-aligned-newbrings the crash back.What is changed and the side effects?
config_brpc.shlink a small over-alignednew/deletewith-faligned-new, which checks both the compiler and the C++ runtime. If that fails, configuration stops and names the required toolchain (GCC 7+, Clang 4+ or libc++).Cflagsin the installedbrpc.pc, and, for Make, toconfig.mk.ResourcePoolTest.plain_new_honors_cacheline_alignmentfails when aligned new is not in effect.Tests
Ubuntu 22.04, Clang 14 and GCC 11 (C++14): the probe passes, the pool tests pass, and
brpc.pccarries the flag. With a compiler that rejects-faligned-new, both CMake andconfig_brpc.shstop with the message above. #3559 with this change passes its FlatBuffers ON gate with Clang 14. macOS Apple clang 15 configures with the flag.