| Message ID | 1464361598-30776-1-git-send-email-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/27/2016 08:06 AM, Peter Maydell wrote: > @@ -31,6 +32,8 @@ safe_syscall_base: > * does not list any ABI differences regarding stack alignment.) > */ > push %rbp > + .cfi_def_cfa_offset 16 > + .cfi_offset rbp,-16 While this is correct, there are two other directives that make it easier to describe changes without having to compute globally correct constants. Here they would be: .cfi_adjust_cfa_offset 8 Add 8 to the offset, i.e. decrement the SP by 8. .cfi_rel_offset rbp, 0 Save rbp at the current top-of-stack. The assembler will compute the absolute values from these relative values. Using them makes it easy to see from a narrow window of code that the annotations are correct. Otherwise, Reviewed-by: Richard Henderson <rth@twiddle.net> r~
On 27 May 2016 at 17:21, Richard Henderson <rth@twiddle.net> wrote: > On 05/27/2016 08:06 AM, Peter Maydell wrote: >> >> @@ -31,6 +32,8 @@ safe_syscall_base: >> * does not list any ABI differences regarding stack alignment.) >> */ >> push %rbp >> + .cfi_def_cfa_offset 16 >> + .cfi_offset rbp,-16 > > > While this is correct, there are two other directives that make it easier to > describe changes without having to compute globally correct constants. Here > they would be: > > .cfi_adjust_cfa_offset 8 > > Add 8 to the offset, i.e. decrement the SP by 8. Presumably .cfi_startproc sets the initial offset to 8? (It's not documented that it does so, which is I think partly why I preferred to use a directive that definitely set the offset to the right thing.) > .cfi_rel_offset rbp, 0 > > Save rbp at the current top-of-stack. > > The assembler will compute the absolute values from these relative values. > Using them makes it easy to see from a narrow window of code that the > annotations are correct. > > Otherwise, > > Reviewed-by: Richard Henderson <rth@twiddle.net> Thanks. I'll spin a v2 with your changes in it next week. -- PMM
On 27 May 2016 at 16:06, Peter Maydell <peter.maydell@linaro.org> wrote: > return_ERESTARTSYS: > /* code path when we didn't execute the syscall */ > + .cfi_restore_state > mov $-TARGET_ERESTARTSYS, %rax > pop %rbp > + .cfi_def_cfa_offset 8 > + .cfi_restore ebp These should read '.cfi_restore rbp'. I have no idea how that got through testing since it doesn't compile at all... thanks -- PMM
On 05/27/2016 09:34 AM, Peter Maydell wrote: > On 27 May 2016 at 17:21, Richard Henderson <rth@twiddle.net> wrote: >> On 05/27/2016 08:06 AM, Peter Maydell wrote: >>> >>> @@ -31,6 +32,8 @@ safe_syscall_base: >>> * does not list any ABI differences regarding stack alignment.) >>> */ >>> push %rbp >>> + .cfi_def_cfa_offset 16 >>> + .cfi_offset rbp,-16 >> >> >> While this is correct, there are two other directives that make it easier to >> describe changes without having to compute globally correct constants. Here >> they would be: >> >> .cfi_adjust_cfa_offset 8 >> >> Add 8 to the offset, i.e. decrement the SP by 8. > > Presumably .cfi_startproc sets the initial offset to 8? > (It's not documented that it does so, which is I think partly why > I preferred to use a directive that definitely set the offset > to the right thing.) It is documented to set up the normal no-instructions-executed call frame. Which in the case of x86, does have a non-zero offset. There is a ".cfi_startproc simple" that begins a frame with no opcodes at all. r~
diff --git a/linux-user/host/x86_64/safe-syscall.inc.S b/linux-user/host/x86_64/safe-syscall.inc.S index dde434c..bbb1eca 100644 --- a/linux-user/host/x86_64/safe-syscall.inc.S +++ b/linux-user/host/x86_64/safe-syscall.inc.S @@ -24,6 +24,7 @@ * -1-and-errno-set convention is done by the calling wrapper. */ safe_syscall_base: + .cfi_startproc /* This saves a frame pointer and aligns the stack for the syscall. * (It's unclear if the syscall ABI has the same stack alignment * requirements as the userspace function call ABI, but better safe than @@ -31,6 +32,8 @@ safe_syscall_base: * does not list any ABI differences regarding stack alignment.) */ push %rbp + .cfi_def_cfa_offset 16 + .cfi_offset rbp,-16 /* The syscall calling convention isn't the same as the * C one: @@ -70,12 +73,19 @@ safe_syscall_start: safe_syscall_end: /* code path for having successfully executed the syscall */ pop %rbp + .cfi_remember_state + .cfi_def_cfa_offset 8 + .cfi_restore ebp ret return_ERESTARTSYS: /* code path when we didn't execute the syscall */ + .cfi_restore_state mov $-TARGET_ERESTARTSYS, %rax pop %rbp + .cfi_def_cfa_offset 8 + .cfi_restore ebp ret + .cfi_endproc .size safe_syscall_base, .-safe_syscall_base
Use cfi directives in the x86-64 safe_syscall to allow gdb to get backtraces right from within it. (In particular this will be quite a common situation if the user interrupts QEMU while it's in a blocked safe-syscall: at the point of the syscall insn RBP is in use for something else, and so gdb can't find the frame then without assistance.) Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- The requirements for frame information annotations seem to be a bit of an undocumented black art, but I think I have these right. At any rate, gdb now gives correct backtraces from all points in the routine as far as I can see. Review appreciated... linux-user/host/x86_64/safe-syscall.inc.S | 10 ++++++++++ 1 file changed, 10 insertions(+)