Re: [PATCH] AMD64, Prologue: Recognize stack decrementation as prologue operation.

[Bernhard Heckel <bernhard.heckel@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4439 bytes --]

On 01/12/2016 16:31, Luis Machado wrote:
> On 12/01/2016 08:16 AM, Bernhard Heckel wrote:
>> Some compiler decrement stack pointer within the prologue
>> sequence in order to reserve memory for local variables.
>> Recognize this subtraction to stop at the very end of the
>> prologue.
>
> I suppose this was exercised with GCC as well via the testsuite?
Yes
GCC,ICC and Clang 6.0 (llvm 3.5)

No regression with GCC nor with ICC.

But, there is a major issue when running with Clang.
Clang associate this "subtraction instruction" with the line after the 
prologue sequence.
This causes regressions on Mac.

I attached disassembly of Clang and GCC for the same program. ICC 
behaves like GCC.
I was trying to file a ticket for Clang, but I don't have access to 
bugzilla. Auto-registration
is not available and manual account registration is still ongoing.

>
>>
>> 2016-10-20  Bernhard Heckel  <bernhard.heckel@intel.com>
>>
>> gdb/Changelog:
>>     amd64-tdep.c (amd64_analyze_prologue): Recognize stack 
>> decrementation
>>     as prologue operation.
>
> gdb/ChangeLog above the date line, adjust date and add "*" before the 
> filename.
>
>>
>> ---
>>  gdb/amd64-tdep.c | 30 ++++++++++++++++++++++++++++++
>>  1 file changed, 30 insertions(+)
>>
>> diff --git a/gdb/amd64-tdep.c b/gdb/amd64-tdep.c
>> index a3a1fde..795d78e 100644
>> --- a/gdb/amd64-tdep.c
>> +++ b/gdb/amd64-tdep.c
>> @@ -2283,6 +2283,12 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>>    /* Ditto for movl %esp, %ebp.  */
>>    static const gdb_byte mov_esp_ebp_1[2] = { 0x89, 0xe5 };
>>    static const gdb_byte mov_esp_ebp_2[2] = { 0x8b, 0xec };
>> +  /* Ditto for subtraction on the stack pointer.  */
>> +  static const gdb_byte sub_rsp_imm8[3] = { 0x48, 0x83, 0xec };
>> +  static const gdb_byte sub_rsp_imm32[3] = { 0x48, 0x81, 0xec };
>> +  /* Ditto for subtraction on the stack pointer.  */
>> +  static const gdb_byte sub_esp_imm8[2] = { 0x83, 0xec };
>> +  static const gdb_byte sub_esp_imm32[2] = { 0x81, 0xec };
>
> Should we add a comment making it explicit which instruction patterns 
> we're looking at matching here?
You mean, adding it to the function description. There we have 
description for push and mov instruction.

>
> I looked up sub esp imm32, for example, and i got no meaningful hits 
> other than some nasm posix entry.
>
>>
>>    gdb_byte buf[3];
>>    gdb_byte op;
>> @@ -2316,6 +2322,18 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>>      {
>>        /* OK, we actually have a frame.  */
>>        cache->frameless_p = 0;
>> +
>> +      /* Some compiler do subtraction on the stack pointer
>> +         to reserve memory for local variables.
>> +         Two common variants exist to do so.  */
>
> What compiler exactly? Would be nice to know, otherwise this is a bit 
> vague.
Actually, GCC, ICC and Clang are using this approach.

>
> The comment seems to imply a specific compiler does this, or did you 
> mean "some compilers"?
>
>> +      read_code (pc + 4, buf, 3);
>> +      if (memcmp (buf, sub_rsp_imm8, 3) == 0)
>> +        /* Operand is 1 byte.  */
>> +        return pc + 8;
>> +      else if (memcmp (buf, sub_rsp_imm32, 3) == 0)
>> +        /* Operand is 4 bytes.  */
>> +        return pc + 11;
>> +
>>        return pc + 4;
>>      }
>>
>> @@ -2327,6 +2345,18 @@ amd64_analyze_prologue (struct gdbarch *gdbarch,
>>          {
>>            /* OK, we actually have a frame.  */
>>            cache->frameless_p = 0;
>> +
>> +          /* Some compiler do subtraction on the stack pointer
>> +         to reserve memory for local variables.
>> +         Two common variants exist to do so.  */
>> +          read_code (pc + 3, buf, 2);
>> +          if (memcmp (buf, sub_esp_imm8, 2) == 0)
>> +        /* Operand is 1 byte.  */
>> +        return pc + 6;
>> +          else if (memcmp (buf, sub_esp_imm32, 2) == 0)
>> +        /* Operand is 4 bytes.  */
>> +        return pc + 9;
>> +
>>            return pc + 3;
>>          }
>>      }
>>
>
> Otherwise LGTM.

Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[-- Attachment #2: disassembly_clang.txt --]
[-- Type: text/plain, Size: 496 bytes --]

disas /m caller1

9	{
   0x0000000100000f60 <+0>:	push   %rbp
   0x0000000100000f61 <+1>:	mov    %rsp,%rbp

10	  int i = 1;
   0x0000000100000f64 <+4>:	sub    $0x10,%rsp
   0x0000000100000f68 <+8>:	movl   $0x1,-0x4(%rbp)

11	  int b = 2;
   0x0000000100000f6f <+15>:	movl   $0x2,-0x8(%rbp)

12	  caller2();
   0x0000000100000f76 <+22>:	callq  0x100000f50 <caller2>

13	}
   0x0000000100000f7b <+27>:	add    $0x10,%rsp
   0x0000000100000f7f <+31>:	pop    %rbp
   0x0000000100000f80 <+32>:	retq   

[-- Attachment #3: disassembly_gcc.txt --]
[-- Type: text/plain, Size: 443 bytes --]

disas /m caller1

9	{
   0x00000000004004fd <+0>:	push   %rbp
   0x00000000004004fe <+1>:	mov    %rsp,%rbp
   0x0000000000400501 <+4>:	sub    $0x10,%rsp

10	  int i = 1;
   0x0000000000400505 <+8>:	movl   $0x1,-0x4(%rbp)

11	  int b = 2;
   0x000000000040050c <+15>:	movl   $0x2,-0x8(%rbp)

12	  caller2();
   0x0000000000400513 <+22>:	callq  0x4004f0 <caller2>

13	}
   0x0000000000400518 <+27>:	leaveq 
   0x0000000000400519 <+28>:	retq   


[-- Attachment #4: test.c --]
[-- Type: text/plain, Size: 131 bytes --]

void
caller2 (void)
{
  int i = 1;
}

void
caller1 (void)
{
  int i = 1;
  int b = 2;
  caller2();
}

int main ()
{
  caller1();
}