Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Ulrich Weigand]

Andrew Cagney wrote:

>        * infrun.c (handle_inferior_event): For non legacy frames, use the
>        frame ID and frame type to identify a signal trampoline.  Update
>        comments.

This breaks signals.exp on s390 again.

What now happens is the following:

- we are in main and do 'next'; step_frame_id is set to the current frame
- gdb starts single-stepping
- in the meantime, a signal has arrived and the kernel invokes the handler
  Note that the kernel directly jumps to the handler, and sets up the
  return address to point to the signal return trampoline.
- gdb single-steps and stops on the first instruction of the handler
  at this point, the call-chain is
    handler
    <signal trampoline>
    main
- gdb thinks it has stepped into a subroutine, and does
  handle_step_into_function, which calls keep_going
- keep_going saves the current pc (i.e. start of handler) into prev_pc
- we run until the return from handler, i.e. we stop on the first
  (and only) instruction of the signal trampoline
- now the 'if' you changed hits because we are in fact in a signal trampoline
  frame. 
- within the if, there's a second if that tries to distiguish between
  stepping into a trampoline before the signal handler from one after
  the signal handler.  This is attempted via
    if (frame_id_inner (current_frame, step_frame_id))
- unfortunately, step_frame_id is still the frame of 'main', and thus
  the current (i.e. trampoline) frame *is* 'inner', even though we
  actually have a trampoline after, not before the handler.
- gdb now does
            sr_sal.line = 0;
            sr_sal.pc = prev_pc;
            /* We could probably be setting the frame to
               step_frame_id; I don't think anyone thought to try it.  */
            check_for_old_step_resume_breakpoint ();
            step_resume_breakpoint =
              set_momentary_breakpoint (sr_sal, null_frame_id, bp_step_resume);
  However, prev_pc was set to first instruction of handler above
- since we never again enter the handler, the breakpoint is never hit
  and we run until the end of main


I'm not sure what the correct way to fix this issue would be.

However, simply removing the whole 'if' block makes signals.exp pass on s390.
This is because both the handler and the signal return trampoline are now
simply treated as calls into subroutines, and both are skipped with
step_over_function, so that everything works just as expected.

Why is this if needed in the first place?  Isn't this just to work around
frame problems that caused step_over_function to not handle signal handlers
correctly?  I.e. if we have new-style frames that work properly, can't we
just skip that whole if?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

> Why is this if needed in the first place?  Isn't this just to work around
> frame problems that caused step_over_function to not handle signal handlers
> correctly?  I.e. if we have new-style frames that work properly, can't we
> just skip that whole if?

Literally, who knows!  Do quote daniel, it's before my time :-)  Removal 
sounds tempting, but first I think I'll yank all the legacy paths.

Andrew

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

> I'm not sure what the correct way to fix this issue would be.
> 
> However, simply removing the whole 'if' block makes signals.exp pass on s390.
> This is because both the handler and the signal return trampoline are now
> simply treated as calls into subroutines, and both are skipped with
> step_over_function, so that everything works just as expected.
> 
> Why is this if needed in the first place?  Isn't this just to work around
> frame problems that caused step_over_function to not handle signal handlers
> correctly?  I.e. if we have new-style frames that work properly, can't we
> just skip that whole if?

Joel, from memory you had a change to:

   if (((stop_pc == ecs->stop_func_start	/* Quick test */
	|| in_prologue (stop_pc, ecs->stop_func_start))
        && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
       || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
       || ecs->stop_func_name == 0)
     {
       /* It's a subroutine call.  */
       handle_step_into_function (ecs);
       return;
     }

pending?  If we do pull the sigtramp code I think it would be prudent to 
first have that committed - Joel's change greatly clarifies the logic.

Andrew

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Joel Brobecker]

