[rfc] Teach i386 prologue reader about _alloca and __main

[rfc] Teach i386 prologue reader about _alloca and __main

[Pedro Alves]

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

Hi guys,

This is a new take on solving the issue described here:

http://www.sourceware.org/ml/gdb/2006-11/msg00140.html

On a nutshell, breaking on main is broken on Cygwin, and
all targets where gcc emits a call to __main in main,
right after the prologue.  __main is responsible for calling
the c++ constructors of global objects, and registering the
destructors of the same objects to run at exit.  This call
is inserted on targets that don't have an .init section.

http://sources.redhat.com/ml/gdb-patches/2006-12/msg00140.html

My first aproach tackled it by making i386 skip the prologue
using the debug info, and teaching gcc to put the line marker
that marks the end of the prologue after the __main call.

http://sources.redhat.com/ml/gdb-patches/2006-12/msg00140.html
http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00633.html

Mark didn't like it, because:
"The last time I tried using sals on i386, I simply encountered too
many cases where the line number information couldn't be trusted and
putting a breakpoint on a function that was defenitely called never
hit."
http://sources.redhat.com/ml/gdb-patches/2006-12/msg00375.html

To which Daniel responded:
"Yeah.  That's definitely the biggest risk.
I think the first step, for Pedro's specific problem, should be
to recognize the call to __main as special and skippable.  We can
play with sals later."

The attached patch does that - make __main skippable.  This version
doesn't need any gcc changes.

There are a few complications, though.  On NT, the stack must be grown
page wise, by touching a guard page.  Because of that we often see calls
to _alloca AKA __chkstk (gcc/config/i386/cygwin.asm) in the prologue:

http://cygwin.com/ml/cygwin-developers/2001-10/msg00108.html
http://cygwin.com/ml/cygwin-developers/2001-10/msg00118.html

There is another problem with the prologue analysing, which is visible
on i386 targets using stabs debug info on gcc >= 4.1.0.

