arch/x86: Fix comments about stack frame with -pg / -mfentry #96

Taeung · 2017-05-16T03:40:56Z

I fixed comments about stack frame (with -pg, -mfentry)
based on your mention yesterday. 😄
I'd appreciate some feedback on this PR!

Thanks,
Taeung

namhyung

I'd like to describe the addresses before mcount adjusts the stack pointer. Note that in -pg case, the "%rsp" might be changed already for local variables, so you should be "%rbp" to get the addresses. On the other hand, in -pg -mfentry case, the "%rbp" is not set yet.

Taeung · 2017-05-16T09:41:39Z

@namhyung , I modified the comments again based on your mention.
What about that ?

namhyung · 2017-05-17T00:05:27Z

arch/x86_64/fentry.S

@@ -1,8 +1,44 @@
 /* argument passing: %rdi, %rsi, %rdx, %rcx, %r8, %r9 */
 /* return value: %rax */
 /* callee saved: %rbx, %rbp, %rsp, %r12-r15 */
-/* stack frame (with -pg): return addr = 8(%rbp), prev fp = 0(%rbp) */
-/* stack frame (with -fentry): return addr = (%rsp), prev fp = 8(%rsp) */
+/* stack frame (with -pg -mfentry): parent location = addr of 52(%rsp), child ip = 48(%rsp) */


52 seems wrong. And I'd like to see stack frame info at the beginning of mcount() - not after libmcount.so:mcount subtracts the stack pointer. Also please just show parent addr and child addr like "parent addr = 8(%rsp), child addr = (%rsp)"

Oops, parent addr is 56(%rsp), right ? https://github.com/namhyung/uftrace/blob/master/arch/x86_64/fentry.S#L29-L30

Okey, will change the format for parent and child addr !

@namhyung , You mean that we can show stack frame info
at the beginning of mcount() or __fentry__() like below, right ?

For example:

__fentry__ case

(gdb) info f Stack level 0, frame at 0x7fffffffdd48: rip = 0x7ffff7bc9d70 in __fentry__; saved rip = 0x4006b3 called by frame at 0x7fffffffdd50 Arglist at 0x7fffffffdd38, args: Locals at 0x7fffffffdd38, Previous frame's sp is 0x7fffffffdd48 Saved registers: rip at 0x7fffffffdd40

mcount case

Stack level 0, frame at 0x7fffffffdd30: rip = 0x7ffff7bc9ce0 in mcount; saved rip = 0x40069b called by frame at 0x7fffffffdd50 Arglist at 0x7fffffffdd20, args: Locals at 0x7fffffffdd20, Previous frame's sp is 0x7fffffffdd30 Saved registers: rip at 0x7fffffffdd28

namhyung · 2017-05-17T00:06:16Z

arch/x86_64/mcount.S

@@ -1,8 +1,43 @@
 /* argument passing: %rdi, %rsi, %rdx, %rcx, %r8, %r9 */
 /* return value: %rax */
 /* callee saved: %rbx, %rbp, %rsp, %r12-r15 */
-/* stack frame (with -pg): return addr = 8(%rbp), prev fp = 0(%rbp) */
-/* stack frame (with -fentry): return addr = (%rsp), prev fp = %rbp */
+/* stack frame (with -pg): parent location = addr of 8(%rbp), child ip = 56(%rsp) */


Taeung · 2017-05-17T01:52:08Z

@namhyung , I sent v3 based on your mention,
What about that ?

namhyung · 2017-05-17T09:38:53Z

arch/x86_64/fentry.S

@@ -1,8 +1,55 @@
 /* argument passing: %rdi, %rsi, %rdx, %rcx, %r8, %r9 */
 /* return value: %rax */
 /* callee saved: %rbx, %rbp, %rsp, %r12-r15 */
