db_nextframe/i386: reduce the number of special frame types

[avg]

Summary

This change removes TRAP_INTERRUPT and TRAP_TIMERINT frame types.

Their names are a bit confusing: trap + interrupt, what is that?
The TRAP_TIMERINT name is too specific -- can it only be used for timer
"trap-interrupts"? What is so special about them?

My understanding of the code is that INTERRUPT, TRAP_INTERRUPT and
TRAP_TIMERINT differ only in how an offset from callee's frame pointer to a
trap frame on the stack is calculated. And that depends on a number of
arguments that a special handler passes to a callee (a function with a
normal C calling convention).

So, this change makes that logic explicit and collapses all interrupt frame
types into the INTERRUPT type.

[kib]

Ok, I think I am convinced enough. As result, I do not like this code even more.

Wouldn't it be much more reliable and simpler if we set %ebp in syscall/trap/interrupt entry code to point to the beginning of struct trapframe ? Then it would just work to get tf as (struct trapframe *)(*fp->f_frame) ?

[avg]

In D22303#487872, @kib wrote:

Ok, I think I am convinced enough. As result, I do not like this code even more.

Wouldn't it be much more reliable and simpler if we set %ebp in syscall/trap/interrupt entry code to point to the beginning of struct trapframe ? Then it would just work to get tf as (struct trapframe *)(*fp->f_frame) ?

Yes, I think so.
Even if all such entry points pushed the trap frame base as arg0, as most of them already do, then the code would still be simpler than it is now -- (struct trapframe *)(*fp->f_arg0).

[jhb]

Note that gdb also knows which symbols have which behavior, so changing the stack frame layout will break kgdb, and then kgdb would have to somehow detect which i386 kernels had which behavior. We've done that once before by peeking at instructions [1], but I think it's not worth spending that much effort on i386 and easier to just let the frame layouts stay as-is. The reason for the enums was to handle the separate cases as you note for args. I don't mind changing this to use 'narg'.

1: https://github.com/bsdjhb/gdb/blob/kgdb-8.3/gdb/i386fbsd-kern.c#L82

For reference: here is how we currently decode frame offsets in kgdb now:

https://github.com/bsdjhb/gdb/blob/kgdb-8.3/gdb/i386fbsd-kern.c#L337

sys/i386/i386/db_trace.c

321 ↗

(On Diff #64145)

A few tweaks to the comment:

s/This is a/This is the/

s/The number/This number/

Last sentence:

"In most cases there is one argument: the trap frame address." (Since it's always a struct trapframe *)

393 ↗

(On Diff #64145)

I think having i386_frame here is kind of confusing compared to:

/*
  * Point to the base of the trapframe which is just above the
  * current frame.  The gap includes the return address and
  * function arguments.
  */
tf = (struct trapframe *)((char *)*fp + 4 + 4 * narg);

i386_frame includes a saved %ebp which we don't have, so I think for anyone familiar with normal i386 frames it would leave them confused since they don't see a normal frame in the assembly.

[avg]

sys/i386/i386/db_trace.c

393 ↗

(On Diff #64145)

I think that the formula that you suggest would be incorrect. It certainly would be different from what the current code does. That's because we certainly have a saved %ebp. The first instruction of a C callee is push %ebp. For example:

Dump of assembler code for function lapic_handle_intr:
   0x01de1780 <+0>:     push   %ebp

IMO, this would be correct:

tf = (struct trapframe *)((char *)*fp + 8 + 4 * narg);

Just in case, this is the definition of i386_frame:

struct i386_frame {
        struct i386_frame       *f_frame;
        u_int                   f_retaddr;
        u_int                   f_arg0;
};

In the end I am not sure that the code I suggested (including the comment) is confusing.

[avg]

sys/i386/i386/db_trace.c

393 ↗

(On Diff #64145)

Just want to clarify that we do have a saved %ebp, but it's certainly not a saved frame pointer as its value is kind of arbitrary. Is this what you meant?
So, using 8 + 4*narg is better than sizeof(i386_frame) + 4*(narg-1) ?

[kib]

In D22303#487963, @jhb wrote:

Note that gdb also knows which symbols have which behavior, so changing the stack frame layout will break kgdb, and then kgdb would have to somehow detect which i386 kernels had which behavior. We've done that once before by peeking at instructions [1], but I think it's not worth spending that much effort on i386 and easier to just let the frame layouts stay as-is. The reason for the enums was to handle the separate cases as you note for args. I don't mind changing this to use 'narg'.

1: https://github.com/bsdjhb/gdb/blob/kgdb-8.3/gdb/i386fbsd-kern.c#L82

For reference: here is how we currently decode frame offsets in kgdb now:

https://github.com/bsdjhb/gdb/blob/kgdb-8.3/gdb/i386fbsd-kern.c#L337

What I propose would not change the layout. It only makes %ebp saved by the called C handler to point to the start of struct trapframe.

I propose Andrey commit the review as is, taking the suggestions about comments, and then I will redo the unwinding, eliminating the nargs/formula at all.

[avg]

Thank you for the comments and suggestions.
I updated the comment per John's suggestion (but the tf formula) and took the path forward suggested by Kostik.