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 always > > 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 > unmaintainable" statement, Tom Rini. I think that Piet Delaney is totally > right about this, and I think he refers to the official coding style (see > below for references). Documentation/SubmittingPatches explains this (it > 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 code
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 header
based macros/inlines to conditionally link in the kgdb stub sounds like
a solution that I suspect everybody can live with. Trap.c may be handled
best with a global variable indicating that the early trap handlers have
been registered.
> So, I do not understand why you are discussing an issue which has been
> virtually standardized.
>
> Arguing that this style causes unmaintainability is probably a bit like
> being "heretic" (no offense intended) since, for what I learnt while becoming
> a Linux kernel hacker (i.e. the past two years, but I learned it well since
> it was my first large C project and I'm just 20 years old), this is
> the "official" style.
>
> Look at almost any function in include/linux/security.h, for instance, say
> security_ptrace() or security_init(). They are inline functions and have
> 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 because it
> became obvious (that name may be appropriate or not).
>
> I also do not understand why I'm here, since I usually work on UML, but I
> like
> 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 global
variable say 'early_traps_established" that gets set via a kgdb inline
that used in setup_arch(). Then if early_traps_established == 0 the
normal trap code registers the 'debug' trap handlers:
int early_traps_established = 0;
/*
* Some trap handlers need to be set early
* for kernel debuggers (Ex: kgdb, kdb)
*/
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 = 1;
}
.
.
.
void __init trap_init(void)
{
#ifdef CONFIG_EISA
void __iomem *p = ioremap(0x0FFFD9, 4);
if (readl(p) == 'E'+('I'<<8)+('S'<<16)+('A'<<24)) {
EISA_bus = 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.
Clearly we can be virtually identical to the existing kernel.org
code without cluttering up the C code with #ifdef's.
-piet
--
Piet Delaney Phone: (408) 200-5256
Blue Lane Technologies Fax: (408) 200-5299
10450 Bubb Rd.
Cupertino, Ca. 95014 Email: [EMAIL PROTECTED]
signature.asc
Description: This is a digitally signed message part
------------------------------------------------------------------------- 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=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________ Kgdb-bugreport mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
