wwbmmm commented on code in PR #3696:
URL: https://github.com/apache/brpc/pull/3696#discussion_r4500915938
##########
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
Review Comment:
[replied by brpc-oncall robot] Still the last functional point: this branch
is nested inside `#if defined(__linux__)`, so on FreeBSD ppc64 ELFv2 neither
`BTHREAD_CONTEXT_PLATFORM_linux_ppc64` nor `BTHREAD_CONTEXT_CALL_CONVENTION` is
defined here — the declarations below do not compile and no context switch
implementation is built, which contradicts the FreeBSD build/test results
quoted in the description (and makes big-endian ELFv2 reachable only through
musl on Linux). Either move the ppc64 ELFv2 selection out of the `__linux__`
block (renaming the platform macro and adjusting the `context.cpp` guards) or
drop FreeBSD from the description. Please also write `defined(_CALL_ELF) &&
_CALL_ELF == 2` since `_CALL_ELF` is undefined on ELFv1 targets, and consider
an explicit `#error` for `__powerpc64__` without ELFv2 (glibc big-endian),
which currently falls through all branches and fails later with a confusing
message.
##########
src/bthread/context.cpp:
##########
@@ -1012,3 +1012,253 @@ __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"
+" .localentry bthread_jump_fcontext, . - bthread_jump_fcontext\n"
+" # reserve space on stack\n"
+" subi %r1, %r1, 528\n"
+" std %r14, 352(%r1)\n"
+" std %r15, 360(%r1)\n"
+" std %r16, 368(%r1)\n"
+" std %r17, 376(%r1)\n"
+" std %r18, 384(%r1)\n"
+" std %r19, 392(%r1)\n"
+" std %r20, 400(%r1)\n"
+" std %r21, 408(%r1)\n"
+" std %r22, 416(%r1)\n"
+" std %r23, 424(%r1)\n"
+" std %r24, 432(%r1)\n"
+" std %r25, 440(%r1)\n"
+" std %r26, 448(%r1)\n"
+" std %r27, 456(%r1)\n"
+" std %r28, 464(%r1)\n"
+" std %r29, 472(%r1)\n"
+" std %r30, 480(%r1)\n"
+" std %r31, 488(%r1)\n"
+" # save CR\n"
+" mfcr %r0\n"
+" std %r0, 496(%r1)\n"
+" # save LR, and LR as PC\n"
+" mflr %r0\n"
+" std %r0, 504(%r1)\n"
+" std %r0, 512(%r1)\n"
+" # the fourth argument (preserve_fpu) is ignored: the compiler may keep\n"
+" # any value in the non-volatile FPRs/VRs across the call\n"
+" stfd %f14, 200(%r1)\n"
+" stfd %f15, 208(%r1)\n"
+" stfd %f16, 216(%r1)\n"
+" stfd %f17, 224(%r1)\n"
+" stfd %f18, 232(%r1)\n"
+" stfd %f19, 240(%r1)\n"
+" stfd %f20, 248(%r1)\n"
+" stfd %f21, 256(%r1)\n"
+" stfd %f22, 264(%r1)\n"
+" stfd %f23, 272(%r1)\n"
+" stfd %f24, 280(%r1)\n"
+" stfd %f25, 288(%r1)\n"
+" stfd %f26, 296(%r1)\n"
+" stfd %f27, 304(%r1)\n"
+" stfd %f28, 312(%r1)\n"
+" stfd %f29, 320(%r1)\n"
+" stfd %f30, 328(%r1)\n"
+" stfd %f31, 336(%r1)\n"
+" mffs %f0\n"
+" stfd %f0, 344(%r1)\n"
+#ifdef __ALTIVEC__
Review Comment:
[replied by brpc-oncall robot] v20-v31 are non-volatile in the ELFv2 ABI
regardless of the flags used for this translation unit, so gating their
save/restore on `__ALTIVEC__` means a libbrpc built without AltiVec (the
compiler default on big-endian ppc64) silently clobbers vector registers of
callers that do use AltiVec, corrupting any value user code keeps there across
a bthread call. Saving/restoring them unconditionally costs 12 stvx + 12 lvx
next to the 18 FPR pairs already saved, so I would prefer that; otherwise the
requirement that libbrpc is built with the same -maltivec/-mcpu settings as its
callers needs to be documented.
##########
src/bthread/context.cpp:
##########
@@ -1012,3 +1012,253 @@ __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"
+" .localentry bthread_jump_fcontext, . - bthread_jump_fcontext\n"
+" # reserve space on stack\n"
+" subi %r1, %r1, 528\n"
+" std %r14, 352(%r1)\n"
+" std %r15, 360(%r1)\n"
+" std %r16, 368(%r1)\n"
+" std %r17, 376(%r1)\n"
+" std %r18, 384(%r1)\n"
+" std %r19, 392(%r1)\n"
+" std %r20, 400(%r1)\n"
+" std %r21, 408(%r1)\n"
+" std %r22, 416(%r1)\n"
+" std %r23, 424(%r1)\n"
+" std %r24, 432(%r1)\n"
+" std %r25, 440(%r1)\n"
+" std %r26, 448(%r1)\n"
+" std %r27, 456(%r1)\n"
+" std %r28, 464(%r1)\n"
+" std %r29, 472(%r1)\n"
+" std %r30, 480(%r1)\n"
+" std %r31, 488(%r1)\n"
+" # save CR\n"
+" mfcr %r0\n"
+" std %r0, 496(%r1)\n"
+" # save LR, and LR as PC\n"
+" mflr %r0\n"
+" std %r0, 504(%r1)\n"
+" std %r0, 512(%r1)\n"
+" # the fourth argument (preserve_fpu) is ignored: the compiler may keep\n"
+" # any value in the non-volatile FPRs/VRs across the call\n"
+" stfd %f14, 200(%r1)\n"
+" stfd %f15, 208(%r1)\n"
+" stfd %f16, 216(%r1)\n"
+" stfd %f17, 224(%r1)\n"
+" stfd %f18, 232(%r1)\n"
+" stfd %f19, 240(%r1)\n"
+" stfd %f20, 248(%r1)\n"
+" stfd %f21, 256(%r1)\n"
+" stfd %f22, 264(%r1)\n"
+" stfd %f23, 272(%r1)\n"
+" stfd %f24, 280(%r1)\n"
+" stfd %f25, 288(%r1)\n"
+" stfd %f26, 296(%r1)\n"
+" stfd %f27, 304(%r1)\n"
+" stfd %f28, 312(%r1)\n"
+" stfd %f29, 320(%r1)\n"
+" stfd %f30, 328(%r1)\n"
+" stfd %f31, 336(%r1)\n"
+" mffs %f0\n"
+" stfd %f0, 344(%r1)\n"
+#ifdef __ALTIVEC__
+" mfspr %r0, 256\n"
+" std %r0, 192(%r1)\n"
+" li %r11, 0\n"
+" stvx %v20, %r1, %r11\n"
+" li %r11, 16\n"
+" stvx %v21, %r1, %r11\n"
+" li %r11, 32\n"
+" stvx %v22, %r1, %r11\n"
+" li %r11, 48\n"
+" stvx %v23, %r1, %r11\n"
+" li %r11, 64\n"
+" stvx %v24, %r1, %r11\n"
+" li %r11, 80\n"
+" stvx %v25, %r1, %r11\n"
+" li %r11, 96\n"
+" stvx %v26, %r1, %r11\n"
+" li %r11, 112\n"
+" stvx %v27, %r1, %r11\n"
+" li %r11, 128\n"
+" stvx %v28, %r1, %r11\n"
+" li %r11, 144\n"
+" stvx %v29, %r1, %r11\n"
+" li %r11, 160\n"
+" stvx %v30, %r1, %r11\n"
+" li %r11, 176\n"
+" stvx %v31, %r1, %r11\n"
+#endif
+" # store RSP (pointing to context-data) in R3\n"
+" std %r1, 0(%r3)\n"
+" # restore RSP (pointing to context-data) from R4\n"
+" mr %r1, %r4\n"
+#ifdef __ALTIVEC__
+" li %r11, 0\n"
+" lvx %v20, %r1, %r11\n"
+" li %r11, 16\n"
+" lvx %v21, %r1, %r11\n"
+" li %r11, 32\n"
+" lvx %v22, %r1, %r11\n"
+" li %r11, 48\n"
+" lvx %v23, %r1, %r11\n"
+" li %r11, 64\n"
+" lvx %v24, %r1, %r11\n"
+" li %r11, 80\n"
+" lvx %v25, %r1, %r11\n"
+" li %r11, 96\n"
+" lvx %v26, %r1, %r11\n"
+" li %r11, 112\n"
+" lvx %v27, %r1, %r11\n"
+" li %r11, 128\n"
+" lvx %v28, %r1, %r11\n"
+" li %r11, 144\n"
+" lvx %v29, %r1, %r11\n"
+" li %r11, 160\n"
+" lvx %v30, %r1, %r11\n"
+" li %r11, 176\n"
+" lvx %v31, %r1, %r11\n"
+" ld %r0, 192(%r1)\n"
+" mtspr 256, %r0\n"
+#endif
+" lfd %f0, 344(%r1)\n"
+" mtfsf 0xff, %f0\n"
+" lfd %f14, 200(%r1)\n"
+" lfd %f15, 208(%r1)\n"
+" lfd %f16, 216(%r1)\n"
+" lfd %f17, 224(%r1)\n"
+" lfd %f18, 232(%r1)\n"
+" lfd %f19, 240(%r1)\n"
+" lfd %f20, 248(%r1)\n"
+" lfd %f21, 256(%r1)\n"
+" lfd %f22, 264(%r1)\n"
+" lfd %f23, 272(%r1)\n"
+" lfd %f24, 280(%r1)\n"
+" lfd %f25, 288(%r1)\n"
+" lfd %f26, 296(%r1)\n"
+" lfd %f27, 304(%r1)\n"
+" lfd %f28, 312(%r1)\n"
+" lfd %f29, 320(%r1)\n"
+" lfd %f30, 328(%r1)\n"
+" lfd %f31, 336(%r1)\n"
+" ld %r14, 352(%r1)\n"
+" ld %r15, 360(%r1)\n"
+" ld %r16, 368(%r1)\n"
+" ld %r17, 376(%r1)\n"
+" ld %r18, 384(%r1)\n"
+" ld %r19, 392(%r1)\n"
+" ld %r20, 400(%r1)\n"
+" ld %r21, 408(%r1)\n"
+" ld %r22, 416(%r1)\n"
+" ld %r23, 424(%r1)\n"
+" ld %r24, 432(%r1)\n"
+" ld %r25, 440(%r1)\n"
+" ld %r26, 448(%r1)\n"
+" ld %r27, 456(%r1)\n"
+" ld %r28, 464(%r1)\n"
+" ld %r29, 472(%r1)\n"
+" ld %r30, 480(%r1)\n"
+" ld %r31, 488(%r1)\n"
+" # restore CR\n"
+" ld %r0, 496(%r1)\n"
+" mtcr %r0\n"
+" # restore LR\n"
+" ld %r0, 504(%r1)\n"
+" mtlr %r0\n"
+" # load PC into CTR (and R12 for the callee's TOC setup)\n"
+" ld %r12, 512(%r1)\n"
+" mtctr %r12\n"
+" # adjust stack\n"
+" addi %r1, %r1, 528\n"
+" # use third arg as return value after jump\n"
+" # and as first arg in context function\n"
+" mr %r3, %r5\n"
+" # jump to context\n"
+" bctr\n"
+".size bthread_jump_fcontext, .-bthread_jump_fcontext\n"
+".section .note.GNU-stack,\"\",%progbits\n"
Review Comment:
[replied by brpc-oncall robot] Every other asm block in this file ends with
`".previous\n"` after `.section .note.GNU-stack`, while the two new ppc64
blocks (here and the `bthread_make_fcontext` one) do not, leaving the assembler
in the `.note.GNU-stack` section for anything emitted afterwards. Please add
the trailing `.previous` to both for consistency.
--
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]