eg:
int main (int argc, char **argv)
{

On stabs, the debugger only knows that argc is the first parameter to
the function and that its type is int.  It is up to the debugger
to know how to extract it from the frame.

Gcc >= 4.1 can put code in the prologue to realign the stack pointer.
In that case, the frame base will not be the same as the incoming args
address, so gdb prints garbage for the arguments:

main (argc=2280856, argv=0x61006198) at main.c:8

The prologue analyser already knows about a saved_sp, and that it
can be extracted from %ecx, but on cygwin, %ecx will be clobbered
by the __main call, before we get to user code:

int
main (int argc, char ** argv)
{
  401050:       8d 4c 24 04             lea    0x4(%esp),%ecx
   401054:       83 e4 f0                and    $0xfffffff0,%esp
   401057:       ff 71 fc                pushl  0xfffffffc(%ecx)
   40105a:       55                      push   %ebp
   40105b:       89 e5                   mov    %esp,%ebp
   40105d:       53                      push   %ebx
   40105e:       51                      push   %ecx
   40105f:       b8 20 40 00 00          mov    $0x4020,%eax
   401064:       e8 d7 00 00 00          call   401140 <___chkstk>
   401069:       89 cb                   mov    %ecx,%ebx
   40106b:       e8 60 01 00 00          call   4011d0 <___main>
   char asdf[0x4001];

   printf ("argc %d\n", argc);
   401070:       8b 03                   mov    (%ebx),%eax

Luckilly, %ecx is pushed on the stack, and we already know how to
get at it thanks to i386_analyze_register_saves.

Follows a listing of prologue patterns I see here on i386 Cygwin.

---

#include <stdio.h>

int
main (int argc, char ** argv)
{
   char asdf[0x4001];

   printf ("argc %d\n", argc);
...
}


gcc3.4.4 (cygming special) -O0

    _main:
    .stabn 68,0,8,LM1-_main
    LM1:
            pushl   %ebp               ;frame setup
            movl    %esp, %ebp         ;

            movl    $16440, %eax       ;local space
            call    __alloca           ;

            andl    $-16, %esp         ;alignment

            movl    $0, %eax           ;local space
            addl    $15, %eax          ;
            addl    $15, %eax          ;
            shrl    $4, %eax           ;
            sall    $4, %eax           ;
            movl    %eax, -16412(%ebp) ;
            movl    -16412(%ebp), %eax ;
            call    __alloca           ;
    .stabn 68,0,8,LM2-_main
    LM2:
            call    ___main            ;__main call
    .stabn 68,0,11,LM3-_main
    LM3:

---
gcc3.4.4 (cygming special) -O1

    _main:
    .stabn 68,0,8,LM1-_main
    LM1:
            pushl   %ebp               ;frame setup
            movl    %esp, %ebp         ;

            pushl   %ebx

            movl    $16420, %eax       ;local space
            call    __alloca           ;

            movl    8(%ebp), %ebx

            andl    $-16, %esp         ;alignment

            movl    $16, %eax          ;local space
            call    __alloca
    .stabn 68,0,8,LM2-_main
    LM2:
            call    ___main            ;__main call
    .stabn 68,0,11,LM3-_main
    LM3:

---
gcc3.4.4 (cygming special) -O2

    _main:
    .stabn 68,0,8,LM1-_main
    LM1:
            pushl   %ebp               ;frame setup
            movl    $16420, %eax       ;parameter to __alloca migrated here.
            movl    %esp, %ebp         ;frame setup
            pushl   %ebx
            call    __alloca
    .stabn 68,0,8,LM2-_main
    LM2:
            movl    8(%ebp), %ebx
            andl    $-16, %esp
            movl    $16, %eax
            call    __alloca
            call    ___main
    .stabn 68,0,11,LM3-_main
    LM3:

---
gcc 4.3.0 20070824 -O0

    _main:
            .stabd  46,0,0
            .stabn  68,0,8,LM0-LFBB1
    LM0:
    LFBB1:
            leal    4(%esp), %ecx      ;stack alignment
            andl    $-16, %esp         ;
            pushl   -4(%ecx)           ;

            pushl   %ebp               ;frame setup
            movl    %esp, %ebp         ;

            pushl   %ebx               ;register saves
            pushl   %ecx               ;

            movl    $16416, %eax       ;local space
            call    __alloca           ;

            movl    %ecx, %ebx
            .stabn  68,0,8,LM1-LFBB1
    LM1:
            call    ___main            ;__main call
            .stabn  68,0,11,LM2-LFBB1
    LM2:

gcc 4.3.0 20070824 -O1

    _main:
            .stabd  46,0,0
            .stabn  68,0,8,LM0-LFBB1
    LM0:
    LFBB1:
            leal    4(%esp), %ecx      ;stack alignment
            andl    $-16, %esp         ;
            pushl   -4(%ecx)           ;

            pushl   %ebp               ;frame setup
            movl    %esp, %ebp         ;

            pushl   %esi               ;register saves
            pushl   %ebx               ;register saves
            pushl   %ecx               ;register saves

            subl    $12, %esp          ;local space

            movl    (%ecx), %esi
            movl    4(%ecx), %ebx
            .stabn  68,0,8,LM1-LFBB1
    LM1:
            call    ___main            ;main call
            .stabn  68,0,11,LM2-LFBB1

gcc 4.3.0 20070824 -O2

    _main:
            .stabd  46,0,0
            .stabn  68,0,8,LM0-LFBB1
    LM0:
    LFBB1:
            leal    4(%esp), %ecx      ;stack alignment
            andl    $-16, %esp         ;
            pushl   -4(%ecx)           ;

            pushl   %ebp               ;frame setup
            movl    %esp, %ebp         ;

            pushl   %esi               ;register saves
            pushl   %ebx               ;register saves
            pushl   %ecx               ;register saves

            subl    $12, %esp          ;local space

            .stabn  68,0,8,LM1-LFBB1
    LM1:
            movl    (%ecx), %esi
            movl    4(%ecx), %ebx
            call    ___main            ;main call
            .stabn  68,0,11,LM2-LFBB1
    LM2:


Note that the standard compiler on Cygwin is still gcc3 based, and it
is unknown when it will switch to a newer one.

The patch teaches gdb about the -O0 level good behaved prologues,
and where easy makes it understand optimized ones.

The __main call is only expected on targets that gdb knows it should
be there.  A new gdbarch callback is introduced for it, and cygwin
registers it.
The patch intruduces a few minsyms lookups, which I'm not sure we
can do without.  It looks like the risk is high to get it wrong
otherwise.

If left a few things to clean up, as it would make the patch harder
to read.

With this patch, all the run_to_main related failures and
timeouts are gone, both with stock cygwin gcc, and with 4.3 trunk (*),
and we start to see the real failures that are masked currently.

(*) - In the mean time, cygming switched to dwarf debug info by default
in 4.3.  MinGW land has a gcc 4.2 based compiler in testing
with the prospect of becoming the officially supported compiler.

With nosignals on the board file, I get the FAIL rate down to
< 150, and no new problems.   Much better.

It will need a lot more testing and tweaking, but I'd like to get
comments before I spend more time in this.

If this looks too much, the gcc3 patterns support could be
dropped, as if the reader stops mid-line, the rest of the line is
skipped, and that at -O0 lands us in the __main call.  For gcc4, we
need to read the prologue, due to the need to know where the args
are on the stack - but gcc4 prologues are much more consistent.

So...

Comments?

Cheers,
Pedro Alves

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

2007-10-13  Pedro Alves  <pedro_alves@portugalmail.pt>

	* gdbarch.sh (gdbarch_skip___main_call): New.
	* gdbarch.h, gdbarch.c: Regenerate.

	* i386-tdep.h (i386_skip___main_call): Declare.
	* i386-tdep.c (i386_skip_alloca_call, i386_analyze_alloca2)
	(i386_analyze_alloca): New functions.
	(i386_analyze_frame_setup): Account for register saves before
	stack adjustment, for stack adjustment with _alloca and for stack
	adjustment with andl.  Relax the insns requence.
	(i386___main_skip_insns): New.
	(i386_skip___main_call): New.
	(i386_analyze_prologue): Call i386_analyze_alloca.
	(i386_skip_prologue): Skip calls to __main on targets where gcc
	emits that call.
	(i386_frame_cache): If needed, extract %ecx from the stack.
	(i386_frame_args_address): New.
	(i386_frame_base): Set i386_frame_args_address as args method.
	
	* i386-cygwin-tdep.c (i386_cygwin_init_abi): Register
	i386_skip___main_call as gdbarch_skip___main_call gdbarch
	callback.

---
 gdb/gdbarch.c          |   33 ++++
 gdb/gdbarch.h          |    6 
 gdb/gdbarch.sh         |    1 
 gdb/i386-cygwin-tdep.c |    2 
 gdb/i386-tdep.c        |  341 ++++++++++++++++++++++++++++++++++++++++++++++---
 gdb/i386-tdep.h        |    3 
 6 files changed, 368 insertions(+), 18 deletions(-)

Index: src/gdb/i386-tdep.c
===================================================================
--- src.orig/gdb/i386-tdep.c	2007-10-13 01:51:08.000000000 +0100
+++ src/gdb/i386-tdep.c	2007-10-13 20:32:54.000000000 +0100
@@ -579,6 +579,185 @@ i386_match_insn (CORE_ADDR pc, struct i3
   return NULL;
 }
 
+/* Check that the code pointed to by PC corresponds to a call to
+   __chkstk/_alloca and skip it if so.  Return PC otherwise.  */
+
+static CORE_ADDR
+i386_skip_alloca_call (CORE_ADDR pc, CORE_ADDR limit)
+{
+  gdb_byte op;
+
+  /* If we don't have enough code for a call, return now.  */
+  if (pc + 5 > limit)
+    return pc;
+
+  read_memory_nobpt (pc, &op, 1);
+  if (op == 0xe8)
+    {
+      gdb_byte buf[4];
+      if (target_read_memory (pc + 1, buf, sizeof buf) == 0)
+	{
+	  CORE_ADDR call_dest = pc + 5 + extract_unsigned_integer (buf, 4);
+
+	  struct minimal_symbol *s =
+	    lookup_minimal_symbol_by_pc (call_dest);
+	  if (s != NULL
+	      && SYMBOL_LINKAGE_NAME (s) != NULL
+	      && (strcmp (SYMBOL_LINKAGE_NAME (s), "__chkstk") == 0
+		  || strcmp (SYMBOL_LINKAGE_NAME (s), "_alloca") == 0))
+	    pc += 5;
+	}
+    }
+
+  return pc;
+}
+
+/* Check that the code pointed to by PC corresponds to a call to
+   __chkstk/_alloca and skip it if so.  Return PC otherwise.  */
+
+static CORE_ADDR
+i386_analyze_alloca2 (CORE_ADDR pc, CORE_ADDR limit,
+		      struct i386_frame_cache *cache)
+{
+  /* On gcc3 ,and at -O0, the usual sequence in main is given by
+     setting %eax, storing/retrieving it (this is unoptimized code),
+     and then an _alloca/__chkstk call, which takes it's parameter in
+     %eax:
+
+     eg:
+    mov    $0x0,%eax
+    add    $0xf,%eax
+    add    $0xf,%eax
+    shr    $0x4,%eax
+    shl    $0x4,%eax
+    mov %eax,xxx(%ebp)
+    mov xxx(%ebp),%eax
+    call __alloca
+
+    Also, if local variables take more than a page (4k), the stack
+    adjustment will be done using _alloca, because of the need to
+    touch the guard page on NT.  The sequence will be:
+
+    mov $xxx,%eax
+    call __alloca
+
+    That's valid for main, and for every other function.  At -O1,
+    long sequence above turns into the second sequence (mov,call).
+
+    On gcc3, at -O2 and above, the `mov $xxx,%eax' is often migrated
+    into the frame setup, but we don't support that currently.  Eg:
+
+    push   %ebp
+    mov    $0x10,%eax
+    mov    %esp,%ebp
+    sub    $0x8,%esp
+    and    $0xfffffff0,%esp
+    call   __alloca
+    call   ___main
+
+  */
+
+  gdb_byte op;
+
+  const gdb_byte insns[] = {
+    0xb8, 0x00, 0x00, 0x00, 0x00,    /* mov    $0x0,%eax */
+    0x83, 0xc0, 0x0f,                /* add    $0xf,%eax */
+    0x83, 0xc0, 0x0f,                /* add    $0xf,%eax */
+    0xc1, 0xe8, 0x04,                /* shr    $0x4,%eax */
+    0xc1, 0xe0, 0x04,                /* shl    $0x4,%eax */
+  };
+  gdb_byte buf[sizeof insns];
+
+  if (target_read_memory (pc, buf, sizeof buf) == 0
+      && memcmp (buf, insns, sizeof insns) == 0)
+    {
+      CORE_ADDR pc2 = pc + sizeof insns;
+      CORE_ADDR pos;
+
+      /* mov %eax,xxx(%ebp) */
+      read_memory_nobpt (pc2, &op, 1);
+      if (op == 0x89)
+	{
+	  read_memory_nobpt (pc2 + 1, &op, 1);
+	  switch (op)
+	    {
+	    case 0x45:
+	      pc2 += 3;
+	      break;
+	    case 0x85:
+	      pc2 += 6;
+	      break;
+	    default:
+	      goto other;
+	    }
+	}
+      else
+	goto other;
+
+      /* mov xxx(%ebp),%eax */
+      read_memory_nobpt (pc2, &op, 1);
+      if (op == 0x8b)
+	{
+	  read_memory_nobpt (pc2 + 1, &op, 1);
+	  switch (op)
+	    {
+	    case 0x45:
+	      pc2 += 3;
+	      break;
+	    case 0x85:
+	      pc2 += 6;
+	      break;
+	    default:
+	      goto other;
+	    }
+	}
+
+      pos = i386_skip_alloca_call (pc2, limit);
+      if (pos != pc2)
+	{
+	  cache->locals += 0x10;
+	  pc = pos;
+
+	  if (limit <= pc)
+	    return limit;
+	}
+    }
+
+ other:
+  /* mov $xxx,%eax */
+  read_memory_nobpt (pc, &op, 1);
+  if (op == 0xb8)
+    {
+      CORE_ADDR pos = i386_skip_alloca_call (pc + 5, limit);
+      if (pos != pc + 5)
+	{
+	  ULONGEST eax = read_memory_unsigned_integer (pc + 1, 4);
+	  cache->locals += eax;
+	  pc = pos;
+
+	  if (limit <= pc)
+	    return limit;
+	}
+    }
+
+  return pc;
+}
+
+/* Check that the code pointed to by PC corresponds to a call to
+   __chkstk/_alloca and skip it if so.  Return PC otherwise.  */
+
+static CORE_ADDR
+i386_analyze_alloca (CORE_ADDR pc, CORE_ADDR limit,
+		     struct i386_frame_cache *cache)
+{
+  CORE_ADDR next_pc;
+
+  while ((next_pc = i386_analyze_alloca2 (pc, limit, cache)) != pc)
+    pc = next_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
@@ -634,6 +813,10 @@ struct i386_insn i386_frame_setup_skip_i
   { 0 }
 };
 
+static CORE_ADDR
+i386_analyze_register_saves (CORE_ADDR pc, CORE_ADDR current_pc,
+			     struct i386_frame_cache *cache);
+
 /* Check whether PC points at a code that sets up a new stack frame.
    If so, it updates CACHE and returns the address of the first
    instruction after the sequence that sets up the frame or LIMIT,
@@ -713,7 +896,11 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
       if (limit <= pc)
 	return limit;
 
-      /* Check for stack adjustment 
+      /* Check for registers saves seen before stack adjustment.  Seen
+       on targets that emit a call to __main.  */
+      pc = i386_analyze_register_saves (pc, limit, cache);
+
+      /* Check for stack adjustment
 
 	    subl $XXX, %esp
 
@@ -730,7 +917,7 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
 	  /* `subl' with signed 8-bit immediate (though it wouldn't
 	     make sense to be negative).  */
 	  cache->locals = read_memory_integer (pc + 2, 1);
-	  return pc + 3;
+	  pc += 3;
 	}
       else if (op == 0x81)
 	{
@@ -741,13 +928,26 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
 
 	  /* It is `subl' with a 32-bit immediate.  */
 	  cache->locals = read_memory_integer (pc + 2, 4);
-	  return pc + 6;
-	}
-      else
-	{
-	  /* Some instruction other than `subl'.  */
-	  return pc;
+	  pc += 6;
 	}
+
+      /* _alloca call */
+      pc = i386_analyze_alloca (pc, limit, cache);
+
+      /* and $-16,%esp */
+      {
+	const gdb_byte insns[] = {
+	  0x83, 0xe4, 0xf0                /* and    $0xfffffff0,%esp */
+	};
+	gdb_byte buf[sizeof insns];
+
+	if (target_read_memory (pc, buf, sizeof buf) == 0
+	    && memcmp (buf, insns, sizeof insns) == 0)
+	  {
+	    cache->locals &= -16L;
+	    return pc + 3;
+	  }
+      }
     }
   else if (op == 0xc8)		/* enter */
     {
@@ -758,6 +958,73 @@ i386_analyze_frame_setup (CORE_ADDR pc, 
   return pc;
 }
 
+/* Some instructions that might be migrated by GCC into after the
+   prologue and just before the __main call.  */
+
+struct i386_insn i386___main_skip_insns[] =
+{
+  /* `mov %ecx,%ebx' */
+  { 2, { 0x89, 0xcb }, { 0xff, 0xff } },
+
+  /* `mov (%ecx),%esi' */
+  { 2, { 0x8b, 0x31 }, { 0xff, 0xff } },
+
+  /* `mov  xxx(%ecx),%ebx' */
+  { 3, { 0x8b, 0x59 }, { 0xff, 0xff } },
+
+  { 0 }
+};
+
+
+/* Check that the code pointed to by PC corresponds to a call to
+   __main, skip it if so.  Return PC otherwise.  */
+
+CORE_ADDR
+i386_skip___main_call (struct gdbarch *gdbarch, CORE_ADDR pc)
+{
+  gdb_byte op;
+  int skip = 0;
+  CORE_ADDR limit = pc + 20;
+
+  /* Check for some instructions that might be migrated by
+     GCC into before the __main call and skip them.
+
+     Make sure we only skip these instructions if we later see the
+     `call ___main'.  */
+  while (pc + skip < limit)
+    {
+      struct i386_insn *insn;
+      insn = i386_match_insn (pc + skip, i386___main_skip_insns);
+      if (insn == NULL)
+	break;
+
+      skip += insn->len;
+    }
+
+  /* If that's all, return now.  */
+  if (limit <= pc)
+    return limit;
+
+  read_memory_nobpt (pc + skip, &op, 1);
+  if (op == 0xe8)
+    {
+      gdb_byte buf[4];
+      if (target_read_memory (pc + skip + 1, buf, sizeof buf) == 0)
+	{
+	  CORE_ADDR call_dest = pc + skip + 5 + extract_unsigned_integer (buf, 4);
+
+	  struct minimal_symbol *s =
+	    lookup_minimal_symbol_by_pc (call_dest);
+	  if (s != NULL
+	      && SYMBOL_LINKAGE_NAME (s) != NULL
+	      && strcmp (SYMBOL_LINKAGE_NAME (s), "__main") == 0)
+	    pc += skip + 5;
+	}
+    }
+
+  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
@@ -802,16 +1069,18 @@ i386_analyze_register_saves (CORE_ADDR p
    once used in the System V compiler).
 
    Local space is allocated just below the saved %ebp by either the
-   'enter' instruction, or by "subl $<size>, %esp".  'enter' has a
-   16-bit unsigned argument for space to allocate, and the 'addl'
-   instruction could have either a signed byte, or 32-bit immediate.
+   'enter' instruction, or by "subl $<size>, %esp", or by a call to
+   _alloca (libgcc/Cygwin).  'enter' has a 16-bit unsigned argument
+   for space to allocate; the 'addl' instruction could have either a
+   signed byte, or 32-bit immediate; the _alloca call is passed the
+   stack space to allocate in %eax.
 
    Next, the registers used by this function are pushed.  With the
    System V compiler they will always be in the order: %edi, %esi,
    %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.  */
 
@@ -824,7 +1093,10 @@ i386_analyze_prologue (CORE_ADDR pc, COR
   pc = i386_skip_probe (pc);
   pc = i386_analyze_stack_align (pc, current_pc, cache);
   pc = i386_analyze_frame_setup (pc, current_pc, cache);
-  return i386_analyze_register_saves (pc, current_pc, cache);
+  pc = i386_analyze_register_saves (pc, current_pc, cache);
+  pc = i386_analyze_alloca (pc, current_pc, cache);
+
+  return pc;
 }
 
 /* Return PC of first real instruction.  */
@@ -902,6 +1174,17 @@ i386_skip_prologue (CORE_ADDR start_pc)
   if (i386_follow_jump (start_pc) != start_pc)
     pc = i386_follow_jump (pc);
 
+  /* Skip __main calls in main.  */
+  if (gdbarch_skip___main_call_p (current_gdbarch))
+    {
+      struct minimal_symbol *mainf = lookup_minimal_symbol_by_pc (start_pc);
+
+      if (mainf != NULL
+	  && SYMBOL_LINKAGE_NAME (mainf)
+	  && strcmp (SYMBOL_LINKAGE_NAME (mainf), "main") == 0)
+	pc = gdbarch_skip___main_call (current_gdbarch, pc);
+    }
+
   return pc;
 }
 
@@ -956,8 +1239,19 @@ 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);
+
+      /* If we are analysing a `main', and this target emits __main
+	 calls, %ecx will be clobbered by __main, and possibly pushed
+	 on the stack.  */
+      if (cache->saved_regs[I386_ECX_REGNUM] != -1)
+	{
+	  CORE_ADDR ecx = cache->base + cache->saved_regs[I386_ECX_REGNUM];
+	  read_memory (ecx, buf, 4);
+	}
+      else
+	frame_unwind_register (next_frame, I386_ECX_REGNUM, buf);
+
+      cache->saved_sp = extract_unsigned_integer (buf, 4);
     }
 
   if (cache->locals < 0)
