Copilot commented on code in PR #3696:
URL: https://github.com/apache/brpc/pull/3696#discussion_r4434166899
##########
src/bthread/context.cpp:
##########
@@ -1012,3 +1012,247 @@ __asm (
" call _exit\n"
);
#endif
+
+#if defined(BTHREAD_CONTEXT_PLATFORM_linux_ppc64) &&
defined(BTHREAD_CONTEXT_COMPILER_gcc)
+__asm (
+".text\n"
+".align 2\n"
+".globl bthread_jump_fcontext\n"
+".type bthread_jump_fcontext, @function\n"
+"bthread_jump_fcontext:\n"
+" addis %r2, %r12, .TOC.-bthread_jump_fcontext@ha\n"
+" addi %r2, %r2, .TOC.-bthread_jump_fcontext@l\n"
Review Comment:
The ELFv2 TOC pointer in r2 is non-volatile, but this entry immediately
overwrites r2 with the context routine's TOC and the frame never saves or
restores the caller/target r2. Because the resume path branches directly to an
arbitrary saved PC rather than a function's global entry, the target TOC is not
re-established; after a yield, code in a bthread can access globals through the
wrong DSO's TOC. Save r2 in each context frame before this setup and restore it
when switching contexts.
##########
src/bthread/context.h:
##########
@@ -45,6 +45,9 @@
#elif __riscv
#define BTHREAD_CONTEXT_PLATFORM_linux_riscv64
#define BTHREAD_CONTEXT_CALL_CONVENTION
+ #elif defined(__powerpc64__) && _CALL_ELF == 2
+ #define BTHREAD_CONTEXT_PLATFORM_linux_ppc64
+ #define BTHREAD_CONTEXT_CALL_CONVENTION
Review Comment:
This branch is nested inside `#if defined(__linux__)`, so a FreeBSD ppc64
ELFv2 build never defines either the PPC context platform or
`BTHREAD_CONTEXT_CALL_CONVENTION`; the new assembly blocks are consequently
omitted while the declarations still reference an undefined calling-convention
macro. That contradicts the stated FreeBSD support and leaves bthread context
switching unavailable there. Make the platform selection and the corresponding
`context.cpp` guards cover the intended ELFv2 non-Linux targets (or explicitly
drop FreeBSD from the supported side effects).
##########
src/bthread/context.cpp:
##########
@@ -1012,3 +1012,247 @@ __asm (
" call _exit\n"
);
#endif
+
+#if defined(BTHREAD_CONTEXT_PLATFORM_linux_ppc64) &&
defined(BTHREAD_CONTEXT_COMPILER_gcc)
+__asm (
+".text\n"
+".align 2\n"
Review Comment:
There is no in-tree test for this new PPC64 context-switch path:
`bthread_context_unittest.cpp` is compiled only for x86_64, while the generic
context test does not check the PPC-specific nonvolatile GPR/FPR, FPSCR, or
AltiVec state that this assembly saves. Please add a PPC64-ELFv2
register-preservation test (or extend the existing architecture-specific test)
so regressions in this high-risk assembly are exercised by the repository's
test targets.
--
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]
