From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18255 invoked by alias); 6 Aug 2008 21:06:23 -0000 Received: (qmail 18245 invoked by uid 22791); 6 Aug 2008 21:06:21 -0000 X-Spam-Check-By: sourceware.org Received: from sibelius.xs4all.nl (HELO sibelius.xs4all.nl) (82.92.89.47) by sourceware.org (qpsmtpd/0.31) with ESMTP; Wed, 06 Aug 2008 21:05:13 +0000 Received: from brahms.sibelius.xs4all.nl (kettenis@localhost.sibelius.xs4all.nl [127.0.0.1]) by brahms.sibelius.xs4all.nl (8.14.3/8.14.3) with ESMTP id m76L52xt016990; Wed, 6 Aug 2008 23:05:02 +0200 (CEST) Received: (from kettenis@localhost) by brahms.sibelius.xs4all.nl (8.14.3/8.14.3/Submit) id m76L51xN016378; Wed, 6 Aug 2008 23:05:01 +0200 (CEST) Date: Wed, 06 Aug 2008 21:06:00 -0000 Message-Id: <200808062105.m76L51xN016378@brahms.sibelius.xs4all.nl> From: Mark Kettenis To: hongjiu.lu@intel.com CC: gdb-patches@sourceware.org, hjl.tools@gmail.com, xuepeng.guo@intel.com, joey.ye@intel.com In-reply-to: <20080730211152.GA9645@lucon.org> (hongjiu.lu@intel.com) Subject: Re: PATCH: Update x86 stack align analyzer References: <20080730211152.GA9645@lucon.org> Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2008-08/txt/msg00141.txt.bz2 > Date: Wed, 30 Jul 2008 14:11:52 -0700 > From: "H.J. Lu" > > 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.