[i386/stabs] Arguments of main on gcc >= 4.1

[i386/stabs] Arguments of main on gcc >= 4.1

[Pedro Alves]

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

Hi,

On gcc/stabs, a function with a normal frame setup
sequence has the parameter offsets output as offsets
from the frame pointer:

E.g., from funcargs.c:

void call0a (char c, short s, int i, long l);

           .stabs  "call0a:F(0,15)",36,0,32,call0a
           .stabs  "c:p(0,1)",160,0,31,8
           .stabs  "s:p(0,1)",160,0,31,12
           .stabs  "i:p(0,1)",160,0,31,16
           .stabs  "l:p(0,3)",160,0,31,20
.globl call0a
           .type   call0a, @function
call0a:
           .stabd  46,0,0

Notice the 8 on the first parameter ("c:p(0,1)",160,0,31,8)
-- that would be argument word 0 at 8($ebp).

Since gcc 4.1 and later started putting code in the prologue
to realign the stack pointer, the offset on the
parameter stabs changed from being relative to the frame pointer
to being relative to the argument's address.

E.g.:
     int main (int argc, char **argv, char **envp);

notice the 0, 4, 8:

           .zero   16
           .text
           .stabs  "main:F(0,1)",36,0,10,main
           .stabs  "argc:p(0,1)",160,0,9,0
           .stabs  "argv:p(0,16)=*(3,54)",160,0,9,4
           .stabs  "envp:p(0,16)",160,0,9,8
.globl main
           .type   main, @function

I don't know if this is a gcc bug or if this is
specified somewhere.  Several gcc's have been out with
this problem, so I guess the best option is to account
for it in gdb.

The attached patch adds that special casing.

In order to fully fix the issue, there is another tweak that
needs to be made.  By the time we're analyzing the frame,
to read the value of a parameter, the register that holds
the %esp of the previous frame may have been clobbered.  If
we see it being saved in the stack before being clobbered,
we can still get to it.

Eg from args.exp:

