Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1709039 > unrolled thread

[PATCH 2/2] objtool: Track DRAP separately from callee-saved registers

Started byJosh Poimboeuf <jpoimboe@redhat.com>
First post2017-08-10 23:40 +0200
Last post2017-08-11 19:40 +0200
Articles 6 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] objtool: Track DRAP separately from callee-saved registers Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-10 23:40 +0200
    [tip:x86/asm] objtool: Track DRAP separately from callee-saved  registers tip-bot for Josh Poimboeuf <tipbot@zytor.com> - 2017-08-11 14:20 +0200
      Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers Andy Lutomirski <luto@kernel.org> - 2017-08-11 18:30 +0200
        Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-11 18:30 +0200
        Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved  registers Josh Poimboeuf <jpoimboe@redhat.com> - 2017-08-11 19:00 +0200
          Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers hpa@zytor.com - 2017-08-11 19:40 +0200

#1709039 — [PATCH 2/2] objtool: Track DRAP separately from callee-saved registers

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-08-10 23:40 +0200
Subject[PATCH 2/2] objtool: Track DRAP separately from callee-saved registers
Message-ID<ud3lo-31s-25@gated-at.bofh.it>
When GCC realigns a function's stack, it sometimes uses %r13 as the DRAP
register, like:

  push	%r13
  lea	0x10(%rsp), %r13
  and	$0xfffffffffffffff0, %rsp
  pushq	-0x8(%r13)
  push	%rbp
  mov	%rsp, %rbp
  push	%r13
  ...
  mov	-0x8(%rbp),%r13
  leaveq
  lea	-0x10(%r13), %rsp
  pop	%r13
  retq

Since %r13 was pushed onto the stack twice, its two stack locations need
to be stored separately.  The first push of %r13 is its original value,
and the second push of %r13 is the caller's stack frame address.

Since %r13 is a callee-saved register, we need to track the stack
location of its original value separately from the DRAP register.

This fixes the following false positive warning:

  lib/ubsan.o: warning: objtool: val_to_string.constprop.7()+0x97: leave instruction with modified stack frame

Reported-by: Arnd Bergmann <arnd@arndb.de>
Fixes: baa41469a7b9 ("objtool: Implement stack validation 2.0")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c | 54 ++++++++++++++++++++++++++++-----------------------
 tools/objtool/check.h |  2 +-
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5814e907f8c2..17375925e7aa 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -221,6 +221,7 @@ static void clear_insn_state(struct insn_state *state)
 	for (i = 0; i < CFI_NUM_REGS; i++)
 		state->regs[i].base = CFI_UNDEFINED;
 	state->drap_reg = CFI_UNDEFINED;
+	state->drap_offset = -1;
 }
 
 /*
@@ -1110,8 +1111,7 @@ static int update_insn_state_regs(struct instruction *insn, struct insn_state *s
 static void save_reg(struct insn_state *state, unsigned char reg, int base,
 		     int offset)
 {
-	if ((arch_callee_saved_reg(reg) ||
-	    (state->drap && reg == state->drap_reg)) &&
+	if (arch_callee_saved_reg(reg) &&
 	    state->regs[reg].base == CFI_UNDEFINED) {
 		state->regs[reg].base = base;
 		state->regs[reg].offset = offset;
@@ -1281,7 +1281,6 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = state->drap_reg;
 				cfa->offset = state->stack_size = 0;
 				state->drap = true;
-
 			}
 
 			/*
@@ -1299,17 +1298,19 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = CFI_SP;
 			}
 
-			if (regs[op->dest.reg].offset == -state->stack_size) {
+			if (state->drap && cfa->base == CFI_BP_INDIRECT &&
+			    op->dest.type == OP_DEST_REG &&
+			    op->dest.reg == state->drap_reg &&
+			    state->drap_offset == -state->stack_size) {
 
-				if (state->drap && cfa->base == CFI_BP_INDIRECT &&
-				    op->dest.type == OP_DEST_REG &&
-				    op->dest.reg == state->drap_reg) {
+				/* drap: pop %drap */
+				cfa->base = state->drap_reg;
+				cfa->offset = 0;
+				state->drap_offset = -1;
 