@@ -1233,12 +1527,23 @@ 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)
+    return cache->saved_sp;
+
+  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
Index: src/gdb/gdbarch.c
===================================================================
--- src.orig/gdb/gdbarch.c	2007-10-12 20:25:22.000000000 +0100
+++ src/gdb/gdbarch.c	2007-10-13 03:52:14.000000000 +0100
@@ -186,6 +186,7 @@ struct gdbarch
   gdbarch_store_return_value_ftype *store_return_value;
   gdbarch_deprecated_use_struct_convention_ftype *deprecated_use_struct_convention;
   gdbarch_skip_prologue_ftype *skip_prologue;
+  gdbarch_skip___main_call_ftype *skip___main_call;
   gdbarch_inner_than_ftype *inner_than;
   gdbarch_breakpoint_from_pc_ftype *breakpoint_from_pc;
   gdbarch_adjust_breakpoint_address_ftype *adjust_breakpoint_address;
@@ -308,6 +309,7 @@ struct gdbarch startup_gdbarch =
   0,  /* store_return_value */
   0,  /* deprecated_use_struct_convention */
   0,  /* skip_prologue */
+  0,  /* skip___main_call */
   0,  /* inner_than */
   0,  /* breakpoint_from_pc */
   0,  /* adjust_breakpoint_address */