> Joel, from memory you had a change to:
> 
>   if (((stop_pc == ecs->stop_func_start	/* Quick test */
> 	|| in_prologue (stop_pc, ecs->stop_func_start))
>        && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
>       || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
>       || ecs->stop_func_name == 0)
>     {
>       /* It's a subroutine call.  */
>       handle_step_into_function (ecs);
>       return;
>     }
> 
> pending?  If we do pull the sigtramp code I think it would be prudent to 
> first have that committed - Joel's change greatly clarifies the logic.

Yes. Something that would isolate this code to the platforms who haven't
been framified yet (are there still any?). But the problem is that I
got some regressions mostly on HP/UX, but also one on Tru64. The
testsuite results were clean on sparc-solaris and x86-linux.

The problem was that it was difficult for me to investigate the problems
on HP/UX given the current status. So I started looking at the current
failures on HP/UX to see if I could improve them a bit.

I should probably redo some testing and see where we stand. Yeah, let
me do that.

-- 
Joel

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Joel Brobecker]

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

> Joel, from memory you had a change to:
> 
>   if (((stop_pc == ecs->stop_func_start	/* Quick test */
> 	|| in_prologue (stop_pc, ecs->stop_func_start))
>        && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
>       || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
>       || ecs->stop_func_name == 0)
>     {
>       /* It's a subroutine call.  */
>       handle_step_into_function (ecs);
>       return;
>     }
> 
> pending?  If we do pull the sigtramp code I think it would be prudent to 
> first have that committed - Joel's change greatly clarifies the logic.

Just to make sure we're talking about the same patch, attached is the
patch I was working on (may need to be updated to the current sources).
Is that what you were refering to?

Thanks,
-- 
Joel

[-- Attachment #2: infrun.c.diff --]
[-- Type: text/plain, Size: 2752 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.140
diff -u -p -r1.140 infrun.c
--- infrun.c	15 Mar 2004 17:12:50 -0000	1.140
+++ infrun.c	3 Apr 2004 00:06:46 -0000
@@ -2516,6 +2516,18 @@ process_event_stop_test:
       return;
     }
 
+  if (step_over_calls == STEP_OVER_UNDEBUGGABLE
+      && ecs->stop_func_name == NULL)
+    {
+      /* There is no symbol, not even a minimal symbol, corresponding
+         to the address where we just stopped.  So we just stepped
+         inside undebuggable code.  Since we want to step over this
+         kind of code, we keep going until the inferior returns from
+         the current function.  */
+      handle_step_into_function (ecs);
+      return;
+    }
+
   /* We can't update step_sp every time through the loop, because
      reading the stack pointer would slow down stepping too much.
      But we can update it every time we leave the step range.  */
@@ -2605,15 +2617,35 @@ process_event_stop_test:
       return;
     }
 
-  if (((stop_pc == ecs->stop_func_start	/* Quick test */
-	|| in_prologue (stop_pc, ecs->stop_func_start))
-       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
-      || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
-      || ecs->stop_func_name == 0)
+  if (legacy_frame_p (current_gdbarch))
     {
-      /* It's a subroutine call.  */
-      handle_step_into_function (ecs);
-      return;
+      /* FIXME: brobecker/2004-03-04: The current architecture is still
+         using the legacy frame code, so we prefer not to rely on frame IDs
+         to check whether we just stepped into a function or not.  Some
+         experiments conducted on sparc-solaris before it was converted
+         to the new frame code showed that it could introduce some
+         severe problems.  Once all targets have transitioned to the new
+         frame code, this block can be deleted.  */
+      if (((stop_pc == ecs->stop_func_start	/* Quick test */
+            || in_prologue (stop_pc, ecs->stop_func_start))
+           && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
+          || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
+          || ecs->stop_func_name == 0)
+        {
+          /* It's a subroutine call.  */
+          handle_step_into_function (ecs);
+          return;
+        }
+    }
+  else
+    {
+      if (frame_id_eq (get_frame_id (get_prev_frame (get_current_frame ())),
+                       step_frame_id))
+        {
+          /* It's a subroutine call.  */
+          handle_step_into_function (ecs);
+          return;
+        }
     }
 
   /* We've wandered out of the step range.  */

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

> 
> Just to make sure we're talking about the same patch, attached is the
> patch I was working on (may need to be updated to the current sources).
> Is that what you were refering to?

Yes, but no longer worry about the if(legacy_frame_p) path.

Andrew

> -  if (((stop_pc == ecs->stop_func_start	/* Quick test */
> -	|| in_prologue (stop_pc, ecs->stop_func_start))
> -       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
> -      || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
> -      || ecs->stop_func_name == 0)
> +  if (legacy_frame_p (current_gdbarch))

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Joel Brobecker]

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

Andrew,

> >Just to make sure we're talking about the same patch, attached is the
> >patch I was working on (may need to be updated to the current sources).
> >Is that what you were refering to?
> 
> Yes, but no longer worry about the if(legacy_frame_p) path.

I have tested the attached patch on several platforms, and as expected,
found a few regression here and there. I started analyzing the
regressions on hp/ux, but this takes time, and I have to do something
else for a while. A quick glance doesn't say much about the source of
regressions, but my feeling after the few ones that I analyzed is that
they are a consequence of a latent bug somewhere else more than a
problem with the patch itself.  The fact that we have no regression on
both x86-linux and sparc-solaris is also encouraging. 

Below is a quick summary of the difference in the testsuite results
before and after the patch. It sounded like this patch would make your
life much easier. I'll let you and the other maintainers decide whether
it's acceptable to include this patch now, as a small step backward to
allow you to jump farther.

2004-04-05  Joel Brobecker  <brobecker@gnat.com>

        * infrun.c (handle_inferior_event): Rely on frame IDs to detect
        function calls.

Tested on x86-linux and sparc-solaris 2.8: No regression.

On hppa32 (hppa-hp-hpux11.00), we can argue that it's not necessarily
worse. We do have some improvement too. See hppa.txt attached.

On alpha-tru64, we have two regressions, but they are actually one
regression (we do the test twice in mi, and mi2), and it is really
minor:

        1  gdb.mi/mi-until.exp: until after while loop    PASS  FAIL
        2  gdb.mi/mi2-until.exp: until after while loop   PASS  FAIL
        
        The behavior remains the same, except we also get this warning:
        warning: (Internal error: pc 0x1200011c4 in read in psymtab,
        but not in symtab.)

On powerpc-aix 5.1, I get the following results:

        1 gdb.mi/gdb669.exp: next, try 1                  PASS  FAIL
        2 gdb.mi/gdb669.exp: next, try 2                  PASS  FAIL
        3 gdb.mi/gdb669.exp: next, try 3                  PASS  FAIL
        4 gdb.mi/mi2-pthreads.exp: mi runto 
          done_making_threads                             FAIL  PASS

Again, I think the regression do not show a problem with the patch
itself. I get the following output for the first failure:

     ~"[Switching to Thread 258]\n"^M
     220*stopped,reason="signal-received",signal-name="SIGTRAP",signal-meaning="Trace/breakpoint trap",thread-id="2",frame={addr="0x00000000",func="??",args=[]}^M

Thread support has been badly broken for me. I started investigating
a while ago but couldn't understand where this SIGTRAP was coming from
and had to pull out and work on some more urgent things. Still on my
list. (any hint as to how to track down that problem appreciated, btw :-).

Is for the other two failures, they *seem* to be a consequence of the
first one:

    &"Cannot find bounds of current function\n"^M
    220^error,msg="Cannot find bounds of current function"^M
    
I also ran the testsuite on an amd64-linux machine, and I know there
were some regressions there. Unfortunately, the machine appears to be
down right now, so I probably won't have access to it before tomorrow.
If my memory serves me well, I think there were 30 or 40 differences.

I didn't test it on mips-irix, though, because I've been having some
severe problems with that port :-/. I haven't had time to investigate
whether this is a build problem on my side, or some problem with the
code. But right now, GDB on that platform simply does not work, not even
"break main; run". Building without -O2 helps a bit, which suggests
either an optimization bug, or a code problem, such as an undefined
variable for instance. I'll have to investigate when I have a moment.

I'll let you decide whether the patch is acceptable or not. Sorry to
suggest this patch like this. I really want to get to the bottom of
these regressions, but I simply don't have the time right now. If you
didn't mention about it helping you with the frame code, I would
probably have delayed a bit its re-submission.

-- 
Joel

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

1  gdb.base/annota1.exp: Core dumped and removed                       PASS
2  gdb.base/annota1.exp: No core dumped                       PASS
3  gdb.base/call-ar-st.exp: continue to 1241                  FAIL     PASS
4  gdb.base/call-ar-st.exp: continue to 1281                  FAIL     PASS
5  gdb.base/call-ar-st.exp: continue to 1286                  FAIL     PASS
6  gdb.base/call-ar-st.exp: continue to 1300                  FAIL     PASS
7  gdb.base/call-ar-st.exp: continue to 1305                  FAIL     PASS
8  gdb.base/call-ar-st.exp: continue to 1311                  FAIL     PASS
9  gdb.base/call-ar-st.exp: continuing to 1236                FAIL     PASS
10 gdb.base/call-ar-st.exp: next to 1237                      FAIL     PASS
11 gdb.base/call-ar-st.exp: print compute_with_small_structs  FAIL     PASS
12 gdb.base/call-ar-st.exp: print print_bit_flags_combo from  FAIL     PASS
   init_bit_flags_combo
13 gdb.base/call-ar-st.exp: print print_one_large_struct      FAIL     PASS
14 gdb.base/call-ar-st.exp: print print_struct_rep            FAIL     PASS
15 gdb.base/call-ar-st.exp: print sum_array_print             FAIL     PASS
16 gdb.base/call-ar-st.exp: step into init_bit_flags_combo    FAIL     PASS
17 gdb.base/call-ar-st.exp: step into print_long_arg_list     FAIL     PASS
18 gdb.base/foll-fork.exp: default parent follow, no          FAIL     PASS
   catchpoints
19 gdb.base/foll-fork.exp: explicit child follow, catch fork  PASS
20 gdb.base/foll-fork.exp: explicit child follow, no          FAIL     PASS
   catchpoints
21 gdb.base/foll-fork.exp: explicit child follow, set catch   PASS
   fork
22 gdb.base/foll-fork.exp: explicit parent follow, no         FAIL     PASS
   catchpoints
23 gdb.base/foll-fork.exp: explicit parent follow, set tcatch PASS
   fork
24 gdb.base/foll-fork.exp: explicit parent follow, tcatch     PASS
   fork
25 gdb.base/foll-fork.exp: info shows catchpoint pid          PASS
26 gdb.base/foll-fork.exp: info shows catchpoint without pid  PASS
27 gdb.base/foll-fork.exp: set follow child, cleanup          PASS
28 gdb.base/foll-fork.exp: set follow child, hit tbreak       FAIL
29 gdb.base/foll-fork.exp: set follow child, tbreak           PASS
30 gdb.base/foll-fork.exp: set follow parent, cleanup         PASS
31 gdb.base/foll-fork.exp: set follow parent, hit tbreak      PASS
32 gdb.base/foll-fork.exp: set follow parent, tbreak          PASS
33 gdb.base/funcargs.exp: backtrace through call with         PASS     FAIL
   trampolines
34 gdb.base/funcargs.exp: stepping back to main from function PASS     FAIL
   called with trampolines
35 gdb.base/funcargs.exp: stepping into function called with  PASS     FAIL
   trampolines
36 gdb.base/selftest.exp: send SIGINT signal to child process PASS     FAIL
37 gdb.base/selftest.exp: send ^C to child process            PASS     FAIL
38 gdb.base/signals.exp: break func1                          PASS     FAIL
39 gdb.base/signals.exp: break func2                          PASS     FAIL
40 gdb.base/signals.exp: break handler                        PASS     FAIL
41 gdb.base/signals.exp: break handler if 0                   PASS
42 gdb.base/signals.exp: bt in signals.exp                    FAIL
43 gdb.base/signals.exp: condition $handler_breakpoint_number PASS
44 gdb.base/signals.exp: continue in signals.exp              FAIL
45 gdb.base/signals.exp: handle SIG parses all legal actions  PASS
46 gdb.base/signals.exp: handle SIG with bogus action         PASS
47 gdb.base/signals.exp: handle SIG with multiple conflicting PASS
   actions
48 gdb.base/signals.exp: handle multiple SIGs                 PASS
49 gdb.base/signals.exp: handle multiple SIGs via integer     PASS
   range
50 gdb.base/signals.exp: handle with bogus SIG                PASS
51 gdb.base/signals.exp: handle without arguments             PASS
52 gdb.base/signals.exp: info signal 5                        PASS
53 gdb.base/signals.exp: info signal SIGTRAP                  PASS
54 gdb.base/signals.exp: info signals                         PASS
55 gdb.base/signals.exp: invalid signal number rejected       PASS
56 gdb.base/signals.exp: next to ++count #1 in signals.exp    FAIL
57 gdb.base/signals.exp: next to ++count #2 in signals.exp    FAIL
58 gdb.base/signals.exp: next to 2nd alarm                    FAIL
59 gdb.base/signals.exp: next to 2nd alarm;                            FAIL
60 gdb.base/signals.exp: next to alarm #1 in signals.exp      PASS
61 gdb.base/signals.exp: next to alarm #2 in signals.exp      FAIL
62 gdb.base/signals.exp: next to signal in signals.exp        PASS
63 gdb.base/signals.exp: override SIGINT                      PASS
64 gdb.base/signals.exp: override SIGTRAP                     PASS
65 gdb.base/signals.exp: p count #1 in signals.exp            FAIL
66 gdb.base/signals.exp: p count #2 in signals.exp            FAIL
67 gdb.base/signals.exp: p func1 #1 in signals.exp            FAIL
68 gdb.base/signals.exp: p func1 #2 in signals.exp            FAIL
69 gdb.base/signals.exp: sent signal 5                        FAIL
70 gdb.base/signals.exp: set $handler_breakpoint_number =     PASS
   $bpnum
71 gdb.base/signals.exp: set variable count = 0               PASS
72 gdb.base/signals.exp: signal without arguments disallowed  FAIL
73 gdb.base/step-line.exp: next over dummy 2                  PASS     FAIL
74 gdb.base/step-line.exp: step into f2                       PASS     FAIL

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Joel Brobecker]

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

> 2004-04-05  Joel Brobecker  <brobecker@gnat.com>
> 
>         * infrun.c (handle_inferior_event): Rely on frame IDs to detect
>         function calls.

Sorry Andrew, here is the patch (sigh).

-- 
Joel

[-- Attachment #2: infrun.c.diff --]
[-- Type: text/plain, Size: 1554 bytes --]

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.145
diff -u -p -r1.145 infrun.c
--- infrun.c	2 Apr 2004 22:38:43 -0000	1.145
+++ infrun.c	6 Apr 2004 00:11:59 -0000
@@ -2464,6 +2464,18 @@ process_event_stop_test:
       return;
     }
 
+  if (step_over_calls == STEP_OVER_UNDEBUGGABLE
+      && ecs->stop_func_name == NULL)
+    {
+      /* There is no symbol, not even a minimal symbol, corresponding
+         to the address where we just stopped.  So we just stepped
+         inside undebuggable code.  Since we want to step over this
+         kind of code, we keep going until the inferior returns from
+         the current function.  */
+      handle_step_into_function (ecs);
+      return;
+    }
+
   /* We can't update step_sp every time through the loop, because
      reading the stack pointer would slow down stepping too much.
      But we can update it every time we leave the step range.  */
@@ -2542,11 +2554,8 @@ process_event_stop_test:
       return;
     }
 
-  if (((stop_pc == ecs->stop_func_start	/* Quick test */
-	|| in_prologue (stop_pc, ecs->stop_func_start))
-       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
-      || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
-      || ecs->stop_func_name == 0)
+  if (frame_id_eq (get_frame_id (get_prev_frame (get_current_frame ())),
+                   step_frame_id))
     {
       /* It's a subroutine call.  */
       handle_step_into_function (ecs);

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

> -	|| in_prologue (stop_pc, ecs->stop_func_start))
> -       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
> -      || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
> -      || ecs->stop_func_name == 0)
> +  if (frame_id_eq (get_frame_id (get_prev_frame (get_current_frame ())),
> +                   step_frame_id))

based on other discussion, I suspect the test will need to be:

if (frame_id_inner (this_frame_id, step_frame_id)
     || frame_id_eq (....))

the former is to handle a signal delivery that resumed the inferior at 
the signal handler and not the signal trampoline.

Andrew

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Daniel Jacobowitz]

