[PATCH 1/2] objtool: Fix validate_branch() return codes

[PATCH 1/2] objtool: Fix validate_branch() return codes

[Josh Poimboeuf]

The validate_branch() function should never return a negative value.
Errors are treated as warnings so that even if something goes wrong,
objtool does its best to generate ORC data for the rest of the file.

Fixes: baa41469a7b9 ("objtool: Implement stack validation 2.0")
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 tools/objtool/check.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4f0c4aea8f6f..5814e907f8c2 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1524,7 +1524,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 	if (insn->alt_group && list_empty(&insn->alts)) {
 		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
 			  sec, insn->offset);
-		return -1;
+		return 1;
 	}
 
 	while (1) {
@@ -1543,7 +1543,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 		if (func && insn->ignore) {
 			WARN_FUNC("BUG: why am I validating an ignored function?",
 				  sec, insn->offset);
-			return -1;
+			return 1;
 		}
 
 		if (insn->visited) {
@@ -1681,7 +1681,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 
 		case INSN_STACK:
 			if (update_insn_state(insn, &state))
-				return -1;
+				return 1;
 
 			break;
 
-- 
2.13.3

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

[Josh Poimboeuf]

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

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

[tip-bot for Josh Poimboeuf]

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 {

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

[Andy Lutomirski]

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.

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

[Linus Torvalds]

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

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

[Josh Poimboeuf]

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

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

[hpa]

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.

[tip:x86/asm] objtool: Fix validate_branch() return codes

[tip-bot for Josh Poimboeuf]

Commit-ID:  12b25729a194198e3c4289adaddc4115b10b094e
Gitweb:     http://git.kernel.org/tip/12b25729a194198e3c4289adaddc4115b10b094e
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Thu, 10 Aug 2017 16:37:25 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 11 Aug 2017 14:06:14 +0200

objtool: Fix validate_branch() return codes

The validate_branch() function should never return a negative value.
Errors are treated as warnings so that even if something goes wrong,
objtool does its best to generate ORC data for the rest of the file.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
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/d86671cfde823b50477cd2f6f548dfe54871e24d.1502401017.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 tools/objtool/check.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/objtool/check.c b/tools/objtool/check.c
index 4f0c4ae..5814e90 100644
--- a/tools/objtool/check.c
+++ b/tools/objtool/check.c
@@ -1524,7 +1524,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 	if (insn->alt_group && list_empty(&insn->alts)) {
 		WARN_FUNC("don't know how to handle branch to middle of alternative instruction group",
 			  sec, insn->offset);
-		return -1;
+		return 1;
 	}
 
 	while (1) {
@@ -1543,7 +1543,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 		if (func && insn->ignore) {
 			WARN_FUNC("BUG: why am I validating an ignored function?",
 				  sec, insn->offset);
-			return -1;
+			return 1;
 		}
 
 		if (insn->visited) {
@@ -1681,7 +1681,7 @@ static int validate_branch(struct objtool_file *file, struct instruction *first,
 
 		case INSN_STACK:
 			if (update_insn_state(insn, &state))
-				return -1;
+				return 1;
 
 			break;