[RFA/i386] 2 more patterns in i386_analyze_stack_align

[RFA/i386] 2 more patterns in i386_analyze_stack_align

[Joel Brobecker]

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

Hello,

A coworker and I noticed a problem when we tried gdb-6.4 to debug
some code compiled with GCC 4.1: Some stack alignment code was added
at the start of certain functions. I soon found out that this problem
had been reported under breakpoints/2080 and fixed. However, when
I looked at the fix, I noticed that it was incomplete. According
to my collegue (who worked on the compiler part), the register used
during the stack alignment is either ecx, edx, or eax (in this order
of preference).

So I enhanced the function i386_analyze_stack_align to recognize all
three patterns. I also added testing for these cases in i386-prologue.exp.

2006-12-20  Joel Brobecker  <brobecker@adacore.com>

        * i386-tdep.c (i386_analyze_stack_align): Add handling of two
        other possible code sequences that perform a stack realignment.

2006-12-20  Joel Brobecker  <brobecker@adacore.com>

        * gdb.arch/i386-prologue.c (stack_align_ecx): Renamed from stack_align.
        (stack_align_edx): New function.
        (stack_align_eax): New function.
        (main): Add calls to stack_align_edx and stack_align_eax.
        * gdb.arch/i386-prologue.exp: Replace stack_align with stack_align_ecx.
        Add testing for the cases where the register used during a stack
        realignment is edx. Same for eax.

Fix and testsuite modification tested on x86-linux. No regression.
The new tests fail before my change is applied, and pass after.

OK to apply?

Thanks,
-- 
Joel

[-- Attachment #2: align.diff --]
[-- Type: text/plain, Size: 1376 bytes --]

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.225
diff -u -p -r1.225 i386-tdep.c
--- i386-tdep.c	8 Aug 2006 21:36:46 -0000	1.225
+++ i386-tdep.c	20 Dec 2006 10:21:58 -0000
@@ -497,15 +497,27 @@ static CORE_ADDR
 i386_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
 			  struct i386_frame_cache *cache)
 {
-  static const gdb_byte insns[10] = { 
+  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) */
+  };
   gdb_byte buf[10];
 
   if (target_read_memory (pc, buf, sizeof buf)
-      || memcmp (buf, insns, sizeof buf) != 0)
+      || (memcmp (buf, insns_ecx, sizeof buf) != 0
+          && memcmp (buf, insns_edx, sizeof buf) != 0
+          && memcmp (buf, insns_eax, sizeof buf) != 0))
     return pc;
 
   if (current_pc > pc + 4)

