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:
> >
> >>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.
>
> 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.
> >
> >
> >> 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.
>
> The difference is that I wanted to turn off the interrupt system with the
> trap and the standard code did not. My notion was that a break point should
> "stop the world" including interrupts. The down side is that this required
> code in the trap handler to turn them back on if the trap turned out not to
> 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.
> >
> > Think we will ever get the kgdb patch into the kernel?
>
> I hope so. The problem I always had in the commercial world was that one
> could never convince management that a kernel debugger (or even most
> debugging code) was worth wild as it could not be sold, only given away. I,
> on the other hand, always thought any time I spent on the debugger would
> shorten my development time more than enough to pay for it. If more of us
> felt this way we might put enough energy into the wave to get it to the beach.
> >
> > 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.
>
> I suspect you mean it is confusing gdb rather than kgdb. I really don't
> think you want to restrict code to match what the debugger can do, but rather
> kick the butt of the debugger folks to catch up.
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
> in catch up mode (if the code style is evolving). Thus the need for macros
> and other such extensible features in gdb. See, for example, my macros to
> dereference per_cpu structures where we struggle with the lack of a "type of"
> 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.
> > 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.
>
> I am not sure I can go along with this. It seems to me that code should and
> must be developed to do the right thing as efficiently as possible.
> Debugging aids, when they add to the code size, should be removed (hopefully
> by macro so they can be returned when needed) when the code is properly
> working.
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
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
>
> --
> George
> >
> > -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
> >>>
> >>
> >>
> >>------------------------------------------------------------------------
> >>
> >>-------------------------------------------------------------------------
> >>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
>
--
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
------------------------------------------------------------------------- Using Tomcat but need to do more? Need to support web services, security? Get stuff done quickly with pre-integrated technology to make your job easier Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________ Kgdb-bugreport mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport
