Signal Trampoline Frames

Signal Trampoline Frames

[Mark Kettenis]

Andrew,

It's probably too late now, but I have the feeling this new
tramp-frame.c is not generic enough.  It assumes fixed-length
instructions (which makes it unsuitable for IA-32 and AMD64) and uses
the arbitrary number 8 for the number of instructions (which makes it
not quite suitable for SPARC).  The whole thing seems a bit
over-engineered to me :-(.

Anyway, you might want to conseider the attached patch, which makes
the comments catch up with reality.  The extra whitespace is there to
guide the eye and make the distinction between the instruction
sequence members and the initialization function clear.

Mark


Index: ChangeLog
from  Mark Kettenis  <kettenis@gnu.org>

	* tramp-frame.h (struct tramp_frame): Fix comments.

Index: tramp-frame.h
===================================================================
RCS file: /cvs/src/src/gdb/tramp-frame.h,v
retrieving revision 1.2
diff -u -p -r1.2 tramp-frame.h
--- tramp-frame.h 23 Mar 2004 14:12:30 -0000 1.2
+++ tramp-frame.h 24 Mar 2004 22:51:03 -0000
@@ -43,15 +43,16 @@ struct tramp_frame
 {
   /* The trampoline's entire instruction sequence.  Search for this in
      the inferior at or around the frame's PC.  It is assumed that the
-     PC is INSN_SIZE aligned, and that each element of TRAMP contains
-     one INSN_SIZE instruction.  It is also assumed that TRAMP[0]
+     PC is INSN_SIZE aligned, and that each element of INSN contains
+     one INSN_SIZE instruction.  It is also assumed that INSN[0]
      contains the first instruction of the trampoline and hence the
-     address of the instruction matching TRAMP[0] is the trampoline's
+     address of the instruction matching INSN[0] is the trampoline's
      "func" address.  */
   int insn_size;
   ULONGEST insn[8];
+
   /* Initialize a trad-frame cache corresponding to the tramp-frame.
-     FUNC is the address of the instruction TRAMP[0] in memory.  */
+     FUNC is the address of the instruction INSN[0] in memory.  */
   void (*init) (const struct tramp_frame *self,
 		struct frame_info *next_frame,
 		struct trad_frame_cache *this_cache,

Re: Signal Trampoline Frames

[Andrew Cagney]

> Andrew,
> 
> It's probably too late now, but I have the feeling this new
> tramp-frame.c is not generic enough.  It assumes fixed-length
> instructions (which makes it unsuitable for IA-32 and AMD64)

I didn't forget IA-32, I figured that it is always little endian so a 
sequence of one byte "insns" would work.

 > and uses
> the arbitrary number 8 for the number of instructions (which makes it
> not quite suitable for SPARC).

I'm assuming people will increase it (It's like the arbitrary number 16 
for the largest possible size of an instruction).

 > The whole thing seems a bit
> over-engineered to me :-(.

In what way?

I wrote it after churn out 4 almost identical signal trampolines - so it 
works for one type but not for others.  You also haven't seen my test 
cases :-)

> Anyway, you might want to conseider the attached patch, which makes
> the comments catch up with reality.  The extra whitespace is there to
> guide the eye and make the distinction between the instruction
> sequence members and the initialization function clear.

Andrew

Re: Signal Trampoline Frames

[Mark Kettenis]

   Date: Wed, 24 Mar 2004 19:26:09 -0500
   From: Andrew Cagney <cagney@gnu.org>

   > Andrew,
   > 
   > It's probably too late now, but I have the feeling this new
   > tramp-frame.c is not generic enough.  It assumes fixed-length
   > instructions (which makes it unsuitable for IA-32 and AMD64)

   I didn't forget IA-32, I figured that it is always little endian so a 
   sequence of one byte "insns" would work.

Ah, that would probably work :-).

    > and uses
   > the arbitrary number 8 for the number of instructions (which makes it
   > not quite suitable for SPARC).

   I'm assuming people will increase it (It's like the arbitrary number 16 
   for the largest possible size of an instruction).

OK.  Although you must realize that for SPARC we might need to
increase it to 80 or so.  Then it makes more sense to check only for a
small number of key instructions I think.

    > The whole thing seems a bit
   > over-engineered to me :-(.

   In what way?

   I wrote it after churn out 4 almost identical signal trampolines - so it 
   works for one type but not for others.  You also haven't seen my test 
   cases :-)

My experience is that there is too much variation for a
one-size-fits-all trampoline recognition function.  I was under the
impression that there were only two cases where this could be used.
Turns out I'm wrong.  I still think we run the risk of
over-engineering this by adding to much knobs if we try to make it
work for too many slightly different signal tramps.

Mark

Re: Signal Trampoline Frames

[Andrew Cagney]

>     > and uses
>    > the arbitrary number 8 for the number of instructions (which makes it
>    > not quite suitable for SPARC).
> 
>    I'm assuming people will increase it (It's like the arbitrary number 16 
>    for the largest possible size of an instruction).
> 
> OK.  Although you must realize that for SPARC we might need to
> increase it to 80 or so.  Then it makes more sense to check only for a
> small number of key instructions I think.

In that case, don't use it.

BTW, what happens if the inferior is interrupted while executing one of 
those 80 instructions?

>     > The whole thing seems a bit
>    > over-engineered to me :-(.
> 
>    In what way?
> 
>    I wrote it after churn out 4 almost identical signal trampolines - so it 
>    works for one type but not for others.  You also haven't seen my test 
>    cases :-)
> 
> My experience is that there is too much variation for a
> one-size-fits-all trampoline recognition function.  I was under the
> impression that there were only two cases where this could be used.
> Turns out I'm wrong.  I still think we run the risk of
> over-engineering this by adding to much knobs if we try to make it
> work for too many slightly different signal tramps.

Right.

It's a helper for a specific, but fairly common, case.  It works for me 
(and appears to work for daniel :-) but is definitly not a one size fits 
all.

Andrew