Patchwork powerpc/uprobes: teach uprobes to ignore gdb breakpoints

login
register
mail settings
Submitter Oleg Nesterov
Date March 20, 2013, 12:43 p.m.
Message ID <20130320124301.GA30887@redhat.com>
Download mbox | patch
Permalink /patch/229375/
State Superseded, archived
Headers show

Comments

Oleg Nesterov - March 20, 2013, 12:43 p.m.
On 03/20, Oleg Nesterov wrote:
>
> But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to
> verify. If not, we need 2 definitions. is_uprobe_insn() should still check
> insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap().
>
> And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> differ?

IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
Suppose we apply the patch below. Will uprobes on powerpc work?

If yes, then your patch should be fine. If not, we probably need more
changes.

And, forgot to mention. If you change is_swbp_insn(), you can remove
is_trap() from arch_uprobe_analyze_insn().

Oleg.
Ananth N Mavinakayanahalli - March 20, 2013, 3:42 p.m.
On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> On 03/20, Oleg Nesterov wrote:
> >
> > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to
> > verify. If not, we need 2 definitions. is_uprobe_insn() should still check
> > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap().
> >
> > And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> > differ?
> 
> IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> Suppose we apply the patch below. Will uprobes on powerpc work?
> 
> If yes, then your patch should be fine. If not, we probably need more
> changes.

Yes, it will work fine.

> And, forgot to mention. If you change is_swbp_insn(), you can remove
> is_trap() from arch_uprobe_analyze_insn().

Yeah, that check is no longer needed. I'll send a separate cleanup after
this patch gets applied.

Ananth
Oleg Nesterov - March 20, 2013, 4:07 p.m.
On 03/20, Ananth N Mavinakayanahalli wrote:
>
> On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > On 03/20, Oleg Nesterov wrote:
> > >
> > > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to
> > > verify. If not, we need 2 definitions. is_uprobe_insn() should still check
> > > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap().
> > >
> > > And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> > > differ?
> >
> > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > Suppose we apply the patch below. Will uprobes on powerpc work?
> >
> > If yes, then your patch should be fine. If not, we probably need more
> > changes.
>
> Yes, it will work fine.

Even if this new insn is conditional?

Oleg.
Ananth N Mavinakayanahalli - March 21, 2013, 7:17 a.m.
On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote:
> On 03/20, Ananth N Mavinakayanahalli wrote:
> >
> > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > > On 03/20, Oleg Nesterov wrote:
> > > >
> > > > But we did not install UPROBE_SWBP_INSN. Is it fine? I hope yes, just to
> > > > verify. If not, we need 2 definitions. is_uprobe_insn() should still check
> > > > insns == UPROBE_SWBP_INSN, and is_swbp_insn() should check is_trap().
> > > >
> > > > And I am just curious, could you explain how X and UPROBE_SWBP_INSN
> > > > differ?
> > >
> > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > > Suppose we apply the patch below. Will uprobes on powerpc work?
> > >
> > > If yes, then your patch should be fine. If not, we probably need more
> > > changes.
> >
> > Yes, it will work fine.
> 
> Even if this new insn is conditional?

Yes.
Oleg Nesterov - March 21, 2013, 4 p.m.
On 03/21, Ananth N Mavinakayanahalli wrote:
?
> On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote:
> > On 03/20, Ananth N Mavinakayanahalli wrote:
> > >
> > > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > > > On 03/20, Oleg Nesterov wrote:
> > > > >
> > > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > > > Suppose we apply the patch below. Will uprobes on powerpc work?
> > > >
> > > > If yes, then your patch should be fine. If not, we probably need more
> > > > changes.
> > >
> > > Yes, it will work fine.
> >
> > Even if this new insn is conditional?
>
> Yes.

But this can't be true. If it is conditional, it won't always generate a
trap, this means uprobe won't actually work.

Oleg.
Ananth N Mavinakayanahalli - March 22, 2013, 4:37 a.m.
On Thu, Mar 21, 2013 at 05:00:02PM +0100, Oleg Nesterov wrote:
> On 03/21, Ananth N Mavinakayanahalli wrote:
> ?
> > On Wed, Mar 20, 2013 at 05:07:28PM +0100, Oleg Nesterov wrote:
> > > On 03/20, Ananth N Mavinakayanahalli wrote:
> > > >
> > > > On Wed, Mar 20, 2013 at 01:43:01PM +0100, Oleg Nesterov wrote:
> > > > > On 03/20, Oleg Nesterov wrote:
> > > > > >
> > > > > IOW, if I wasn't clear... Lets forget about gdb/etc for the moment.
> > > > > Suppose we apply the patch below. Will uprobes on powerpc work?
> > > > >
> > > > > If yes, then your patch should be fine. If not, we probably need more
> > > > > changes.
> > > >
> > > > Yes, it will work fine.
> > >
> > > Even if this new insn is conditional?
> >
> > Yes.
> 
> But this can't be true. If it is conditional, it won't always generate a
> trap, this means uprobe won't actually work.

I meant to say, uprobes if we use a conditional trap instruction as long
as the condition is met.

Ananth

Patch

--- x/arch/powerpc/include/asm/uprobes.h
+++ x/arch/powerpc/include/asm/uprobes.h
@@ -31,7 +31,7 @@  typedef ppc_opcode_t uprobe_opcode_t;
 #define UPROBE_XOL_SLOT_BYTES	(MAX_UINSN_BYTES)
 
 /* The following alias is needed for reference from arch-agnostic code */
-#define UPROBE_SWBP_INSN	BREAKPOINT_INSTRUCTION
+#define UPROBE_SWBP_INSN	TRAP_INSN_USED_BY_GDB
 #define UPROBE_SWBP_INSN_SIZE	4 /* swbp insn size in bytes */
 
 struct arch_uprobe {