Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: linux/arm64 sigtramp seems wrong #31827

Open
coypoop opened this issue May 3, 2019 · 6 comments

Comments

Projects
None yet
5 participants
@coypoop
Copy link
Contributor

commented May 3, 2019

As I understand it:

  • Go doesn't have callee-saved registers
  • Even if the normal C ABI does.
  • This is true on arm64 as well.
  • A Go signal handler may be called when running C code

darwin/arm64 runtime.sigtramp saves callee-saved registers (r19-r27, g aka r28, r29) for this scenario

// Save callee-save registers.
MOVD R19, (8*4)(RSP)
MOVD R20, (8*5)(RSP)
MOVD R21, (8*6)(RSP)
MOVD R22, (8*7)(RSP)
MOVD R23, (8*8)(RSP)
MOVD R24, (8*9)(RSP)
MOVD R25, (8*10)(RSP)
MOVD R26, (8*11)(RSP)
MOVD R27, (8*12)(RSP)
MOVD g, (8*13)(RSP)
MOVD R29, (8*14)(RSP)

linux/arm64 sigtramp does not do this.

TEXT runtime·sigtramp(SB),NOSPLIT,$24
// this might be called in external code context,
// where g is not set.
// first save R0, because runtime·load_g will clobber it
MOVW R0, 8(RSP)
MOVBU runtime·iscgo(SB), R0
CMP $0, R0
BEQ 2(PC)
BL runtime·load_g(SB)
MOVD R1, 16(RSP)
MOVD R2, 24(RSP)
MOVD $runtime·sigtrampgo(SB), R0
BL (R0)
RET
TEXT runtime·cgoSigtramp(SB),NOSPLIT,$0
MOVD $runtime·sigtramp(SB), R3
B (R3)

I don't have a reproducer for this bug (or a setup for it), but it seems wrong.
Tell me your thoughts.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Yes, clearly sigtramp in runtime/sys_linux_arm64.s must save all callee-saved registers.

Same for sys_openbsd_arm64.s and sys_netbsd_arm64.s.

Thanks for pointing this out.

CC @4a6f656c

@gopherbot

This comment has been minimized.

Copy link

commented May 15, 2019

Change https://golang.org/cl/177045 mentions this issue: runtime: save/restore callee-saved registers in arm64's sigtramp

@Zheng-Xu

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

I doubt there is really need to save/restore the registers in sigtramp. We shouldn't allow the C code to call Go functions directly without going through CGO. It is true that A Go signal handler may be called when running C code. But it is not called from C code directly. It is actually called from kernel. Kernel will preserve the context anyway.

I think the question should be: Why we save/restore those registers on darwin system? Is it necessary?

@4a6f656c

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@ianlancetaylor - I don't see why this would be necessary. When a signal is delivered the kernel preserves all of the register state in the trap frame, restoring all of the (possibly modified) register state on return from the signal handler. In the case of OpenBSD/arm64, see sendsig() and sys_sigreturn() in:

http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/sys/arch/arm64/arm64/sig_machdep.c?rev=1.6

This is fairly standard behaviour - NetBSD is effectively the same via cpu_getmcontext() and the setcontext system call:

http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/arch/aarch64/aarch64/sig_machdep.c?rev=1.1.28.2

I would expect Linux to do the same.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@4a6f656c This is not a theoretical problem. Signal handlers are used in many different ways by different programs. See #18328.

@4a6f656c

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

@ianlancetaylor - fair enough, one could argue that the signal forwarding code should be preserving and restoring context (as the kernel would). I guess saving/restoring is the safe solution, although at a cost for the normal/non-forwarding case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.