[-- Attachment #3: test.align.diff --]
[-- Type: text/plain, Size: 4933 bytes --]

Index: gdb.arch/i386-prologue.c
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/i386-prologue.c,v
retrieving revision 1.7
diff -u -p -r1.7 i386-prologue.c
--- gdb.arch/i386-prologue.c	13 Feb 2006 22:33:26 -0000	1.7
+++ gdb.arch/i386-prologue.c	20 Dec 2006 10:43:10 -0000
@@ -34,7 +34,9 @@ int
 main (void)
 {
   standard ();
-  stack_align ();
+  stack_align_ecx ();
+  stack_align_edx ();
+  stack_align_eax ();
   gdb1253 ();
   gdb1718 ();
   gdb1338 ();
@@ -114,7 +116,7 @@ asm(".text\n"
 
 asm(".text\n"
     "    .align 8\n"
-    SYMBOL (stack_align) ":\n"
+    SYMBOL (stack_align_ecx) ":\n"
     "    leal  4(%esp), %ecx\n"
     "    andl  $-16, %esp\n"
     "    pushl -4(%ecx)\n"
@@ -128,3 +130,38 @@ asm(".text\n"
     "    popl  %ebp\n"
     "    leal  -4(%ecx), %esp\n"
     "    ret\n");
+
+asm(".text\n"
+    "    .align 8\n"
+    SYMBOL (stack_align_edx) ":\n"
+    "    leal  4(%esp), %edx\n"
+    "    andl  $-16, %esp\n"
+    "    pushl -4(%edx)\n"
+    "    pushl %ebp\n"
+    "    movl  %esp, %ebp\n"
+    "    pushl %edi\n"
+    "    pushl %ecx\n"
+    "    int   $0x03\n"
+    "    popl  %ecx\n"
+    "    popl  %edi\n"
+    "    popl  %ebp\n"
+    "    leal  -4(%edx), %esp\n"
+    "    ret\n");
+
+asm(".text\n"
+    "    .align 8\n"
+    SYMBOL (stack_align_eax) ":\n"
+    "    leal  4(%esp), %eax\n"
+    "    andl  $-16, %esp\n"
+    "    pushl -4(%eax)\n"
+    "    pushl %ebp\n"
+    "    movl  %esp, %ebp\n"
+    "    pushl %edi\n"
+    "    pushl %ecx\n"
+    "    int   $0x03\n"
+    "    popl  %ecx\n"
+    "    popl  %edi\n"
+    "    popl  %ebp\n"
+    "    leal  -4(%eax), %esp\n"
+    "    ret\n");
+
Index: gdb.arch/i386-prologue.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/i386-prologue.exp,v
retrieving revision 1.13
diff -u -p -r1.13 i386-prologue.exp
--- gdb.arch/i386-prologue.exp	20 Dec 2006 09:51:52 -0000	1.13
+++ gdb.arch/i386-prologue.exp	20 Dec 2006 10:43:10 -0000
@@ -95,32 +95,88 @@ gdb_test "info frame" \
 	"saved registers in standard"
 
 
-# Testcase from breakpoints/2080.
+# Testcase from breakpoints/2080 (when %ecx is used)
 
-gdb_test "break *(stack_align + 7)" \
+gdb_test "break *(stack_align_ecx + 7)" \
 	"Breakpoint \[0-9\]* at $hex"
 
 gdb_test "continue" \
-	"Breakpoint \[0-9\]*.*stack_align.*" \
-	"continue to stack_align + 7"
+	"Breakpoint \[0-9\]*.*stack_align_ecx.*" \
+	"continue to stack_align_ecx + 7"
 
 gdb_test "backtrace 10" \
-	"#0\[ \t\]*$hex in stack_align.*\r\n#1\[ \t\]*$hex in main.*" \
-	"first backtrace in stack_align"
+	"#0\[ \t\]*$hex in stack_align_ecx.*\r\n#1\[ \t\]*$hex in main.*" \
+	"first backtrace in stack_align_ecx"
 
 gdb_test "continue" \
 	"Program received signal SIGTRAP.*" \
-	"continue in stack_align"
+	"continue in stack_align_ecx"
 
-skip_breakpoint stack_align
+skip_breakpoint stack_align_ecx
 
 gdb_test "backtrace 10" \
-	"#0\[ \t\]*$hex in stack_align.*\r\n#1\[ \t\]*$hex in main.*" \
-	"second backtrace in stack_align"
+	"#0\[ \t\]*$hex in stack_align_ecx.*\r\n#1\[ \t\]*$hex in main.*" \
+	"second backtrace in stack_align_ecx"
 
 gdb_test "info frame" \
 	".*Saved registers:.*ecx at.*ebp at.*edi at.*eip at.*" \
-	"saved registers in stack_align"
+	"saved registers in stack_align_ecx"
+
+
+# Testcase from breakpoints/2080 (when %edx is used)
+
+gdb_test "break *(stack_align_edx + 7)" \
+	"Breakpoint \[0-9\]* at $hex"
+
+gdb_test "continue" \
+	"Breakpoint \[0-9\]*.*stack_align_edx.*" \
+	"continue to stack_align_edx + 7"
+
+gdb_test "backtrace 10" \
+	"#0\[ \t\]*$hex in stack_align_edx.*\r\n#1\[ \t\]*$hex in main.*" \
+	"first backtrace in stack_align_edx"
+
+gdb_test "continue" \
+	"Program received signal SIGTRAP.*" \
+	"continue in stack_align_edx"
+
+skip_breakpoint stack_align_edx
+
+gdb_test "backtrace 10" \
+	"#0\[ \t\]*$hex in stack_align_edx.*\r\n#1\[ \t\]*$hex in main.*" \
+	"second backtrace in stack_align_edx"
+
+gdb_test "info frame" \
+	".*Saved registers:.*ecx at.*ebp at.*edi at.*eip at.*" \
+	"saved registers in stack_align_edx"
+
+
+# Testcase from breakpoints/2080 (when %eax is used)
+
+gdb_test "break *(stack_align_eax + 7)" \
+	"Breakpoint \[0-9\]* at $hex"
+
+gdb_test "continue" \
+	"Breakpoint \[0-9\]*.*stack_align_eax.*" \
+	"continue to stack_align_eax + 7"
+
+gdb_test "backtrace 10" \
+	"#0\[ \t\]*$hex in stack_align_eax.*\r\n#1\[ \t\]*$hex in main.*" \
+	"first backtrace in stack_align_eax"
+
+gdb_test "continue" \
+	"Program received signal SIGTRAP.*" \
+	"continue in stack_align_eax"
+
+skip_breakpoint stack_align_eax
+
+gdb_test "backtrace 10" \
+	"#0\[ \t\]*$hex in stack_align_eax.*\r\n#1\[ \t\]*$hex in main.*" \
+	"second backtrace in stack_align_eax"
+
+gdb_test "info frame" \
+	".*Saved registers:.*ecx at.*ebp at.*edi at.*eip at.*" \
+	"saved registers in stack_align_eax"
 
 
 # Testcase from symtab/1253.

PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Joel Brobecker]

Ping?

No rush. I just read a message from Mark saying that he was losing
messages, so I'm resending this message, JIC.

Thank you!

On Wed, Dec 20, 2006 at 02:49:45PM +0400, Joel Brobecker wrote:
> Hello,
> 
> A coworker and I noticed a problem when we tried gdb-6.4 to debug
> some code compiled with GCC 4.1: Some stack alignment code was added
> at the start of certain functions. I soon found out that this problem
> had been reported under breakpoints/2080 and fixed. However, when
> I looked at the fix, I noticed that it was incomplete. According
> to my collegue (who worked on the compiler part), the register used
> during the stack alignment is either ecx, edx, or eax (in this order
> of preference).
> 
> So I enhanced the function i386_analyze_stack_align to recognize all
> three patterns. I also added testing for these cases in i386-prologue.exp.
> 
> 2006-12-20  Joel Brobecker  <brobecker@adacore.com>
> 
>         * i386-tdep.c (i386_analyze_stack_align): Add handling of two
>         other possible code sequences that perform a stack realignment.
> 
> 2006-12-20  Joel Brobecker  <brobecker@adacore.com>
> 
>         * gdb.arch/i386-prologue.c (stack_align_ecx): Renamed from stack_align.
>         (stack_align_edx): New function.
>         (stack_align_eax): New function.
>         (main): Add calls to stack_align_edx and stack_align_eax.
>         * gdb.arch/i386-prologue.exp: Replace stack_align with stack_align_ecx.
>         Add testing for the cases where the register used during a stack
>         realignment is edx. Same for eax.
> 
> Fix and testsuite modification tested on x86-linux. No regression.
> The new tests fail before my change is applied, and pass after.
> 
> OK to apply?
> 
> Thanks,
> -- 
> Joel

> Index: i386-tdep.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> retrieving revision 1.225
> diff -u -p -r1.225 i386-tdep.c
> --- i386-tdep.c	8 Aug 2006 21:36:46 -0000	1.225
> +++ i386-tdep.c	20 Dec 2006 10:21:58 -0000
> @@ -497,15 +497,27 @@ static CORE_ADDR
>  i386_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
>  			  struct i386_frame_cache *cache)
>  {
> -  static const gdb_byte insns[10] = { 
> +  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) */
> +  };
>    gdb_byte buf[10];
>  
>    if (target_read_memory (pc, buf, sizeof buf)
> -      || memcmp (buf, insns, sizeof buf) != 0)
> +      || (memcmp (buf, insns_ecx, sizeof buf) != 0
> +          && memcmp (buf, insns_edx, sizeof buf) != 0
> +          && memcmp (buf, insns_eax, sizeof buf) != 0))
>      return pc;
>  
>    if (current_pc > pc + 4)

> Index: gdb.arch/i386-prologue.c
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/i386-prologue.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 i386-prologue.c
> --- gdb.arch/i386-prologue.c	13 Feb 2006 22:33:26 -0000	1.7
> +++ gdb.arch/i386-prologue.c	20 Dec 2006 10:43:10 -0000
> @@ -34,7 +34,9 @@ int
>  main (void)
>  {
>    standard ();
> -  stack_align ();
> +  stack_align_ecx ();
> +  stack_align_edx ();
> +  stack_align_eax ();
>    gdb1253 ();
>    gdb1718 ();
>    gdb1338 ();
> @@ -114,7 +116,7 @@ asm(".text\n"
>  
>  asm(".text\n"
>      "    .align 8\n"
> -    SYMBOL (stack_align) ":\n"
> +    SYMBOL (stack_align_ecx) ":\n"
>      "    leal  4(%esp), %ecx\n"
>      "    andl  $-16, %esp\n"
>      "    pushl -4(%ecx)\n"
> @@ -128,3 +130,38 @@ asm(".text\n"
>      "    popl  %ebp\n"
>      "    leal  -4(%ecx), %esp\n"
>      "    ret\n");
> +
> +asm(".text\n"
> +    "    .align 8\n"
> +    SYMBOL (stack_align_edx) ":\n"
> +    "    leal  4(%esp), %edx\n"
> +    "    andl  $-16, %esp\n"
> +    "    pushl -4(%edx)\n"
> +    "    pushl %ebp\n"
> +    "    movl  %esp, %ebp\n"
> +    "    pushl %edi\n"
> +    "    pushl %ecx\n"
> +    "    int   $0x03\n"
> +    "    popl  %ecx\n"
> +    "    popl  %edi\n"
> +    "    popl  %ebp\n"
> +    "    leal  -4(%edx), %esp\n"
> +    "    ret\n");
> +
> +asm(".text\n"
> +    "    .align 8\n"
> +    SYMBOL (stack_align_eax) ":\n"
> +    "    leal  4(%esp), %eax\n"
> +    "    andl  $-16, %esp\n"
> +    "    pushl -4(%eax)\n"
> +    "    pushl %ebp\n"
> +    "    movl  %esp, %ebp\n"
> +    "    pushl %edi\n"
> +    "    pushl %ecx\n"
> +    "    int   $0x03\n"
> +    "    popl  %ecx\n"
> +    "    popl  %edi\n"
> +    "    popl  %ebp\n"
> +    "    leal  -4(%eax), %esp\n"
> +    "    ret\n");
> +
> Index: gdb.arch/i386-prologue.exp
> ===================================================================
> RCS file: /cvs/src/src/gdb/testsuite/gdb.arch/i386-prologue.exp,v
> retrieving revision 1.13
> diff -u -p -r1.13 i386-prologue.exp
> --- gdb.arch/i386-prologue.exp	20 Dec 2006 09:51:52 -0000	1.13
> +++ gdb.arch/i386-prologue.exp	20 Dec 2006 10:43:10 -0000
> @@ -95,32 +95,88 @@ gdb_test "info frame" \
>  	"saved registers in standard"
>  
>  
> -# Testcase from breakpoints/2080.
> +# Testcase from breakpoints/2080 (when %ecx is used)
>  
> -gdb_test "break *(stack_align + 7)" \
> +gdb_test "break *(stack_align_ecx + 7)" \
>  	"Breakpoint \[0-9\]* at $hex"
>  
>  gdb_test "continue" \
> -	"Breakpoint \[0-9\]*.*stack_align.*" \
> -	"continue to stack_align + 7"
> +	"Breakpoint \[0-9\]*.*stack_align_ecx.*" \
> +	"continue to stack_align_ecx + 7"
>  
>  gdb_test "backtrace 10" \
> -	"#0\[ \t\]*$hex in stack_align.*\r\n#1\[ \t\]*$hex in main.*" \
> -	"first backtrace in stack_align"
> +	"#0\[ \t\]*$hex in stack_align_ecx.*\r\n#1\[ \t\]*$hex in main.*" \
> +	"first backtrace in stack_align_ecx"
>  
>  gdb_test "continue" \
>  	"Program received signal SIGTRAP.*" \
> -	"continue in stack_align"
> +	"continue in stack_align_ecx"
>  
> -skip_breakpoint stack_align
> +skip_breakpoint stack_align_ecx
>  
>  gdb_test "backtrace 10" \
> -	"#0\[ \t\]*$hex in stack_align.*\r\n#1\[ \t\]*$hex in main.*" \
> -	"second backtrace in stack_align"
> +	"#0\[ \t\]*$hex in stack_align_ecx.*\r\n#1\[ \t\]*$hex in main.*" \
> +	"second backtrace in stack_align_ecx"
>  
>  gdb_test "info frame" \
>  	".*Saved registers:.*ecx at.*ebp at.*edi at.*eip at.*" \
> -	"saved registers in stack_align"
> +	"saved registers in stack_align_ecx"
> +
> +
> +# Testcase from breakpoints/2080 (when %edx is used)
> +
> +gdb_test "break *(stack_align_edx + 7)" \
> +	"Breakpoint \[0-9\]* at $hex"
> +
> +gdb_test "continue" \
> +	"Breakpoint \[0-9\]*.*stack_align_edx.*" \
> +	"continue to stack_align_edx + 7"
> +
> +gdb_test "backtrace 10" \
> +	"#0\[ \t\]*$hex in stack_align_edx.*\r\n#1\[ \t\]*$hex in main.*" \
> +	"first backtrace in stack_align_edx"
> +
> +gdb_test "continue" \
> +	"Program received signal SIGTRAP.*" \
> +	"continue in stack_align_edx"
> +
> +skip_breakpoint stack_align_edx
> +
> +gdb_test "backtrace 10" \
> +	"#0\[ \t\]*$hex in stack_align_edx.*\r\n#1\[ \t\]*$hex in main.*" \
> +	"second backtrace in stack_align_edx"
> +
> +gdb_test "info frame" \
> +	".*Saved registers:.*ecx at.*ebp at.*edi at.*eip at.*" \
> +	"saved registers in stack_align_edx"
> +
> +
> +# Testcase from breakpoints/2080 (when %eax is used)
> +
> +gdb_test "break *(stack_align_eax + 7)" \
> +	"Breakpoint \[0-9\]* at $hex"
> +
> +gdb_test "continue" \
> +	"Breakpoint \[0-9\]*.*stack_align_eax.*" \
> +	"continue to stack_align_eax + 7"
> +
> +gdb_test "backtrace 10" \
> +	"#0\[ \t\]*$hex in stack_align_eax.*\r\n#1\[ \t\]*$hex in main.*" \
> +	"first backtrace in stack_align_eax"
> +
> +gdb_test "continue" \
> +	"Program received signal SIGTRAP.*" \
> +	"continue in stack_align_eax"
> +
> +skip_breakpoint stack_align_eax
> +
> +gdb_test "backtrace 10" \
> +	"#0\[ \t\]*$hex in stack_align_eax.*\r\n#1\[ \t\]*$hex in main.*" \
> +	"second backtrace in stack_align_eax"
> +
> +gdb_test "info frame" \
> +	".*Saved registers:.*ecx at.*ebp at.*edi at.*eip at.*" \
> +	"saved registers in stack_align_eax"
>  
>  
>  # Testcase from symtab/1253.


-- 
Joel

Re: PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Mark Kettenis]

> Date: Sun, 31 Dec 2006 10:08:44 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> Ping?
> 
> No rush. I just read a message from Mark saying that he was losing
> messages, so I'm resending this message, JIC.

Probably a good idea ;).

> > Index: i386-tdep.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/i386-tdep.c,v
> > retrieving revision 1.225
> > diff -u -p -r1.225 i386-tdep.c
> > --- i386-tdep.c	8 Aug 2006 21:36:46 -0000	1.225
> > +++ i386-tdep.c	20 Dec 2006 10:21:58 -0000
> > @@ -497,15 +497,27 @@ static CORE_ADDR
> >  i386_analyze_stack_align (CORE_ADDR pc, CORE_ADDR current_pc,
> >  			  struct i386_frame_cache *cache)
> >  {
> > -  static const gdb_byte insns[10] = { 
> > +  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) */
> > +  };
> >    gdb_byte buf[10];

Hmm, you're missing the %ebx case here.  Now on ELF systems, you'll
probably never see it since %ebx is used for GOT access, but on other
object formats I don't think there is any reason why GCC wouldn't
choose to use %ebx as well.

This looks reasonable otherwise, except that I would sort the patterns
in a more logical order.

Mark

Re: PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Joel Brobecker]

> Hmm, you're missing the %ebx case here.  Now on ELF systems, you'll
> probably never see it since %ebx is used for GOT access, but on other
> object formats I don't think there is any reason why GCC wouldn't
> choose to use %ebx as well.

I can easily add the %ebx case as well, but I don't think GCC would
currently use that register in any configuration. I'll double-check
with one of my coworkers who told me about the other alternatives to
using %ecx, but it will have to wait for a few days until the holidays
are over. He told me about this after having looked at the code in GCC,
so I would be surprised if he did not see the %ebx case... Note that
even if GCC did not currently use %ebx, I don't mind adding support
for it, JIC.

> This looks reasonable otherwise, except that I would sort the patterns
> in a more logical order.

I sorted them in the order that GCC would use them. I don't mind sorting
them in a different order, if you like.

I will take this as approval, and wait for your comments to see if
you'd like me to submit followup patches: adding support for the use
of the %ebx case, and altering the patterns order.

Thanks Mark,
-- 
Joel

Re: PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Joel Brobecker]

Hi Mark,

A followup on a recent discussion:

> Hmm, you're missing the %ebx case here.  Now on ELF systems, you'll
> probably never see it since %ebx is used for GOT access, but on other
> object formats I don't think there is any reason why GCC wouldn't
> choose to use %ebx as well.

I consulted with Olivier Hainque and here is what I learnt:

  . The current FSF GCC only uses %ecx, and punts on any realignment
    request for a function which needs ecx for other purposes, like
    neted functions with a static chain.

  . We have a local enhancement that takes advantage of the fact
    that when ecx is not available, edx and then eax are used.

    I wasn't aware of the fact that this change was local when
    I submitted my patch.  I don't know yet why this change was
    not contributed, probably lack of time. Hopefully it will be
    included soon.

  . In terms of what registers can be ued in the realignment sequence,
    Olivier said:

      The "available" registers are the ABI caller-saved registers "dead"
      on both entry and exit of the function, that is, not used for arg
      passing, static chain or value returning.

  . The current implementation is SVR4 ABI oriented AFAICT, and ebx is
    not a possible candidate because it is callee-saved. We're not sure
    about the status of non-elf targets.

As a result, I think it's 50/50 in terms of adding the %ebx sequence.
I would recommend adding it anyway, with a small comment, just to be
on the safe side. I don't think we can break anything in the debugger
with such a change, and yet nothing worse than a broken callstack
when you're trying to track a bug down.

