Copilot commented on code in PR #3276:
URL: https://github.com/apache/brpc/pull/3276#discussion_r3116221382
##########
src/bthread/task_group.cpp:
##########
@@ -635,6 +635,12 @@ int TaskGroup::join(bthread_t tid, void** return_value) {
return errno;
}
}
+#if defined(__aarch64__) || defined(__arm__)
+ // On ARM's weak memory model, ensure all memory writes made by the
+ // joined bthread are visible to the joining thread after join returns.
+ // This matches the semantic guarantee provided by pthread_join().
+ butil::atomic_thread_fence(butil::memory_order_acquire);
+#endif
Review Comment:
The acquire fence is currently compiled only for `__aarch64__`/`__arm__`,
but the same missing-join-synchronization problem can occur on other
weak-memory architectures already recognized by the codebase (e.g. RISC-V /
LoongArch). Consider making the fence unconditional (it’s typically a no-op on
strong memory-ordering CPUs) or gating it on the repo’s architecture macros
(e.g. `ARCH_CPU_ARM_FAMILY || ARCH_CPU_RISCV_FAMILY ||
ARCH_CPU_LOONGARCH64_FAMILY`) from `butil/build_config.h` so `bthread_join()`
provides consistent visibility guarantees across supported platforms.
--
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]