-					/* drap: pop %drap */
-					cfa->base = state->drap_reg;
-					cfa->offset = 0;
-				}
+			} else if (regs[op->dest.reg].offset == -state->stack_size) {
 
+				/* pop %reg */
 				restore_reg(state, op->dest.reg);
 			}
 
@@ -1321,14 +1322,18 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 
 		case OP_SRC_REG_INDIRECT:
 			if (state->drap && op->src.reg == CFI_BP &&
+			    op->src.offset == state->drap_offset) {
+
+				/* drap: mov disp(%rbp), %drap */
+				cfa->base = state->drap_reg;
+				cfa->offset = 0;
+				state->drap_offset = -1;
+			}
+
+			if (state->drap && op->src.reg == CFI_BP &&
 			    op->src.offset == regs[op->dest.reg].offset) {
 
 				/* drap: mov disp(%rbp), %reg */
-				if (op->dest.reg == state->drap_reg) {
-					cfa->base = state->drap_reg;
-					cfa->offset = 0;
-				}
-
 				restore_reg(state, op->dest.reg);
 
 			} else if (op->src.reg == cfa->base &&
@@ -1364,8 +1369,8 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = CFI_BP_INDIRECT;
 				cfa->offset = -state->stack_size;
 
-				/* save drap so we know when to undefine it */
-				save_reg(state, op->src.reg, CFI_CFA, -state->stack_size);
+				/* save drap so we know when to restore it */
+				state->drap_offset = -state->stack_size;
 
 			} else if (op->src.reg == CFI_BP && cfa->base == state->drap_reg) {
 
@@ -1399,8 +1404,8 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = CFI_BP_INDIRECT;
 				cfa->offset = op->dest.offset;
 
-				/* save drap so we know when to undefine it */
-				save_reg(state, op->src.reg, CFI_CFA, op->dest.offset);
+				/* save drap offset so we know when to restore it */
+				state->drap_offset = op->dest.offset;
 			}
 
 			else if (regs[op->src.reg].base == CFI_UNDEFINED) {
@@ -1491,11 +1496,12 @@ static bool insn_state_match(struct instruction *insn, struct insn_state *state)
 			  insn->sec, insn->offset, state1->type, state2->type);
 
 	} else if (state1->drap != state2->drap ||
-		 (state1->drap && state1->drap_reg != state2->drap_reg)) {
-		WARN_FUNC("stack state mismatch: drap1=%d(%d) drap2=%d(%d)",
+		 (state1->drap && state1->drap_reg != state2->drap_reg) ||
+		 (state1->drap && state1->drap_offset != state2->drap_offset)) {
+		WARN_FUNC("stack state mismatch: drap1=%d(%d,%d) drap2=%d(%d,%d)",
 			  insn->sec, insn->offset,
-			  state1->drap, state1->drap_reg,
-			  state2->drap, state2->drap_reg);
+			  state1->drap, state1->drap_reg, state1->drap_offset,
+			  state2->drap, state2->drap_reg, state2->drap_offset);
 
 	} else
 		return true;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index c9af11f0c8af..9f113016bf8c 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -32,7 +32,7 @@ struct insn_state {
 	unsigned char type;
 	bool bp_scratch;
 	bool drap;
-	int drap_reg;
+	int drap_reg, drap_offset;
 };
 
 struct instruction {
-- 
2.13.3

[toc] | [next] | [standalone]

#1709523 — [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers

Fromtip-bot for Josh Poimboeuf <tipbot@zytor.com>
Date2017-08-11 14:20 +0200
Subject[tip:x86/asm] objtool: Track DRAP separately from callee-saved registers
Message-ID<udh50-3qa-3@gated-at.bofh.it>
In reply to#1709039
Commit-ID:  bf4d1a83758368c842c94cab9661a75ca98bc848
Gitweb:     http://git.kernel.org/tip/bf4d1a83758368c842c94cab9661a75ca98bc848
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Thu, 10 Aug 2017 16:37:26 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 11 Aug 2017 14:06:15 +0200

objtool: Track DRAP separately from callee-saved registers

When GCC realigns a function's stack, it sometimes uses %r13 as the DRAP
register, like:

  push	%r13
  lea	0x10(%rsp), %r13
  and	$0xfffffffffffffff0, %rsp
  pushq	-0x8(%r13)
  push	%rbp
  mov	%rsp, %rbp
  push	%r13
  ...
  mov	-0x8(%rbp),%r13
  leaveq
  lea	-0x10(%r13), %rsp
  pop	%r13
  retq

Since %r13 was pushed onto the stack twice, its two stack locations need
to be stored separately.  The first push of %r13 is its original value,
and the second push of %r13 is the caller's stack frame address.

Since %r13 is a callee-saved register, we need to track the stack
location of its original value separately from the DRAP register.

This fixes the following false positive warning:

  lib/ubsan.o: warning: objtool: val_to_string.constprop.7()+0x97: leave instruction with modified stack frame

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Fixes: baa41469a7b9 ("objtool: Implement stack validation 2.0")
Link: http://lkml.kernel.org/r/3da23a6d4c5b3c1e21fc2ccc21a73941b97ff20a.1502401017.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 tools/objtool/check.c | 54 ++++++++++++++++++++++++++++-----------------------
 tools/objtool/check.h |  2 +-
 2 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 5814e90..1737592 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -221,6 +221,7 @@ static void clear_insn_state(struct insn_state *state)
 	for (i = 0; i < CFI_NUM_REGS; i++)
 		state->regs[i].base = CFI_UNDEFINED;
 	state->drap_reg = CFI_UNDEFINED;
+	state->drap_offset = -1;
 }
 
 /*
@@ -1110,8 +1111,7 @@ static int update_insn_state_regs(struct instruction *insn, struct insn_state *s
 static void save_reg(struct insn_state *state, unsigned char reg, int base,
 		     int offset)
 {
-	if ((arch_callee_saved_reg(reg) ||
-	    (state->drap && reg == state->drap_reg)) &&
+	if (arch_callee_saved_reg(reg) &&
 	    state->regs[reg].base == CFI_UNDEFINED) {
 		state->regs[reg].base = base;
 		state->regs[reg].offset = offset;
@@ -1281,7 +1281,6 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = state->drap_reg;
 				cfa->offset = state->stack_size = 0;
 				state->drap = true;
-
 			}
 
 			/*
@@ -1299,17 +1298,19 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = CFI_SP;
 			}
 
-			if (regs[op->dest.reg].offset == -state->stack_size) {
+			if (state->drap && cfa->base == CFI_BP_INDIRECT &&
+			    op->dest.type == OP_DEST_REG &&
+			    op->dest.reg == state->drap_reg &&
+			    state->drap_offset == -state->stack_size) {
 
-				if (state->drap && cfa->base == CFI_BP_INDIRECT &&
-				    op->dest.type == OP_DEST_REG &&
-				    op->dest.reg == state->drap_reg) {
+				/* drap: pop %drap */
+				cfa->base = state->drap_reg;
+				cfa->offset = 0;
+				state->drap_offset = -1;
 
