Copilot commented on code in PR #3698:
URL: https://github.com/apache/brpc/pull/3698#discussion_r4438432241
##########
CMakeLists.txt:
##########
@@ -150,6 +150,15 @@ if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
endif()
endif()
+# Cacheline-aligned types such as ResourcePool<Socket> are created with plain
+# `new'. C++17 honors their alignment by default, C++14 only with
+# -faligned-new; without it glibc returns 16-byte aligned storage.
+include(CheckCXXCompilerFlag)
+check_cxx_compiler_flag(-faligned-new BRPC_HAS_ALIGNED_NEW)
Review Comment:
This probe only compiles and links an empty `main`, so it never references
the aligned allocation overload. A compiler can accept `-faligned-new` while
the selected C++14 standard library lacks `operator new(std::size_t,
std::align_val_t)`; this sets `BRPC_HAS_ALIGNED_NEW` and propagates the flag,
then the first over-aligned `new` fails at link time. Probe by compiling and
linking a small `alignas(64)` type that is allocated and deleted before
enabling the option.
##########
config_brpc.sh:
##########
@@ -396,6 +396,14 @@ else
print_success "Found protobuf version $PROTOBUF_VERSION"
fi
+# Cacheline-aligned types such as ResourcePool<Socket> are created with plain
+# `new'. C++17 honors their alignment by default, C++14 only with
+# -faligned-new; without it glibc returns 16-byte aligned storage.
+if echo 'int main() { return 0; }' | \
+ $CXX $CXXFLAGS -faligned-new -x c++ - -o /dev/null >/dev/null 2>&1;
then
Review Comment:
The shell probe has the same gap as the CMake check: an empty `main` proves
only that the compiler accepts the option, not that the linked C++ runtime
supplies the aligned `operator new` used by C++14 over-aligned allocations.
With an older libstdc++ this can append `-faligned-new` to `config.mk` and turn
the eventual brpc link into a failure. Make the probe allocate and delete an
`alignas(64)` object so the runtime is checked as well.
##########
CMakeLists.txt:
##########
@@ -150,6 +150,15 @@ if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
endif()
endif()
+# Cacheline-aligned types such as ResourcePool<Socket> are created with plain
+# `new'. C++17 honors their alignment by default, C++14 only with
+# -faligned-new; without it glibc returns 16-byte aligned storage.
+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>)
Review Comment:
This option is applied only to in-tree CMake targets. Installed consumers do
not get an exported target, and `cmake/brpc.pc.in` currently emits only
`-I${includedir}` in `Cflags`; therefore a C++14 consumer that instantiates the
public `butil::ResourcePool` or `ObjectPool` headers emits their inline `new
ResourcePool`/`new ObjectPool` without `-faligned-new` and can still get
16-byte storage. Propagate the requirement through the installed
pkg-config/export metadata, or move these allocations behind compiled library
code.
##########
CMakeLists.txt:
##########
@@ -150,6 +150,15 @@ if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU")
endif()
endif()
+# Cacheline-aligned types such as ResourcePool<Socket> are created with plain
+# `new'. C++17 honors their alignment by default, C++14 only with
+# -faligned-new; without it glibc returns 16-byte aligned storage.
+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>)
Review Comment:
When this probe is false, the build continues without `-faligned-new`, so
C++14 production targets still compile `new ResourcePool`/`new ObjectPool`/`new
SocketPool` with the under-aligned allocation that this PR is meant to prevent.
The new unit test only exposes this when tests are run; a normal library build
remains broken. Since there is no fallback allocator, reject this configuration
at configure time (and report the required compiler/runtime) or provide an
explicit aligned allocation path instead of silently omitting the flag.
##########
config_brpc.sh:
##########
@@ -396,6 +396,14 @@ else
print_success "Found protobuf version $PROTOBUF_VERSION"
fi
+# Cacheline-aligned types such as ResourcePool<Socket> are created with plain
+# `new'. C++17 honors their alignment by default, C++14 only with
+# -faligned-new; without it glibc returns 16-byte aligned storage.
+if echo 'int main() { return 0; }' | \
+ $CXX $CXXFLAGS -faligned-new -x c++ - -o /dev/null >/dev/null 2>&1;
then
+ CXXFLAGS="${CXXFLAGS} -faligned-new"
+fi
Review Comment:
If the compiler probe fails, this script still generates a C++14 `config.mk`
and lets the production build proceed with the original under-aligned `new`
behavior. That means the crash remains possible whenever the unit tests are not
built. Please make configuration fail with a clear aligned-new/compiler-runtime
requirement (or implement an aligned allocator) rather than treating an
unsupported probe as a valid build.
##########
test/resource_pool_unittest.cpp:
##########
@@ -456,4 +456,34 @@ TEST_F(ResourcePoolTest, verify_get) {
clear_resources<int>();
}
+
+struct BAIDU_CACHELINE_ALIGNMENT CachelineObject {
+ int value;
+};
+
+TEST_F(ResourcePoolTest, plain_new_honors_cacheline_alignment) {
+ // Pools, their per-thread LocalPool and brpc's SocketPool are
+ // cacheline-aligned and created with plain `new'. C++14 builds need
+ // -faligned-new for that; glibc malloc then returns 16-byte aligned
+ // storage. tcmalloc, linked into this test, may hide the problem, so
+ // also require the compiler to use aligned new.
+#if !defined(__cpp_aligned_new)
+ FAIL() << "Build with C++17 or -faligned-new";
Review Comment:
When the compiler does not support `-faligned-new`, the CMake probe leaves
this translation unit in C++14 mode without `__cpp_aligned_new`, so every
unit-test run fails here. The surrounding CMake logic still accepts such C++14
toolchains and the production build is not made safe for them either; please
either make configuration fail with an actionable unsupported-toolchain error
or conditionally skip this test while providing a real alignment fallback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