On Tue, Apr 06, 2004 at 01:48:05PM -0400, Andrew Cagney wrote:
> >-	|| in_prologue (stop_pc, ecs->stop_func_start))
> >-       && !IN_SOLIB_RETURN_TRAMPOLINE (stop_pc, ecs->stop_func_name))
> >-      || IN_SOLIB_CALL_TRAMPOLINE (stop_pc, ecs->stop_func_name)
> >-      || ecs->stop_func_name == 0)
> >+  if (frame_id_eq (get_frame_id (get_prev_frame (get_current_frame ())),
> >+                   step_frame_id))
> 
> based on other discussion, I suspect the test will need to be:
> 
> if (frame_id_inner (this_frame_id, step_frame_id)
>     || frame_id_eq (....))
> 
> the former is to handle a signal delivery that resumed the inferior at 
> the signal handler and not the signal trampoline.

Won't anywhere you do that frame_id_inner test add another breakage for
sigaltstack?

-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

>>based on other discussion, I suspect the test will need to be:
>>> 
>>> if (frame_id_inner (this_frame_id, step_frame_id)
>>>     || frame_id_eq (....))
>>> 
>>> the former is to handle a signal delivery that resumed the inferior at 
>>> the signal handler and not the signal trampoline.
> 
> 
> Won't anywhere you do that frame_id_inner test add another breakage for
> sigaltstack?

