Repository navigation
Conversation
brpc did not build on 64-bit PowerPC: butil/build_config.h and butil/atomicops.h had no entry for the architecture, butil/time.h hit "unsupported arch" in clock_cycles(), and bthread had no context switch for it. - build_config.h: define ARCH_CPU_PPC64_FAMILY / ARCH_CPU_PPC64 and pick the endianness from __BYTE_ORDER__. - atomicops: use the GCC builtin implementation and give it the Atomic64 operations it was missing. - time.h: read the time base (mftb) in clock_cycles(). - processor.h: cpu_relax() lowers the SMT priority briefly, like the Linux kernel does. - debugger_posix.cc: use the trap instruction to break. - bthread/context: ELFv2 implementation of bthread_jump_fcontext and bthread_make_fcontext, derived from boost.context's jump_ppc64_sysv_elf_gas.S / make_ppc64_sysv_elf_gas.S. In addition to r14-r31, CR and LR it preserves f14-f31, FPSCR and, with AltiVec, v20-v31 and VRSAVE: those registers are non-volatile in the ELFv2 ABI and the compiler keeps values in them across the call. A new context starts with the current CR, FPSCR and VRSAVE instead of whatever was on the stack. - mongo_head.h: ARCH_CPU_LITTLE_ENDIAN is only defined on little-endian targets, so test it with the preprocessor. Only the ELFv2 ABI is supported (ppc64le everywhere, ppc64 on FreeBSD and musl); ELFv1 targets keep failing as before. Tested on FreeBSD 15.1 powerpc64le (POWER9, clang 19): the library, tools and the bthread unit tests build; 18 of the 20 bthread_*unittest binaries pass (butex, cond, countdown_event, execution_queue, futex, id, key, list, once, ping_pong, rwlock, sched_yield, semaphore, timer_thread, work_stealing_queue, bthread_unittest, ...). bthread_mutex_unittest passes test by test but times out when the whole binary runs MutexTest.mix_thread_types after the performance tests, and bthread_setconcurrency_unittest fails two worker-count assertions (min_concurrency, concurrency_by_tag); neither involves the context switch. bthread_dispatcher_unittest and bthread_fd_unittest do not compile on non-Linux hosts (undeclared n / epfd / wait_rc). The library also builds on big-endian FreeBSD powerpc64 (ELFv2). A standalone register-pinned test of the context switch (distinct patterns in f14-f31 and v20-v31 on each side, different FPSCR rounding modes per context, 300000 round trips) shows no corruption on either endianness. Closes apache#2697 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Approval is blocked by an unresolved critical PPC64 TOC restoration bug, an additional critical FreeBSD guard issue, and missing architecture-specific regression coverage.
3 open issues
What changed in this PR
Adds initial PPC64 ELFv2 support across bthread and butil, including architecture detection, atomic operations, timing, processor relaxation, debugger traps, endian handling, and context switching.
Changes:
- Adds PPC64 architecture and endianness support plus GCC atomic operations.
- Implements PPC64-specific timing,
cpu_relax(), debugger traps, and MongoDB byte-order handling. - Adds ELFv2 context switching with nonvolatile integer, floating-point, condition-register, FPSCR, and optional AltiVec state.
Review findings remain unresolved:
- Critical (3 votes):
src/bthread/context.cppdoes not preserve and restore the ELFv2 TOC pointer inr2. - Critical (4 votes):
src/bthread/context.hlimits PPC context support to Linux, excluding the stated FreeBSD ELFv2 target. - Moderate (2 votes): No in-tree PPC64-specific register-preservation test covers the new context-switch implementation.
| File | Description |
|---|---|
src/butil/time.h |
Reads the PPC64 time base for cycle timing. |
src/butil/processor.h |
Adds the PPC64 cpu_relax() implementation. |
src/butil/debug/debugger_posix.cc |
Uses the PPC64 trap instruction for debugger breaks. |
src/butil/build_config.h |
Defines PPC64 architecture, bitness, and endianness macros. |
src/butil/atomicops.h |
Selects GCC atomic operations for PPC64. |
src/butil/atomicops_internals_gcc.h |
Adds 64-bit atomic implementations for GCC-based PPC64 builds. |
src/bthread/context.h |
Adds ELFv2 PPC64 context platform selection; its non-Linux guards require correction for FreeBSD support. |
src/bthread/context.cpp |
Implements PPC64 ELFv2 context creation and switching; TOC pointer preservation requires correction. |
src/brpc/mongo_head.h |
Makes MongoDB byte swapping compile-time endian-aware. |
Review effort: Lite
bthread_context_unittest.cpp only has a body for x86_64. Add a ppc64 ELFv2 section that checks what the context switch has to preserve: - f14-f31 and, with AltiVec, v20-v31 are loaded with one pattern in the main context and another in the test context; each side reads them back after being resumed and expects its own pattern. As in the x86_64 test, the register setup, the switch and the capture are one assembly function so the compiler cannot touch the registers in between. The output pointer is kept in r31 across the switch, which covers the GPR part of the frame. - The FPSCR rounding mode set in one context must not leak into the other, and a new context starts with the rounding mode of its creator. Tested with the unittest source built standalone against the ppc64 switch (a minimal Google Test stand-in instead of gtest) on FreeBSD 16.0-CURRENT powerpc64le (clang 21) and FreeBSD 15.1 powerpc64 big-endian (clang 19), at -O2, -O0 and with -mno-altivec, 5000 iterations each: all pass. Removing any single restore from the switch (f14, f31, v20, v31, r31, FPSCR) or the FPSCR seed in bthread_make_fcontext makes the test fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved PPC64 issues can violate acquire-CAS ordering and crash shared-library context returns, with additional callee-saved register corruption and stack-unwinding risks.
4 open issues
1 resolved since last review
Review effort: Balanced
Comment on lines
+1242
to
+1244
| "finish:\n" | ||
| " # save return address into R0\n" | ||
| " mflr %r0\n" |
Comment on lines
+107
to
+113
| Atomic64 prev_value; | ||
| do { | ||
| if (__sync_bool_compare_and_swap(ptr, old_value, new_value)) | ||
| return old_value; | ||
| prev_value = *ptr; | ||
| } while (prev_value == old_value); | ||
| return prev_value; |
| " clrrdi %r3, %r3, 4\n" | ||
| " # reserve space for context-data on context-stack\n" | ||
| " # including 64 byte of linkage + parameter area (R1 % 16 == 0)\n" | ||
| " subi %r3, %r3, 592\n" |
Comment on lines
+256
to
+260
| " ld %r31, 32(%r1)\n" | ||
| " addi %r1, %r1, 48\n" | ||
| " ld %r0, 16(%r1)\n" | ||
| " mtlr %r0\n" | ||
| " blr\n" |
bthread_make_fcontext left the back chain of the context's first frame uninitialized. bthread reuses stacks, so a stack walker that follows back chains (gdb, perf, the frame-chain samplers) could run past the context's root frame into stale data. Zero it, as boost.context does. finish, the return address of a context function, called _exit through the PLT with whatever r2 the context function left in place. When that function lives in another DSO than the library, that is the wrong TOC and the PLT stub jumps into garbage. LR holds the address of finish when it is entered, so derive the module's TOC from it first. bthread's own task runner never returns, so this only matters for other users of bthread_make_fcontext. Add a test that walks the back chains from inside a new context, on a stack filled with non-zero bytes, and expects the walk to end on a zero word without leaving the stack. Tested with the unittest source built standalone against the ppc64 switch (a minimal Google Test stand-in) on FreeBSD 15.1 powerpc64le and big-endian powerpc64 (clang 19), at -O2, -O0 and with -mno-altivec, 5000 iterations each. Without the back chain store the new test fails. For finish, the switch was built into a shared object and a context function in the executable, which uses the executable's TOC, returned into it: with this change the process exits with status 0, without it the _exit call faults. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
NoBarrier_CompareAndSwap returned a plain reload of *ptr after a failed __sync_bool_compare_and_swap, and Acquire_CompareAndSwap forwards to it. That reload is not part of the barriered atomic operation: on a weakly ordered target such as POWER a later load can be satisfied before it, so a caller could act on the returned value without seeing the stores published before it. Return __sync_val_compare_and_swap directly, for Atomic32 and Atomic64; it yields the value the atomic sequence itself observed. This header is used for ppc64 (and the retired OS_NACL configuration). Tested with a standalone check of the CAS, exchange, increment and load/store semantics (success and failure paths, both widths) on FreeBSD 15.1 powerpc64le and big-endian powerpc64 with clang 19. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jump_with_fp_state loaded f14-f31 and v20-v31 with the test pattern and returned without restoring them. They are callee-saved in ELFv2, so the C++ caller may keep values in them across the call (the x86_64 helper gets away with the same pattern because xmm6-xmm15 are volatile there). Save them in the helper's frame on entry and restore them after the switched-in values have been captured. Tested as in the previous commits: 3 tests x 5000 iterations at -O2, -O0 and -mno-altivec on FreeBSD 15.1 powerpc64le and big-endian powerpc64 (clang 19); removing any single register restore from the switch still makes the tests fail. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Number: #2697
Problem Summary: brpc does not build on 64-bit PowerPC.
butil/build_config.hand
butil/atomicops.hhave no entry for the architecture,butil/time.hfails with "unsupported arch" in
clock_cycles(), and bthread has no contextswitch for it.
What is changed and the side effects?
Changed:
build_config.h:ARCH_CPU_PPC64_FAMILY/ARCH_CPU_PPC64, endiannessfrom
__BYTE_ORDER__.atomicops: useatomicops_internals_gcc.h, which gains theAtomic64operations it was missing (the builtins already handle 64-bit values).
time.h:clock_cycles()reads the time base withmftb.processor.h:cpu_relax()lowers the SMT thread priority briefly(
or 1,1,1/or 2,2,2), as the Linux kernel'scpu_relax()does.debugger_posix.cc:trapas the break instruction.bthread/context.{h,cpp}: ELFv2 implementation ofbthread_jump_fcontext/bthread_make_fcontext, derived fromboost.context's
jump_ppc64_sysv_elf_gas.S/make_ppc64_sysv_elf_gas.S.The frame holds r14-r31, CR, LR, f14-f31, FPSCR and, when AltiVec is
enabled, v20-v31 and VRSAVE; all of these are non-volatile in the ELFv2
ABI, so a switch that dropped them would corrupt values the compiler keeps
live across the call (the patch attached to Support brpc for PPC64 by HX #2697 saves only the FPRs and
does not restore FPSCR). A new context starts with the current CR, FPSCR
and VRSAVE.
mongo_head.h:ARCH_CPU_LITTLE_ENDIANis only defined on little-endiantargets, so the byte-swap is decided by the preprocessor.
Side effects:
ELFv1 (glibc ppc64 big-endian) keeps failing as before.
context.hkeeps its existing Linux-only platform selection; the FreeBSD builds
below used the FreeBSD port's patch that adds a
__FreeBSD__block(upstream has none for any architecture).
__powerpc64__/ARCH_CPU_PPC64_FAMILY/ the new platform macro.Tested
FreeBSD 15.1 powerpc64le (POWER9, clang 19, protobuf 5.29, system
googletest 1.16):
BUILD_UNIT_TESTS=ONbuild. 18 of the 20bthread_*unittestbinaries pass completely: butex, butex_multi_tag, cond,cond_bug, countdown_event, execution_queue, futex, id, key, list, once,
ping_pong, rwlock, sched_yield, semaphore, timer_thread,
work_stealing_queue and bthread_unittest.
bthread_mutex_unittest: every test passes when run on its own(
--gtest_filter), but the whole binary hangs inMutexTest.mix_thread_typesafter the performance tests (killed after5 min).
bthread_setconcurrency_unittest:min_concurrencyandconcurrency_by_tagfail their worker-count assertions (250 expected,201 seen; "Invalid concurrency_by_tag=1"). Both look like scheduler
accounting / test-order issues rather than the context switch, which the
passing butex/cond/ping_pong/work-stealing tests exercise heavily; I could
not compare against an amd64 FreeBSD run.
bthread_dispatcher_unittestandbthread_fd_unittestdo not compile onnon-Linux hosts (undeclared
n,wait_rc,epfd), independent of thischange.
test/CMakeLists.txtcallsuse_cxx11(), whichpins the debug butil/bvar copies to C++11 while protobuf 5 / abseil need
C++14+ (the library itself gets C++17 from the top-level file); I
commented that call out for the test run. That is a separate issue.
distinct patterns into f14-f31 and v20-v31 on each side of the switch,
sets different FPSCR rounding modes per context, 300 000 round trips at
-O2 and 100 000 at -O0) shows no register or FPSCR corruption on
powerpc64le or big-endian powerpc64.
bthread_context_unittestgained a ppc64 section (FPR/VR/GPRpreservation and FPSCR rounding mode across switches); it passes on
powerpc64le and big-endian powerpc64 at -O2, -O0 and without AltiVec, and
fails when any single register restore is removed from the switch.
Written with the help of Claude Code.