Copilot commented on code in PR #3713:
URL: https://github.com/apache/brpc/pull/3713#discussion_r4480384583
##########
src/butil/find_cstr.h:
##########
@@ -103,58 +103,58 @@ struct StringMapThreadLocalTemp {
}
};
-extern thread_local StringMapThreadLocalTemp tls_stringmap_temp;
+StringMapThreadLocalTemp& tls_stringmap_temp();
template <typename T, typename C, typename A>
typename std::map<std::string, T, C, A>::const_iterator
find_cstr(const std::map<std::string, T, C, A>& m, const char* key) {
- return m.find(*tls_stringmap_temp.get_string(key));
+ return m.find(*tls_stringmap_temp().get_string(key));
Review Comment:
This changes `tls_stringmap_temp` from an `extern thread_local` variable to
a function accessor, which is a source-breaking API change for any external
code that directly referenced the variable. If this header is part of the
public surface, consider keeping a compatibility layer (e.g., retain the
variable name in some form, or provide a transitional alias) or explicitly
documenting the breaking change (including in release notes / PR description).
##########
src/butil/find_cstr.h:
##########
@@ -103,58 +103,58 @@ struct StringMapThreadLocalTemp {
}
};
Review Comment:
This changes `tls_stringmap_temp` from an `extern thread_local` variable to
a function accessor, which is a source-breaking API change for any external
code that directly referenced the variable. If this header is part of the
public surface, consider keeping a compatibility layer (e.g., retain the
variable name in some form, or provide a transitional alias) or explicitly
documenting the breaking change (including in release notes / PR description).
##########
docs/cn/thread_local.md:
##########
@@ -62,3 +62,29 @@ Use *p ... - still the errno of original
pthread, undefined b
把`__const__`定义为`__unused__`对程序其他部分的影响几乎为0。另外如果你没有**直接**使用errno（即你的项目中没有出现errno），或使用的是gcc
3.4，即使没有定义`-D__const__=__unused__`，程序的正确性也不会受影响，但为了防止未来可能的问题，我们强烈建议加上。
需要说明的是，和errno类似，pthread_self也有类似的问题，不过一般pthread_self除了打日志没有其他用途，影响面较小，在`-D__const__=__unused__`后pthread_self也会正常。
+
+# 迁移安全的 pthread-local 访问
+
+仓库内部使用 `butil/thread_local.h` 的 `BUTIL_THREAD_LOCAL`，将 TLS
存储封装在不可内联的访问函数中。每次调用都重新取得当前 pthread 的 TLS 地址，并通过编译器屏障防止 LTO
将返回地址视为不变值。即使源码没有显式保存 TLS 指针，直接使用 `__thread` 或 `thread_local`
也可能被编译器优化成跨挂起点复用旧地址，见 [issue #3520](https://github.com/apache/brpc/issues/3520)。
Review Comment:
The PR description leaves 'Breaking backward compatibility' empty, but the
code changes include public-header surface changes (e.g., switching some TLS
entities from variables/macros to accessor functions). Either update the PR
description to explicitly call out the breaking/source-incompatible changes, or
add compatibility shims where feasible.
##########
src/butil/thread_local.h:
##########
@@ -29,6 +30,34 @@
#define BAIDU_THREAD_LOCAL __thread
#endif // _MSC_VER
+// Define a pthread-local variable accessed through name():
+// BUTIL_THREAD_LOCAL(int, counter, 0);
+// ++counter();
+// counter() = 10;
+// Prefix with static for internal linkage or a static class member, or inline
+// for a definition in a header. A declaration in another translation unit is
+// simply `type& name();`. Use a type alias for arrays or types containing
commas.
+//
+// A bthread can resume on a different pthread. Keep TLS address resolution in
+// a non-inlined function so the compiler cannot hoist it across a suspension.
+// The opaque asm output also prevents interprocedural optimization (including
+// LTO) from treating the returned address as invariant. A memory barrier
alone,
+// or an inline function taking &tls as its argument, does not achieve this.
+//
+// The returned reference/pointer is only valid for the current pthread: never
+// retain it across a call that may suspend. In particular, evaluate a possibly
+// suspending RHS before assigning it to name(). Initializers must not suspend.
+// Constant-initialized trivial types have no dynamic initialization/exit hook.
+#define BUTIL_THREAD_LOCAL(type, name, ...) \
+ __attribute__((noinline, unused)) type& name() { \
+ static thread_local type tls_value = __VA_ARGS__; \
+ type* tls_ptr = std::addressof(tls_value); \
+ asm volatile("" : "+r"(tls_ptr) : : "memory"); \
+ return *tls_ptr; \
+ }
Review Comment:
`BUTIL_THREAD_LOCAL` uses GCC/Clang-only syntax (`__attribute__` and GNU
inline asm). This will not compile on MSVC (and may fail under some non-GNU
toolchains), which is a hard build break when consumers include this header.
Consider adding an `_MSC_VER` implementation (e.g., using
`__declspec(noinline)` plus a suitable compiler barrier) and guarding the GNU
asm version behind `#if defined(__GNUC__) || defined(__clang__)`.
##########
src/butil/find_cstr.cpp:
##########
@@ -21,6 +21,6 @@
namespace butil {
-thread_local StringMapThreadLocalTemp tls_stringmap_temp = { false, {} };
+BUTIL_THREAD_LOCAL(StringMapThreadLocalTemp, tls_stringmap_temp, { false, {}
});
Review Comment:
`BUTIL_THREAD_LOCAL` is used here but this diff doesn’t show an include of
`butil/thread_local.h`. Even if it currently compiles via transitive includes,
it’s brittle. Prefer including `butil/thread_local.h` directly in this
translation unit (or in the defining header, if that’s the intended pattern) to
make the dependency explicit.
##########
src/butil/thread_local.h:
##########
@@ -29,6 +30,34 @@
#define BAIDU_THREAD_LOCAL __thread
#endif // _MSC_VER
+// Define a pthread-local variable accessed through name():
+// BUTIL_THREAD_LOCAL(int, counter, 0);
+// ++counter();
+// counter() = 10;
+// Prefix with static for internal linkage or a static class member, or inline
+// for a definition in a header. A declaration in another translation unit is
+// simply `type& name();`. Use a type alias for arrays or types containing
commas.
+//
+// A bthread can resume on a different pthread. Keep TLS address resolution in
+// a non-inlined function so the compiler cannot hoist it across a suspension.
+// The opaque asm output also prevents interprocedural optimization (including
+// LTO) from treating the returned address as invariant. A memory barrier
alone,
+// or an inline function taking &tls as its argument, does not achieve this.
+//
+// The returned reference/pointer is only valid for the current pthread: never
+// retain it across a call that may suspend. In particular, evaluate a possibly
+// suspending RHS before assigning it to name(). Initializers must not suspend.
+// Constant-initialized trivial types have no dynamic initialization/exit hook.
+#define BUTIL_THREAD_LOCAL(type, name, ...) \
+ __attribute__((noinline, unused)) type& name() { \
+ static thread_local type tls_value = __VA_ARGS__; \
+ type* tls_ptr = std::addressof(tls_value); \
+ asm volatile("" : "+r"(tls_ptr) : : "memory"); \
+ return *tls_ptr; \
+ }
Review Comment:
As written, the macro generates a non-`inline` free function unless callers
remember to prefix it with `inline` (for header definitions) or put it in an
anonymous namespace / mark it `static`. That’s easy to misuse and can lead to
ODR/linker failures. Consider baking `inline` into the macro’s function
signature (it won’t force inlining, and it can coexist with `noinline`) so
header usage is safe by default.
--
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]