int
main (int argc, char **argv)
{
    80483e4:       8d 4c 24 04    lea    0x4(%esp),%ecx
    80483e8:       83 e4 f0       and    $0xfffffff0,%esp
    80483eb:       ff 71 fc       pushl  0xfffffffc(%ecx)
    80483ee:       55             push   %ebp
    80483ef:       89 e5          mov    %esp,%ebp
    80483f1:       51             push   %ecx       << register save,
    80483f2:       83 ec 24       sub    $0x24,%esp << before stack adjustment
    80483f5:       89 4d e8       mov    %ecx,0xffffffe8(%ebp)
     int i = 0;

While fixing this, I noticed that i386_analyze_stack_align
recognizes %ecx, %edx or %eax as the register that is used
to hold the %esp of the calling frame, but i386_frame_cache
always expected it in %ecx.  I've added a new variable in
struct i386_frame_cache to represent which register it was,
and use it in the i386_frame_cache function.

Another change, which is purely cosmetic, is the
s/saved_sp/prev_frame_sp/ renaming.  For normal,
non-main frames, this field doesn't hold anything
"saved", but computed in i386_frame_cache:
   cache->saved_sp = cache->base + 8;

The patch has been tested on
     i686-pc-linux-gnu/dwarf,
     i686-pc-linux-gnu/"--target_board=unix/gdb:debug_flags=-gstabs+"
     and on i686-pc-cygwin.

On stabs, I get these fixes:

--- 0/gdb.sum	2007-11-30 01:30:02.000000000 +0000
+++ 1/gdb.sum	2007-11-30 01:45:33.000000000 +0000
@@ -1,4 +1,4 @@
-Test Run By pedro on Fri Nov 30 01:16:23 2007
+Test Run By pedro on Fri Nov 30 01:31:55 2007
    Native configuration is i686-pc-linux-gnu

    		=== gdb tests ===
@@ -283,31 +283,31 @@ PASS: gdb.base/annota3.exp: signal sent
    PASS: gdb.base/annota3.exp: cleanup core file (removed)
    Running
/home/pedro/gdb/autotester/frame_args_stabs/src/gdb/testsuite/gdb.base/args.exp ...
    PASS: gdb.base/args.exp: continue to breakpoint: breakpoint for basic
-FAIL: gdb.base/args.exp: argc for basic
-FAIL: gdb.base/args.exp: argv[1] for basic
-FAIL: gdb.base/args.exp: argv[2] for basic
+PASS: gdb.base/args.exp: argc for basic
+PASS: gdb.base/args.exp: argv[1] for basic
+PASS: gdb.base/args.exp: argv[2] for basic
    PASS: gdb.base/args.exp: continue to breakpoint: breakpoint for one empty
-FAIL: gdb.base/args.exp: argc for one empty
-FAIL: gdb.base/args.exp: argv[1] for one empty
-FAIL: gdb.base/args.exp: argv[2] for one empty
-FAIL: gdb.base/args.exp: argv[3] for one empty
+PASS: gdb.base/args.exp: argc for one empty
+PASS: gdb.base/args.exp: argv[1] for one empty
+PASS: gdb.base/args.exp: argv[2] for one empty
+PASS: gdb.base/args.exp: argv[3] for one empty
    PASS: gdb.base/args.exp: continue to breakpoint: breakpoint for two empty
-FAIL: gdb.base/args.exp: argc for two empty
-FAIL: gdb.base/args.exp: argv[1] for two empty
-FAIL: gdb.base/args.exp: argv[2] for two empty
-FAIL: gdb.base/args.exp: argv[3] for two empty
-FAIL: gdb.base/args.exp: argv[4] for two empty
+PASS: gdb.base/args.exp: argc for two empty
+PASS: gdb.base/args.exp: argv[1] for two empty
+PASS: gdb.base/args.exp: argv[2] for two empty
+PASS: gdb.base/args.exp: argv[3] for two empty
+PASS: gdb.base/args.exp: argv[4] for two empty
    PASS: gdb.base/args.exp: continue to breakpoint: breakpoint for one empty
-FAIL: gdb.base/args.exp: argc for one empty
-FAIL: gdb.base/args.exp: argv[1] for one empty
-FAIL: gdb.base/args.exp: argv[2] for one empty
-FAIL: gdb.base/args.exp: argv[3] for one empty
+PASS: gdb.base/args.exp: argc for one empty
+PASS: gdb.base/args.exp: argv[1] for one empty
+PASS: gdb.base/args.exp: argv[2] for one empty
+PASS: gdb.base/args.exp: argv[3] for one empty
    PASS: gdb.base/args.exp: continue to breakpoint: breakpoint for two empty
-FAIL: gdb.base/args.exp: argc for two empty
-FAIL: gdb.base/args.exp: argv[1] for two empty
-FAIL: gdb.base/args.exp: argv[2] for two empty
-FAIL: gdb.base/args.exp: argv[3] for two empty
-FAIL: gdb.base/args.exp: argv[4] for two empty
+PASS: gdb.base/args.exp: argc for two empty
+PASS: gdb.base/args.exp: argv[1] for two empty
+PASS: gdb.base/args.exp: argv[2] for two empty
+PASS: gdb.base/args.exp: argv[3] for two empty
+PASS: gdb.base/args.exp: argv[4] for two empty
    Running
/home/pedro/gdb/autotester/frame_args_stabs/src/gdb/testsuite/gdb.base/arithmet.exp
...
    PASS: gdb.base/arithmet.exp: set variable x=14
    PASS: gdb.base/arithmet.exp: set variable y=2
@@ -12259,8 +12259,8 @@ PASS: gdb.xml/tdesc-xinclude.exp: set td

    		=== gdb Summary ===

-# of expected passes		11491
-# of unexpected failures	207
+# of expected passes		11512
+# of unexpected failures	186
    # of unexpected successes	4
    # of expected failures		52
    # of unknown successes		6

On dwarf, there are no changes to the testsuite, as in that case,
get_frame_args_address is only used for 'info frame'.

-- 
Pedro Alves


[-- Attachment #2: i386_frame_args.diff --]
[-- Type: text/x-diff, Size: 10167 bytes --]

2007-11-30  Pedro Alves  <pedro_alves@portugalmail.pt>

	* i386-tdep.c (struct i386_frame_cache): Rename saved_sp to
	prev_frame_sp.  Add saved_sp_regnum field.
	(i386_alloc_frame_cache): Update.
	(i386_analyze_stack_align): Record which register holds %esp in
	saved_sp_regnum.
	(i386_analyze_register_saves): Move higher on the file.
	(i386_analyze_frame_setup): Account for register saves before
	stack adjustment.
	(i386_frame_cache): If possible, prefer reading the register that
	holds the previous stack pointer from the stack .
	(i386_frame_prev_register): Update.
	(i386_frame_args_address): New.
	(i386_frame_base): Set i386_frame_args_address as args method.

---
 gdb/i386-tdep.c |  177 +++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 126 insertions(+), 51 deletions(-)

Index: src/gdb/i386-tdep.c
===================================================================
--- src.orig/gdb/i386-tdep.c	2007-11-29 01:09:37.000000000 +0000
+++ src/gdb/i386-tdep.c	2007-11-30 15:58:07.000000000 +0000
@@ -298,6 +298,24 @@ i386_breakpoint_from_pc (struct gdbarch 
    registers mentioned above, and %eip.  */
 #define I386_NUM_SAVED_REGS	I386_NUM_GREGS
 
+/* The standard stack frame according to the System V ABI:
+
+      Position  |         Contents         |   Frame
+     -----------+--------------------------+-----------
+     4n+8(%ebp) |      argument word n     |
+                |      ...                 |  Previous
+     8(%ebp)    |      argument word 0     |
+     -----------+--------------------------+-----------
+     4(%ebp)    |      return address      |
+                +--------------------------+
+     0(%ebp)    | previous %ebp (optional) |
+                +--------------------------+  Current
+    -4(%ebp)    |       unspecified        |
+                |           ...            |
+     0(%esp)    |      variable size       |
+     -----------+--------------------------+-----------
+*/
+
 struct i386_frame_cache
 {
   /* Base address.  */
@@ -307,8 +325,17 @@ struct i386_frame_cache
 
   /* Saved registers.  */
   CORE_ADDR saved_regs[I386_NUM_SAVED_REGS];
-  CORE_ADDR saved_sp;
+
+  /* The value of %esp in the calling frame.  */
+  CORE_ADDR prev_frame_sp;
+
+  /* Non-zero if this frame has enforced stack alignment.  */
   int stack_align;
+
+  /* If STACK_ALIGN, which register holds %esp of the calling frame,
+     else undefined.  */
+  int saved_sp_regnum;
+
   int pc_in_eax;
 
   /* Stack space reserved for local variables.  */
@@ -334,7 +361,7 @@ i386_alloc_frame_cache (void)
      offset (that's where %ebp is supposed to be stored).  */
   for (i = 0; i < I386_NUM_SAVED_REGS; i++)
     cache->saved_regs[i] = -1;
-  cache->saved_sp = 0;
+  cache->prev_frame_sp = 0;
   cache->stack_align = 0;
   cache->pc_in_eax = 0;
 
@@ -517,15 +544,25 @@ i386_analyze_stack_align (CORE_ADDR pc, 
     0xff, 0x70, 0xfc		/* pushl -4(%eax) */
   };
   gdb_byte buf[10];
+  int regnum;
 
-  if (target_read_memory (pc, buf, sizeof buf)
-      || (memcmp (buf, insns_ecx, sizeof buf) != 0
-          && memcmp (buf, insns_edx, sizeof buf) != 0
-          && memcmp (buf, insns_eax, sizeof buf) != 0))
+  if (target_read_memory (pc, buf, sizeof buf))
+    return pc;
+
+  if (memcmp (buf, insns_ecx, sizeof buf) == 0)
+    regnum = I386_ECX_REGNUM;
+  else if (memcmp (buf, insns_edx, sizeof buf) == 0)
+    regnum = I386_EDX_REGNUM;
+  else if (memcmp (buf, insns_eax, sizeof buf) == 0)
+    regnum = I386_EAX_REGNUM;
+  else
     return pc;
 
   if (current_pc > pc + 4)
-    cache->stack_align = 1;
+    {
+      cache->stack_align = 1;
+      cache->saved_sp_regnum = regnum;
+    }
 
   return min (pc + 10, current_pc);
 }
@@ -579,6 +616,36 @@ i386_match_insn (CORE_ADDR pc, struct i3
   return NULL;
 }
 
+/* Check whether PC points at code that saves registers on the stack.
+   If so, it updates CACHE and returns the address of the first
+   instruction after the register saves or CURRENT_PC, whichever is
+   smaller.  Otherwise, return PC.  */
+
+static CORE_ADDR
+i386_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
+			     struct i386_frame_cache *cache)
+{
+  CORE_ADDR offset = 0;
+  gdb_byte op;
+  int i;
+
+  if (cache->locals > 0)
+    offset -= cache->locals;
+  for (i = 0; i < 8 && pc < current_pc; i++)
+    {
+      read_memory_nobpt (pc, &op, 1);
+      if (op < 0x50 || op > 0x57)
+	break;
+
+      offset -= 4;
+      cache->saved_regs[op - 0x50] = offset;
+      cache->sp_offset += 4;
+      pc++;
+    }
+
+  return pc;
+}
+
 /* Some special instructions that might be migrated by GCC into the
    part of the prologue that sets up the new stack frame.  Because the
    stack frame hasn't been setup yet, no registers have been saved
@@ -713,7 +780,11 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
       if (limit <= pc)
 	return limit;
 
-      /* Check for stack adjustment 
+      /* There may be registers saves before the stack is grown for
+	 locals.  */
+      pc = i386_analyze_register_saves (pc, limit, cache);
+
+      /* Check for stack adjustment
 
 	    subl $XXX, %esp
 
@@ -758,36 +829,6 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
   return pc;
 }
 
-/* Check whether PC points at code that saves registers on the stack.
-   If so, it updates CACHE and returns the address of the first
-   instruction after the register saves or CURRENT_PC, whichever is
-   smaller.  Otherwise, return PC.  */
-
-static CORE_ADDR
-i386_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
-			     struct i386_frame_cache *cache)
-{
-  CORE_ADDR offset = 0;
-  gdb_byte op;
-  int i;
-
-  if (cache->locals > 0)
-    offset -= cache->locals;
-  for (i = 0; i < 8 && pc < current_pc; i++)
-    {
-      read_memory_nobpt (pc, &op, 1);
-      if (op < 0x50 || op > 0x57)
-	break;
-
-      offset -= 4;
-      cache->saved_regs[op - 0x50] = offset;
-      cache->sp_offset += 4;
-      pc++;
-    }
-
-  return pc;
-}
-
 /* Do a full 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.
@@ -811,7 +852,7 @@ i386_analyze_register_saves (CORE_ADDR p
    %ebx (and sometimes a harmless bug causes it to also save but not
    restore %eax); however, the code below is willing to see the pushes
    in any order, and will handle up to 8 of them.
- 
+
    If the setup sequence is at the end of the function, then the next
    instruction will be a branch back to the start.  */
 
@@ -925,6 +966,7 @@ i386_frame_cache (struct frame_info *nex
   struct i386_frame_cache *cache;
   gdb_byte buf[4];
   int i;
+  CORE_ADDR saved_sp = 0;
 
   if (*this_cache)
     return *this_cache;
@@ -955,9 +997,23 @@ i386_frame_cache (struct frame_info *nex
 
   if (cache->stack_align)
     {
-      /* Saved stack pointer has been saved in %ecx.  */
-      frame_unwind_register (next_frame, I386_ECX_REGNUM, buf);
-      cache->saved_sp = extract_unsigned_integer(buf, 4);
+      /* The previous frame stack pointer has been saved in %ecx, %edx
+	 or %eax at the beginning of the frame setup.  Since the
+	 storing register may have been clobbered by the time we're
+	 analyzing this frame, we prefer reading it from the stack if
+	 we've seen it being pushed.  */
+      if (cache->saved_regs[cache->saved_sp_regnum] != -1)
+	{
+	  /* We can only see it being pushed if we found a valid
+	     frame, so we can safely access cache->base here.  */
+	  CORE_ADDR reg
+	    = cache->base + cache->saved_regs[cache->saved_sp_regnum];
+	  read_memory (reg, buf, 4);
+	}
+      else
+	frame_unwind_register (next_frame, cache->saved_sp_regnum, buf);
+
+      saved_sp = extract_unsigned_integer (buf, 4);
     }
 
   if (cache->locals < 0)
@@ -973,8 +1029,8 @@ i386_frame_cache (struct frame_info *nex
       if (cache->stack_align)
 	{
 	  /* We're halfway aligning the stack.  */
-	  cache->base = ((cache->saved_sp - 4) & 0xfffffff0) - 4;
-	  cache->saved_regs[I386_EIP_REGNUM] = cache->saved_sp - 4;
+	  cache->base = ((saved_sp - 4) & 0xfffffff0) - 4;
+	  cache->saved_regs[I386_EIP_REGNUM] = saved_sp - 4;
 
 	  /* This will be added back below.  */
 	  cache->saved_regs[I386_EIP_REGNUM] -= cache->base;
@@ -988,8 +1044,11 @@ i386_frame_cache (struct frame_info *nex
 
   /* Now that we have the base address for the stack frame we can
      calculate the value of %esp in the calling frame.  */
-  if (cache->saved_sp == 0)
-    cache->saved_sp = cache->base + 8;
+
+  if (cache->stack_align)
+    cache->prev_frame_sp = saved_sp;
+  else
+    cache->prev_frame_sp = cache->base + 8;
 
   /* Adjust all the saved registers such that they contain addresses
      instead of offsets.  */
@@ -1074,7 +1133,7 @@ i386_frame_prev_register (struct frame_i
       return;
     }
 
-  if (regnum == I386_ESP_REGNUM && cache->saved_sp)
+  if (regnum == I386_ESP_REGNUM && cache->prev_frame_sp)
     {
       *optimizedp = 0;
       *lvalp = not_lval;
@@ -1083,7 +1142,7 @@ i386_frame_prev_register (struct frame_i
       if (valuep)
 	{
 	  /* Store the value.  */
-	  store_unsigned_integer (valuep, 4, cache->saved_sp);
+	  store_unsigned_integer (valuep, 4, cache->prev_frame_sp);
 	}
       return;
     }
@@ -1233,12 +1292,28 @@ i386_frame_base_address (struct frame_in
   return cache->base;
 }
 
+static CORE_ADDR
+i386_frame_args_address (struct frame_info *next_frame, void **this_cache)
+{
+  struct i386_frame_cache *cache = i386_frame_cache (next_frame, this_cache);
+
+  if (cache->stack_align)
+    /* For `main', the values of LOC_ARG variables are offsets
+       relative to the arglist address, while for other functions the
+       offsets are relative to the frame base.  */
+    return cache->prev_frame_sp;
+
+  /* Arguments are found at 8(%ebp), but STABS parameter values are
+     offsets from %ebp.  */
+  return cache->base;
+}
+
 static const struct frame_base i386_frame_base =
 {
   &i386_frame_unwind,
-  i386_frame_base_address,
-  i386_frame_base_address,
-  i386_frame_base_address
+  i386_frame_base_address, /* base */
+  i386_frame_base_address, /* locals */
+  i386_frame_args_address  /* args */
 };
 
 static struct frame_id

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Joel Brobecker]

> Since gcc 4.1 and later started putting code in the prologue
> to realign the stack pointer, the offset on the
> parameter stabs changed from being relative to the frame pointer
> to being relative to the argument's address.

This is somewhat fuzzy because the stabs format has never been really
well defined like DWARF is, but IMO this is a GCC bug.

> E.g.:
>     int main (int argc, char **argv, char **envp);
> 
> notice the 0, 4, 8:
> 
>           .zero   16
>           .text
>           .stabs  "main:F(0,1)",36,0,10,main
>           .stabs  "argc:p(0,1)",160,0,9,0
>           .stabs  "argv:p(0,16)=*(3,54)",160,0,9,4
>           .stabs  "envp:p(0,16)",160,0,9,8
> .globl main
>           .type   main, @function

Looking at your example above, what would work is if GCC was using
N_LSYM (128) symbols instead of N_PSYM symbols (160).

I would definitely be interested in what others think on this.

-- 
Joel

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Daniel Jacobowitz]

On Mon, Dec 03, 2007 at 10:25:40AM -0800, Joel Brobecker wrote:
> > E.g.:
> >     int main (int argc, char **argv, char **envp);
> > 
> > notice the 0, 4, 8:
> > 
> >           .zero   16
> >           .text
> >           .stabs  "main:F(0,1)",36,0,10,main
> >           .stabs  "argc:p(0,1)",160,0,9,0
> >           .stabs  "argv:p(0,16)=*(3,54)",160,0,9,4
> >           .stabs  "envp:p(0,16)",160,0,9,8
> > .globl main
> >           .type   main, @function
> 
> Looking at your example above, what would work is if GCC was using
> N_LSYM (128) symbols instead of N_PSYM symbols (160).

If it did that, they wouldn't be treated as arguments any more, just
as local variables.  I think.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Joel Brobecker]

> > Looking at your example above, what would work is if GCC was using
> > N_LSYM (128) symbols instead of N_PSYM symbols (160).
> 
> If it did that, they wouldn't be treated as arguments any more, just
> as local variables.  I think.

Duh, of course! Not sure what's best in this case - I would tend to
say it's a GCC bug, but perhaps others disagree.

-- 
Joel

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Daniel Jacobowitz]

On Mon, Dec 17, 2007 at 08:11:59AM +0400, Joel Brobecker wrote:
> > > Looking at your example above, what would work is if GCC was using
> > > N_LSYM (128) symbols instead of N_PSYM symbols (160).
> > 
> > If it did that, they wouldn't be treated as arguments any more, just
> > as local variables.  I think.
> 
> Duh, of course! Not sure what's best in this case - I would tend to
> say it's a GCC bug, but perhaps others disagree.

I think that the major difference between a GDB bug and a GCC bug is
whether we have to get GCC fixed - either way, since it's been in
several releases, handling it in GDB is at least worth considering.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Joel Brobecker]

> I think that the major difference between a GDB bug and a GCC bug is
> whether we have to get GCC fixed - either way, since it's been in
> several releases, handling it in GDB is at least worth considering.

That makes sense.

I put a "patch champion" hat on and had a look at the proposed patch.
If I understand correctly, it looks like the code is detecting stack
alignment code, and if it does, then it considers that the parameters
will be relative to the arguments region address.

I wonder how this all works if GCC < 4.1 is being used.

-- 
Joel

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Pedro Alves]

Joel Brobecker wrote:
> Daniel Jacobowitz wrote:
>> I think that the major difference between a GDB bug and a GCC bug is
>> whether we have to get GCC fixed - either way, since it's been in
>> several releases, handling it in GDB is at least worth considering.
> 

That was my opinion when I posted the patch, and still is.

> That makes sense.
> 

> I put a "patch champion" hat on and had a look at the proposed patch.
> If I understand correctly, it looks like the code is detecting stack
> alignment code, and if it does, then it considers that the parameters
> will be relative to the arguments region address.
> 

That's correct.  Hummm, does gcc currently align the stack in
functions other than 'main' ?  If so, I'll have to check if this
is a 'main' only problem, or if it happens on other functions.

> I wonder how this all works if GCC < 4.1 is being used.
> 

Gcc 3.4.4-cygwin works ok and doesn't need this patch.
I'll have to build a few gcc's more to check that the
problem was introduced when the stack alignment was
introduced.

I was fearing that if the bug would be later fixed on
gcc side, we'd have no way to detect it.  I see some
movement at gcc@/gcc-patches@ about changing the stack
alignment scheme on i386.  That may be perfect.  If we get the
debug output fixed in the same release the prologue code
changes, all will be fine.

	* i386-tdep.c (struct i386_frame_cache): Rename saved_sp to
	prev_frame_sp.  Add saved_sp_regnum field.
	(i386_alloc_frame_cache): Update.
	(i386_analyze_stack_align): Record which register holds %esp in
	saved_sp_regnum.
	(i386_analyze_register_saves): Move higher on the file.
	(i386_analyze_frame_setup): Account for register saves before
	stack adjustment.
	(i386_frame_cache): If possible, prefer reading the register that
	holds the previous stack pointer from the stack .
	(i386_frame_prev_register): Update.

All these could go in independently of the below hunks, though.

	(i386_frame_args_address): New.
	(i386_frame_base): Set i386_frame_args_address as args method.


-- 
Pedro Alves

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Joel Brobecker]

> Hummm, does gcc currently align the stack in functions other than
> 'main' ?  If so, I'll have to check if this is a 'main' only problem,
> or if it happens on other functions.

I am not a compiler expert, but I remember a light discussion I was
having with Olivier Hainque, and he was mentioning alignment issues
when calling C subprograms for Ada subprograms. So this does suggest
that this is not a main-only problem.  To be confirmed, however...

> >I wonder how this all works if GCC < 4.1 is being used.
> >
> 
> Gcc 3.4.4-cygwin works ok and doesn't need this patch.

My concern at this point is whether GDB still works in this case
after you applied your patch. Unless GCC 3.4.4 doesn't emit the
stack-alignment code, it should no longer work... The questions
at this point are: Can we support both conventions? Do we even
want to?

The fact that this has nobody before you reported that this is
broken since 4.1 shows that this is probably not an extremely
important issue. Perhaps it's fine to only support the new
convention. To be discussed with the other maintainers, in
particular Mark Kettenis, who is the area maintainer on this.

> I was fearing that if the bug would be later fixed on
> gcc side, we'd have no way to detect it.

That too :-).

