This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: PATCH: Update x86 stack align analyzer
- From: Mark Kettenis <mark dot kettenis at xs4all dot nl>
- To: hongjiu dot lu at intel dot com
- Cc: gdb-patches at sourceware dot org, hjl dot tools at gmail dot com, xuepeng dot guo at intel dot com, joey dot ye at intel dot com
- Date: Wed, 6 Aug 2008 23:05:01 +0200 (CEST)
- Subject: Re: PATCH: Update x86 stack align analyzer
- References: <20080730211152.GA9645@lucon.org>
> Date: Wed, 30 Jul 2008 14:11:52 -0700
> From: "H.J. Lu" <hongjiu.lu@intel.com>
>
> Gcc 4.4 revision 138335:
>
> http://gcc.gnu.org/ml/gcc-cvs/2008-07/msg01048.html
>
> introduced new ways to align stack. This patch teaches gdb how to
> recognize the new stack prologue. OK for trunk?
A few comments inline below.
> --- ../gdb/src/gdb/amd64-tdep.c 2008-07-16 22:30:04.000000000 -0700
> +++ gdb/gdb/amd64-tdep.c 2008-07-25 11:57:37.000000000 -0700
> @@ -680,6 +680,7 @@ struct amd64_frame_cache
> /* Saved registers. */
> CORE_ADDR saved_regs[AMD64_NUM_SAVED_REGS];
> CORE_ADDR saved_sp;
> + enum i386_regnum saved_sp_reg;
I suppose you meant 'enum amd64_regnum' here. I'd prefer if you'd
simply use 'int' as the type for registers instead of the enum.
That's what all the other code in GDB does.
> +/* GCC 4.4 and later, can put code in the prologue to realign the
> + stack pointer. Check whether PC points to such code, and update
> + CACHE accordingly. Return the first instruction after the code
> + sequence or CURRENT_PC, whichever is smaller. If we don't
> + recognize the code, return PC. */
There are older versions of GCC that use at least method #2, so this
comment is a bit misleading.
> +static CORE_ADDR
> +amd64_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
> + struct amd64_frame_cache *cache)
> +{
> + /* There are 3 code sequences to re-align stack:
> +
> + 1. Use %ebp:
> +
> + pushq %rbp
> + movq %rsp, %rbp
> + andq $-XXX, %rsp
But this is basically the standard prologue sequence. I don't see why
you need to handle this sequence here, since it is already handled in
amd64_analyze_prologue().
> + 2. Use a caller-saved saved register:
> +
> + leaq 8(%rsp), %reg
> + andq $-XXX, %rsp
> + pushq -8(%reg)
> +
> + 3. Use a callee-saved saved register:
> +
> + pushq %reg
> + leaq 16(%rsp), %reg
> + andq $-XXX, %rsp
> + pushq -8(%reg)
> +
> + "andq $-XXX, %rsp" can be either 4 bytes or 7 bytes:
> +
> + 0x48 0x83 0xe4 0xf0 andq $-16, %rsp
> + 0x48 0x81 0xe4 0x00 0xff 0xff 0xff andq $-256, %rsp
> + */
> +
> + gdb_byte buf[18];
> + int reg, r;
> + int offset, offset_and;
> + static enum amd64_regnum regnums[16] = {
> + AMD64_RAX_REGNUM, /* %rax */
> + AMD64_RCX_REGNUM, /* %rcx */
> + AMD64_RDX_REGNUM, /* %rdx */
> + AMD64_RBX_REGNUM, /* %rbx */
> + AMD64_RSP_REGNUM, /* %rsp */
> + AMD64_RBP_REGNUM, /* %rbp */
> + AMD64_RSI_REGNUM, /* %rsi */
> + AMD64_RDI_REGNUM, /* %rdi */
> + AMD64_R8_REGNUM, /* %r8 */
> + AMD64_R9_REGNUM, /* %r9 */
> + AMD64_R10_REGNUM, /* %r10 */
> + AMD64_R11_REGNUM, /* %r11 */
> + AMD64_R12_REGNUM, /* %r12 */
> + AMD64_R13_REGNUM, /* %r13 */
> + AMD64_R14_REGNUM, /* %r14 */
> + AMD64_R15_REGNUM, /* %r15 */
> + };
> +
> + if (target_read_memory (pc, buf, sizeof buf))
> + return pc;
> +
> + /* First check "pushq %rbp". */
> + if (buf[0] == 0x55)
> + {
> + /* The next instruction has to be "movq %rsp, %rbp". */
> + if (buf[1] != 0x48 || buf[2] != 0x89 || buf[3] != 0xe5)
> + return pc;
> +
> + /* The next instruction has to be "andq $-XXX, %rsp". */
> + if (buf[4] != 0x48
> + || buf[6] != 0xe4
> + || (buf[5] != 0x81 && buf[5] != 0x83))
> + return pc;
> +
> + if (current_pc > pc + 4)
> + cache->saved_sp_reg = AMD64_RBP_REGNUM;
> +
> + /* Keep the prologue for amd64_analyze_prologue. */
> + return pc;
> + }
> +
> + /* Check caller-saved saved register. The first instruction has
> + to be "leaq 8(%rsp), %reg". */
> + if ((buf[0] & 0xfb) == 0x48
> + && buf[1] == 0x8d
> + && buf[3] == 0x24
> + && buf[4] == 0x8)
> + {
> + /* MOD must be binary 10 and R/M must be binary 100. */
> + if ((buf[2] & 0xc7) != 0x44)
> + return pc;
> +
> + /* REG has register number. */
> + reg = (buf[2] >> 3) & 7;
> +
> + /* Check the REX.R bit. */
> + if (buf[0] == 0x4c)
> + reg += 8;
> +
> + offset = 5;
> + }
> + else
> + {
> + /* Check callee-saved saved register. The first instruction
> + has to be "pushq %reg". */
> + reg = 0;
> + if ((buf[0] & 0xf8) == 0x50)
> + offset = 0;
> + else if ((buf[0] & 0xf6) == 0x40
> + && (buf[1] & 0xf8) == 0x50)
> + {
> + /* Check the REX.B bit. */
> + if ((buf[0] & 1) != 0)
> + reg = 8;
> +
> + offset = 1;
> + }
> + else
> + return pc;
> +
> + /* Get register. */
> + reg += buf[offset] & 0x7;
> +
> + offset++;
> +
> + /* The next instruction has to be "leaq 16(%rsp), %reg". */
> + if ((buf[offset] & 0xfb) != 0x48
> + || buf[offset + 1] != 0x8d
> + || buf[offset + 3] != 0x24
> + || buf[offset + 4] != 0x10)
> + return pc;
> +
> + /* MOD must be binary 10 and R/M must be binary 100. */
> + if ((buf[offset + 2] & 0xc7) != 0x44)
> + return pc;
> +
> + /* REG has register number. */
> + r = (buf[offset + 2] >> 3) & 7;
> +
> + /* Check the REX.R bit. */
> + if (buf[offset] == 0x4c)
> + r += 8;
> +
> + /* Registers in pushq and leaq have to be the same. */
> + if (reg != r)
> + return pc;
> +
> + offset += 5;
> + }
> +
> + /* Rigister can't be %rsp nor %rbp. */
> + if (reg == 4 || reg == 5)
> + return pc;
> +
> + /* The next instruction has to be "andq $-XXX, %rsp". */
> + if (buf[offset] != 0x48
> + || buf[offset + 2] != 0xe4
> + || (buf[offset + 1] != 0x81 && buf[offset + 1] != 0x83))
> + return pc;
> +
> + offset_and = offset;
> + offset += buf[offset + 1] == 0x81 ? 7 : 4;
> +
> + /* The next instruction has to be "pushq -8(%reg)". */
> + r = 0;
> + if (buf[offset] == 0xff)
> + offset++;
> + else if ((buf[offset] & 0xf6) == 0x40
> + && buf[offset + 1] == 0xff)
> + {
> + /* Check the REX.B bit. */
> + if ((buf[offset] & 0x1) != 0)
> + r = 8;
> + offset += 2;
> + }
> + else
> + return pc;
> +
> + /* 8bit -8 is 0xf8. REG must be binary 110 and MOD must be binary
> + 01. */
> + if (buf[offset + 1] != 0xf8
> + || (buf[offset] & 0xf8) != 0x70)
> + return pc;
> +
> + /* R/M has register. */
> + r += buf[offset] & 7;
> +
> + /* Registers in leaq and pushq have to be the same. */
> + if (reg != r)
> + return pc;
> +
> + if (current_pc > pc + offset_and)
> + cache->saved_sp_reg = regnums[reg];
> +
> + return min (pc + offset + 2, current_pc);
> +}
> +
> /* Do a limited analysis of the prologue at PC and update CACHE
> accordingly. Bail out early if CURRENT_PC is reached. Return the
> address where the analysis stopped.
> @@ -742,6 +942,8 @@ amd64_analyze_prologue (CORE_ADDR pc, CO
> if (current_pc <= pc)
> return current_pc;
>
> + pc = amd64_analyze_stack_align (pc, current_pc, cache);
> +
> op = read_memory_unsigned_integer (pc, 1);
>
> if (op == 0x55) /* pushq %rbp */
> @@ -813,8 +1015,23 @@ amd64_frame_cache (struct frame_info *th
> at the stack pointer. For truly "frameless" functions this
> might work too. */
>
> - get_frame_register (this_frame, AMD64_RSP_REGNUM, buf);
> - cache->base = extract_unsigned_integer (buf, 8) + cache->sp_offset;
> + if (cache->saved_sp_reg != AMD64_RSP_REGNUM)
> + {
> + /* We're halfway aligning the stack. Saved stack pointer
> + has been saved in saved_sp_reg. */
> + get_frame_register (this_frame, cache->saved_sp_reg, buf);
> + cache->saved_sp = extract_unsigned_integer(buf, 8);
> + cache->base = ((cache->saved_sp - 8) & 0xfffffffffffffff0LL) - 8;
> + cache->saved_regs[AMD64_RIP_REGNUM] = cache->saved_sp - 8;
I don't see how this could possibly work if saved_regs[AMD64_RIP_REGNUM]
is set to 8 unconditionally a few lines further on.
> +
> + /* This will be added back below. */
> + cache->saved_regs[AMD64_RIP_REGNUM] -= cache->base;
> + }
> + else
> + {
> + get_frame_register (this_frame, AMD64_RSP_REGNUM, buf);
> + cache->base = extract_unsigned_integer (buf, 8) + cache->sp_offset;
> + }
> }
> else
> {
> diff -x .svn -upr ../gdb/src/gdb/i386-tdep.c gdb/gdb/i386-tdep.c
> --- ../gdb/src/gdb/i386-tdep.c 2008-07-16 22:30:17.000000000 -0700
> +++ gdb/gdb/i386-tdep.c 2008-07-18 17:42:03.000000000 -0700
> @@ -707,37 +707,134 @@ static CORE_ADDR
> i386_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
> struct i386_frame_cache *cache)
> {
> - /* The register used by the compiler to perform the stack re-alignment
> - is, in order of preference, either %ecx, %edx, or %eax. GCC should
> - never use %ebx as it always treats it as callee-saved, whereas
> - the compiler can only use caller-saved registers. */
> - static const gdb_byte insns_ecx[10] = {
> - 0x8d, 0x4c, 0x24, 0x04, /* leal 4(%esp), %ecx */
> - 0x83, 0xe4, 0xf0, /* andl $-16, %esp */
> - 0xff, 0x71, 0xfc /* pushl -4(%ecx) */
> - };
> - static const gdb_byte insns_edx[10] = {
> - 0x8d, 0x54, 0x24, 0x04, /* leal 4(%esp), %edx */
> - 0x83, 0xe4, 0xf0, /* andl $-16, %esp */
> - 0xff, 0x72, 0xfc /* pushl -4(%edx) */
> - };
> - static const gdb_byte insns_eax[10] = {
> - 0x8d, 0x44, 0x24, 0x04, /* leal 4(%esp), %eax */
> - 0x83, 0xe4, 0xf0, /* andl $-16, %esp */
> - 0xff, 0x70, 0xfc /* pushl -4(%eax) */
> + /* There are 3 code sequences to re-align stack:
> +
> + 1. Use %ebp:
> +
> + pushl %ebp
> + movl %esp, %ebp
> + andl $-XXX, %esp
See my comment about amd64.