Hi.
Thanks for your review.
I updated the patch following your comments except
the interrupt-related related comment.
I included your signed-off-by to the patch, is it ok?
On Fri, Jan 13, 2006 at 09:46:04AM -0600, Hollis Blanchard wrote:
> Thanks Isaku! This whole patch has whitespace issues though; please
> replace your tabs with 4 spaces.
untabified.
> >+ . = ALIGN(32);
> >+ __gdbstub_text_start = . ;
> >+ *(.gdbstub.text)
> >+ __gdbstub_text_end = . ;
> >+ . = ALIGN(32) ;
> ...
> >+/* Append this to the declaration of any function needed to be used by
> >+ * the gdb stub. Any functions marked with this will not be allowed
> >+ * to have a breakpoint set in them. */
> >+#define __gdbstub __attribute__((section(".gdbstub.text")))
>
> These make sense if we want to avoid setting a breakpoint in GDB stub
> code or code called by the GDB stub, such as serial output. However,
> these symbols aren't currently used. (You probably inherited this from
> the PPC stub.) At least it's worth a TODO comment, possibly in
> gdb_cmd_write_mem(): we shouldn't be writing to memory between those
> two symbols.
added __gdbstub_text_start, __gdbstub_text_end declarations and
a comment above gdb_cmd_write_mem()
> >+/* XXX move to some shared location... */
> >+char
> >+hex2char(unsigned long x)
>
> With hex2char, char2hex, str2hex, str2ulong, I expect that they will be
> needed in the arch-specific gdb backends. Actually I guess they just
> need to be added to gdbstub.h; I'm not sure why I added that comment.
added protptypes to gdb.h and removed two XXX comments.
> >+ for (ctx->in_bytes = 0; ctx->in_bytes < sizeof(ctx->in_buf);
> >ctx->in_bytes++) {
> Exceeds 80 chars.
fixed.
> >+#ifdef CONFIG_GDB_NO_CSUM
> >+ return 0;
> >+#else
> >+ if (received_csum == csum) {
> >+ return 0;
> >+ } else {
> >+ return -1;
> >+ }
> >+#endif
> What is this about? As far as I know, the GDB protocol unconditionally
> requires the checksum.
removed.
> >+ //XXX optimization to use best native load size
> ...
> >+ //XXX optimization to use best native store size
> I think it's reasonable to leave it up to the arch code to decide what
> size access to use, so these comments could go.
removed these comments.
> >+ .signum = 1,
> >+
> >+ .in_bytes = 0,
> >+
> >+ .out_offset = 0,
>
> Blank lines.
removed blank lines.
> >+ /* Try to make things a little more stable by disabling
> >+ interrupts while we're here. */
> >+ local_irq_save(flags);
> ...
> >+ local_irq_restore(flags);
> Shouldn't we enter __trap_to_gdb with interrupts disabled already?
> That's how it's done on PPC, I believe x86 can choose in the IDT, and
> I'm sure IA64 can too?
Left as it was.
As Keir explained, it is a good safe guard.
--
yamahata
8605:3ca0840ea69e.patch
Description: Text document
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home