> I see some movement at gcc@/gcc-patches@ about changing the stack
> alignment scheme on i386.  That may be perfect.  If we get the debug
> output fixed in the same release the prologue code changes, all will
> be fine.

OK, so I will consider that this thread is currently on hold, pending
discussions in GCC.

> 	* i386-tdep.c (struct i386_frame_cache): Rename saved_sp to
> 	prev_frame_sp.  Add saved_sp_regnum field.
> 	(i386_alloc_frame_cache): Update.
> 	(i386_analyze_stack_align): Record which register holds %esp in
> 	saved_sp_regnum.
> 	(i386_analyze_register_saves): Move higher on the file.
> 	(i386_analyze_frame_setup): Account for register saves before
> 	stack adjustment.
> 	(i386_frame_cache): If possible, prefer reading the register that
> 	holds the previous stack pointer from the stack .
> 	(i386_frame_prev_register): Update.
> 
> All these could go in independently of the below hunks, though.

I would suggest you ask Mark. (resend him a new patch, I'm sure
he'll appreciate not having to sort which parts you want to be
included ;-).

-- 
Joel

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Pedro Alves]

Joel Brobecker wrote:
> 
>>> I wonder how this all works if GCC < 4.1 is being used.
>>>
>> Gcc 3.4.4-cygwin works ok and doesn't need this patch.
> 
> My concern at this point is whether GDB still works in this case
> after you applied your patch. Unless GCC 3.4.4 doesn't emit the
> stack-alignment code, it should no longer work... The questions
> at this point are: Can we support both conventions? Do we even
> want to?
> 

