Re: [PATCH] align break instruction address in kgdb_set_sw_break

Amit Kale wrote:        [Mon Jan 16 2006, 08:46:10AM EST]
Amit,
> Bob,
> 
> Phew! Got that. This patch is correct, though I would prefer doing the same 
> thing though a weak definition of kgdb_arch_validate_breakpoint as shown in 
> attached patch. It also fixes remove_all_breakalong with kgdb_set_sw_break.
That's fine with me and a nicer approach.
> 
> It includes some more minor changes. I haven't compiled it. Will compile it 
> before checking in if no-one finds any problems with it.
I'll try and find time later today to test your patch.
> 
> -Amit
thanks,

bob
> 
> On Friday 13 Jan 2006 10:06 pm, Bob Picco wrote:
> > Amit Kale wrote:    [Fri Jan 13 2006, 05:11:33AM EST]
> >
> > > Bob,
> >
> > Amit,
> >
> > Sorry for delay but fighting fires this morning.
> >
> > > Does gdb actually place breakpoints in middle of an instruction? If gdb
> > > does ever ask for a non-aligned breakpoint, when the breakpoint occurs,
> > > it'll be
> >
> > Well a bundle is three instructions. Slot 0 (first instruction) is 16 byte
> > aligned but slot 1 and slot 2 are not byte aligned.  That is why gdb
> > uses buddle-address+slot to identify where the breakpoint is set. To add
> > to the complexity each slot could actually use a different kind of
> > break instruction because the break instruction must match the exeuction
> > unit of the instruction's slot.
> >
> > > on a different address, not the one which gdb asked. gdb won't know that
> > > the breakpoint has occured. Won't it treat it as a spurious trap?
> > >
> > > Am I missing something?
> >
> > Hopefully the text above has helped.  If not, feel free to ask a
> > different way.
> >
> > > I believe kgdb stub should refuse a non-aligned breakpoint.
> > > -Amit
> >
> > bob
> >
> > > On Friday 13 Jan 2006 5:45 am, Bob Picco wrote:
> > > > The check in kgdb_set_sw_break for reading the breakpoint location
> > > > isn't exactly correct for ia64.  My brain hurts each time I must
> > > > remember the ia64 instructionset layout but some description is
> > > > required here. For a breakpoint address gdb sends kgdb the instruction
> > > > bundle address+slot as the target address. An ia64 instruction is in a
> > > > 128 bit bundle. Each bundle has three instruction slots and is 16 bytes
> > > > aligned.  There are 41 bits per instruction and a 5
> > > > bit template to identify what execution unit is used for each
> > > > instruction slot; (41*3+5 = 128). Unless the breakpoint is in slot
> > > > zero, which would be just luck, then the address sent to kgdb won't
> > > > start on a bundle 16 byte aligned address. We need to align the address
> > > > correctly before checking whether the entire ia64 bundle can be read.
> > > >
> > > > To summarize bundle slot 1 and slot 2 result in one byte and two bytes
> > > > respectively being examined beyond the target bundle which is
> > > > incorrect. In 2.6.14 and before this worked because the ia64 arch
> > > > specific code, kgdb_arch_set_breakpoint, uses the correct magic.
> > > >
> > > > My other arch machines are busy.  I need to reinstall my i386 machine
> > > > and then will test. Should someone test this patch then please email
> > > > me.
> > > >
> > > > bob
> > > >
> > > > Signed-off-by: Bob Picco <bob.picco@xxxxxx>
> > > >
> > > >  include/asm-ia64/kgdb.h |    1 +
> > > >  include/linux/kgdb.h    |    4 ++++
> > > >  kernel/kgdb.c           |    4 ++--
> > > >  3 files changed, 7 insertions(+), 2 deletions(-)
> > > >
> > > > Index: linux-2.6.15-kgdb/include/asm-ia64/kgdb.h
> > > > ===================================================================
> > > > --- linux-2.6.15-kgdb.orig/include/asm-ia64/kgdb.h      2006-01-11
> > > > 13:45:23.000000000 -0500 +++
> > > > linux-2.6.15-kgdb/include/asm-ia64/kgdb.h       2006-01-11 
> > > > 17:33:43.000000000
> > > > -0500 @@ -26,6 +26,7 @@
> > > >  #define KGDBBREAKNUM           0x6665UL
> > > >  #define BREAKPOINT()           asm volatile ("break.m 0x6665")
> > > >  #define BREAK_INSTR_SIZE       16
> > > > +#define BREAK_INSTR_ALIGN      (~0xfUL)
> > > >  #define CACHE_FLUSH_IS_SAFE    1
> > > >
> > > >  struct pt_regs;
> > > > Index: linux-2.6.15-kgdb/include/linux/kgdb.h
> > > > ===================================================================
> > > > --- linux-2.6.15-kgdb.orig/include/linux/kgdb.h 2006-01-11
> > > > 13:45:23.000000000 -0500 +++
> > > > linux-2.6.15-kgdb/include/linux/kgdb.h  2006-01-11 17:33:43.000000000
> > > > -0500 @@ -46,6 +46,10 @@ extern atomic_t kgdb_sync_softlockup[NR_
> > > >
> > > >  extern struct task_struct *kgdb_usethread, *kgdb_contthread;
> > > >
> > > > +#ifndef BREAK_INSTR_ALIGN
> > > > +#define BREAK_INSTR_ALIGN      (~0UL)
> > > > +#endif
> > > > +
> > > >  enum kgdb_bptype {
> > > >         bp_breakpoint = '0',
> > > >         bp_hardware_breakpoint,
> > > > Index: linux-2.6.15-kgdb/kernel/kgdb.c
> > > > ===================================================================
> > > > --- linux-2.6.15-kgdb.orig/kernel/kgdb.c        2006-01-11 
> > > > 13:45:23.000000000
> > > > -0500 +++ linux-2.6.15-kgdb/kernel/kgdb.c       2006-01-11 
> > > > 17:34:35.000000000
> > > > -0500 @@ -829,8 +829,8 @@ static int kgdb_set_sw_break(unsigned lo
> > > >         int i, breakno = -1;
> > > >         int error = 0;
> > > >         char tmp_variable[BREAK_INSTR_SIZE];
> > > > -       if ((error = kgdb_get_mem((char *)addr, tmp_variable,
> > > > -                               BREAK_INSTR_SIZE)) < 0)
> > > > +       if ((error = kgdb_get_mem((char *)(addr & BREAK_INSTR_ALIGN),
> > > > +               tmp_variable, BREAK_INSTR_SIZE)) < 0)
> > > >                 return error;
> > > >         for (i = 0; i < MAX_BREAKPOINTS; i++) {
> > > >                 if ((kgdb_break[i].state == bp_set) &&
> > > >
> > > >
> > > >
> > > > -------------------------------------------------------
> > > > 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://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click
> > > > _______________________________________________
> > > > Kgdb-bugreport mailing list
> > > > Kgdb-bugreport@xxxxxxxxxxxxxxxxxxxxx
> > > > https://lists.sourceforge.net/lists/listinfo/kgdb-bugreport

> Index: linux-2.6.15/kernel/kgdb.c
> ===================================================================
> --- linux-2.6.15.orig/kernel/kgdb.c   2006-01-16 18:48:47.000000000 +0530
> +++ linux-2.6.15/kernel/kgdb.c        2006-01-16 19:04:40.000000000 +0530
> @@ -294,6 +294,14 @@
>  }
>  
>  int __attribute__ ((weak))
> +     kgdb_arch_validate_breakpoint(unsigned long addr)
> +{
> +     int error;
> +     error = kgdb_get_mem((char *)addr, tmp_variable, BREAK_INSTR_SIZE);
> +     return error;
> +}
> +
> +int __attribute__ ((weak))
>       kgdb_arch_set_breakpoint(unsigned long addr, char *saved_instr)
>  {
>       int error = 0;
> @@ -829,8 +837,7 @@
>       int i, breakno = -1;
>       int error = 0;
>       char tmp_variable[BREAK_INSTR_SIZE];
> -     if ((error = kgdb_get_mem((char *)addr, tmp_variable,
> -                             BREAK_INSTR_SIZE)) < 0)
> +     if ((error = kgdb_validate_break_address(addr)) < 0)
>               return error;
>       for (i = 0; i < MAX_BREAKPOINTS; i++) {
>               if ((kgdb_break[i].state == bp_set) &&
> @@ -914,30 +921,18 @@
>       return 0;
>  }
>  
> -int remove_all_break(void)
> +int kgdb_remove_all_break(void)
>  {
>       int i;
>       int error;
>       unsigned long addr;
>  
> -     /* Clear memory breakpoints. */
> -     for (i = 0; i < MAX_BREAKPOINTS; i++) {
> -             if (kgdb_break[i].state != bp_set) 
> -                     continue;
> -             addr = kgdb_break[i].bpt_addr;
> -             if ((error = kgdb_set_mem((char *)addr,
> -                             kgdb_break[i].saved_instr,
> -                             BREAK_INSTR_SIZE)) < 0)
> -                     return error;
> -             if (CACHE_FLUSH_IS_SAFE && current->mm &&
> -                             addr < TASK_SIZE)
> -                     flush_cache_range(current->mm->mmap_cache,
> -                                     addr, addr + BREAK_INSTR_SIZE);
> -             else if (CACHE_FLUSH_IS_SAFE)
> -                     flush_icache_range(addr,
> -                                     addr + BREAK_INSTR_SIZE);
> -             kgdb_break[i].state = bp_none;
> -     }
> +     /* Deactive breakpoints. */
> +     kgdb_deactivate_sw_breakpoints();
> +     
> +     /* Remove them. */
> +     for (i = 0; i < MAX_BREAKPOINTS; i++)
> +             kgdb_break[i].state = bp_removed;
>  
>       /* Clear hardware breakpoints. */
>       kgdb_remove_all_hw_break();
> @@ -1146,7 +1141,7 @@
>                        * during initial connect.  So to be safe,
>                        * we clear out our breakpoints now incase
>                        * GDB is reconnecting. */
> -                     remove_all_break();
> +                     kgdb_remove_all_break();
>                       /* Also, if we haven't been able to roundup all
>                        * CPUs, send an 'O' packet informing the user
>                        * as much.  Only need to do this once. */
> @@ -1254,7 +1249,7 @@
>                        * continue.
>                        */
>               case 'D':
> -                     if ((error = remove_all_break()) < 0) {
> +                     if ((error = kgdb_remove_all_break()) < 0) {
>                               error_packet(remcom_out_buffer, error);
>                       } else {
>                               strcpy(remcom_out_buffer, "OK");
> @@ -1265,7 +1260,7 @@
>  
>               case 'k':
>                       /* Don't care about error from remove_all_break */
> -                     remove_all_break();
> +                     kgdb_remove_all_break();
>                       kgdb_connected = 0;
>                       goto default_handle;
>  
> @@ -1731,7 +1726,7 @@
>                       printk(KERN_CRIT "kgdb: I/O module was unloaded while "
>                                       "a debugging session was running.  "
>                                       "KGDB will be reset.\n");
> -                     if (remove_all_break() < 0)
> +                     if (kgdb_remove_all_break() < 0)
>                               printk(KERN_CRIT "kgdb: Reset failed.\n");
>                       kgdb_connected = 0;
>               }
> Index: linux-2.6.15/arch/ia64/kernel/kgdb.c
> ===================================================================
> --- linux-2.6.15.orig/arch/ia64/kernel/kgdb.c 2006-01-16 18:48:44.000000000 
> +0530
> +++ linux-2.6.15/arch/ia64/kernel/kgdb.c      2006-01-16 19:06:34.000000000 
> +0530
> @@ -83,6 +83,8 @@
>  
>  #define      REGISTER_INDEX(N)       (REGISTER_BYTE(N) / sizeof (unsigned 
> long))
>  
> +#define BREAK_INSTR_ALIGN      (~0xfULL)
> +
>  #define      ptoff(V)        ((unsigned int) &((struct pt_regs *)0x0)->V)
>  struct reg_to_ptreg_index {
>       unsigned int reg;
> @@ -502,10 +504,18 @@
>       {u, u, u},              /* 1F */
>  };
>  
> +int kgdb_arch_validate_breakpoint(unsigned long addr)
> +{
> +     int error;
> +     error = kgdb_get_mem((char *)(addr & BREAK_INSTR_ALIGN), tmp_variable,
> +                     BREAK_INSTR_SIZE);
> +     return error;
> +}
> +
>  int kgdb_arch_set_breakpoint(unsigned long addr, char *saved_instr)
>  {
>       extern unsigned long _start[];
> -     unsigned long slot = addr & 0xf, bundle_addr;
> +     unsigned long slot = addr & BREAK_INSTR_ALIGN, bundle_addr;
>       unsigned long template;
>       struct bundle {
>               struct {
> @@ -560,7 +570,7 @@
>  {
>       extern unsigned long _start[];
>  
> -     addr = addr & ~0xFULL;
> +     addr = addr & BREAK_INSTR_ALIGN;
>       if (addr == (unsigned long)_start)
>               return 0;
>       return kgdb_set_mem((char *)addr, (char *)bundle, BREAK_INSTR_SIZE);
> Index: linux-2.6.15/include/linux/kgdb.h
> ===================================================================
> --- linux-2.6.15.orig/include/linux/kgdb.h    2006-01-16 18:48:47.000000000 
> +0530
> +++ linux-2.6.15/include/linux/kgdb.h 2006-01-16 19:11:14.000000000 +0530
> @@ -160,21 +160,29 @@
>   */
>  extern asmlinkage void kgdb_fault_longjmp(unsigned long *curr_context);
>  
> -/* Optional functions. */
> +/* These are defined in kernel/kgdb.c with a weak attribute. They
> + * may be overridden in arch dependent code. */
> +struct kgdb_io;
> +extern struct kgdb_io kgdb_io_ops;
> +
>  extern int kgdb_arch_init(void);
>  extern void kgdb_disable_hw_debug(struct pt_regs *regs);
> -extern void kgdb_post_master_code(struct pt_regs *regs, int e_vector,
> -                               int err_code);
> -extern void kgdb_roundup_cpus(unsigned long flags);
> +extern int kgdb_skipexception(int exception, struct pt_regs *regs);
>  extern int kgdb_set_hw_break(unsigned long addr);
>  extern int kgdb_remove_hw_break(unsigned long addr);
>  extern void kgdb_remove_all_hw_break(void);
>  extern void kgdb_correct_hw_break(void);
> +extern void kgdb_post_master_code(struct pt_regs *regs, int e_vector,
> +                               int err_code);
> +extern void kgdb_roundup_cpus(unsigned long flags);
>  extern void kgdb_shadowinfo(struct pt_regs *regs, char *buffer,
>                           unsigned threadid);
>  extern struct task_struct *kgdb_get_shadow_thread(struct pt_regs *regs,
>                                                 int threadid);
>  extern struct pt_regs *kgdb_shadow_regs(struct pt_regs *regs, int threadid);
> +extern int kgdb_arch_validate_breakpoint(unsigned long addr);
> +extern int kgdb_arch_set_breakpoint(unsigned long addr, char *saved_instr);
> +extern int kgdb_arch_remove_breakpoint(unsigned long addr, char *bundle);
>  
>  /**
>   * struct kgdb_arch - Desribe architecture specific values.
> @@ -240,7 +248,6 @@
>       void (*post_exception) (void);
>  };
>  
> -extern struct kgdb_io kgdb_io_ops;
>  extern struct kgdb_arch arch_kgdb_ops;
>  extern int kgdb_initialized;
>  
> @@ -257,7 +264,6 @@
>  extern int kgdb_set_mem(char *addr, unsigned char *buf, int count);
>  
>  int kgdb_isremovedbreak(unsigned long addr);
> -int kgdb_skipexception(int exception, struct pt_regs *regs);
>  
>  extern int kgdb_handle_exception(int ex_vector, int signo, int err_code,
>                               struct pt_regs *regs);



-------------------------------------------------------
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://ads.osdn.com/?ad_id=7637&alloc_id=16865&op=click