Let me know what you think.

-- 
Joel

Re: PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Mark Kettenis]

> Date: Fri, 5 Jan 2007 10:49:16 +0400
> From: Joel Brobecker <brobecker@adacore.com>
> 
> Hi Mark,
> 
> A followup on a recent discussion:
> 
> > Hmm, you're missing the %ebx case here.  Now on ELF systems, you'll
> > probably never see it since %ebx is used for GOT access, but on other
> > object formats I don't think there is any reason why GCC wouldn't
> > choose to use %ebx as well.
> 
> I consulted with Olivier Hainque and here is what I learnt:
> 
>   . The current FSF GCC only uses %ecx, and punts on any realignment
>     request for a function which needs ecx for other purposes, like
>     neted functions with a static chain.
> 
>   . We have a local enhancement that takes advantage of the fact
>     that when ecx is not available, edx and then eax are used.
> 
>     I wasn't aware of the fact that this change was local when
>     I submitted my patch.  I don't know yet why this change was
>     not contributed, probably lack of time. Hopefully it will be
>     included soon.

Well, it seems a valid generalisation, so I have no problems in adding
the patterns.  Actually that's why I think we should also add %ebx.

>   . The current implementation is SVR4 ABI oriented AFAICT, and ebx is
>     not a possible candidate because it is callee-saved. We're not sure
>     about the status of non-elf targets.

