fix compiler optimize thread local variable access by ehds · Pull Request #2156 · apache/brpc

ehds · 2023-03-07T15:24:10Z

What problem does this PR solve?

Issue Number:#1776, #1407, #845, #1860, apache/doris#13270

Problem Summary:
一些编译器会优化 thread_local 的变量的读写，在同一个函数中间如果再次获取相同变量的值时，会直接从缓存的地址里面进行加载。但是如果在函数过程中间，线程发生切换（ bthread 切换到另一个线程执行)，那么 thread_local 变量需要重新获当前运行线程的，而不应该使用之前的旧值。

经测试，Apple M1 Apple clang version 14.0.0, 编译依然存在该问题：
task_runner 函数的部分汇编代码：

 1923 00000000000017cc <__ZN7bthread9TaskGroup11task_runnerEl>:
 1924 ; void TaskGroup::task_runner(intptr_t skip_remained) {
 1925     17cc: ff c3 03 d1   sub     sp, sp, #240
 1926     17d0: fd 7b 0e a9   stp     x29, x30, [sp, #224]
 1927     17d4: fd 83 03 91   add     x29, sp, #224
 1928     17d8: e8 03 00 aa   mov     x8, x0
 1929     17dc: 00 00 00 90   adrp    x0, 0x1000 <__ZN7bthread9TaskGroup11task_runnerEl+0x10>
 1930     17e0: 00 00 40 f9   ldr     x0, [x0]
 1931     17e4: 09 00 40 f9   ldr     x9, [x0]
 1932     17e8: 20 01 3f d6   blr     x9
 1933     17ec: e0 33 00 f9   str     x0, [sp, #96]
 1934     17f0: 00 00 00 90   adrp    x0, 0x1000 <__ZN7bthread9TaskGroup11task_runnerEl+0x24>
 1935     17f4: 00 00 40 f9   ldr     x0, [x0]
 1936     17f8: 09 00 40 f9   ldr     x9, [x0]
 1937     17fc: 20 01 3f d6   blr     x9
 1938     1800: e9 03 00 aa   mov     x9, x0
 1939     1804: e0 33 40 f9   ldr     x0, [sp, #96]
 1940     1808: e9 37 00 f9   str     x9, [sp, #104]
 1941     180c: a8 83 1f f8   stur    x8, [x29, #-8]
 1942 ;     TaskGroup* g = tls_task_group; // first load tls_task_group
 1943     1810: 08 00 40 f9   ldr     x8, [x0]
 1944     1814: a8 03 1f f8   stur    x8, [x29, #-16]
 ....
 2039     1934: 00 00 00 94   bl      0x1934 <__ZN7bthread9TaskGroup11task_runnerEl+0x168>
 2040     1938: 01 00 00 14   b       0x193c <__ZN7bthread9TaskGroup11task_runnerEl+0x170>
 2041     193c: e8 33 40 f9   ldr     x8, [sp, #96]
 2042 ;         g =  tls_task_group; // thread maybe changed, but load tls_task_group form [x8]
 2043     1940: 08 01 40 f9   ldr     x8, [x8]
 2044     1944: a8 03 1f f8   stur    x8, [x29, #-16]
 2045 ;         if (m->attr.flags & BTHREAD_LOG_START_AND_FINISH) {

1933 处首次读取了 tls_task_group，并放入了 [sp, #96] 位置，2043 行时，线程可能已经切换，但是依然从 [sp, #96]处中加载 tls_task_group ，导致读取到了上一个旧值。导致错误发生。

gcc 通过开启 fno-gcse 选项禁止该优化，clang 未能找到对应控制选项。

参考：

What is changed and the side effects?

Changed:

新增 NOT_ALLOW_OPTIMIZE_THREAD_LOCAL_ACCESS 用于控制是否允许编译器优化，如果为 true 是，对应的 thread local 变量必须通过 noinline 的函数调用方式来获取最新值，绕开编译器的优化。
Side effects:

Performance effects(性能影响):
如果开启 NOT_ALLOW_OPTIMIZE_THREAD_LOCAL_ACCESS , 会造成多一次的函数调用开销。
Breaking backward compatibility(向后兼容性):
无

Check List:

Please make sure your changes are compilable(请确保你的更改可以通过编译).
When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

wwbmmm · 2023-03-10T02:16:13Z

新增 NOT_ALLOW_OPTIMIZE_THREAD_LOCAL_ACCESS 用于控制是否允许编译器优化

能否自动根据当前编译器和平台来决定是否开启这个宏？

…arch

ehds · 2023-03-10T04:15:13Z

新增 NOT_ALLOW_OPTIMIZE_THREAD_LOCAL_ACCESS 用于控制是否允许编译器优化

能否自动根据当前编译器和平台来决定是否开启这个宏？

done, 根据issue的反馈信息，目前仅针对 Clang/AppleClang 并且是 ARM/AArch 架构平台关闭优化。

wwbmmm · 2023-03-10T05:09:49Z

    endif()
 endif()

+if(CMAKE_CXX_COMPILER_ID MATCHES "(AppleClang)|(Clang)" AND CMAKE_SYSTEM_PROCESSOR MATCHES "(aarch64)|(arm64)")


在这里判断的话只对cmake生效，我们还有make和bazel两种编译方式
是不是在头文件里进行判断，普适性更好一些

wwbmmm · 2023-03-10T11:02:45Z

LGTM

* fix compiler optimize thread local variable access * change __thread to BAIDU_THREAD_LOCAL * Add NOT_ALLOW_OPTIMIZE_THREAD_LOCAL_ACCESS according to compiler and arch * move thread local access optimization condition to thread_local.h

ehds · 2024-02-02T07:58:55Z

加上这个patch后，稳定出现第一种错误，是 socket.cpp 也有类似的问题吗？

是在哪个平台上出现呢？arm 还是 x86 ？
可以贴下 PoC 代码。

zcfh · 2024-02-02T11:28:30Z

加上这个patch后，稳定出现第一种错误，是 socket.cpp 也有类似的问题吗？

是在哪个平台上出现呢？arm 还是 x86 ？可以贴下 PoC 代码。

x86, (defined(__aarch64__) || defined(__arm64__))，去掉后不再core在前面提到的两个位置。

core在了其他地方，找了下调用栈，有地方是这么用的 static thread_local Object obj;。是用了btread的地方，只要用到 thread_local 的都需要做类似的变更吗？

我之前没用过brpc，如何能够构造一个，问题里提到到示例呢？

ehds · 2024-02-02T12:31:35Z

加上这个patch后，稳定出现第一种错误，是 socket.cpp 也有类似的问题吗？

是在哪个平台上出现呢？arm 还是 x86 ？可以贴下 PoC 代码。

x86, (defined(__aarch64__) || defined(__arm64__))，去掉后不再core在前面提到的两个位置。

core在了其他地方，找了下调用栈，有地方是这么用的 static thread_local Object obj;。是用了btread的地方，只要用到 thread_local 的都需要做类似的变更吗？

我之前没用过brpc，如何能够构造一个，问题里提到到示例呢？

如果使用 thread_local 变量时，前后两次访问该变量的中间阶段发生了 bthread 的切换（由于 start_urgent 或者 butex 等待，bthread_usleep 等操作），那么 thread_local 变量就可能不是之前的对象了，这个和 bthread 的设计相关，因为 bthread 运行周期可以被调度到不同的 pthread 上执行，所以前后两次所在的 pthread 上下文已经变化，其 thread_local 变量也就变化了。具体见：https://brpc.apache.org/docs/bthread/thread-local/#thread-local%E9%97%AE%E9%A2%98

是用了btread的地方，只要用到 thread_local 的都需要做类似的变更吗？

这个得分情况：如果想要在 bthread 中使用类似 "thread_local" 的行为，即无论 bthread 被调度到哪，都获得相同的 thread_local 值，建议使用 bthread_local https://brpc.apache.org/zh/docs/server/basics/#bthread-local

如果代码就是想要获得当前被调度运行的 pthread 线程的 thread_local 变量(例如 brpc 中的 tls_task_group ），那么就建议如果会出现上述导致 bthread 切换的行为，那么建议使用这个宏，避免编译器的优化。

zcfh · 2024-02-05T03:22:58Z

感觉还是 thread_local 读写的问题，请问下除了这个patch外，后面还有其他关于这个问题的修复吗？
目前去掉 src/bthread/task_group.cpp 的lto后，可以正常工作。

task_runner 循环外的 tls，也加上 BAIDU_GET_VOLATILE_THREAD_LOCAL，后似乎问题不再出现了。

 void TaskGroup::task_runner(intptr_t skip_remained) {
     // NOTE: tls_task_group is volatile since tasks are moved around
     //       different groups.
-    TaskGroup* g = tls_task_group;
+    TaskGroup* g = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_task_group);

ehds · 2024-02-05T05:49:33Z

感觉还是 thread_local 读写的问题，请问下除了这个patch外，后面还有其他关于这个问题的修复吗？目前去掉 src/bthread/task_group.cpp 的lto后，可以正常工作。

task_runner 循环外的 tls，也加上 BAIDU_GET_VOLATILE_THREAD_LOCAL，后似乎问题不再出现了。
 void TaskGroup::task_runner(intptr_t skip_remained) {
     // NOTE: tls_task_group is volatile since tasks are moved around
     //       different groups.
-    TaskGroup* g = tls_task_group;
+    TaskGroup* g = BAIDU_GET_VOLATILE_THREAD_LOCAL(tls_task_group);

后续还有3个改动：

根据编译器是否开启该宏
fix compiler optimize thread local variable access on x86_64 #2248
Fix unit test running error under arch64 #2324
添加漏掉的需要添加的访问 tls_task_group 的地方
add missing BAIDU_GET_VOLATILE_THREAD_LOCAL in task_group.cpp #2262

怀疑可能更跟 #2262 这个修改有关。

zcfh · 2024-02-05T06:51:53Z

另外还有一个疑问是，函数 task_runner 和 tls_task_group 不都是在一个编译单元内吗？为什么是开启lto后才会触发这个问题呢。

ehds · 2024-02-05T11:01:48Z

另外还有一个疑问是，函数 task_runner 和 tls_task_group 不都是在一个编译单元内吗？为什么是开启lto后才会触发这个问题呢。

不开启 lto 也是会有这个问题的，具体跟编译器的类别、版本以及编译参数等有关。具体的话可以通过 llvm-objdump/objdump 工具把可执行的产物 dump 出来，看下 task_runner 函数里面获取 tls_task_group 是否符合预期来进行分析。

zcfh · 2024-03-04T12:04:29Z

目前从clang11 切换到clang16 遇到了一些新的问题。和 #1776 提到的问题应该是一样的，不过我看1776对应的代码并没有合入, 是有做了其他修复吗?

对比了汇编，这样差异在于326-331 行这段代码（version 0.96）, 并且把这段代码拆到一个函数里，加上禁用优化的标注，就没问题了。

        // Clean tls variables, must be done before changing version_butex
        // otherwise another thread just joined this thread may not see side
        // effects of destructing tls variables.
        KeyTable* kt = tls_bls.keytable;
        if (kt != NULL) {
            return_keytable(m->attr.keytable_pool, kt);
            // After deletion: tls may be set during deletion.
            tls_bls.keytable = NULL;
            m->local_storage.keytable = NULL; // optional
        }

clang11 对于 tls_bls 成员变量的读写，每次都会经过一次call。而clang16会在do循环外缓存tls_bls的地址。
具体汇编差异如下
clang11

;third_party/brpc-0.9.6/src/bthread/task_group.cpp:325 
# KeyTable* kt = tls_bls.keytable;
     a9e:	66 48 8d 3d 00 00 00 	data16 lea 0x0(%rip),%rdi        # aa6 <bthread::TaskGroup::task_runner(long)+0x196>
     aa5:	00
     aa6:	66 66 48 e8 00 00 00 	data16 data16 callq aae <bthread::TaskGroup::task_runner(long)+0x19e>
     aad:	00
     aae:	48 8b 30             	mov    (%rax),%rsi
third_party/brpc-0.9.6/src/bthread/task_group.cpp:326
     ab1:	48 85 f6             	test   %rsi,%rsi
     ab4:	74 28                	je     ade <bthread::TaskGroup::task_runner(long)+0x1ce>
...
third_party/brpc-0.9.6/src/bthread/task_group.cpp:329
# tls_bls.keytable = NULL;
     abf:	66 48 8d 3d 00 00 00 	data16 lea 0x0(%rip),%rdi        # ac7 <bthread::TaskGroup::task_runner(long)+0x1b7>
     ac6:	00
     ac7:	66 66 48 e8 00 00 00 	data16 data16 callq acf <bthread::TaskGroup::task_runner(long)+0x1bf>
     ace:	00
     acf:	48 c7 00 00 00 00 00 	movq   $0x0,(%rax)

clang16

;third_party/brpc-0.9.6/src/bthread/task_group.cpp:265
     838:	48 8b 40 30          	mov    0x30(%rax),%rax
     83c:	48 85 c0             	test   %rax,%rax
     83f:	75 df                	jne    820 <bthread::TaskGroup::task_runner(long)+0x30>
     841:	4c 89 65 d0          	mov    %r12,-0x30(%rbp)
     845:	66 48 8d 3d 00 00 00 	data16 lea 0x0(%rip),%rdi        # 84d <bthread::TaskGroup::task_runner(long)+0x5d>
     84c:	00
     84d:	66 66 48 e8 00 00 00 	data16 data16 callq 855 <bthread::TaskGroup::task_runner(long)+0x65>
     854:	00
     855:	48 89 c3             	mov    %rax,%rbx  # do 循环外保存 tls_bls 的地址到rbx
...
;third_party/brpc-0.9.6/src/bthread/task_group.cpp:325
# KeyTable* kt = tls_bls.keytable;
     96e:	48 8b 33             	mov    (%rbx),%rsi
;third_party/brpc-0.9.6/src/bthread/task_group.cpp:326
     971:	48 85 f6             	test   %rsi,%rsi
     974:	74 18                	je     98e <bthread::TaskGroup::task_runner(long)+0x19e>
...
;third_party/brpc-0.9.6/src/bthread/task_group.cpp:329
# tls_bls.keytable = NULL;
     97f:	48 c7 03 00 00 00 00 	movq   $0x0,(%rbx)

zcfh · 2024-05-22T11:49:50Z

另外还有一个疑问是，函数 task_runner 和 tls_task_group 不都是在一个编译单元内吗？为什么是开启lto后才会触发这个问题呢。

这个差异找到了（定位不易，踩坑的场景比较特殊，不知道有没有人会遇到）。和clang对tls变量访问的选择有关。如果使用-fPIC编译 + 分布式 thinlto ，clang-16之前都会出现这种行为差异，具体原因。

`bthread_usleep`/`bthread_yield` contains access to TLS variables. In LTO mode, exceptions may occur due to cross-module optimization. For example, bthread_usleep is inlined into WatchConnections, the compiler(clang-17.0.6) cache the address outside the loop, triggering the error mentioned in apache#2156.

ehds added 2 commits March 7, 2023 23:03

fix compiler optimize thread local variable access

2eac300

change __thread to BAIDU_THREAD_LOCAL

20f5566

Add NOT_ALLOW_OPTIMIZE_THREAD_LOCAL_ACCESS according to compiler and …

0964a6c

…arch

wwbmmm reviewed Mar 10, 2023

View reviewed changes

move thread local access optimization condition to thread_local.h

b87dfe8

anakinxc mentioned this pull request Apr 23, 2023

bazel run //exmaples/cpp:simple_psi 出错 (Mac M1) secretflow/spu#170

Closed

wwbmmm merged commit 37c31fb into apache:master Apr 26, 2023

lengmoXXL mentioned this pull request Sep 19, 2024

arm64 架构 leader 创建的 replicator 有概率不再发送日志给 follower baidu/braft#473

Closed

chenBright mentioned this pull request Sep 22, 2024

Optimize keytablelist implementation #2768

Merged

zcfh mentioned this pull request Mar 18, 2025

Fix compiler optimize thread local variable access #2918

Merged

This was referenced Apr 2, 2025

add attribute noinline to tls_bls #2933

Closed

use BAIDU_VOLATILE_THREAD_LOCAL to declare tls_bls to avoid error compiler optimization #2934

Merged

zhoukangsheng mentioned this pull request Sep 9, 2025

fix: fix bthread_id_unlock failed in aarch64 cpu(fix issue #3083) #3084

Merged

zcfh mentioned this pull request Apr 9, 2026

求助：socket.cpp:1036, IsWriteComplete] Check failed: NULL == old_head->next #2572

Closed

Uh oh!

Conversation

ehds commented Mar 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and the side effects?

Check List:

Uh oh!

wwbmmm commented Mar 10, 2023

Uh oh!

ehds commented Mar 10, 2023

Uh oh!

wwbmmm Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

ehds Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

wwbmmm commented Mar 10, 2023

Uh oh!

ehds commented Feb 2, 2024

Uh oh!

zcfh commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehds commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zcfh commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ehds commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zcfh commented Feb 5, 2024

Uh oh!

ehds commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zcfh commented Mar 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zcfh commented May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ehds commented Mar 7, 2023 •

edited

Loading

zcfh commented Feb 2, 2024 •

edited

Loading

ehds commented Feb 2, 2024 •

edited

Loading

zcfh commented Feb 5, 2024 •

edited

Loading

ehds commented Feb 5, 2024 •

edited

Loading

ehds commented Feb 5, 2024 •

edited

Loading

zcfh commented Mar 4, 2024 •

edited

Loading

zcfh commented May 22, 2024 •

edited

Loading