Re: Separate 'continue' and 'step' handling from other remote packet handling

Jim

This would be a great feature. I understand that much more work is going to be 
needed. For now improving single step and continue code is ok.

I didn't understand why at_addr parameter is needed. Could you send us the 
outline of single-step, continue function calls from present stub code and 
future changes?

Ensuring that the code on other platforms doesn't break is essential. Perhaps 
you want to do it in small steps so that it's easier to verify each step.

George had implemented some gdb macros (andthen) which offered a similar 
functionality, though was limited because gdb's debug info parsing engine 
wasn't involved.
-Amit

On Wednesday 25 Jan 2006 12:00 pm, Jim Blandy wrote:
> I'm working on getting GDB tracepoints working with KGDB.  For a
> general explanation of tracepoints, see:
>
> http://sources.redhat.com/gdb/talks/esc-west-1999/
>
> When the kernel hits a tracepoint, KGDB logs some data of the user's
> choosing, and then immediately continues, without talking to GDB.
> This means that KGDB itself has to handle the process of stepping off
> a breakpoint (i.e., remove breakpoint; single-step; replace
> breakpoint; continue); in the past, KGDB simply let GDB take care of
> all that.
>
> Anyway, it's a pain that the 'single step' and 'continue'
> functionality is wrapped up in kgdb_arch_handle_exception.  To do a
> single-step, I'd have to fake up an 's' packet and pass it to
> kgdb_arch_handle_exception, which seems dopey.  And then there's the
> fact that you've got the 'c' and 's' packet parsing replicated in
> every per-arch patch.
>
> So would something generally like the below be welcome?  This is just
> a sketch, it's untested; I wanted to see what folks thought before I
> invested much in it.
>
> Similar changes to those shown for the i386 would be needed for the
> other architectures.  Or, if you want to avoid retrofitting all the
> architectures, I could make kgdb_continue optional as well, and fall
> back to letting kgdb_arch_handle_exception handle 's' and 'c' packets.
>  But I don't mind doing the retrofitting, if someone can help me test
> them.
>
> Index: linux-2.6.11/include/linux/kgdb.h
> ===================================================================
> *** linux-2.6.11.orig/include/linux/kgdb.h
> --- linux-2.6.11/include/linux/kgdb.h
> *************** extern void sleeping_thread_to_gdb_regs(
> *** 106,111 ****
> --- 106,129 ----
>    */
>   extern void gdb_regs_to_regs(unsigned long *gdb_regs, struct pt_regs
> *regs);
>
> +
> + /**     kgdb_continue - set process up for continuing
> +  *      @regs - registers for the process
> +  *      @at_addr - true if address to continue at is given
> +  *      @addr - if @at_addr is true, address to continue at
> +  *
> +  *      Make any changes to the processor state in @regs needed to
> +  *      resume execution of that thread.  (Clear single-step flag
> +  *      bits, etc.)  If @at_addr is zero, continue at whatever
> +  *      location the PC in @regs suggests.  If @at_addr is non-zero,
> +  *      however, continue at @addr.  (We could use @addr == 0 to
> +  *      indicate "continue at present location", but it's nice to
> +  *      allow GDB to actually say "continue at address zero".)
> +  */
> + extern void kgdb_continue (struct pt_regs *regs,
> +                            int at_addr,
> +                            unsigned long addr);
> +
>   /**
>    *  kgdb_arch_handle_exception - Handle architecture specific GDB packets.
>    *  @vector: The error vector of the exception that happened.
> *************** extern struct task_struct *kgdb_get_shad
> *** 170,175 ****
> --- 188,214 ----
>   extern struct pt_regs *kgdb_shadow_regs(struct pt_regs *regs, int
> threadid);
>
>   /**
> +  *      kgdb_single_step - set process up for single-stepping
> +  *      @regs - registers for the process
> +  *      @at_addr - true if address to step at is given
> +  *      @addr - if @at_addr is true, address to step at
> +  *
> +  *      Make any changes to the processor state in @regs needed to
> +  *      step a single instruction.  (Set single-step flag bits, etc.)
> +  *      If @at_addr is zero, continue at whatever location the PC in
> +  *      @regs suggests.  If @at_addr is non-zero, however, continue at
> +  *      @addr.  (We could use @addr == 0 to indicate "continue at
> +  *      present location", but it's nice to allow GDB to actually say
> +  *      "continue at address zero".)
> +  *
> +  *      On success, return a positive value.  If the current target
> +  *      doesn't implement single-stepping, return -EINVAL.
> +  */
> + extern int kgdb_single_step (struct pt_regs *regs,
> +                              int at_addr,
> +                              unsigned long addr);
> +
> + /**
>    * struct kgdb_arch - Desribe architecture specific values.
>    * @gdb_bpt_instr: The instruction to trigger a breakpoint.
>    * @flags: Flags for the breakpoint, currently just %KGDB_HW_BREAKPOINT.
> Index: linux-2.6.11/kernel/kgdb.c
> ===================================================================
> *** linux-2.6.11.orig/kernel/kgdb.c
> --- linux-2.6.11/kernel/kgdb.c
> *************** struct pt_regs __attribute__ ((weak))
> *** 288,293 ****
> --- 288,299 ----
>       return NULL;
>   }
>
> + extern int __attribute__ ((weak))
> +     kgdb_single_step (struct pt_regs *regs, int at_addr, unsigned long
> addr) + {
> +         return -EINVAL;
> + }
> +
>   static int hex(char ch)
>   {
>       if ((ch >= 'a') && (ch <= 'f'))
> *************** int kgdb_handle_exception(int ex_vector,
> *** 1388,1394 ****
>                               break;
>                       }
>
> !                     /* Followthrough to default processing */
>               default:
>                     default_handle:
>                       error = kgdb_arch_handle_exception(ex_vector, signo,
> --- 1394,1422 ----
>                               break;
>                       }
>
> !                         /* try to read optional parameter, pc
> !                          * unchanged if no parm */
> !                         ptr = &remcom_in_buffer[1];
> !                         length = kgdb_hex2long(&ptr, &addr);
> !                         if (*ptr != '\0') {
> !                                 error_packet (remcom_out_buffer,
> -EINVAL); !                                 break;
> !                         }
> !
> !                         if (remcom_in_buffer[0] == 'c')
> !                                 kgdb_continue (linux_regs, length, addr);
> !                         else if ((error = kgdb_single_step
> (linux_regs, length,
> !                                                             addr))
> !                                  < 0) {
> !                                 if (error != -EINVAL)
> !                                         error_packet
> (remcom_out_buffer, error);
> !                                 /* Otherwise, leave it blank, which
> !                                  * means "unrecognized".  */
> !                                 break;
> !                         }
> !
> !                         goto kgdb_exit;
> !
>               default:
>                     default_handle:
>                       error = kgdb_arch_handle_exception(ex_vector, signo,
> Index: linux-2.6.11/arch/i386/kernel/kgdb.c
> ===================================================================
> *** linux-2.6.11.orig/arch/i386/kernel/kgdb.c
> --- linux-2.6.11/arch/i386/kernel/kgdb.c
> *************** int kgdb_arch_handle_exception(int e_vec
> *** 255,307 ****
>                              char *remcom_out_buffer,
>                              struct pt_regs *linux_regs)
>   {
> -     long addr;
> -     char *ptr;
> -     int newPC, dr6;
> -
> -     switch (remcom_in_buffer[0]) {
> -     case 'c':
> -     case 's':
> -             /* try to read optional parameter, pc unchanged if no parm */
> -             ptr = &remcom_in_buffer[1];
> -             if (kgdb_hex2long(&ptr, &addr))
> -                     linux_regs->eip = addr;
> -             newPC = linux_regs->eip;
> -
> -             /* clear the trace bit */
> -             linux_regs->eflags &= ~TF_MASK;
> -             atomic_set(&cpu_doing_single_step, -1);
> -
> -             /* set the trace bit if we're stepping */
> -             if (remcom_in_buffer[0] == 's') {
> -                     linux_regs->eflags |= TF_MASK;
> -                     debugger_step = 1;
> -                     if (kgdb_contthread)
> -                             atomic_set(&cpu_doing_single_step,
> -                                        smp_processor_id());
> -             }
> -
> -             asm volatile ("movl %%db6, %0\n":"=r" (dr6));
> -             if (!(dr6 & 0x4000)) {
> -                     long breakno;
> -                     for (breakno = 0; breakno < 4; ++breakno) {
> -                             if (dr6 & (1 << breakno) &&
> -                                 breakinfo[breakno].type == 0) {
> -                                     /* Set restore flag */
> -                                     linux_regs->eflags |= X86_EFLAGS_RF;
> -                                     break;
> -                             }
> -                     }
> -             }
> -             kgdb_correct_hw_break();
> -             asm volatile ("movl %0, %%db6\n"::"r" (0));
> -
> -             return (0);
> -     }                       /* switch */
>       /* this means that we do not want to exit from the handler */
>       return -1;
>   }
>
>   /* Register KGDB with the i386die_chain so that we hook into all of the
> right * spots. */
>   static int kgdb_notify(struct notifier_block *self, unsigned long cmd,
> --- 255,320 ----
>                              char *remcom_out_buffer,
>                              struct pt_regs *linux_regs)
>   {
>       /* this means that we do not want to exit from the handler */
>       return -1;
>   }
>
> + static void check_for_restore (struct pt_regs *regs)
> + {
> +         int dr6;
> +
> +         asm volatile ("movl %%db6, %0\n":"=r" (dr6));
> +         if (!(dr6 & 0x4000)) {
> +                 long breakno;
> +                 for (breakno = 0; breakno < 4; ++breakno) {
> +                         if (dr6 & (1 << breakno) &&
> +                             breakinfo[breakno].type == 0) {
> +                                 /* Set restore flag */
> +                                 regs->eflags |= X86_EFLAGS_RF;
> +                                 break;
> +                         }
> +                 }
> +         }
> + }
> +
> + void kgdb_continue(struct pt_regs *regs,
> +                    int at_addr,
> +                    unsigned long addr)
> + {
> +         if (at_addr)
> +                 regs->eip = addr;
> +
> +         /* clear the trace bit */
> +         regs->eflags &= ~TF_MASK;
> +         atomic_set(&cpu_doing_single_step, -1);
> +
> +         check_for_restore (regs);
> +         kgdb_correct_hw_break();
> +         asm volatile ("movl %0, %%db6\n"::"r" (0));
> + }
> +
> + int kgdb_single_step (struct pt_regs *regs,
> +                       int at_addr,
> +                       unsigned long addr)
> + {
> +         /* set the trace bit */
> +         regs->eflags |= TF_MASK;
> +         debugger_step = 1;
> +
> +         if (kgdb_contthread)
> +                 atomic_set(&cpu_doing_single_step, smp_processor_id());
> +         else
> +                 atomic_set(&cpu_doing_single_step, -1);
> +
> +         check_for_restore (regs);
> +         kgdb_correct_hw_break();
> +         asm volatile ("movl %0, %%db6\n"::"r" (0));
> +
> +         return 0;
> + }
> +
> +
> +
>   /* Register KGDB with the i386die_chain so that we hook into all of the
> right * spots. */
>   static int kgdb_notify(struct notifier_block *self, unsigned long cmd,
>
>
> -------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc. Do you grep through log
> files for problems?  Stop!  Download the new AJAX search engine that makes
> searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid3432&bid#0486&dat1642
> _______________________________________________
> Kgdb-bugreport mailing list
> Kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642