Paths

Table of Contentst

db_nextframe/i386: reduce the number of special frame types
ClosedPublic
Actions

Authored by avg on Nov 10 2019, 11:39 AM.

Details

Reviewers

kib
jhb
markj
royger

Commits

rS354627: db_nextframe/i386: reduce the number of special frame types

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.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 27428
Build 25666: arc lint + arc unit

Event Timeline

avg created this revision.Nov 10 2019, 11:39 AM

Herald added a subscriber: imp. · View Herald TranscriptNov 10 2019, 11:39 AM

Harbormaster completed remote builds in B27428: Diff 64145.Nov 10 2019, 11:39 AM

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) ?

markj accepted this revision.Nov 11 2019, 2:00 PM

This revision is now accepted and ready to land.Nov 11 2019, 2:00 PM

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).

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	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	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 added inline comments.Nov 11 2019, 5:05 PM

sys/i386/i386/db_trace.c
393	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 added inline comments.Nov 11 2019, 5:18 PM

sys/i386/i386/db_trace.c
393	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 + 4narg is better than sizeof(i386_frame) + 4(narg-1) ?

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.

Closed by commit rS354627: db_nextframe/i386: reduce the number of special frame types (authored by avg). · Explain WhyNov 11 2019, 7:06 PM

This revision was automatically updated to reflect the committed changes.

avg added a commit: rS354627: db_nextframe/i386: reduce the number of special frame types.

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.

Revision Contents
Changeset List

Path

Size

sys/

i386/

db_trace.c

45 lines

Diff 64145

View Options

db_nextframe/i386: reduce the number of special frame typesClosedPublicActions