-/* stack frame (with -pg): return addr = 8(%rbp), prev fp = 0(%rbp) */
-/* stack frame (with -fentry): return addr = (%rsp), prev fp = 8(%rsp) */
+/* stack frame (with -pg -mfentry): parent addr = 56(%rsp), child addr = 48(%rsp) */


I meant you need to subtrace 48 from the offsets since it was adjusted by our __fentry__.

Understood !
At the very beginning of __fentry__(),

parent addr (an address that has the return address of hello()): 8(%rsp)
child addr (the return address of __fentry__()): %rsp

Because the "%rbp" is not set yet.
Right ?

Child addr is in "(%rsp)".

You mean the below ?

child addr == (%rsp)
child ip == %rsp

The reason I wrote %rsp instead of (%rsp) is
the value of %rsp is the return address of __fentry__().
(e.g. 0x00000000004006ab)

Dump of assembler code for function hello: 0x00000000004006a6 <+0>: callq 0x400550 <__fentry__@plt> child addr => 0x00000000004006ab <+5>: push %rbp 0x00000000004006ac <+6>: mov %rsp,%rbp

Sorry for my confusion..

I meant "child addr == child ip".

Really? %rsp is a stack pointer so its value should point to a stack address. Dereferencing %rsp can result in a return address (of a function).

For example:

%rsp = 0x7fffff8720

(%rsp) = 0x4006ab

Sorry, I got it. will change that !

namhyung · 2017-05-17T09:39:50Z

arch/x86_64/fentry.S

+                     0x00000000004006b9 <+19>:	nop
+                     0x00000000004006ba <+20>:	pop    %rbp
+                     0x00000000004006bb <+21>:	retq
+


It seems too verbose, and comments below from here can be omitted IMHO.

namhyung · 2017-05-17T09:40:27Z

arch/x86_64/fentry.S

 	movq 48(%rsp), %rsi
-	/* parent ip */
+	/* parent addr */


Oh, it should be "parent location" since it takes address by "lea" insn.

namhyung · 2017-05-17T09:44:36Z

arch/x86_64/mcount.S

@@ -1,8 +1,54 @@
 /* argument passing: %rdi, %rsi, %rdx, %rcx, %r8, %r9 */
 /* return value: %rax */
 /* callee saved: %rbx, %rbp, %rsp, %r12-r15 */
-/* stack frame (with -pg): return addr = 8(%rbp), prev fp = 0(%rbp) */
-/* stack frame (with -fentry): return addr = (%rsp), prev fp = %rbp */
+/* stack frame (with -pg): parent addr = 8(%rbp), child addr = 56(%rsp) */


Ditto. Please remove 56 here. Also it'd be great if you could mention why fentry needs 48 and mcount needs 56 (regarding #92). It's because of the 16-byte stack alignment required from GCC 7..

@namhyung , What about adding the explanation about #92 at fentry.S:27~ and mcount.S:27~ ?

@namhyung ,
If there isn't the problem about the 16-byte stack alignment,
we could just use push and pop insn.
However, because it is required from GCC7, we should manually adjust stack pointer %rsp, right ?

No, using push/pop or manual adjusting is essentially same. What matters is the offset it changes.

namhyung · 2017-05-17T09:46:01Z

arch/x86_64/mcount.S

+                     0x0000000000400687 <+1>:	mov    %rsp,%rbp
+                     0x000000000040068a <+4>:	sub    $0x10,%rsp
+                     0x000000000040068e <+8>:	callq  0x400520 <mcount@plt>
+       child addr => 0x0000000000400693 <+13>:	movl   $0x1,-0x4(%rbp)


Ditto. Please omit below..

namhyung · 2017-05-17T11:03:35Z

arch/x86_64/mcount.S

 	movq 56(%rsp), %rsi
 	sub $9, %rsi

-	/* parent location */
+	/* parent addr */


"parent location" is correct.

Taeung · 2017-05-19T07:12:12Z

@namhyung , I sent v4 !
I just added the comment about the 16-byte stack alignment
above sub $56, %rsp for mcount and sub $48, %rsp for fentry

