Issue 306800043: i#1569 AArch64: Enable inlining of system calls. (Closed)
|
Created:
7 years, 9 months ago by Edmund.Grimley.Evans Modified:
7 years, 9 months ago Reviewers:
bruening CC:
dynamorio-devs_googlegroups.com Visibility:
Public. |
DescriptionCommit log for first patchset:
---------------
i#1569 AArch64: Enable inlining of system calls.
In mangle_syscall_arch, remove the code to save and restore the stolen
register as Linux system calls have never clobbered registers on ARM.
---------------
Patch Set 1 #
Total comments: 7
Patch Set 2 : comments #
Total comments: 2
Patch Set 3 : Committed #
MessagesTotal messages: 9
Sign in to reply to this message.
https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (left): https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c#ol... core/arch/aarchxx/mangle.c:947: /* We do need to save the stolen reg if it is caller-saved. What is the evidence that this does not need to be done? I am skeptical, as we have run into real problems in the past where calling-convention-scratch registers (such as various xmm registers on x86) are clobbered inside system calls on both Linux and Windows. I don't see why A32 or A64 would be different. https://codereview.appspot.com/306800043/diff/1/core/arch/opnd.h File core/arch/opnd.h (right): https://codereview.appspot.com/306800043/diff/1/core/arch/opnd.h#newcode2282 core/arch/opnd.h:2282: # define DR_REG_STOLEN_MIN IF_X64_ELSE(DR_REG_X9, DR_REG_R8) /* no syscall regs */ Why x9 and not x8?
Sign in to reply to this message.
https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (left): https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c#ol... core/arch/aarchxx/mangle.c:947: /* We do need to save the stolen reg if it is caller-saved. On 2016/07/21 04:48:03, bruening wrote: > What is the evidence that this does not need to be done? 1. I've never seen the kernel clobber a register. 2. I asked an ARM kernel developer, who said they'd never done that. 3. While working on DynamoRIO we found a bug where DynamoRIO clobbered a caller-saved register (X1), stopping a C program from working. 4. C library source (sysdeps/unix/sysv/linux/aarch64/sysdep.h, sysdeps/unix/sysv/linux/arm/sysdep.h) uses a SWI/SVC in an asm with no indication of any registers getting clobbered, so it's no surprise that C programs expect no registers to get clobbered. > I am skeptical, as we > have run into real problems in the past where calling-convention-scratch > registers (such as various xmm registers on x86) are clobbered inside system > calls on both Linux and Windows. I don't see why A32 or A64 would be different. Was that kernel aware of xmm? If it was an old kernel, or one configured not to know about xmm, then those registers could be clobbered by another thread. In any case, I wouldn't conclude anything about ARM core registers from something that happened with an Intel extension register. https://codereview.appspot.com/306800043/diff/1/core/arch/opnd.h File core/arch/opnd.h (right): https://codereview.appspot.com/306800043/diff/1/core/arch/opnd.h#newcode2282 core/arch/opnd.h:2282: # define DR_REG_STOLEN_MIN IF_X64_ELSE(DR_REG_X9, DR_REG_R8) /* no syscall regs */ On 2016/07/21 04:48:03, bruening wrote: > Why x9 and not x8? W8 is used for the syscall number. I could change the comment to "/* DR_REG_SYSNUM + 1 */", but I doubt it would be possible/wise to use that as the definition because DR_REG_SYSNUM is OS-specific and opnd.h isn't.
Sign in to reply to this message.
https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (left): https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c#ol... core/arch/aarchxx/mangle.c:947: /* We do need to save the stolen reg if it is caller-saved. On 2016/07/21 09:06:35, Edmund.Grimley.Evans wrote: > On 2016/07/21 04:48:03, bruening wrote: > > What is the evidence that this does not need to be done? > > 1. I've never seen the kernel clobber a register. > > 2. I asked an ARM kernel developer, who said they'd never done that. > > 3. While working on DynamoRIO we found a bug where DynamoRIO clobbered a > caller-saved register (X1), stopping a C program from working. > > 4. C library source (sysdeps/unix/sysv/linux/aarch64/sysdep.h, > sysdeps/unix/sysv/linux/arm/sysdep.h) uses a SWI/SVC in an asm with no > indication of any registers getting clobbered, so it's no surprise that C > programs expect no registers to get clobbered. > > > I am skeptical, as we > > have run into real problems in the past where calling-convention-scratch > > registers (such as various xmm registers on x86) are clobbered inside system > > calls on both Linux and Windows. I don't see why A32 or A64 would be > different. > > Was that kernel aware of xmm? If it was an old kernel, or one configured not to > know about xmm, then those registers could be clobbered by another thread. > > In any case, I wouldn't conclude anything about ARM core registers from > something that happened with an Intel extension register. Please bear with me, I want to look into the x86 behavior so it may take me more time here. Yes, these were xmm-aware kernels. Other issues such as having to record all of the pre-syscall param values b/c they are not available post-syscall have led to a general feeling that the kernel is not preserving the entire user context.
Sign in to reply to this message.
https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (left): https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c#ol... core/arch/aarchxx/mangle.c:947: /* We do need to save the stolen reg if it is caller-saved. On 2016/07/22 20:49:14, bruening wrote: > On 2016/07/21 09:06:35, Edmund.Grimley.Evans wrote: > > On 2016/07/21 04:48:03, bruening wrote: > > > What is the evidence that this does not need to be done? > > > > 1. I've never seen the kernel clobber a register. > > > > 2. I asked an ARM kernel developer, who said they'd never done that. > > > > 3. While working on DynamoRIO we found a bug where DynamoRIO clobbered a > > caller-saved register (X1), stopping a C program from working. > > > > 4. C library source (sysdeps/unix/sysv/linux/aarch64/sysdep.h, > > sysdeps/unix/sysv/linux/arm/sysdep.h) uses a SWI/SVC in an asm with no > > indication of any registers getting clobbered, so it's no surprise that C > > programs expect no registers to get clobbered. > > > > > I am skeptical, as we > > > have run into real problems in the past where calling-convention-scratch > > > registers (such as various xmm registers on x86) are clobbered inside system > > > calls on both Linux and Windows. I don't see why A32 or A64 would be > > different. > > > > Was that kernel aware of xmm? If it was an old kernel, or one configured not > to > > know about xmm, then those registers could be clobbered by another thread. > > > > In any case, I wouldn't conclude anything about ARM core registers from > > something that happened with an Intel extension register. > > Please bear with me, I want to look into the x86 behavior so it may take me more > time here. Yes, these were xmm-aware kernels. Other issues such as having to > record all of the pre-syscall param values b/c they are not available > post-syscall have led to a general feeling that the kernel is not preserving the > entire user context. Based on empirical tests on Windows 7, the Windows kernel can be observed to clobber pretty much every single scratch register, implying that it is optimized to not save the user scratch registers across transitions into the kernel. 64-bit changes rax (of course), rcx, rdx, r8, r9, r10, r11, and zeroes xmm0-5. 32-bit changes eax (of course), ecx, edx, and zeroes xmm0-xmm5. (The edx for 32-bit is related to OP_sysenter.) This is where our intuition is coming from and our paranoia to not trust the kernel to preserve scratch registers. On Linux x86, for 64-bit, the kernel clobbers rax (of course), rcx, and r11. For 32-bit, the kernel clobbers eax (of course), and for OP_sysenter also ecx and ebp (note: callee-saved, not scratch). These are all related to the syscall gateway mechanism: OP_syscall and OP_sysenter. The conclusion is that on Windows many registers will get clobbered; on Linux some will get clobbered based on the gateway mechanism. There's a chance we'll add support for Windows on ARM in the future, so even if it's the case that Linux ARM does preserve every single register (except r0), and even if Windows ARM decided to do the same, there should at least be a comment explaining that this is an assumption, that it does not hold for all kernels, and that making the assumption could break things in the future.
Sign in to reply to this message.
https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (left): https://codereview.appspot.com/306800043/diff/1/core/arch/aarchxx/mangle.c#ol... core/arch/aarchxx/mangle.c:947: /* We do need to save the stolen reg if it is caller-saved. On 2016/07/25 15:47:13, bruening wrote: > On 2016/07/22 20:49:14, bruening wrote: > > On 2016/07/21 09:06:35, Edmund.Grimley.Evans wrote: > > > On 2016/07/21 04:48:03, bruening wrote: > > > > What is the evidence that this does not need to be done? > > > > > > 1. I've never seen the kernel clobber a register. > > > > > > 2. I asked an ARM kernel developer, who said they'd never done that. > > > > > > 3. While working on DynamoRIO we found a bug where DynamoRIO clobbered a > > > caller-saved register (X1), stopping a C program from working. > > > > > > 4. C library source (sysdeps/unix/sysv/linux/aarch64/sysdep.h, > > > sysdeps/unix/sysv/linux/arm/sysdep.h) uses a SWI/SVC in an asm with no > > > indication of any registers getting clobbered, so it's no surprise that C > > > programs expect no registers to get clobbered. > > > > > > > I am skeptical, as we > > > > have run into real problems in the past where calling-convention-scratch > > > > registers (such as various xmm registers on x86) are clobbered inside > system > > > > calls on both Linux and Windows. I don't see why A32 or A64 would be > > > different. > > > > > > Was that kernel aware of xmm? If it was an old kernel, or one configured not > > to > > > know about xmm, then those registers could be clobbered by another thread. > > > > > > In any case, I wouldn't conclude anything about ARM core registers from > > > something that happened with an Intel extension register. > > > > Please bear with me, I want to look into the x86 behavior so it may take me > more > > time here. Yes, these were xmm-aware kernels. Other issues such as having to > > record all of the pre-syscall param values b/c they are not available > > post-syscall have led to a general feeling that the kernel is not preserving > the > > entire user context. > > Based on empirical tests on Windows 7, the Windows kernel can be observed to > clobber pretty much every single scratch register, implying that it is optimized > to not save the user scratch registers across transitions into the kernel. > 64-bit changes rax (of course), rcx, rdx, r8, r9, r10, r11, and zeroes xmm0-5. > 32-bit changes eax (of course), ecx, edx, and zeroes xmm0-xmm5. (The edx for > 32-bit is related to OP_sysenter.) This is where our intuition is coming from > and our paranoia to not trust the kernel to preserve scratch registers. > > On Linux x86, for 64-bit, the kernel clobbers rax (of course), rcx, and r11. > For 32-bit, the kernel clobbers eax (of course), and for OP_sysenter also ecx > and ebp (note: callee-saved, not scratch). These are all related to the syscall > gateway mechanism: OP_syscall and OP_sysenter. > > The conclusion is that on Windows many registers will get clobbered; on Linux > some will get clobbered based on the gateway mechanism. > > There's a chance we'll add support for Windows on ARM in the future, so even if > it's the case that Linux ARM does preserve every single register (except r0), > and even if Windows ARM decided to do the same, there should at least be a > comment explaining that this is an assumption, that it does not hold for all > kernels, and that making the assumption could break things in the future. Yes, though we have to assume something, namely that some particular register doesn't get clobbered, or we are doomed. There's no reason to assume that system calls follow the standard calling convention when we know, in fact, that they don't (passing arguments in R4). I wonder if Windows is careful about not leaking potentially secret information into those clobbered registers...
Sign in to reply to this message.
Commit log for latest patchset: --------------- i#1569 AArch64: Enable inlining of system calls. In mangle_syscall_arch, remove the code to save and restore the stolen register as Linux system calls have never clobbered registers on ARM. Review-URL: https://codereview.appspot.com/306800043 ---------------
Sign in to reply to this message.
LGTM https://codereview.appspot.com/306800043/diff/20001/core/arch/aarchxx/mangle.c File core/arch/aarchxx/mangle.c (right): https://codereview.appspot.com/306800043/diff/20001/core/arch/aarchxx/mangle.... core/arch/aarchxx/mangle.c:835: OUT instr_t **first, OUT instr_t **last) Please try not to rebase in between patchsets -- it makes reviewing difficult as now the view of patchset 2 vs patchset 1 shows unrelated changes. Please look at the review view to see what I mean. https://codereview.appspot.com/306800043/diff/20001/core/arch/aarchxx/mangle.... core/arch/aarchxx/mangle.c:943: * safer ("callee-saved") register before the system call and restore it callee-saved *and* not used by the gateway mechanism (let's say ARM adds a "fast system call" mechanism like OP_sysenter or OP_syscall: those typically end up clobbering registers)
Sign in to reply to this message.
Committed as https://github.com/DynamoRIO/dynamorio/commit/7ee7189b30d14911660b60c9184047f... Final commit log: --------------- i#1569 AArch64: Enable inlining of system calls. In mangle_syscall_arch, remove the code to save and restore the stolen register as Linux system calls have never clobbered registers on ARM. Review-URL: https://codereview.appspot.com/306800043 ---------------
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
