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

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

[Bernhard Heckel]

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.

2016-10-20  Bernhard Heckel  <bernhard.heckel@intel.com>

gdb/Changelog:
	amd64-tdep.c (amd64_analyze_prologue): Recognize stack decrementation
	as prologue operation.

---
 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 };
 
   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.  */
+	  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;
 	    }
 	}
-- 
2.7.1.339.g0233b80

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

[Luis Machado]

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?

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

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.

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.

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

[Bernhard Heckel]

[-- 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();
}

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

[Luis Machado]

On 12/02/2016 02:40 AM, Bernhard Heckel wrote:
> 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.
>

To add it to these sub_[esp|rsp|_imm* bits, if meaningful. I don't know 
if these are documented/used somewhere else in gdb. Just a suggestion 
that could improve visual identification of such instructions when going 
through the prologue in disassembly view.

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

I guess you'd want "some compilers" then.

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

[Yao Qi]

On 16-12-01 15:16:44, Bernhard Heckel wrote:
> Some compiler decrement stack pointer within the prologue

As Luis reviewed, it is clear to mention the name of the compilers
here.

> sequence in order to reserve memory for local variables.
> Recognize this subtraction to stop at the very end of the
> prologue.
> 
> 2016-10-20  Bernhard Heckel  <bernhard.heckel@intel.com>
> 
> gdb/Changelog:
> 	amd64-tdep.c (amd64_analyze_prologue): Recognize stack decrementation

File name should be started with "*".

> 	as prologue operation.
> 
> ---
>  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 };
>  
>    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

Please mention the name of the compilers here.

> +	     to reserve memory for local variables.
> +	     Two common variants exist to do so.  */
> +	  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

Likewise.

> +		 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;
> +

Could you add a unit test for this new prologue sequences?  Like what we
did in this patch
https://sourceware.org/ml/gdb-patches/2016-12/msg00071.html  Both x86_64
and x32 prologue should be tested, because your patch is for both of
them.

-- 
Yao