Re: [PATCH] align break instruction address in kgdb_set_sw_break

Attached patch does break instruction alignment on ia64. I've created
this patch on top of previous patch I sent on KGDB-Bugreport
(breakpoint state transition in remove_all_break).

I tested both of them on i386 and they worked fine.

On Tue, 2006-01-17 at 20:49 +0530, Milind Dumbare wrote:
> 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

break-align.patch
Description: Text Data