-					/* drap: pop %drap */
-					cfa->base = state->drap_reg;
-					cfa->offset = 0;
-				}
+			} else if (regs[op->dest.reg].offset == -state->stack_size) {
 
+				/* pop %reg */
 				restore_reg(state, op->dest.reg);
 			}
 
@@ -1321,14 +1322,18 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 
 		case OP_SRC_REG_INDIRECT:
 			if (state->drap && op->src.reg == CFI_BP &&
+			    op->src.offset == state->drap_offset) {
+
+				/* drap: mov disp(%rbp), %drap */
+				cfa->base = state->drap_reg;
+				cfa->offset = 0;
+				state->drap_offset = -1;
+			}
+
+			if (state->drap && op->src.reg == CFI_BP &&
 			    op->src.offset == regs[op->dest.reg].offset) {
 
 				/* drap: mov disp(%rbp), %reg */
-				if (op->dest.reg == state->drap_reg) {
-					cfa->base = state->drap_reg;
-					cfa->offset = 0;
-				}
-
 				restore_reg(state, op->dest.reg);
 
 			} else if (op->src.reg == cfa->base &&
@@ -1364,8 +1369,8 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = CFI_BP_INDIRECT;
 				cfa->offset = -state->stack_size;
 
-				/* save drap so we know when to undefine it */
-				save_reg(state, op->src.reg, CFI_CFA, -state->stack_size);
+				/* save drap so we know when to restore it */
+				state->drap_offset = -state->stack_size;
 
 			} else if (op->src.reg == CFI_BP && cfa->base == state->drap_reg) {
 
@@ -1399,8 +1404,8 @@ static int update_insn_state(struct instruction *insn, struct insn_state *state)
 				cfa->base = CFI_BP_INDIRECT;
 				cfa->offset = op->dest.offset;
 
-				/* save drap so we know when to undefine it */
-				save_reg(state, op->src.reg, CFI_CFA, op->dest.offset);
+				/* save drap offset so we know when to restore it */
+				state->drap_offset = op->dest.offset;
 			}
 
 			else if (regs[op->src.reg].base == CFI_UNDEFINED) {
@@ -1491,11 +1496,12 @@ static bool insn_state_match(struct instruction *insn, struct insn_state *state)
 			  insn->sec, insn->offset, state1->type, state2->type);
 
 	} else if (state1->drap != state2->drap ||
-		 (state1->drap && state1->drap_reg != state2->drap_reg)) {
-		WARN_FUNC("stack state mismatch: drap1=%d(%d) drap2=%d(%d)",
+		 (state1->drap && state1->drap_reg != state2->drap_reg) ||
+		 (state1->drap && state1->drap_offset != state2->drap_offset)) {
+		WARN_FUNC("stack state mismatch: drap1=%d(%d,%d) drap2=%d(%d,%d)",
 			  insn->sec, insn->offset,
-			  state1->drap, state1->drap_reg,
-			  state2->drap, state2->drap_reg);
+			  state1->drap, state1->drap_reg, state1->drap_offset,
+			  state2->drap, state2->drap_reg, state2->drap_offset);
 
 	} else
 		return true;