Hmm, true.  Sigaltstack will break any inner test.

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

>>> based on other discussion, I suspect the test will need to be:
>>>
>>>>
>>>> if (frame_id_inner (this_frame_id, step_frame_id)
>>>>     || frame_id_eq (....))
>>>>
>>>> the former is to handle a signal delivery that resumed the inferior at the signal handler and not the signal trampoline.
>>
>>
>>
>> Won't anywhere you do that frame_id_inner test add another breakage for
>> sigaltstack?
> 
> 
> Hmm, true.  Sigaltstack will break any inner test.

Ok (talked this through with jeff).  The id_inner test isn't needed. 
The case of GDB doing a nexti and ending up in a signal handler should 
have been handled earlier (in time).  At the point where the signal 
first arrives - GDB should have set a breakpoint at the signal return 
address and then continued the inferior with the signal.

Time to turn infrun.c into a state machine :-/

Andrew

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

> Andrew,

I've bitten the bullet and committed this.  As Joel observes, failures 
are revealing latent bugs.

Andrew

>>>> >Just to make sure we're talking about the same patch, attached is the
>>>> >patch I was working on (may need to be updated to the current sources).
>>>> >Is that what you were refering to?
>>
>>> 
>>> Yes, but no longer worry about the if(legacy_frame_p) path.
> 
> 
> I have tested the attached patch on several platforms, and as expected,
> found a few regression here and there. I started analyzing the
> regressions on hp/ux, but this takes time, and I have to do something
> else for a while. A quick glance doesn't say much about the source of
> regressions, but my feeling after the few ones that I analyzed is that
> they are a consequence of a latent bug somewhere else more than a
> problem with the patch itself.  The fact that we have no regression on
> both x86-linux and sparc-solaris is also encouraging. 
> 
> Below is a quick summary of the difference in the testsuite results
> before and after the patch. It sounded like this patch would make your
> life much easier. I'll let you and the other maintainers decide whether
> it's acceptable to include this patch now, as a small step backward to
> allow you to jump farther.

[patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

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

Hello,

The attached patch modifies infrun.c so that, for the non-legacy case, 
the test:
	frame_type == SIGTRAMP frame
	&& step_frame_id != stop_frame_id
is used to determine if a single-step caused the inferior to stumble 
into a signal trampoline (either via a signal, or by returning from the 
handler).  This replaces two pc_in_sigtramp calls and a stack-pointer 
heuristic -- not the most robust of tests.

The new test:

- eliminates the call pc_in_sigtramp(step_pc)
(i.e., on the PC from before the single step) (this was an unexpected 
bonus :-)

- uses the caching call get_frame_type call
pc_in_sigtramp has to re-do all the computation each time it is called 
(typically duplicating the effort of get_frame_type)

- uses !frame_id_eq
Which is a 100% robust frame-changed test.  The old test couldn't detect 
a signal trampoline calling (via a signal) a singnal trampoline.

thoughts?

I'll look to commit this in a few days (already committed to my 
trad-frame branch).

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2005 bytes --]

2004-03-16  Andrew Cagney  <cagney@redhat.com>

	* infrun.c (handle_inferior_event): For non legacy frames, use the
	frame ID and frame type to identify a signal trampoline.  Update
	comments.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.140
diff -u -r1.140 infrun.c
--- infrun.c	15 Mar 2004 17:12:50 -0000	1.140
+++ infrun.c	16 Mar 2004 18:25:45 -0000
@@ -2521,22 +2521,19 @@
      But we can update it every time we leave the step range.  */
   ecs->update_step_sp = 1;
 