gcc 3.4.4 doesn't emit the stack-alignment code.  Unless
the comments in i386-tdep.c are wrong, any gcc < 4.1 doesn't emit
the stack-alignment code, which makes them immune to the patch.
I did regtest the patch on Cygwin.

> The fact that this has nobody before you reported that this is
> broken since 4.1 shows that this is probably not an extremely
> important issue. 

I've seen this reported at the mingw-users@ list with the
new gcc4.2 binaries.  As you know, mingw support wasn't in the
FSF tree, so problems seen on mingw weren't being reported here.

But sure, this isn't certainly my priority ;-)

>> I see some movement at gcc@/gcc-patches@ about changing the stack
>> alignment scheme on i386.  That may be perfect.  If we get the debug
>> output fixed in the same release the prologue code changes, all will
>> be fine.
> 
> OK, so I will consider that this thread is currently on hold, pending
> discussions in GCC.
> 

Even if gcc debug info isn't fixed, there will be several gcc
releases with the problem and (pending confirmation), no
release with stack alignment without the problem.

I'll try to break the patch further -- the setup for the
workaround is mostly bugfixing, and I'll come back to the
workaround later on if I'm still using a stabs outputting
compiler by then :-)

-- 
Pedro Alves

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Joel Brobecker]

> gcc 3.4.4 doesn't emit the stack-alignment code.  Unless
> the comments in i386-tdep.c are wrong, any gcc < 4.1 doesn't emit
> the stack-alignment code, which makes them immune to the patch.
> I did regtest the patch on Cygwin.

