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

[Mike Frysinger <vapier.adi@gmail.com>]

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