-  /* Did we just take a signal?  */
-  if (pc_in_sigtramp (stop_pc)
-      && !pc_in_sigtramp (prev_pc)
-      && INNER_THAN (read_sp (), step_sp))
+  /* Did we just step into a singal trampoline (either by stepping out
+     of a handler, or by taking a signal)?  */
+  /* NOTE: cagney/2004-03-16: Replaced (except for legacy) a check for
+     "pc_in_sigtramp(stop_pc) != pc_in_sigtramp(step_pc)" with
+     frame_type == SIGTRAMP && !frame_id_eq.  The latter is far more
+     robust as it will correctly handle nested signal trampolines.  */
+  if (legacy_frame_p (current_gdbarch)
+      ? (pc_in_sigtramp (stop_pc)
+	 && !pc_in_sigtramp (prev_pc)
+	 && INNER_THAN (read_sp (), step_sp))
+      : (get_frame_type (get_current_frame ()) == SIGTRAMP_FRAME
+	 && !frame_id_eq (get_frame_id (get_current_frame ()), step_frame_id)))
     {
-      /* We've just taken a signal; go until we are back to
-         the point where we took it and one more.  */
-
-      /* Note: The test above succeeds not only when we stepped
-         into a signal handler, but also when we step past the last
-         statement of a signal handler and end up in the return stub
-         of the signal handler trampoline.  To distinguish between
-         these two cases, check that the frame is INNER_THAN the
-         previous one below. pai/1997-09-11 */
-
-
       {
 	struct frame_id current_frame = get_frame_id (get_current_frame ());
 

[patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

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

Hello,

The attached patch modifies infrun.c so that, for the non-legacy case, 
the test:
	frame_type == SIGTRAMP frame
	&& step_frame_id != stop_frame_id
is used to determine if a single-step caused the inferior to stumble 
into a signal trampoline (either via a signal, or by returning from the 
handler).  This replaces two pc_in_sigtramp calls and a stack-pointer 
heuristic -- not the most robust of tests.

The new test:

- eliminates the call pc_in_sigtramp(step_pc)
(i.e., on the PC from before the single step) (this was an unexpected 
bonus :-)

- uses the caching call get_frame_type call
pc_in_sigtramp has to re-do all the computation each time it is called 
(typically duplicating the effort of get_frame_type)

- uses !frame_id_eq
Which is a 100% robust frame-changed test.  The old test couldn't detect 
a signal trampoline calling (via a signal) a singnal trampoline.

thoughts?

I'll look to commit this in a few days (already committed to my 
trad-frame branch).

Andrew

[-- Attachment #2: diffs --]
[-- Type: text/plain, Size: 2005 bytes --]

2004-03-16  Andrew Cagney  <cagney@redhat.com>

	* infrun.c (handle_inferior_event): For non legacy frames, use the
	frame ID and frame type to identify a signal trampoline.  Update
	comments.

Index: infrun.c
===================================================================
RCS file: /cvs/src/src/gdb/infrun.c,v
retrieving revision 1.140
diff -u -r1.140 infrun.c
--- infrun.c	15 Mar 2004 17:12:50 -0000	1.140
+++ infrun.c	16 Mar 2004 18:25:45 -0000
@@ -2521,22 +2521,19 @@
      But we can update it every time we leave the step range.  */
   ecs->update_step_sp = 1;
 
-  /* Did we just take a signal?  */
-  if (pc_in_sigtramp (stop_pc)
-      && !pc_in_sigtramp (prev_pc)
-      && INNER_THAN (read_sp (), step_sp))
+  /* Did we just step into a singal trampoline (either by stepping out
+     of a handler, or by taking a signal)?  */
+  /* NOTE: cagney/2004-03-16: Replaced (except for legacy) a check for
+     "pc_in_sigtramp(stop_pc) != pc_in_sigtramp(step_pc)" with
+     frame_type == SIGTRAMP && !frame_id_eq.  The latter is far more
+     robust as it will correctly handle nested signal trampolines.  */
+  if (legacy_frame_p (current_gdbarch)
+      ? (pc_in_sigtramp (stop_pc)
+	 && !pc_in_sigtramp (prev_pc)
+	 && INNER_THAN (read_sp (), step_sp))
+      : (get_frame_type (get_current_frame ()) == SIGTRAMP_FRAME
+	 && !frame_id_eq (get_frame_id (get_current_frame ()), step_frame_id)))
     {
-      /* We've just taken a signal; go until we are back to
-         the point where we took it and one more.  */
-
-      /* Note: The test above succeeds not only when we stepped
-         into a signal handler, but also when we step past the last
-         statement of a signal handler and end up in the return stub
-         of the signal handler trampoline.  To distinguish between
-         these two cases, check that the frame is INNER_THAN the
-         previous one below. pai/1997-09-11 */
-
-
       {
 	struct frame_id current_frame = get_frame_id (get_current_frame ());
 

Re: [patch/rfc] Use frame_type for sigtramp test in infrun.c

[Andrew Cagney]

> Hello,
> 
> The attached patch modifies infrun.c so that, for the non-legacy case, the test:
>     frame_type == SIGTRAMP frame
>     && step_frame_id != stop_frame_id
> is used to determine if a single-step caused the inferior to stumble into a signal trampoline (either via a signal, or by returning from the handler).  This replaces two pc_in_sigtramp calls and a stack-pointer heuristic -- not the most robust of tests.
> 
> The new test:
> 
> - eliminates the call pc_in_sigtramp(step_pc)
> (i.e., on the PC from before the single step) (this was an unexpected bonus :-)
> 
> - uses the caching call get_frame_type call
> pc_in_sigtramp has to re-do all the computation each time it is called (typically duplicating the effort of get_frame_type)
> 
> - uses !frame_id_eq
> Which is a 100% robust frame-changed test.  The old test couldn't detect a signal trampoline calling (via a signal) a singnal trampoline.
> 
> thoughts?
> 
> I'll look to commit this in a few days (already committed to my trad-frame branch).

I've checked this in.

Andrew

> 2004-03-16  Andrew Cagney  <cagney@redhat.com>
> 
> 	* infrun.c (handle_inferior_event): For non legacy frames, use the
> 	frame ID and frame type to identify a signal trampoline.  Update
> 	comments.