namhyung · 2017-05-20T04:41:50Z

arch/x86_64/fentry.S

+                     0x00000000004006c2 <+6>:	mov    %rsp,%rbp
+                     0x00000000004006c5 <+9>:	mov    $0x0,%eax
+                     0x00000000004006ca <+14>:	callq  0x4006a6 <hello>
+   return address => 0x00000000004006cf <+19>:	nop


s/return address/parent addr/

namhyung · 2017-05-20T04:42:14Z

arch/x86_64/mcount.S

+                     0x00000000004006b5 <+4>:	callq  0x400520 <mcount@plt>
+                     0x00000000004006ba <+9>:	mov    $0x0,%eax
+                     0x00000000004006bf <+14>:	callq  0x400686 <hello>
+   return address => 0x00000000004006c4 <+19>:	nop


The comment about stack frame (with -pg, -mfentry) had wrong information and seems to be lack of explanation, so fix it and add examples. Reported-by: Namhyung Kim <namhyung@gmail.com> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>

Taeung · 2017-05-22T08:40:47Z

I'm late, I changed this PR !! , @namhyung

namhyung

LGTM

Taeung force-pushed the x86_64/mcount-comment branch from 55f36d7 to e673670 Compare May 16, 2017 05:20

namhyung requested changes May 16, 2017

View reviewed changes

Taeung force-pushed the x86_64/mcount-comment branch 2 times, most recently from 4aa4daf to 1c00c03 Compare May 16, 2017 09:39

namhyung reviewed May 17, 2017

View reviewed changes

Taeung force-pushed the x86_64/mcount-comment branch from 1c00c03 to dbb274f Compare May 17, 2017 01:50

Taeung force-pushed the x86_64/mcount-comment branch 2 times, most recently from e159775 to 86ad061 Compare May 17, 2017 08:35

namhyung reviewed May 17, 2017

View reviewed changes

Taeung force-pushed the x86_64/mcount-comment branch from 86ad061 to af3e1ca Compare May 19, 2017 07:07

namhyung reviewed May 20, 2017

View reviewed changes

Taeung force-pushed the x86_64/mcount-comment branch from af3e1ca to 5ba1a2b Compare May 22, 2017 08:36

Taeung force-pushed the x86_64/mcount-comment branch from 5ba1a2b to 5622b13 Compare May 22, 2017 08:39

namhyung approved these changes May 22, 2017

View reviewed changes

namhyung merged commit 19ac97e into namhyung:master May 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arch/x86: Fix comments about stack frame with -pg / -mfentry #96

arch/x86: Fix comments about stack frame with -pg / -mfentry #96

Taeung commented May 16, 2017

namhyung left a comment

Taeung commented May 16, 2017

namhyung May 17, 2017

Taeung May 17, 2017

Taeung May 17, 2017

namhyung May 17, 2017

Taeung commented May 17, 2017

namhyung May 17, 2017

Taeung May 17, 2017

namhyung May 17, 2017

Taeung May 17, 2017

Taeung May 17, 2017

namhyung May 17, 2017

Taeung May 17, 2017

namhyung May 17, 2017

namhyung May 17, 2017

namhyung May 17, 2017

Taeung May 17, 2017

namhyung May 17, 2017

Taeung May 18, 2017

namhyung May 19, 2017

namhyung May 17, 2017

namhyung May 17, 2017

Taeung commented May 19, 2017

namhyung May 20, 2017

namhyung May 20, 2017

Taeung commented May 22, 2017

namhyung left a comment

arch/x86: Fix comments about stack frame with -pg / -mfentry #96

arch/x86: Fix comments about stack frame with -pg / -mfentry #96

Conversation

Taeung commented May 16, 2017

namhyung left a comment

Choose a reason for hiding this comment

Taeung commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taeung commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taeung commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taeung commented May 22, 2017

namhyung left a comment

Choose a reason for hiding this comment