diff --git a/tools/objtool/check.h b/tools/objtool/check.h
index c9af11f..9f11301 100644
--- a/tools/objtool/check.h
+++ b/tools/objtool/check.h
@@ -32,7 +32,7 @@ struct insn_state {
 	unsigned char type;
 	bool bp_scratch;
 	bool drap;
-	int drap_reg;
+	int drap_reg, drap_offset;
 };
 
 struct instruction {

[toc] | [prev] | [next] | [standalone]

#1709817 — Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers

FromAndy Lutomirski <luto@kernel.org>
Date2017-08-11 18:30 +0200
SubjectRe: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers
Message-ID<udkYW-5NJ-23@gated-at.bofh.it>
In reply to#1709523
On Fri, Aug 11, 2017 at 5:13 AM, tip-bot for Josh Poimboeuf
<tipbot@zytor.com> wrote:
> Commit-ID:  bf4d1a83758368c842c94cab9661a75ca98bc848
> Gitweb:     http://git.kernel.org/tip/bf4d1a83758368c842c94cab9661a75ca98bc848
> Author:     Josh Poimboeuf <jpoimboe@redhat.com>
> AuthorDate: Thu, 10 Aug 2017 16:37:26 -0500
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Fri, 11 Aug 2017 14:06:15 +0200
>
> objtool: Track DRAP separately from callee-saved registers
>
> When GCC realigns a function's stack, it sometimes uses %r13 as the DRAP
> register, like:
>
>   push  %r13
>   lea   0x10(%rsp), %r13
>   and   $0xfffffffffffffff0, %rsp
>   pushq -0x8(%r13)
>   push  %rbp
>   mov   %rsp, %rbp
>   push  %r13
>   ...
>   mov   -0x8(%rbp),%r13
>   leaveq
>   lea   -0x10(%r13), %rsp
>   pop   %r13
>   retq
>

I have a couple questions, mainly to help me understand.

Question 1: What does DRAP stand for?  Duplicate Return Address
Pointer?  Dynamic ReAlignment Pointer?  I tried searching and got
nothing.

Question 2: What's up with the resulting stack layout?  It seems we have:

caller's last stack slot  <-- r13 in function body points here
return address
old r13
[possible padding for alignment]
return address, duplicated (for naive unwinder's benefit?)
old rbp  <-- rbp in body points here
new r13, i.e. pointer to caller's last stack slot

Now we have the function body, and r13 is free for use in here because
it's saved.

In the epilogue, we recover r13, use leaveq (hmm, shorter than pop
%rbp but does more work than needed), restore the old r13, and return.

I don't get it, though.  gcc only ever uses that inner r13 with an
offset.  The code would be considerably shorter if the second
instruction were just mov %rsp, %r13.  That would change the push to
pushq 0x8(%rsp) and the third-to-last instruction to mov %r13, %rsp,
saving something like 8 bytes of code.

I also don't get why any of this is needed.  Couldn't the compiler
just do push %rbp; mov %rsp, %rbp; and $0xfffffffffffffff0, %rsp and
be done with it?

I compiled this:

void func()
{
    int var __attribute__((aligned(32)));
    asm volatile ("" :: "m" (var));
}

and got:

func:
    leaq    8(%rsp), %r10
    andq    $-32, %rsp
    pushq    -8(%r10)
    pushq    %rbp
    movq    %rsp, %rbp
    pushq    %r10
    popq    %r10
    popq    %rbp
    leaq    -8(%r10), %rsp
    ret

Which is better than the crud you pasted, since it at least uses a
caller-saved reg (r10), but we still have the nasty addressing modes
*and* an unnecessary push and pop of r10.

I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81825 and maybe
some GCC person has a clue what's going on.

[toc] | [prev] | [next] | [standalone]

#1709821 — Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers

FromLinus Torvalds <torvalds@linux-foundation.org>
Date2017-08-11 18:30 +0200
SubjectRe: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers
Message-ID<udkYW-5NJ-35@gated-at.bofh.it>
In reply to#1709817
On Fri, Aug 11, 2017 at 9:22 AM, Andy Lutomirski <luto@kernel.org> wrote:
>
> Question 1: What does DRAP stand for?  Duplicate Return Address
> Pointer?  Dynamic ReAlignment Pointer?  I tried searching and got
> nothing.

I think it's "Dynamic Re-Alignment" register, but I may have just made that up.

                   Linus

[toc] | [prev] | [next] | [standalone]

#1709848 — Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers

FromJosh Poimboeuf <jpoimboe@redhat.com>
Date2017-08-11 19:00 +0200
SubjectRe: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers
Message-ID<udlrY-5XG-19@gated-at.bofh.it>
In reply to#1709817
On Fri, Aug 11, 2017 at 09:22:11AM -0700, Andy Lutomirski wrote:
> On Fri, Aug 11, 2017 at 5:13 AM, tip-bot for Josh Poimboeuf
> <tipbot@zytor.com> wrote:
> > Commit-ID:  bf4d1a83758368c842c94cab9661a75ca98bc848
> > Gitweb:     http://git.kernel.org/tip/bf4d1a83758368c842c94cab9661a75ca98bc848
> > Author:     Josh Poimboeuf <jpoimboe@redhat.com>
> > AuthorDate: Thu, 10 Aug 2017 16:37:26 -0500
> > Committer:  Ingo Molnar <mingo@kernel.org>
> > CommitDate: Fri, 11 Aug 2017 14:06:15 +0200
> >
> > objtool: Track DRAP separately from callee-saved registers
> >
> > When GCC realigns a function's stack, it sometimes uses %r13 as the DRAP
> > register, like:
> >
> >   push  %r13
> >   lea   0x10(%rsp), %r13
> >   and   $0xfffffffffffffff0, %rsp
> >   pushq -0x8(%r13)
> >   push  %rbp
> >   mov   %rsp, %rbp
> >   push  %r13
> >   ...
> >   mov   -0x8(%rbp),%r13
> >   leaveq
> >   lea   -0x10(%r13), %rsp
> >   pop   %r13
> >   retq
> >
> 
> I have a couple questions, mainly to help me understand.
> 
> Question 1: What does DRAP stand for?  Duplicate Return Address
> Pointer?  Dynamic ReAlignment Pointer?  I tried searching and got
> nothing.

It seems to be a GCC invention which stands for:

  Dynamic Realign Argument Pointer.

I don't think it's documented anywhere, but there's at least some
comments about it in the GCC sources if you search for DRAP.

> Question 2: What's up with the resulting stack layout?  It seems we have:
> 
> caller's last stack slot  <-- r13 in function body points here
> return address
> old r13
> [possible padding for alignment]
> return address, duplicated (for naive unwinder's benefit?)
> old rbp  <-- rbp in body points here
> new r13, i.e. pointer to caller's last stack slot
> 
> Now we have the function body, and r13 is free for use in here because
> it's saved.
> 
> In the epilogue, we recover r13, use leaveq (hmm, shorter than pop
> %rbp but does more work than needed), restore the old r13, and return.
> 
> I don't get it, though.  gcc only ever uses that inner r13 with an
> offset.  The code would be considerably shorter if the second
> instruction were just mov %rsp, %r13.  That would change the push to
> pushq 0x8(%rsp) and the third-to-last instruction to mov %r13, %rsp,
> saving something like 8 bytes of code.

I don't know why it doesn't do it the way you suggest, but I'm glad it
doesn't because I think it would make the DWARF/ORC data even more
complicated.  Here it's "simple", because r13 == DWARF CFA.

> I also don't get why any of this is needed.  Couldn't the compiler
> just do push %rbp; mov %rsp, %rbp; and $0xfffffffffffffff0, %rsp and
> be done with it?

Good question.  I wish it did just use the frame pointer, because
dealing with DRAP has been a headache.

> I compiled this:
> 
> void func()
> {
>     int var __attribute__((aligned(32)));
>     asm volatile ("" :: "m" (var));
> }
> 
> and got:
> 
> func:
>     leaq    8(%rsp), %r10
>     andq    $-32, %rsp
>     pushq    -8(%r10)
>     pushq    %rbp
>     movq    %rsp, %rbp
>     pushq    %r10
>     popq    %r10
>     popq    %rbp
>     leaq    -8(%r10), %rsp
>     ret
> 
> Which is better than the crud you pasted, since it at least uses a
> caller-saved reg (r10), but we still have the nasty addressing modes
> *and* an unnecessary push and pop of r10.
> 
> I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81825 and maybe
> some GCC person has a clue what's going on.

I've found that, when it does this DRAP pattern, most of the time it
uses r10.  The r13 version seems to be more rare.  I can provide a
real-world r13 example if that would help.

-- 
Josh

[toc] | [prev] | [next] | [standalone]

#1709885 — Re: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers

Fromhpa@zytor.com
Date2017-08-11 19:40 +0200
SubjectRe: [tip:x86/asm] objtool: Track DRAP separately from callee-saved registers
Message-ID<udm4G-6pu-21@gated-at.bofh.it>
In reply to#1709848
On August 11, 2017 9:57:13 AM PDT, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
>On Fri, Aug 11, 2017 at 09:22:11AM -0700, Andy Lutomirski wrote:
>> On Fri, Aug 11, 2017 at 5:13 AM, tip-bot for Josh Poimboeuf
>> <tipbot@zytor.com> wrote:
>> > Commit-ID:  bf4d1a83758368c842c94cab9661a75ca98bc848
>> > Gitweb:    
>http://git.kernel.org/tip/bf4d1a83758368c842c94cab9661a75ca98bc848
>> > Author:     Josh Poimboeuf <jpoimboe@redhat.com>
>> > AuthorDate: Thu, 10 Aug 2017 16:37:26 -0500
>> > Committer:  Ingo Molnar <mingo@kernel.org>
>> > CommitDate: Fri, 11 Aug 2017 14:06:15 +0200
>> >
>> > objtool: Track DRAP separately from callee-saved registers
>> >
>> > When GCC realigns a function's stack, it sometimes uses %r13 as the
>DRAP
>> > register, like:
>> >
>> >   push  %r13
>> >   lea   0x10(%rsp), %r13
>> >   and   $0xfffffffffffffff0, %rsp
>> >   pushq -0x8(%r13)
>> >   push  %rbp
>> >   mov   %rsp, %rbp
>> >   push  %r13
>> >   ...
>> >   mov   -0x8(%rbp),%r13
>> >   leaveq
>> >   lea   -0x10(%r13), %rsp
>> >   pop   %r13
>> >   retq
>> >
>> 
>> I have a couple questions, mainly to help me understand.
>> 
>> Question 1: What does DRAP stand for?  Duplicate Return Address
>> Pointer?  Dynamic ReAlignment Pointer?  I tried searching and got
>> nothing.
>
>It seems to be a GCC invention which stands for:
>
>  Dynamic Realign Argument Pointer.
>
>I don't think it's documented anywhere, but there's at least some
>comments about it in the GCC sources if you search for DRAP.
>
>> Question 2: What's up with the resulting stack layout?  It seems we
>have:
>> 
>> caller's last stack slot  <-- r13 in function body points here
>> return address
>> old r13
>> [possible padding for alignment]
>> return address, duplicated (for naive unwinder's benefit?)
>> old rbp  <-- rbp in body points here
>> new r13, i.e. pointer to caller's last stack slot
>> 
>> Now we have the function body, and r13 is free for use in here
>because
>> it's saved.
>> 
>> In the epilogue, we recover r13, use leaveq (hmm, shorter than pop
>> %rbp but does more work than needed), restore the old r13, and
>return.
>> 
>> I don't get it, though.  gcc only ever uses that inner r13 with an
>> offset.  The code would be considerably shorter if the second
>> instruction were just mov %rsp, %r13.  That would change the push to
>> pushq 0x8(%rsp) and the third-to-last instruction to mov %r13, %rsp,
>> saving something like 8 bytes of code.
>
>I don't know why it doesn't do it the way you suggest, but I'm glad it
>doesn't because I think it would make the DWARF/ORC data even more
>complicated.  Here it's "simple", because r13 == DWARF CFA.
>
>> I also don't get why any of this is needed.  Couldn't the compiler
>> just do push %rbp; mov %rsp, %rbp; and $0xfffffffffffffff0, %rsp and
>> be done with it?
>
>Good question.  I wish it did just use the frame pointer, because
>dealing with DRAP has been a headache.
>
>> I compiled this:
>> 
>> void func()
>> {
>>     int var __attribute__((aligned(32)));
>>     asm volatile ("" :: "m" (var));
>> }
>> 
>> and got:
>> 
>> func:
>>     leaq    8(%rsp), %r10
>>     andq    $-32, %rsp
>>     pushq    -8(%r10)
>>     pushq    %rbp
>>     movq    %rsp, %rbp
>>     pushq    %r10
>>     popq    %r10
>>     popq    %rbp
>>     leaq    -8(%r10), %rsp
>>     ret
>> 
>> Which is better than the crud you pasted, since it at least uses a
>> caller-saved reg (r10), but we still have the nasty addressing modes
>> *and* an unnecessary push and pop of r10.
>> 
>> I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81825 and maybe
>> some GCC person has a clue what's going on.
>
>I've found that, when it does this DRAP pattern, most of the time it
>uses r10.  The r13 version seems to be more rare.  I can provide a
>real-world r13 example if that would help.

One could logically assume %r10 if a clobbered register is sufficient.  It would make sense to do that renaming fairly late in the game.
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web