lkml.org 
[lkml]   [2009]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] x86-64: fix unwind annotations in entry_64.S

* Jan Beulich <jbeulich@novell.com> wrote:

> .macro XCPT_FRAME start=1 offset=0
> - INTR_FRAME \start, RIP+\offset-ORIG_RAX
> - /*CFI_REL_OFFSET orig_rax, ORIG_RAX-ORIG_RAX*/
> + INTR_FRAME \start, __stringify(RIP+\offset-ORIG_RAX)
> .endm
>
> /*
> * frame that enables calling into C.
> */
> .macro PARTIAL_FRAME start=1 offset=0
> - XCPT_FRAME \start, ORIG_RAX+\offset-ARGOFFSET
> + .if \start >= 0
> + XCPT_FRAME \start, __stringify(ORIG_RAX+\offset-ARGOFFSET)
> + .endif

So we pass in a stringified parameter from PARTIAL_FRAME to
XCPT_FRAME and then we stringify it again?

> - movq_cfi rdi, RDI+16-ARGOFFSET
> - movq_cfi rsi, RSI+16-ARGOFFSET
> - movq_cfi rdx, RDX+16-ARGOFFSET
> - movq_cfi rcx, RCX+16-ARGOFFSET
> - movq_cfi rax, RAX+16-ARGOFFSET
> - movq_cfi r8, R8+16-ARGOFFSET
> - movq_cfi r9, R9+16-ARGOFFSET
> - movq_cfi r10, R10+16-ARGOFFSET
> - movq_cfi r11, R11+16-ARGOFFSET
> + movq %rdi, RDI+16-ARGOFFSET(%rsp)
> + movq %rsi, RSI+16-ARGOFFSET(%rsp)
> + movq %rdx, RDX+16-ARGOFFSET(%rsp)
> + movq %rcx, RCX+16-ARGOFFSET(%rsp)
> + movq_cfi rax, __stringify(RAX+16-ARGOFFSET)
> + movq %r8, R8+16-ARGOFFSET(%rsp)
> + movq %r9, R9+16-ARGOFFSET(%rsp)
> + movq %r10, R10+16-ARGOFFSET(%rsp)
> + movq_cfi r11, __stringify(R11+16-ARGOFFSET)

Hm, we used to have annotations for those arguments too, now
they are gone.

> leaq 8(%rsp), %rbp /* mov %rsp, %ebp */
> + CFI_DEF_CFA_REGISTER rbp
> + CFI_ADJUST_CFA_OFFSET -8

This open-codes CFI annotations again - please introduce a
helper instead.

> - popq_cfi %rax /* move return address... */
> + popq %rax /* move return address... */

No annotation needed?

> mov %gs:pda_irqstackptr,%rsp
> - EMPTY_FRAME 0
> - pushq_cfi %rbp /* backlink for unwinder */
> - pushq_cfi %rax /* ... to the new stack */
> + pushq %rbp /* backlink for unwinder */
> + pushq %rax /* ... to the new stack */

Ditto?

> ENTRY(save_rest)
> - PARTIAL_FRAME 1 REST_SKIP+8
> + CFI_STARTPROC
> movq 5*8+16(%rsp), %r11 /* save return address */
> - movq_cfi rbx, RBX+16
> - movq_cfi rbp, RBP+16
> - movq_cfi r12, R12+16
> - movq_cfi r13, R13+16
> - movq_cfi r14, R14+16
> - movq_cfi r15, R15+16
> + movq %rbx, RBX+16(%rsp)
> + movq %rbp, RBP+16(%rsp)
> + movq %r12, R12+16(%rsp)
> + movq %r13, R13+16(%rsp)
> + movq %r14, R14+16(%rsp)
> + movq %r15, R15+16(%rsp)

Same here?

> CFI_ADJUST_CFA_OFFSET REST_SKIP
> call save_rest
> - DEFAULT_FRAME 0 8 /* offset 8: return address */
> + DEFAULT_FRAME -2 8 /* offset 8: return address */
> leaq 8(%rsp), \arg /* pt_regs pointer */

Open-coded CFI annotation again.

I havent checked the rest of the patch but it shows similar
patters.

The thing is, this patch is completely unacceptable - it just
reintroduces the same ugly crufty CFI code we used to have
there.

The current annotations might be completely broken but at least
they _look_ structured and attempt to bring some order into the
CFI annotations chaos.

There's really just two options here:

- You either submit clean, gradual patches that fix the problems
and explain what is done and why, and documents the annotation
rules and makes CFI annotations maintainable,

- Or we'll remove all CFI annotations from all x86 assembly
files and wait for future, clean patches.

Ingo


\
 
 \ /
  Last update: 2009-03-12 11:51    [W:0.262 / U:0.016 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site