Ah wait, that's true even for "absolute" code.  I had a quick look at
the GCC code and it seems to always treat %ebx as callee-saved.  So
I'm happy with leaving it out, but you could add a comment saying so
to prevent us from having this same discussion in about two months
;-).

Mark

Re: PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Joel Brobecker]

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

> Ah wait, that's true even for "absolute" code.  I had a quick look at
> the GCC code and it seems to always treat %ebx as callee-saved.  So
> I'm happy with leaving it out, but you could add a comment saying so
> to prevent us from having this same discussion in about two months
> ;-).

Sure! How does the attached patch look? Does it contain enough info?

2007-01-05  Joel Brobecker  <brobecker@adacore.com>

        * i386-tdep.c (i386_analyze_stack_align): Add comment.

-- 
Joel

[-- Attachment #2: comment.diff --]
[-- Type: text/plain, Size: 840 bytes --]

Index: i386-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/i386-tdep.c,v
retrieving revision 1.227
diff -u -p -r1.227 i386-tdep.c
--- i386-tdep.c	3 Jan 2007 19:01:25 -0000	1.227
+++ i386-tdep.c	5 Jan 2007 14:22:11 -0000
@@ -497,6 +497,10 @@ 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 */

Re: PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Mark Kettenis]

> Date: Fri, 5 Jan 2007 18:37:28 +0400
> From: Joel Brobecker <brobecker@adacore.com>

> > Ah wait, that's true even for "absolute" code.  I had a quick look at
> > the GCC code and it seems to always treat %ebx as callee-saved.  So
> > I'm happy with leaving it out, but you could add a comment saying so
> > to prevent us from having this same discussion in about two months
> > ;-).
> 
> Sure! How does the attached patch look? Does it contain enough info?

More than enough.  Please commit after adding the missing space before
"GCC should never...".

> 2007-01-05  Joel Brobecker  <brobecker@adacore.com>
> 
>         * i386-tdep.c (i386_analyze_stack_align): Add comment.

Re: PING: [RFA/i386] 2 more patterns in i386_analyze_stack_align

[Joel Brobecker]

> More than enough.  Please commit after adding the missing space before
> "GCC should never...".
> 
> > 2007-01-05  Joel Brobecker  <brobecker@adacore.com>
> > 
> >         * i386-tdep.c (i386_analyze_stack_align): Add comment.

Excellent. Checked in with the correction you spotted.

Thanks!
-- 
Joel