[PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Mike Frysinger]

The Blackfin arch, like the x86 arch, needs to adjust the PC manually
after a breakpoint is hit as normally this is handled by the remote gdb.
However, rather than starting another arch ifdef mess, create a common
GDB_ADJUSTS_BREAK_OFFSET define for any arch to opt-in via their kgdb.h.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 arch/blackfin/include/asm/kgdb.h |    1 +
 arch/x86/include/asm/kgdb.h      |    1 +
 drivers/misc/kgdbts.c            |   14 ++++++--------
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/blackfin/include/asm/kgdb.h b/arch/blackfin/include/asm/kgdb.h
index c8b256d..8d5fe42 100644
--- a/arch/blackfin/include/asm/kgdb.h
+++ b/arch/blackfin/include/asm/kgdb.h
@@ -107,6 +107,7 @@ static inline void arch_kgdb_breakpoint(void)
 }
 #define BREAK_INSTR_SIZE	2
 #define CACHE_FLUSH_IS_SAFE	1
+#define GDB_ADJUSTS_BREAK_OFFSET
 #define HW_INST_WATCHPOINT_NUM	6
 #define HW_WATCHPOINT_NUM	8
 #define TYPE_INST_WATCHPOINT	0
diff --git a/arch/x86/include/asm/kgdb.h b/arch/x86/include/asm/kgdb.h
index e6c6c80..30671a8 100644
--- a/arch/x86/include/asm/kgdb.h
+++ b/arch/x86/include/asm/kgdb.h
@@ -75,5 +75,6 @@ static inline void arch_kgdb_breakpoint(void)
 }
 #define BREAK_INSTR_SIZE	1
 #define CACHE_FLUSH_IS_SAFE	1
+#define GDB_ADJUSTS_BREAK_OFFSET
 
 #endif /* _ASM_X86_KGDB_H */
diff --git a/drivers/misc/kgdbts.c b/drivers/misc/kgdbts.c
index e4ff50b..faff9cb 100644
--- a/drivers/misc/kgdbts.c
+++ b/drivers/misc/kgdbts.c
@@ -291,10 +291,10 @@ static int check_and_rewind_pc(char *put_str, char *arg)
 		 NUMREGBYTES);
 	gdb_regs_to_pt_regs(kgdbts_gdb_regs, &kgdbts_regs);
 	v2printk("Stopped at IP: %lx\n", instruction_pointer(&kgdbts_regs));
-#ifdef CONFIG_X86
-	/* On x86 a breakpoint stop requires it to be decremented */
-	if (addr + 1 == kgdbts_regs.ip)
-		offset = -1;
+#ifdef GDB_ADJUSTS_BREAK_OFFSET
+	/* On some arches, a breakpoint stop requires it to be decremented */
+	if (addr + BREAK_INSTR_SIZE == instruction_pointer(&kgdbts_regs))
+		offset = -BREAK_INSTR_SIZE;
 #endif
 	if (strcmp(arg, "silent") &&
 		instruction_pointer(&kgdbts_regs) + offset != addr) {
@@ -302,10 +302,8 @@ static int check_and_rewind_pc(char *put_str, char *arg)
 			   instruction_pointer(&kgdbts_regs) + offset, addr);
 		return 1;
 	}
-#ifdef CONFIG_X86
-	/* On x86 adjust the instruction pointer if needed */
-	kgdbts_regs.ip += offset;
-#endif
+	/* Readjust the instruction pointer if needed */
+	instruction_pointer(&kgdbts_regs) += offset;
 	return 0;
 }
 
-- 
1.6.3.1

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Andrew Morton]

On Tue,  2 Jun 2009 03:17:30 -0400
Mike Frysinger <vapier@gentoo.org> wrote:

> +	instruction_pointer(&kgdbts_regs) += offset;

instruction_pointer() cannot be used as an lvalue, thankfully.

x86_64:

drivers/misc/kgdbts.c: In function 'check_and_rewind_pc':
drivers/misc/kgdbts.c:306: error: invalid lvalue in assignment

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Mike Frysinger]

On Thu, Jun 4, 2009 at 20:50, Andrew Morton wrote:
> On Tue,  2 Jun 2009 03:17:30 -0400
> Mike Frysinger <vapier@gentoo.org> wrote:
>
>> +     instruction_pointer(&kgdbts_regs) += offset;
>
> instruction_pointer() cannot be used as an lvalue, thankfully.
>
> x86_64:
>
> drivers/misc/kgdbts.c: In function 'check_and_rewind_pc':
> drivers/misc/kgdbts.c:306: error: invalid lvalue in assignment

should be easy to fix:
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -236,10 +236,7 @@
 #endif
 }

-static inline unsigned long instruction_pointer(struct pt_regs *regs)
-{
-   return regs->ip;
-}
+#define instruction_pointer(regs) ((regs)->ip)

 static inline unsigned long frame_pointer(struct pt_regs *regs)
 {
-mike

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Andrew Morton]

On Thu, 4 Jun 2009 20:55:40 -0400
Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Thu, Jun 4, 2009 at 20:50, Andrew Morton wrote:
> > On Tue, __2 Jun 2009 03:17:30 -0400
> > Mike Frysinger <vapier@gentoo.org> wrote:
> >
> >> + __ __ instruction_pointer(&kgdbts_regs) += offset;
> >
> > instruction_pointer() cannot be used as an lvalue, thankfully.
> >
> > x86_64:
> >
> > drivers/misc/kgdbts.c: In function 'check_and_rewind_pc':
> > drivers/misc/kgdbts.c:306: error: invalid lvalue in assignment
> 
> should be easy to fix:
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -236,10 +236,7 @@
>  #endif
>  }
> 
> -static inline unsigned long instruction_pointer(struct pt_regs *regs)
> -{
> -   return regs->ip;
> -}
> +#define instruction_pointer(regs) ((regs)->ip)
> 
>  static inline unsigned long frame_pointer(struct pt_regs *regs)
>  {

argh, that's soooooo tasteless.  Look, this:

	instruction_pointer(&kgdbts_regs) += offset;

is just daft.  It's not C!  It makes no sense to define something which
looks like a function and to then assign values to it.  It means that
instruction_pointer() _must_ be implemented as a macro, violating basic
concepts of encapsualtion/layering/hiding/etc.

Doing

void instruction_pointer_set(struct pt_regs *regs, some_suitable_type val);

will save many vomit bags.

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Mike Frysinger]

On Thu, Jun 4, 2009 at 21:04, Andrew Morton wrote:
> On Thu, 4 Jun 2009 20:55:40 -0400 Mike Frysinger wrote:
>> On Thu, Jun 4, 2009 at 20:50, Andrew Morton wrote:
>> > On Tue, __2 Jun 2009 03:17:30 -0400 Mike Frysinger wrote:
>> >> + __ __ instruction_pointer(&kgdbts_regs) += offset;
>> >
>> > instruction_pointer() cannot be used as an lvalue, thankfully.
>> >
>> > x86_64:
>> >
>> > drivers/misc/kgdbts.c: In function 'check_and_rewind_pc':
>> > drivers/misc/kgdbts.c:306: error: invalid lvalue in assignment
>>
>> should be easy to fix:
>> --- a/arch/x86/include/asm/ptrace.h
>> +++ b/arch/x86/include/asm/ptrace.h
>> @@ -236,10 +236,7 @@
>>  #endif
>>  }
>>
>> -static inline unsigned long instruction_pointer(struct pt_regs *regs)
>> -{
>> -   return regs->ip;
>> -}
>> +#define instruction_pointer(regs) ((regs)->ip)
>>
>>  static inline unsigned long frame_pointer(struct pt_regs *regs)
>>  {
>
> argh, that's soooooo tasteless.  Look, this:
>
>        instruction_pointer(&kgdbts_regs) += offset;
>
> is just daft.  It's not C!

it is C.  taste is one thing, but valid C is still valid C.

> It makes no sense to define something which
> looks like a function and to then assign values to it.  It means that
> instruction_pointer() _must_ be implemented as a macro, violating basic
> concepts of encapsualtion/layering/hiding/etc.
>
> Doing
>
> void instruction_pointer_set(struct pt_regs *regs, some_suitable_type val);
>
> will save many vomit bags.

and force everyone to implement the same copy & paste set of get/set
modifiers ?  x86 is the only one where instruction_pointer() isnt a
define.
-mike

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Andrew Morton]

On Thu, 4 Jun 2009 21:50:49 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Thu, Jun 4, 2009 at 21:04, Andrew Morton wrote:
> > On Thu, 4 Jun 2009 20:55:40 -0400 Mike Frysinger wrote:
> >> On Thu, Jun 4, 2009 at 20:50, Andrew Morton wrote:
> >> > On Tue, __2 Jun 2009 03:17:30 -0400 Mike Frysinger wrote:
> >> >> + __ __ instruction_pointer(&kgdbts_regs) += offset;
> >> >
> >> > instruction_pointer() cannot be used as an lvalue, thankfully.
> >> >
> >> > x86_64:
> >> >
> >> > drivers/misc/kgdbts.c: In function 'check_and_rewind_pc':
> >> > drivers/misc/kgdbts.c:306: error: invalid lvalue in assignment
> >>
> >> should be easy to fix:
> >> --- a/arch/x86/include/asm/ptrace.h
> >> +++ b/arch/x86/include/asm/ptrace.h
> >> @@ -236,10 +236,7 @@
> >> __#endif
> >> __}
> >>
> >> -static inline unsigned long instruction_pointer(struct pt_regs *regs)
> >> -{
> >> - __ return regs->ip;
> >> -}
> >> +#define instruction_pointer(regs) ((regs)->ip)
> >>
> >> __static inline unsigned long frame_pointer(struct pt_regs *regs)
> >> __{
> >
> > argh, that's soooooo tasteless. __Look, this:
> >
> > __ __ __ __instruction_pointer(&kgdbts_regs) += offset;
> >
> > is just daft. __It's not C!

Gets frustrating when I say correct things and your first reaction is
to argue.

> it is C.  taste is one thing, but valid C is still valid C.

It can be compiled.  But it is not idiomatically C.  You can implement
any level of stupidity with the preprocessor and compile the result. 
That doesn't make it desirable.

Plus all the other things I said which you ignored.  Plus the
conversion to a macros weakens typechecking.

> > It makes no sense to define something which
> > looks like a function and to then assign values to it. __It means that
> > instruction_pointer() _must_ be implemented as a macro, violating basic
> > concepts of encapsualtion/layering/hiding/etc.
> >
> > Doing
> >
> > void instruction_pointer_set(struct pt_regs *regs, some_suitable_type val);
> >
> > will save many vomit bags.
> 
> and force everyone to implement the same copy & paste set of get/set
> modifiers ?  x86 is the only one where instruction_pointer() isnt a
> define.

Please, look at ia64:

# define instruction_pointer(regs) ((regs)->cr_iip + ia64_psr(regs)->ri)

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Mike Frysinger]

On Thu, Jun 4, 2009 at 22:27, Andrew Morton wrote:
> On Thu, 4 Jun 2009 21:50:49 -0400 Mike Frysinger wrote:
>> On Thu, Jun 4, 2009 at 21:04, Andrew Morton wrote:
>> > On Thu, 4 Jun 2009 20:55:40 -0400 Mike Frysinger wrote:
>> >> On Thu, Jun 4, 2009 at 20:50, Andrew Morton wrote:
>> >> > On Tue, __2 Jun 2009 03:17:30 -0400 Mike Frysinger wrote:
>> >> >> + __ __ instruction_pointer(&kgdbts_regs) += offset;
>> >> >
>> >> > instruction_pointer() cannot be used as an lvalue, thankfully.
>> >> >
>> >> > x86_64:
>> >> >
>> >> > drivers/misc/kgdbts.c: In function 'check_and_rewind_pc':
>> >> > drivers/misc/kgdbts.c:306: error: invalid lvalue in assignment
>> >>
>> >> should be easy to fix:
>> >> --- a/arch/x86/include/asm/ptrace.h
>> >> +++ b/arch/x86/include/asm/ptrace.h
>> >> @@ -236,10 +236,7 @@
>> >> __#endif
>> >> __}
>> >>
>> >> -static inline unsigned long instruction_pointer(struct pt_regs *regs)
>> >> -{
>> >> - __ return regs->ip;
>> >> -}
>> >> +#define instruction_pointer(regs) ((regs)->ip)
>> >>
>> >> __static inline unsigned long frame_pointer(struct pt_regs *regs)
>> >> __{
>> >
>> > argh, that's soooooo tasteless. __Look, this:
>> >
>> > __ __ __ __instruction_pointer(&kgdbts_regs) += offset;
>> >
>> > is just daft. __It's not C!
>
> Gets frustrating when I say correct things and your first reaction is
> to argue.

i'm not arguing for fun.  my solution results in less maintenance on
the people who actually have to write and maintain this code.  i never
said your points didnt make sense, just that they required more work
for questionable (imo) results.

>> it is C.  taste is one thing, but valid C is still valid C.
>
> It can be compiled.  But it is not idiomatically C.  You can implement
> any level of stupidity with the preprocessor and compile the result.
> That doesn't make it desirable.

taste vs validity -- different things

> Plus all the other things I said which you ignored.  Plus the
> conversion to a macros weakens typechecking.

i didnt ignore them.  i just didnt think the trade off of actual usage
was worth a single macro.

>> > It makes no sense to define something which
>> > looks like a function and to then assign values to it. __It means that
>> > instruction_pointer() _must_ be implemented as a macro, violating basic
>> > concepts of encapsualtion/layering/hiding/etc.
>> >
>> > Doing
>> >
>> > void instruction_pointer_set(struct pt_regs *regs, some_suitable_type val);
>> >
>> > will save many vomit bags.
>>
>> and force everyone to implement the same copy & paste set of get/set
>> modifiers ?  x86 is the only one where instruction_pointer() isnt a
>> define.
>
> Please, look at ia64:
>
> # define instruction_pointer(regs) ((regs)->cr_iip + ia64_psr(regs)->ri)

that i am willing to accept as a reason to go with inlines.  if all
arches were simply redirecting to a register, then i would disagree.
your version after all requires every arch to copy & paste this crap:
static inline unsigned long instruction_pointer(struct pt_regs *regs)
{
    return regs->ip;
}
static inline void instruction_pointer_set(struct pt_regs *regs,
unsigned long val)
{
    regs->ip = val;
}

and then actual usage turns into:
instruction_pointer_set(regs, instruction_pointer(regs) + foo);

whereas mine is two lines:
#define instruction_pointer(regs) ((regs)->ip)
instruction_pointer(regs) += val;
-mike

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Andrew Morton]

On Fri, 5 Jun 2009 00:00:22 -0400 Mike Frysinger <vapier.adi@gmail.com> wrote:

> your version after all requires every arch to copy & paste this crap:
> static inline unsigned long instruction_pointer(struct pt_regs *regs)
> {
>     return regs->ip;
> }
> static inline void instruction_pointer_set(struct pt_regs *regs,
> unsigned long val)
> {
>     regs->ip = val;
> }
> 
> and then actual usage turns into:
> instruction_pointer_set(regs, instruction_pointer(regs) + foo);
> 
> whereas mine is two lines:
> #define instruction_pointer(regs) ((regs)->ip)
> instruction_pointer(regs) += val;

The aim isn't really to reduce the amount of typing one needs to do. 
Let's get things right, and if getting it right involves more typing
then so be it.


If it really worries you then you could do

#define GET_IP(regs)	((regs)->ip)
#include <asm/generic/instruction_pointer.h>

and

static inline unsigned long instruction_pointer(struct pt_regs *regs)
{
	return GET_IP(regs);
}

static inline void instruction_pointer_set(struct pt_regs *regs,
					unsigned long val)
{
	GET_IP(regs) = val;
}

Note that GET_IP() is all-caps, which says "this is a macro".

But I don't think it's worth the ickyness, unless we also incorporate
kernel_stack_pointer(), frame_pointer(), user_stack_pointer() and
perhaps the _set() versions of those also.



Do we know how to implement instruction_pointer_set() on ia64, btw?

Re: [PATCH] kgdbts: unify/generalize gdb breakpoint adjustment

[Mike Frysinger]

On Fri, Jun 5, 2009 at 00:13, Andrew Morton wrote:
> On Fri, 5 Jun 2009 00:00:22 -0400 Mike Frysinger wrote:
>> your version after all requires every arch to copy & paste this crap:
>> static inline unsigned long instruction_pointer(struct pt_regs *regs)
>> {
>>     return regs->ip;
>> }
>> static inline void instruction_pointer_set(struct pt_regs *regs,
>> unsigned long val)
>> {
>>     regs->ip = val;
>> }
>>
>> and then actual usage turns into:
>> instruction_pointer_set(regs, instruction_pointer(regs) + foo);
>>
>> whereas mine is two lines:
>> #define instruction_pointer(regs) ((regs)->ip)
>> instruction_pointer(regs) += val;
>
> The aim isn't really to reduce the amount of typing one needs to do.
> Let's get things right, and if getting it right involves more typing
> then so be it.

the aim isnt to reduce typing (although that's a nice result), the aim
is to reduce the amount of work to (1) keep things in shape and (2)
force new ports to implement.  the less code there is, the harder it
is for it to bitrot.

> If it really worries you then you could do
>
> #define GET_IP(regs)    ((regs)->ip)
> #include <asm/generic/instruction_pointer.h>
>
> and
>
> static inline unsigned long instruction_pointer(struct pt_regs *regs)
> {
>        return GET_IP(regs);
> }
>
> static inline void instruction_pointer_set(struct pt_regs *regs,
>                                        unsigned long val)
> {
>        GET_IP(regs) = val;
> }
>
> Note that GET_IP() is all-caps, which says "this is a macro".
>
> But I don't think it's worth the ickyness, unless we also incorporate
> kernel_stack_pointer(), frame_pointer(), user_stack_pointer() and
> perhaps the _set() versions of those also.

yes, having a layer along those lines would satisfy my previous points.

> Do we know how to implement instruction_pointer_set() on ia64, btw?

i havent a clue, but it doesnt matter in this case as ia64 doesnt
support KGDB.  they can figure it out when they want to add KGDB
support :).
-mike