Re: [PATCH] align break instruction address in kgdb_set_sw_break

I've tested this patch on i386 and found out some problem with part of
code removed from remove_all_break() in kernel/kgdb.c. 

kgdb_deactivate_sw_breakpoints() which is replaced for that removed code
has different way of setting breakpoint states and that's causing
problem in setting breakpoint. I'm working on it and will come with
correct patch soon.

On Mon, 2006-01-16 at 19:16 +0530, Amit Kale wrote:
> 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.
> 
> 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.
> 
> -Amit
> 
> 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
-- 
...Miline
LinSysSoft Technologies
Ask me about KGDB Pro
http://www.linsyssoft.com/Kgdbpro.html



-------------------------------------------------------
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