@@ -553,6 +555,7 @@ verify_gdbarch (struct gdbarch *current_
   /* Skip verify of deprecated_use_struct_convention, invalid_p == 0 */
   if (current_gdbarch->skip_prologue == 0)
     fprintf_unfiltered (log, "\n\tskip_prologue");
+  /* Skip verify of skip___main_call, has predicate */
   if (current_gdbarch->inner_than == 0)
     fprintf_unfiltered (log, "\n\tinner_than");
   if (current_gdbarch->breakpoint_from_pc == 0)
@@ -966,6 +969,12 @@ gdbarch_dump (struct gdbarch *current_gd
                       "gdbarch_dump: single_step_through_delay = <0x%lx>\n",
                       (long) current_gdbarch->single_step_through_delay);
   fprintf_unfiltered (file,
+                      "gdbarch_dump: gdbarch_skip___main_call_p() = %d\n",
+                      gdbarch_skip___main_call_p (current_gdbarch));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: skip___main_call = <0x%lx>\n",
+                      (long) current_gdbarch->skip___main_call);
+  fprintf_unfiltered (file,
                       "gdbarch_dump: gdbarch_skip_permanent_breakpoint_p() = %d\n",
                       gdbarch_skip_permanent_breakpoint_p (current_gdbarch));
   fprintf_unfiltered (file,
@@ -2136,6 +2145,30 @@ set_gdbarch_skip_prologue (struct gdbarc
 }
 
 int
+gdbarch_skip___main_call_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->skip___main_call != NULL;
+}
+
+CORE_ADDR
+gdbarch_skip___main_call (struct gdbarch *gdbarch, CORE_ADDR ip)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->skip___main_call != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_skip___main_call called\n");
+  return gdbarch->skip___main_call (gdbarch, ip);
+}
+
+void
+set_gdbarch_skip___main_call (struct gdbarch *gdbarch,
+                              gdbarch_skip___main_call_ftype skip___main_call)
+{
+  gdbarch->skip___main_call = skip___main_call;
+}
+
+int
 gdbarch_inner_than (struct gdbarch *gdbarch, CORE_ADDR lhs, CORE_ADDR rhs)
 {
   gdb_assert (gdbarch != NULL);
Index: src/gdb/gdbarch.h
===================================================================
--- src.orig/gdb/gdbarch.h	2007-10-12 20:25:22.000000000 +0100
+++ src/gdb/gdbarch.h	2007-10-13 03:45:06.000000000 +0100
@@ -388,6 +388,12 @@ typedef CORE_ADDR (gdbarch_skip_prologue
 extern CORE_ADDR gdbarch_skip_prologue (struct gdbarch *gdbarch, CORE_ADDR ip);
 extern void set_gdbarch_skip_prologue (struct gdbarch *gdbarch, gdbarch_skip_prologue_ftype *skip_prologue);
 
+extern int gdbarch_skip___main_call_p (struct gdbarch *gdbarch);
+
+typedef CORE_ADDR (gdbarch_skip___main_call_ftype) (struct gdbarch *gdbarch, CORE_ADDR ip);
+extern CORE_ADDR gdbarch_skip___main_call (struct gdbarch *gdbarch, CORE_ADDR ip);
+extern void set_gdbarch_skip___main_call (struct gdbarch *gdbarch, gdbarch_skip___main_call_ftype *skip___main_call);
+
 typedef int (gdbarch_inner_than_ftype) (CORE_ADDR lhs, CORE_ADDR rhs);
 extern int gdbarch_inner_than (struct gdbarch *gdbarch, CORE_ADDR lhs, CORE_ADDR rhs);
 extern void set_gdbarch_inner_than (struct gdbarch *gdbarch, gdbarch_inner_than_ftype *inner_than);
Index: src/gdb/gdbarch.sh
===================================================================
--- src.orig/gdb/gdbarch.sh	2007-10-12 20:25:22.000000000 +0100
+++ src/gdb/gdbarch.sh	2007-10-13 03:42:14.000000000 +0100
@@ -514,6 +514,7 @@ f::void:store_return_value:struct type *
 f::int:deprecated_use_struct_convention:int gcc_p, struct type *value_type:gcc_p, value_type::generic_use_struct_convention::0
 
 f::CORE_ADDR:skip_prologue:CORE_ADDR ip:ip:0:0
+M::CORE_ADDR:skip___main_call:CORE_ADDR ip:ip
 f::int:inner_than:CORE_ADDR lhs, CORE_ADDR rhs:lhs, rhs:0:0
 f::const gdb_byte *:breakpoint_from_pc:CORE_ADDR *pcptr, int *lenptr:pcptr, lenptr::0:
 M::CORE_ADDR:adjust_breakpoint_address:CORE_ADDR bpaddr:bpaddr
Index: src/gdb/i386-cygwin-tdep.c
===================================================================
--- src.orig/gdb/i386-cygwin-tdep.c	2007-09-22 17:08:46.000000000 +0100
+++ src/gdb/i386-cygwin-tdep.c	2007-10-13 03:22:58.000000000 +0100
@@ -225,6 +225,8 @@ i386_cygwin_init_abi (struct gdbarch_inf
 
   set_gdbarch_skip_trampoline_code (gdbarch, i386_cygwin_skip_trampoline_code);
 
+  set_gdbarch_skip___main_call (gdbarch, i386_skip___main_call);
+
   tdep->struct_return = reg_struct_return;
 
   tdep->gregset_reg_offset = i386_win32_gregset_reg_offset;
Index: src/gdb/i386-tdep.h
===================================================================
--- src.orig/gdb/i386-tdep.h	2007-09-22 17:08:46.000000000 +0100
+++ src/gdb/i386-tdep.h	2007-10-13 03:29:48.000000000 +0100
@@ -167,6 +167,9 @@ extern struct type *i386_sse_type (struc
 /* Functions exported from i386-tdep.c.  */
 extern CORE_ADDR i386_pe_skip_trampoline_code (CORE_ADDR pc, char *name);
 
+extern CORE_ADDR i386_skip___main_call (struct gdbarch *gdbarch, CORE_ADDR pc);
+
+
 /* Return the name of register REGNUM.  */
 extern char const *i386_register_name (int regnum);
 

Re: [rfc] Teach i386 prologue reader about _alloca and __main

[Daniel Jacobowitz]

On Sat, Oct 13, 2007 at 08:54:45PM +0100, Pedro Alves wrote:
> 	* gdbarch.sh (gdbarch_skip___main_call): New.
> 	* gdbarch.h, gdbarch.c: Regenerate.

I don't have time or experience to look at this thoroughly right now,
but I did notice this: you can just put this in i386's gdbarch_tdep.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Teach i386 prologue reader about _alloca and __main

[Pedro Alves]

Daniel Jacobowitz wrote:
> On Sat, Oct 13, 2007 at 08:54:45PM +0100, Pedro Alves wrote:
>> 	* gdbarch.sh (gdbarch_skip___main_call): New.
>> 	* gdbarch.h, gdbarch.c: Regenerate.
> 
> I don't have time or experience to look at this thoroughly right now,
> but I did notice this: you can just put this in i386's gdbarch_tdep.
> 

The __main call is not i386 specific.  Eg. arm-wince has the same problem.

More precisely, it matches the targets where this is true in gcc/function.c.

void
expand_main_function (void)
{
#if (defined(INVOKE__main)                              \
      || (!defined(HAS_INIT_SECTION)                     \
          && !defined(INIT_SECTION_ASM_OP)               \
          && !defined(INIT_ARRAY_SECTION_ASM_OP)))
   emit_library_call (init_one_libfunc (NAME__MAIN), LCT_NORMAL, VOIDmode, 0);
#endif
}

I've also experimented with calling skip__main_call from
find_function_start_sal, but later moved it into i386-tdep.c.
That's still an open issue for me.

--- symtab.c    2007-10-13 20:08:38.000000000 +0100
+++ symtab.c.skip_main  2007-10-13 20:08:32.000000000 +0100
@@ -2493,6 +2493,16 @@ find_function_start_sal (struct symbol *
        /* Recalculate the line number (might not be N+1).  */
        sal = find_pc_sect_line (pc, SYMBOL_BFD_SECTION (sym), 0);
      }
+
+  /* Skip __main call in targets without .init section and/or that
+   emit a call to __main in main as the first thing after the
+   prologue, before user code.  */
+  if (funfirstline
+      && gdbarch_skip___main_call_p (current_gdbarch)
+      && SYMBOL_LINKAGE_NAME (sym)
+      && strcmp (SYMBOL_LINKAGE_NAME (sym), "main") == 0)
+    pc = gdbarch_skip___main_call (current_gdbarch, pc);
+
    sal.pc = pc;

    return sal;

I'll experiment a bit more with it.

I'm mostly asking for advice on:

- Is the gdb-only solution preferable?  That is, the line info
   currently marks the prologue ending *before* the __main call.
   Last year's version changed that, this one doesn't.  Does anyone
   see any problem with that?

Cheers,
Pedro Alves

Re: [rfc] Teach i386 prologue reader about _alloca and __main

[Daniel Jacobowitz]

On Sat, Oct 13, 2007 at 10:12:25PM +0100, Pedro Alves wrote:
> The __main call is not i386 specific.  Eg. arm-wince has the same problem.

Yes, but it's only set in i386-specific code and only checked in
i386-specific code; so if it's just a variable in the tdep struct,
then ARM can have a similar one.

If you want to handle it from common code, of course, that's a
different story.

> I'm mostly asking for advice on:
> 
> - Is the gdb-only solution preferable?  That is, the line info
>   currently marks the prologue ending *before* the __main call.
>   Last year's version changed that, this one doesn't.  Does anyone
>   see any problem with that?

I have no strong preference either way.

-- 
Daniel Jacobowitz
CodeSourcery

Re: [rfc] Teach i386 prologue reader about _alloca and __main

[Mark Kettenis]

> Date: Sat, 13 Oct 2007 20:54:45 +0100
> From: Pedro Alves <pedro_alves@portugalmail.pt>
> 
> Hi guys,
> 
> This is a new take on solving the issue described here:
> 
> http://www.sourceware.org/ml/gdb/2006-11/msg00140.html
> 
> On a nutshell, breaking on main is broken on Cygwin, and
> all targets where gcc emits a call to __main in main,
> right after the prologue.  __main is responsible for calling
> the c++ constructors of global objects, and registering the
> destructors of the same objects to run at exit.  This call
> is inserted on targets that don't have an .init section.

Do I understand correctly that this is not really Cygwin specific, but
done for all targets with executable formats that don't have a concept
of constructors (like ELF with .init)?

> There is another problem with the prologue analysing, which is visible
> on i386 targets using stabs debug info on gcc >= 4.1.0.
> 
> eg:
> int main (int argc, char **argv)
> {
> 
> On stabs, the debugger only knows that argc is the first parameter to
> the function and that its type is int.  It is up to the debugger
> to know how to extract it from the frame.
>
> Gcc >= 4.1 can put code in the prologue to realign the stack pointer.
> In that case, the frame base will not be the same as the incoming args
> address, so gdb prints garbage for the arguments:
> 
> main (argc=2280856, argv=0x61006198) at main.c:8

This is why you added i386_frame_args_address() isn't it?  I thought
GCC would actually copy the arguments in that case, but apperently
that doesn't happen (anymore).  Perhaps I'm just mistaken though.

It's probably best to seperate this bit form the __main/_alloca mess.

> The __main call is only expected on targets that gdb knows it should
> be there.  A new gdbarch callback is introduced for it, and cygwin
> registers it.

Like Daniel said already, unless you expect this gdbarch callback to
be necessary on other architectures besides i386/amd64 it's better to
make this a function pointer in 'struct i386_tdep'.

> The patch intruduces a few minsyms lookups, which I'm not sure we
> can do without.  It looks like the risk is high to get it wrong
> otherwise.

As long as they only happen on platforms that suffer from __main I
don't care ;).

> Comments?

I'm not too happy about this.  It adds a lot of complexity for
something I think the compiler should emit proper debug information
for.  Did you raise the issue with the GCC people?  What was their
answer?

I think the alloca analysis should only happen on targets that need
it.  I wonder why GCC needs to make a library call, where it seems
simply touching every page would be enough.

Also, in i386_frame_cache(), I'd make the comment about where to
finding %ecx a bit more general.  I can think of other reasons why
that register might have been pushed onto the stack.  Might as well
seperate out that bit from the diff too.

Re: [rfc] Teach i386 prologue reader about _alloca and __main

[Pedro Alves]

Mark Kettenis wrote:
> 
> Do I understand correctly that this is not really Cygwin specific, but
> done for all targets with executable formats that don't have a concept
> of constructors (like ELF with .init)?
> 

Correct.


>> Gcc >= 4.1 can put code in the prologue to realign the stack pointer.
>> In that case, the frame base will not be the same as the incoming args
>> address, so gdb prints garbage for the arguments:
>>
>> main (argc=2280856, argv=0x61006198) at main.c:8
> 
> This is why you added i386_frame_args_address() isn't it?  I thought
> GCC would actually copy the arguments in that case, but apperently
> that doesn't happen (anymore).  Perhaps I'm just mistaken though.
> 

Yes, that's why I added it.  Humm, I'll have to try it out on
another target to see if its a Cygwin specific gcc problem.

> It's probably best to seperate this bit form the __main/_alloca mess.
> 

Will do.

>> Comments?
> 
> I'm not too happy about this.  It adds a lot of complexity for
> something I think the compiler should emit proper debug information
> for.  Did you raise the issue with the GCC people?  What was their
> answer?
> 

I can't say I am happy either.
I'll see if I can simplify it a bit, by only caring about -O0.

I did raise it with GCC almost a year ago, in the form of a patch:
http://gcc.gnu.org/ml/gcc-patches/2006-12/msg00633.html

Got no replies, but then again, I never pinged it.

We have to live with current compilers, though.

> I think the alloca analysis should only happen on targets that need
> it.  I wonder why GCC needs to make a library call, where it seems
> simply touching every page would be enough.
> 

Dunno.  The MSFT version of __chkstk supposedly compares the amount
of stack space a function requires with the amount the stack space
available, specified in the EXE header, and I guess specified on thread
creation.

http://support.microsoft.com/?scid=kb%3Ben-us%3B100775&x=10&y=10

gcc's version doesn't, so, it looks like work waiting for a volunteer.

> Also, in i386_frame_cache(), I'd make the comment about where to
> finding %ecx a bit more general.  I can think of other reasons why
> that register might have been pushed onto the stack.  Might as well
> seperate out that bit from the diff too.
> 

Will do.  Thanks.

Cheers,
Pedro Alves

[RFC] testsuite: Skip over function prologue in runto and runto_main

[Pierre Muller]

  I was rereading the thread:
http://sourceware.org/ml/gdb-patches/2007-10/msg00361.html
 but if I understood things correctly, it would mean
that if your patch goes in, and I use the 
'start' command, I would end up
past the call to __main.
  This might be an annoying change for people 
used, like me, to either 'step' or 'next'
after 'start' depending on if they want to analyze
what is going on in the startup code.

  Thus, I am asking:
Why do you consider this as broken?
Is it just because this is the reason of a lot of
cygwin testsuite failures?

  If this is the reason, I would propose another route
to solve this:
  Modify gdb.exp runto_main function
so that if it ends up at a line
containing only an open brace, it will 
know that there is some implicit code
that needs to be executed and just issue a 'next' command
to reach the point that is reached directly in
other systems.

  I just tested that change,
and the results are not bad:
  +266 PASS
  -104 FAIL



$ head gdbexp-testsuite-results.diff
--- gdb-cvs-gdbexp.sum  2007-12-04 18:18:56.171875000 +0100
+++ gdb-gdbexp.sum      2007-12-05 09:56:57.687500000 +0100
@@ -1,4 +1,4 @@
-Test Run By Pierre on Tue Dec  4 17:29:46 2007
+Test Run By Pierre on Tue Dec  4 18:32:45 2007
 Native configuration is i686-pc-cygwin

$ tail -20  gdbexp-testsuite-results.diff
 PASS: gdb.trace/while-dyn.exp: Current target does not support trace
 Running ../../../purecvs/gdb/testsuite/gdb.trace/while-stepping.exp ...
 PASS: gdb.trace/while-stepping.exp: 5.12: set a tracepoint, stepcount is
zero
@@ -11473,12 +11634,12 @@

                === gdb Summary ===

-# of expected passes           10288
-# of unexpected failures       503
+# of expected passes           10554
+# of unexpected failures       399
 # of expected failures         59
 # of known failures            29
-# of unresolved testcases      38
+# of unresolved testcases      37
 # of untested testcases                15
-# of unsupported tests         17
+# of unsupported tests         18
 /usr/local/src/cvs/purebuild/gdb/testsuite/../../gdb/gdb version
6.7.50.200712
03-cvs -nx

Here is the proposed patch:

  It basically defines two new globals variables in gdb.exp:
1)  skip_function_prologue
  that should be "yes" if gdb should execute a 'next' statement
if after the 'runto' procedure it arrives at a line that contains
only an opening brace.

2)  skip_main_function_prologue
  Which does the same, but only within runto_main procedure.
This variable is set to "yes" if EXEEXT env is different from "".
But this is probably not the right condition.
I have no good idea how to set that variable, but
maybe setting it unconditionally would be OK too.


  The main problem is that the pattern used to figure out if 
we are still in the function prologue is probably not optimal
and requires that we do put the open brace alone on a separate
line, but that seems to be always the case (at least for the main function)
in all the test sources.
  Is it a coding style requirement?

  The global skip_function_prologue could be used for a
more general approach, like if we test programs that have been
compiled with profiling on.

  Comments greatly appreciated.

Pierre Muller

ChangeLog entry:

	* lib/gdb.exp (skip_function_prologue, skip_main_function_prologue):
New variables.
	(runto): Execute a 'next' command if the breakpoint
	is at the line containing only an open brace if
skip_function_prologue is "yes".
	(runto_main): Idem if skip_main_function_prologue is "yes".


$ cat gdbexp.patch
Index: gdb/testsuite/lib/gdb.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/lib/gdb.exp,v
retrieving revision 1.92
diff -u -p -r1.92 gdb.exp
--- gdb/testsuite/lib/gdb.exp   30 Oct 2007 19:23:18 -0000      1.92
+++ gdb/testsuite/lib/gdb.exp   5 Dec 2007 09:13:02 -0000
@@ -29,7 +29,6 @@ if {$tool == ""} {
 load_lib libgloss.exp

 global GDB
-
 if [info exists TOOL_EXECUTABLE] {
     set GDB $TOOL_EXECUTABLE;
 }
@@ -48,6 +47,16 @@ if ![info exists GDBFLAGS] {
 }
 verbose "using GDBFLAGS = $GDBFLAGS" 2

+global skip_function_prologue
+if ![info exists skip_function_prologue] {
+    set skip_function_prologue "no"
+}
+global skip_main_function_prologue
+if ![info exists skip_main_function_prologue] {
+    set skip_main_function_prologue "no"
+}
+
+
 # The variable gdb_prompt is a regexp which matches the gdb prompt.
 # Set it if it is not already set.
 global gdb_prompt
@@ -83,6 +92,9 @@ if ![info exists env(EXEEXT)] {
     set EXEEXT ""
 } else {
     set EXEEXT $env(EXEEXT)
+    if ( "$EXEEXT" != "" ) {
+      set skip_main_function_prologue "yes"
+    }
 }

 ### Only procedures should come after this point.
@@ -371,6 +383,7 @@ proc gdb_breakpoint { function args } {
 proc runto { function args } {
     global gdb_prompt
     global decimal
+    global skip_function_prologue

     delete_breakpoints

@@ -383,6 +396,23 @@ proc runto { function args } {
     # the "at foo.c:36" output we get with -g.
     # the "in func" output we get without -g.
     gdb_expect 30 {
+       -re "Break.* at .*:$decimal.*\{\[\r\n\]*$gdb_prompt $" {
+            # we are in the function prologue, let's skip it
+           if { "$skip_function_prologue" == "yes" } {
+               verbose "Skipping function prologue for $function"
+               gdb_test "next"
+               }
+           return 2
+       }
+       -re "Breakpoint \[0-9\]*, \[0-9xa-f\]* in .*\{\[\r\n\]*$gdb_prompt
$" {

+            # we are in the function prologue, let's skip it
+           if { "$skip_function_prologue" == "yes" } {
+               verbose "Skipping function prologue for $function"
+               gdb_test "next"
+               }
+           return 2
+       }
+
        -re "Break.* at .*:$decimal.*$gdb_prompt $" {
            return 1
        }
@@ -410,9 +440,15 @@ proc runto { function args } {
 proc runto_main { } {
     global gdb_prompt
     global decimal
+    global skip_function_prologue
+    global skip_main_function_prologue

     if ![target_info exists gdb_stub] {
-       return [runto main]
+       set saved_skip "$skip_function_prologue"
+       set skip_function_prologue "$skip_main_function_prologue"
+       set res [runto main]
+       set skip_function_prologue "$saved_skip"
+       return $res
     }

     delete_breakpoints
@@ -725,7 +761,7 @@ proc gdb_test_multiple { command message
        }
         -re "\r\n$gdb_prompt $" {
            if ![string match "" $message] then {
-               fail "$message"
+               fail "$message (gdb prompt found alone)"
            }
            set result 1
        }
@@ -1802,6 +1838,7 @@ proc gdb_expect { args } {
            }
        }
     }
+    global verbose;
     global suppress_flag;
     global remote_suppress_flag;
     if [info exists remote_suppress_flag] {
@@ -1814,6 +1851,11 @@ proc gdb_expect { args } {
     }
     set code [catch \
        {uplevel remote_expect host $gtimeout $expcode} string];
+    if $verbose>4  then {
+       send_user "Expcode is \"$expcode\""
+       send_user "Returned string is \"$string\""
+    }
+
     if [info exists old_val] {
        set remote_suppress_flag $old_val;
     } else {

Re: [RFC] testsuite: Skip over function prologue in runto and runto_main

[Pedro Alves]

Pierre Muller wrote:
>   I was rereading the thread:
> http://sourceware.org/ml/gdb-patches/2007-10/msg00361.html
>  but if I understood things correctly, it would mean
> that if your patch goes in, and I use the 
> 'start' command, I would end up
> past the call to __main.

(note that I've since split that patch
  into smaller parts -- the skip___main part
  is small, and really non-dependant on the rest.  I can
  post it if you find it useful for the discussion.)

>   This might be an annoying change for people 
> used, like me, to either 'step' or 'next'
> after 'start' depending on if they want to analyze
> what is going on in the startup code.
> 

You can still place a breakpoint on __main then
if you want that -- but that depending on that
behaviour is depending on internal implementation details.
For instance, with a recent enough MinGW runtime, that won't
work, because __main is also called *before* reaching
main.

>   Thus, I am asking:
> Why do you consider this as broken?

Preciselly because I as a user find that is it suprising
that for my c++ global ctors to run after typing start,
I need to either 'step' or 'next'.  And I won't be
convinced that a less gdb savy user won't find it
surprising too.  If we can provide the illusion, then
why not?
(I'm not claiming my patch is best way to provide it,
I'm talking about the concept here.)

> Is it just because this is the reason of a lot of
> cygwin testsuite failures?
> 

That too.

>   If this is the reason, I would propose another route
> to solve this:
>   Modify gdb.exp runto_main function
> so that if it ends up at a line
> containing only an open brace, it will 
> know that there is some implicit code
> that needs to be executed and just issue a 'next' command
> to reach the point that is reached directly in
> other systems.
> 

I considered this too, but this it felt like a
workaround for something gdb should do itself.

> Here is the proposed patch:
> 
>   It basically defines two new globals variables in gdb.exp:
> 1)  skip_function_prologue
>   that should be "yes" if gdb should execute a 'next' statement
> if after the 'runto' procedure it arrives at a line that contains
> only an opening brace.
> 
> 2)  skip_main_function_prologue
>   Which does the same, but only within runto_main procedure.
> This variable is set to "yes" if EXEEXT env is different from "".
> But this is probably not the right condition.
> I have no good idea how to set that variable, but
> maybe setting it unconditionally would be OK too.
> 
> 

What problem is 1) trying to solve?

>   The main problem is that the pattern used to figure out if 
> we are still in the function prologue is probably not optimal
> and requires that we do put the open brace alone on a separate
> line, but that seems to be always the case (at least for the main function)
> in all the test sources.
>   Is it a coding style requirement?
> 

Not all.  Not the whole world is GNU:

>cd testsuite
>grep -rn "main" * | grep ")[ \t]*{"
gdb.base/call-ar-st.c:1146:int main ()  {
gdb.base/call-rt-st.c:696:int main ()  {
gdb.base/gdb1821.c:23:int main(void) { return 0; }
gdb.base/long_long.c:83:int main() {
gdb.base/step-test.c:23:int main () {
gdb.base/varargs.c:26:int main() {
gdb.cp/cplusfuncs.cc:66:int main () {
gdb.cp/hang1.C:3:int main (int argc, char **argv) { return 0; }
gdb.cp/bs15503.cc:60:int main() {
gdb.hp/gdb.defects/bs14602.c:6:int main () {
gdb.java/jprint.java:55:  public static void main(String[] args) {
gdb.threads/print-threads.c:12:int main() {
gdb.threads/schedlock.c:29:int main() {
gdb.threads/thread-specific.c:27:int main() {
gdb.threads/threadapply.c:31:int main() {
gdb.threads/watchthreads.c:31:int main() {
lib/gdb.exp:1284:    puts $f "int main() {"

Just a nit, but __main call is not really considered part
of the prologue.  It's emitted as the first statement
of the body of main.

>   The global skip_function_prologue could be used for a
> more general approach, like if we test programs that have been
> compiled with profiling on.
> 

But skipping the prologue and finding the first line of
a function is a job of gdb, why work around it in
the testsuite?  If you generalize this will be hiding
changes in that part of gdb.  If adding profile support
puts something in the prologue (like a call to mcount)
that brakes gdb's prologue analyzing and skipping, then
it should be fixed in gdb and/or in the debug info.

-- 
Pedro Alves

RE: [RFC] testsuite: Skip over function prologue in runto and runto_main

[Pierre Muller]

> > Here is the proposed patch:
> >
> >   It basically defines two new globals variables in gdb.exp:
> > 1)  skip_function_prologue
> >   that should be "yes" if gdb should execute a 'next' statement
> > if after the 'runto' procedure it arrives at a line that contains
> > only an opening brace.
> >
> > 2)  skip_main_function_prologue
> >   Which does the same, but only within runto_main procedure.
> > This variable is set to "yes" if EXEEXT env is different from "".
> > But this is probably not the right condition.
> > I have no good idea how to set that variable, but
> > maybe setting it unconditionally would be OK too.
> >
> >
> 
> What problem is 1) trying to solve?

  It is mainly because runto_main is
calling runto procedure, and that the patterns
are only inside runto procedure.
  But as said, this would probably 
allow to skip implicit code that is not recognized as part of
the prologue.
  As programmer of the Free Pascal Compiler, I
know that there is quite some implicit code 
hidden inside the 'begin' pascal statement (equivalent of the C open brace),
like exception stack setup, win32 special call to avoid
stack faults for big local stack areas, implicit class
initialization...
  To handle this we would just need to replace the
open brace by a variable possibly named 'function_start_token'
defaulting to '{' but that would be changed into 'begin'
for pascal, and probably also for 'ada' or ''.
> Just a nit, but __main call is not really considered part
> of the prologue.  It's emitted as the first statement
> of the body of main.

  But this is the main problem here:
  many testsuite expect files do assume that
by using runto_main you arrive on the first 
explicit statement in the main function.

  Thus if the call to __main is implicit, we need to 
decide if gdb should place the breakpoint past
that implicit call, which is what your patch does,
but to me this is the same as saying that the call to main
is part of the prologue. But you just said
the opposite.

  My patch would rather be something like,
for the main function:
a call to __main is the first statement on
some targets, but we do not what to make any
differences between those targets and others
(which call __main before main, or not at all)
and thus we skip any implicit code,
which would be better found by the fact that
the open brace is the last part of the line where we stop.
  I think that the pattern that I used 
should also find lines that contain both the main and the open
brace; possible errors would be something like


main (int argc; char** argv) { if (argc >0) {
  ...

where my patch would go too far,
but that is probably easy to correct
with a pattern allowing only a single open brace.


  The call to mcount in case of profiling 
could be handled with the same logic, no?


  So finally the main question remains to 
decide whether the call to __main (in main) (or mcount in 
profiled function) should be considered as part of the prologue 
or handled a real statements, even though they are implicit.

  

Pierre

Re: [RFC] testsuite: Skip over function prologue in runto and runto_main

[Daniel Jacobowitz]

On Thu, Dec 06, 2007 at 05:27:14PM +0100, Pierre Muller wrote:
>   So finally the main question remains to 
> decide whether the call to __main (in main) (or mcount in 
> profiled function) should be considered as part of the prologue 
> or handled a real statements, even though they are implicit.

I don't care whether we label it as part of the prologue or not, but
we should skip it.  Users don't want to see this any more than the GDB
testsuite does.

-- 
Daniel Jacobowitz
CodeSourcery