Re: [PATCH] GDB stuff updated

[Vesa Jääskeläinen]

Hi Lubomir,

Lubomir Rintel wrote:
> Updated GDB stub patch, just header text changes, changelog text fix and
> stripped of junk.
>
> Also -- solved the mystery of the dl.c fix; for some reason I managed to
> generate a reversed diff. It is required to link the gdb module (symbols
> defined with .globl in assembler sources, IIRC)
>
> I am not going to attach all the patches here.
> All that's needed is here [1]:

Are those copyright years correct or result of copy paste error? Most of 
the files in this patch are new to GRUB 2 repository.

> grub2-dlsym-v4.patch

I am not big fan of fallthru's in switches. They can easily lead to 
programming errors. As code is rather short and I assume compiler will 
optimize it anyway, could you just make clean case for STT_NOTYPE 
without fallthru.

> grub2-gdb-macros-v4.patch

Should those be stored on folder like: contrib/gdb/

> grub2-gdb-stub-v4.patch

Do we want to support other communication mechanism than serial in 
future for GDB ? I am asking this because it could be prepared a bit 
with proper interfaces. That would also make code a bit more tidier.

+++ grub2-gdb/gdb/cstub.c       2008-05-07 10:15:52.000000000 +0200

+int (*grub_gdb_getchar) ();

Add void. Perhaps you could make typedef for it and declaring variable 
as static?

+static int
+hex (ch)
+     char ch;
+{

Let's be a bit more modern...

grub_gdb_getpacket (void):
It may be good idea to refer to documentation stating framing structure 
or adding comment about frame.

+++ grub2-gdb/gdb/gdb.c 2008-05-07 09:39:11.000000000 +0200

I do not really like this explicit attachment to serial device. Perhaps 
we should improve serial module and then make gdb to have communication 
interface.

+++ grub2-gdb/gdb/i386/idt.c    2008-05-07 09:39:11.000000000 +0200

Why not move this as own functional block to:
kern/i386/

Also removing trace for gdb :)

I think we need IDT for other features being planned.

+++ grub2-gdb/gdb/i386/machdep.S        2008-05-07 09:39:11.000000000 +0200
We do not do bunnies...

grub_idt_load: Why not move this to startup.S.

+++ grub2-gdb/include/grub/i386/gdb.h   2008-05-07 09:39:11.000000000 +0200

Seperate IDT stuff..

+++ grub2-gdb/include/grub/i386/pc/kernel.h	2008-05-07 
09:39:11.000000000 +0200

+#define GRUB_KERNEL_MACHINE_RAW_SIZE           0x4BC

Ok... this has to be automated... lets see...

+++ grub2-gdb/kern/i386/pc/startup.S    2008-05-07 09:39:11.000000000 +0200

How about raving readmode IDT (or IDT what we started with) so we can 
recall it when we are calling BIOS services... just in case.

+++ grub2-gdb/term/i386/pc/serial.c     2008-05-07 09:39:11.000000000 +0200

Not really happy about this change. Lets try to figure out better 
interface...

> grub2-preserve-symbols-v4.patch

Ok... Needs fine tuning for changelog entry, but otherwise fine.

> [1] http://fedorapeople.org/~lkundrak/grub2/

I hope those keep you busy for a while :)

Thanks,
Vesa Jääskeläinen