On Mon, 2006-10-09 at 19:30 -0700, George Anzinger wrote:
> 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 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:
>
> Don't know about other archs but the x86 trap handlers can just be reset (or
> set a second time) as long as they are to be the same. So, unless you can
> save code, why do the conditional?
I thought it makes the implementation a bit easier to understand and
this isn't a path were Performance is an issue.
> In any case, you might want to make the
> subset up a function and call it from the whole set up, again to save code.
Yep, I thought of that also, that's pretty straight forward.
>
> Now if the trap entry needs to be different, that is a whole different kettle
> of fish...
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.
Think we will ever get the kgdb patch into the kernel?
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.
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.
-piet
>
> --
> George
> >
> > 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
