Re: [Kgdb-bugreport] Integration with kernel.org repository - shouldn't code in traps.c be #ifdef
Status: Beta
Brought to you by:
jwessel
Menu
▾
▴
Re: [Kgdb-bugreport] Integration with kernel.org repository - shouldn't code in traps.c be #ifdef CONFIG_KGDB?
|
From: Piet D. <pi...@bl...> - 2006-10-11 20:40:51
|
On Wed, 2006-10-11 at 00:59 -0700, George Anzinger wrote: > Piet Delaney wrote: > > On Mon, 2006-10-09 at 19:30 -0700, George Anzinger wrote: > >=20 > >>Piet Delaney wrote: > >> > >>>On Sat, 2006-10-07 at 15:37 +0200, Blaisorblade wrote: > >>> > >>> > >>>>On Friday 06 October 2006 02:07, Tom Rini wrote: > >>>> > >>>> > >>>>>On Thu, Oct 05, 2006 at 02:30:11PM -0700, Piet Delaney wrote: > >>>>> > >>>>> > >>>>>>On Thu, 2006-10-05 at 12:17 -0700, Tom Rini wrote: > >>>>>> > >>>>>> > >>>>>>>On Thu, Oct 05, 2006 at 08:46:20AM -0700, George Anzinger wrote: > >>>>>>> > >>>>>>> > >>>>>>>>Tom Rini wrote: > >>>>>>>> > >>>>>>>> > >>>>>>>>>On Wed, Oct 04, 2006 at 08:42:04PM -0700, Piet Delaney wrote: > >>>>>>>>> > >>>>>>>>> > >>>>>>>>>>Ton, George, Amit, et. al: > >>>>>>>>>> > >>>>>>>>>>If CONFIG_KGDB isn't defined, the kernel should be exactly the > >>>>>>>>>>same as when kgdb isn't integrated. So shouldn't we should add > >>>>>>>>>>#ifdef CONFIG_KGDB in the traps.c code where the traps are > >>>>>>>>>>initialized and panic notify is registered. > >>>>>>>>> > >>>>>>>>>No, because that makes the code look way too ugly. > >>>>>> > >>>>>>I agree that it's ugly, but until stock kernel wants to > >>>>>>do it this way I suspect that we will have an easier time > >>>>>>in being assimilated into the kernel.org repository by > >>>>>>being non-invasive. > >>>>> > >>>>>Right, non-invasive like always doing something at one point and alw= ays > >>>>>doing something else at another. > >>>>> > >>>>> > >>>>> > >>>>>>We can likely avoid uglyness with cpp macros. > >>>>> > >>>>>And introduce unmaintainability. > >>>> > >>>>Well, please detail what you mean with your "CPP macros make code=20 > >>>>unmaintainable" statement, Tom Rini. I think that Piet Delaney is tot= ally=20 > >>>>right about this, and I think he refers to the official coding style = (see=20 > >>>>below for references). Documentation/SubmittingPatches explains this = (it=20 > >>>>should be moved in CodingStyle however!) in Section 2, point #2. > >>> > >>> > >>>I didn't know about "Documentation/SubmittingPatches" but having read = it > >>>it's totally consistent with my point of view. Most of my debugging co= de > >>>I do with #defines or static inlines; always putting them in include > >>>files. Perhaps Andy's concern is just the location of the conditional > >>>code, placing it in include files instead of the .c files. Using heade= r > >>>based macros/inlines to conditionally link in the kgdb stub sounds lik= e > >>>a solution that I suspect everybody can live with. Trap.c may be handl= ed > >>>best with a global variable indicating that the early trap handlers ha= ve > >>>been registered. > >>>=20 > >>> > >>> > >>> > >>>>So, I do not understand why you are discussing an issue which has bee= n=20 > >>>>virtually standardized. > >>>> > >>>>Arguing that this style causes unmaintainability is probably a bit li= ke=20 > >>>>being "heretic" (no offense intended) since, for what I learnt while = becoming=20 > >>>>a Linux kernel hacker (i.e. the past two years, but I learned it well= since=20 > >>>>it was my first large C project and I'm just 20 years old), this is=20 > >>>>the "official" style. > >>>> > >>>>Look at almost any function in include/linux/security.h, for instance= , say=20 > >>>>security_ptrace() or security_init(). They are inline functions and h= ave=20 > >>>>different definitions depending on settings. > >>>> > >>>>Pieces of code as: > >>>>trap_init() > >>>>#ifdef CONFIG_KGDB > >>>> if (kgdb_early_setup) > >>>> return; /* Already done */ > >>>>#endif > >>>> > >>>>are usually translated with (chosen names are examples): > >>>> > >>>>header file: > >>>>#ifdef CONFIG_KGDB > >>>>static inline int trap_setup_done() > >>>>{ > >>>> return kgdb_early_setup; > >>>>} > >>>>#else > >>>>static inline int trap_setup_done() > >>>>{ > >>>> return 0; > >>>>} > >>>>#endif > >>>>... > >>>>trap_init(): > >>>> if (trap_setup_done()) > >>>> return; > >>>> > >>>>Note that given the name choice I could also remove the comment becau= se it=20 > >>>>became obvious (that name may be appropriate or not). > >>>> > >>>>I also do not understand why I'm here, since I usually work on UML, b= ut I like=20 > >>>>this. > >>> > >>> > >>>There are other debuggers, kdb for example, that might also want to > >>>register a sub-set of traps during early startup. So how about a globa= l > >>>variable say 'early_traps_established" that gets set via a kgdb inline > >>>that used in setup_arch(). Then if early_traps_established =3D=3D 0 th= e > >>>normal trap code registers the 'debug' trap handlers: > >> > >>Don't know about other archs but the x86 trap handlers can just be rese= t (or=20 > >>set a second time) as long as they are to be the same. So, unless you = can=20 > >>save code, why do the conditional? > >=20 > >=20 > > I thought it makes the implementation a bit easier to understand and > > this isn't a path were Performance is an issue. >=20 > No Performance isn't but code size is always... If we call a common piece of code to set the three interrupt handlers it's just the cost of one subroutine call and possibly one trivial conditional. > >=20 > >=20 > >> In any case, you might want to make = the=20 > >>subset up a function and call it from the whole set up, again to save c= ode. > >=20 > >=20 > > Yep, I thought of that also, that's pretty straight forward. > >=20 > >=20 > >>Now if the trap entry needs to be different, that is a whole different = kettle=20 > >>of fish... > >=20 > >=20 > > Yea, I recall your patch using different trap handlers. How about > > telling us about that kettle of fish. Seems like a nice time to > > learn more about the way that your patch differed from the current > > SF kgdb patch. >=20 > The difference is that I wanted to turn off the interrupt system with the= =20 > trap and the standard code did not. My notion was that a break point sho= uld=20 > "stop the world" including interrupts. The down side is that this requir= ed=20 > code in the trap handler to turn them back on if the trap turned out not = to=20 > be for kgdb. Sounds like a good idea; wonder why we don't steal the idea. Most of your ideas were assimilated into the current stub. How much code was it? I did find your trap code more challenging than our current code. > >=20 > > Think we will ever get the kgdb patch into the kernel? >=20 > I hope so. The problem I always had in the commercial world was that one= =20 > could never convince management that a kernel debugger (or even most=20 > debugging code) was worth wild as it could not be sold, only given away. = I,=20 > on the other hand, always thought any time I spent on the debugger would=20 > shorten my development time more than enough to pay for it. If more of u= s=20 > felt this way we might put enough energy into the wave to get it to the b= each. > >=20 > > So much kernel code is being written without kgdb that > > it's amazing how difficult the code is being made to debug > > due to use of gdb on the kernel. I'm messing with device mapper > > and they are using object originated like programing style by > > using null structure declarations in common code. Confuses kgdb > > and make the code less maintainable.=20 >=20 > I suspect you mean it is confusing gdb rather than kgdb. I really don't=20 > think you want to restrict code to match what the debugger can do, but ra= ther=20 > kick the butt of the debugger folks to catch up.=20 I doubt many of the abuses of C can be compensated by gdb. In this case the struct declaration is different in two different objects. I suppose the debugger could guess that a NULL structure is bogus and pinch the debug info from the other module. I think the code is less confusing if the empty structures are dropped and the structure declaring put in a header and done normally. That's what I've done for the device mapper code that I'm working on. > Still, they will always be=20 > in catch up mode (if the code style is evolving). Thus the need for macr= os=20 > and other such extensible features in gdb. See, for example, my macros t= o=20 > dereference per_cpu structures where we struggle with the lack of a "type= of"=20 > operator in gdb. Macro's are a useful workaround but not easily used with ddd. ddd is great for traversing data structures. I'll attach an example from device mapper in a separate mail; it's > 100k so the list won't allow it.=20 > > The list structures are a similar headache. If we had wide usage > > of kgdb for kernel development then it would be easier to changes > > accepted like modifying the list structure to point back to it's > > parent structure. I do that here but we need broad usage of kgdb > > to get the environment better suited to kgdb development. >=20 > I am not sure I can go along with this. It seems to me that code should = and=20 > must be developed to do the right thing as efficiently as possible.=20 > Debugging aids, when they add to the code size, should be removed (hopefu= lly=20 > by macro so they can be returned when needed) when the code is properly w= orking. Right, that's what I'm doing with my parent structure specific list macros; in a non-debug kernel the list structure doesn't have a=20 pointer back to the structure it's in. Makes list structures more like socket buffer queues. Might be nice for gdb/C to provide an operator to return the type and starting point of a parent structure. In the mean time it might be worthwhile to get ddd to able to use gdb macros better. I haven't been very successfully at that with the exiting button definition capability. -piet >=20 > --=20 > George > >=20 > > -piet > >=20 > >=20 > >>--=20 > >>George > >> > >>> int early_traps_established =3D 0; > >>> > >>> /* > >>> * Some trap handlers need to be set early > >>> * for kernel debuggers (Ex: kgdb, kdb)=20 > >>> */ > >>> void __init early_trap_init(void) { > >>> set_intr_gate(1,&debug); > >>> set_system_intr_gate(3, &int3); /* int3 can be called from all*/ > >>> set_intr_gate(14,&page_fault); > >>> early_traps_established =3D 1; > >>> } > >>>. > >>>. > >>>. > >>> =20 > >>> void __init trap_init(void) > >>> { > >>> #ifdef CONFIG_EISA > >>> void __iomem *p =3D ioremap(0x0FFFD9, 4); > >>> if (readl(p) =3D=3D 'E'+('I'<<8)+('S'<<16)+('A'<<24)) { > >>> EISA_bus =3D 1; > >>> } > >>> iounmap(p); > >>> #endif > >>> > >>> #ifdef CONFIG_X86_LOCAL_APIC > >>> init_apic_mappings(); > >>> #endif > >>> > >>> if (!early_traps_established) { > >>> set_intr_gate(1,&debug); > >>> set_system_intr_gate(3, &int3); > >>> set_intr_gate(14,&page_fault); > >>> } > >>> set_trap_gate(0,÷_error); > >>> set_intr_gate(2,&nmi); > >>> set_system_gate(4,&overflow); /* int4/5 can be called from all > >>>*/ > >>> set_system_gate(5,&bounds); > >>> > >>>Using a inline function, trap_setup_done(), instead of the global > >>>early_traps_established is fine also.=20 > >>> > >>>Clearly we can be virtually identical to the existing kernel.org > >>>code without cluttering up the C code with #ifdef's. > >>> > >>>-piet > >>> > >> > >> > >>-----------------------------------------------------------------------= - > >> > >>-----------------------------------------------------------------------= -- > >>Take Surveys. Earn Cash. Influence the Future of IT > >>Join SourceForge.net's Techsay panel and you'll get the chance to share= your > >>opinions on IT & business topics through brief surveys -- and earn cash > >>http://www.techsay.com/default.php?page=3Djoin.php&p=3Dsourceforge&CID= =3DDEVDEV > >> > >> > >>-----------------------------------------------------------------------= - > >> > >>_______________________________________________ > >>Kgdb-bugreport mailing list > >>Kgd...@li... > >>https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport >=20 --=20 Piet Delaney Phone: (408) 200-5256 Blue Lane Technologies Fax: (408) 200-5299 10450 Bubb Rd. Cupertino, Ca. 95014 Email: pi...@bl... |