That's good to know.

> >The fact that this has nobody before you reported that this is
> >broken since 4.1 shows that this is probably not an extremely
> >important issue. 
> 
> I've seen this reported at the mingw-users@ list with the
> new gcc4.2 binaries.  As you know, mingw support wasn't in the
> FSF tree, so problems seen on mingw weren't being reported here.
> 
> But sure, this isn't certainly my priority ;-)

Just to be clear, I didn't mean to brush off the patch. What I meant
was that, if we break compatibility with old compilers, it isn't
going to be extremely damaging (given that other people have lived
with the same problem for so long already).

> Even if gcc debug info isn't fixed, there will be several gcc
> releases with the problem and (pending confirmation), no
> release with stack alignment without the problem.

The problem if they fix GCC is that we would then have to support
both conventions. If we can't auto-detect, then perhaps we'll have
to introduce a set command. Ugh :-(.

> I'll try to break the patch further -- the setup for the
> workaround is mostly bugfixing, and I'll come back to the
> workaround later on if I'm still using a stabs outputting
> compiler by then :-)

I definitely recommend DWARF. We have been using DWARF on Windows
for a couple of years, I think and it works great!

-- 
Joel

Re: [i386/stabs] Arguments of main on gcc >= 4.1

[Daniel Jacobowitz]

On Fri, Dec 28, 2007 at 01:19:54AM +0000, Pedro Alves wrote:
> That's correct.  Hummm, does gcc currently align the stack in
> functions other than 'main' ?  If so, I'll have to check if this
> is a 'main' only problem, or if it happens on other functions.

Just main.  I expect a future release will do this for other
functions, or at least offer a command-line option to.

-- 
Daniel Jacobowitz
CodeSourcery