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? 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.
Now if the trap entry needs to be different, that is a whole different kettle
of fish...
--
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
>
--
George Anzinger [EMAIL PROTECTED]
-------------------------------------------------------------------------
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
