[BZ,#19490] Add unwind descriptors for x86_64 _mcount and __fentry__
diff mbox

Message ID CALoOobMWtqPyM99G4mX56nV=ymqNGFKEwWMAmc0h_Uff_K2yjA@mail.gmail.com
State New
Headers show

Commit Message

Paul Pluzhnikov Feb. 26, 2016, 1:57 a.m. UTC
On Mon, Feb 22, 2016 at 1:14 AM, Mike Frysinger <vapier@gentoo.org> wrote:
> On 23 Jan 2016 14:52, Paul Pluzhnikov wrote:
...
>> +     cfi_rel_offset (rdi, 32)
>> +     cfi_rel_offset (r8, 40)
>> +     cfi_rel_offset (r9, 48)
>
> don't we usually interleave the insns & cfi calls so that it's harder
> for them to get out of sync ?

I don't believe we are very consistent here. E.g. in sysdeps/i386/submul_1.S:

ENTRY (__mpn_submul_1)
        pushl   %edi
        cfi_adjust_cfa_offset (4)
        pushl   %esi
        cfi_adjust_cfa_offset (4)
        pushl   %ebp
        cfi_adjust_cfa_offset (4)
        pushl   %ebx
        cfi_adjust_cfa_offset (4)
        cfi_rel_offset (edi, 12)
        cfi_rel_offset (esi, 8)
        cfi_rel_offset (ebp, 4)
        cfi_rel_offset (ebx, 0)

But x86_64 does seem to interleave, so I've updated the patch to do the same.

>>       .type C_SYMBOL_NAME(__fentry__), @function
>>       .align ALIGNARG(4)
>>  C_LABEL(__fentry__)
>> -     /* Allocate space for 7 registers.  */
>> +     cfi_startproc
>> +     /* Allocate space for 7 registers (+8 for proper stack alignment).  */
>>       subq    $64,%rsp
>
> mmm, 56 is used above w/_mcount and is 8 byte aligned.  are you saying
> we need 16 byte alignment and thus _mcount should be fixed ?

No: the difference between _mcount and __fentry__ is that the former
is called after function prologue. That is, on entry into _mcount
(%RSP & 0xF) == 8 (as is usual for x86_64), but on entry into
__fentry__ (%RSP & 0xF) == 0 (which is special).

Note that I am only changing comment here, not the actual alignment.

Thanks,

2016-02-26  Paul Pluzhnikov  <ppluzhnikov@google.com>

        [BZ #19490]
        * sysdeps/x86_64/_mcount.S (_mcount): Add unwind descriptor.
        (__fentry__): Likewise

Comments

Mike Frysinger Feb. 26, 2016, 3:40 a.m. UTC | #1
i'm not an x86_64 maintainer, but patch looks ok to me
-mike
Paul Pluzhnikov March 3, 2016, 2:12 a.m. UTC | #2
On Thu, Feb 25, 2016 at 7:40 PM, Mike Frysinger <vapier@gentoo.org> wrote:
> i'm not an x86_64 maintainer, but patch looks ok to me

Can I have an approval for this patch?
https://sourceware.org/ml/libc-alpha/2016-02/msg00823.html

Thanks,
H.J. Lu March 3, 2016, 3:44 a.m. UTC | #3
On Wed, Mar 2, 2016 at 6:12 PM, Paul Pluzhnikov <ppluzhnikov@google.com> wrote:
> On Thu, Feb 25, 2016 at 7:40 PM, Mike Frysinger <vapier@gentoo.org> wrote:
>> i'm not an x86_64 maintainer, but patch looks ok to me
>
> Can I have an approval for this patch?
> https://sourceware.org/ml/libc-alpha/2016-02/msg00823.html

Can you also update it to use ENTRY/END?
Paul Pluzhnikov March 3, 2016, 4:20 a.m. UTC | #4
On Wed, Mar 2, 2016 at 7:44 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

> Can you also update it to use ENTRY/END?

In sysdeps/x86_64/sysdep.h:

/* Define an entry point visible from C.  */
#define ENTRY(name)                                                           \
  .globl C_SYMBOL_NAME(name);                                                 \
  .type C_SYMBOL_NAME(name),@function;                                        \
  .align ALIGNARG(4);                                                         \
  C_LABEL(name)                                                               \
  cfi_startproc;                                                              \
  CALL_MCOUNT

Using CALL_MCOUNT in implementation of _mcount isn't going to end well.
I guess I could '#undef PROF', but that seems fragile and likely to
cause further problems.
Andreas Schwab March 3, 2016, 8:30 a.m. UTC | #5
Paul Pluzhnikov <ppluzhnikov@google.com> writes:

> Using CALL_MCOUNT in implementation of _mcount isn't going to end well.

_mcount needs to be arranged to be compiled without PROF, like the
noprof routines in gmon/Makefile.

Andreas.

Patch
diff mbox

diff --git a/sysdeps/x86_64/_mcount.S b/sysdeps/x86_64/_mcount.S
index 5d7edd2..a498a48 100644
--- a/sysdeps/x86_64/_mcount.S
+++ b/sysdeps/x86_64/_mcount.S
@@ -28,15 +28,24 @@ 
 	.type C_SYMBOL_NAME(_mcount), @function
 	.align ALIGNARG(4)
 C_LABEL(_mcount)
+	cfi_startproc
 	/* Allocate space for 7 registers.  */
 	subq	$56,%rsp
+	cfi_adjust_cfa_offset (56)
 	movq	%rax,(%rsp)
+	cfi_rel_offset (rax, 0)
 	movq	%rcx,8(%rsp)
+	cfi_rel_offset (rcx, 8)
 	movq	%rdx,16(%rsp)
+	cfi_rel_offset (rdx, 16)
 	movq	%rsi,24(%rsp)
+	cfi_rel_offset (rsi, 24)
 	movq	%rdi,32(%rsp)
+	cfi_rel_offset (rdi, 32)
 	movq	%r8,40(%rsp)
+	cfi_rel_offset (r8, 40)
 	movq	%r9,48(%rsp)
+	cfi_rel_offset (r9, 48)
 
 	/* Setup parameter for __mcount_internal.  */
 	/* selfpc is the return address on the stack.  */
@@ -51,14 +60,23 @@  C_LABEL(_mcount)
 	/* Pop the saved registers.  Please note that `mcount' has no
 	   return value.  */
 	movq	48(%rsp),%r9
+	cfi_restore (r9)
 	movq	40(%rsp),%r8
+	cfi_restore (r8)
 	movq	32(%rsp),%rdi
+	cfi_restore (rdi)
 	movq	24(%rsp),%rsi
+	cfi_restore (rsi)
 	movq	16(%rsp),%rdx
+	cfi_restore (rdx)
 	movq	8(%rsp),%rcx
+	cfi_restore (rcx)
 	movq	(%rsp),%rax
+	cfi_restore (rax)
 	addq	$56,%rsp
+	cfi_adjust_cfa_offset (-56)
 	ret
+	cfi_endproc
 
 	ASM_SIZE_DIRECTIVE(C_SYMBOL_NAME(_mcount))
 
@@ -69,15 +87,25 @@  weak_alias (_mcount, mcount)
 	.type C_SYMBOL_NAME(__fentry__), @function
 	.align ALIGNARG(4)
 C_LABEL(__fentry__)
-	/* Allocate space for 7 registers.  */
+	cfi_startproc
+	/* Allocate space for 7 registers
+	   (+8 bytes for proper stack alignment).  */
 	subq	$64,%rsp
+	cfi_adjust_cfa_offset (64)
 	movq	%rax,(%rsp)
+	cfi_rel_offset (rax, 0)
 	movq	%rcx,8(%rsp)
+	cfi_rel_offset (rcx, 8)
 	movq	%rdx,16(%rsp)
+	cfi_rel_offset (rdx, 16)
 	movq	%rsi,24(%rsp)
+	cfi_rel_offset (rsi, 24)
 	movq	%rdi,32(%rsp)
+	cfi_rel_offset (rdi, 32)
 	movq	%r8,40(%rsp)
+	cfi_rel_offset (r8, 40)
 	movq	%r9,48(%rsp)
+	cfi_rel_offset (r9, 48)
 
 	/* Setup parameter for __mcount_internal.  */
 	/* selfpc is the return address on the stack.  */
@@ -92,13 +120,22 @@  C_LABEL(__fentry__)
 	/* Pop the saved registers.  Please note that `__fentry__' has no
 	   return value.  */
 	movq	48(%rsp),%r9
+	cfi_restore (r9)
 	movq	40(%rsp),%r8
+	cfi_restore (r8)
 	movq	32(%rsp),%rdi
+	cfi_restore (rdi)
 	movq	24(%rsp),%rsi
+	cfi_restore (rsi)
 	movq	16(%rsp),%rdx
+	cfi_restore (rdx)
 	movq	8(%rsp),%rcx
+	cfi_restore (rcx)
 	movq	(%rsp),%rax
+	cfi_restore (rax)
 	addq	$64,%rsp
+	cfi_adjust_cfa_offset (-64)
 	ret
+	cfi_endproc
 
 	ASM_SIZE_DIRECTIVE(C_SYMBOL_NAME(__fentry__))