Re: PATCH: Update x86 stack align analyzer

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Aug 6, 2008 at 2:05 PM, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>
>> 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.

Thanks for you review.

>
>> --- ../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.
>

Done.

>> +/* 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.

Gcc 4.1 can align stack to 16byte, but only for ia32, not for x86-64. We
added this capability to gcc 4.4.

>
>> +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().

We need it to set saved_sp_reg.

>
>> -      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.

I changed it not to set %rip to 8 when halfway aligning the stack. That
is similar to i386 code.

>
> See my comment about amd64.
>

I changed to int.

I am enclosed my updated patch.

Thanks.


-- 
H.J.

Attachment: gdb-stack-3.patch.txt
Description: Text document