While writing a testcase for bug #10454 I noticed uaddr() returns one off results for uprobes. That is, it returns the pc as if it is just after the breakpoint (int3 trap) instruction. a) do we consider this a bug? b) where should this be fixed? alternatives are: - uprobes setting task_pt_regs REG_IP back just before the callback - uaddr just always substracting 1 (probably arch dependent)
It would be nice if uprobes did the same adjustment to regs->ip as e.g. arch/x86/kernel/kprobes.c does.
FYI, kprobes on x86 doesn't adjust regs->ip, so regs->ip is always equal to kp->addr + 1.
(In reply to comment #2) > FYI, kprobes on x86 doesn't adjust regs->ip, so regs->ip is always equal to > kp->addr + 1. Correct. Based on the ARCH_BP_INST_PTR definitions in runtime/uprobes/*.h, it appears that on x86 and s390, the breakpoint instruction advances the IP, but on ppc64 it doesn't.
Forgot to mention... the probe address is always available as uprobe->vaddr or uretprobe->u.vaddr.
(In reply to comment #4) > Forgot to mention... the probe address is always available as uprobe->vaddr or > uretprobe->u.vaddr. OK, that is helpful. I am trying to figure out how to tell (in the uaddr() embedded c tapset function) whether or not the current CONTEXT comes from a uprobe so we can use this to provide the correct PC at that point. Anybody know how to easily see what probe type we are in from the struct context?
(In reply to comment #5) > Anybody know how to easily see what probe type we are in from the struct context? You could scan CONTEXT->probe_point, as probefunc() does. Barring that, it may be useful to add a probe_type enum to CONTEXT that categorizes the probe flavors.
> > Anybody know how to easily see what probe type we are in from the struct context? > > You could scan CONTEXT->probe_point, as probefunc() does. Yikes, no, that mess should be fixed separately (perhaps treated like extra context variables). > Barring that, it may be useful to add a probe_type enum to CONTEXT that > categorizes the probe flavors. Perhaps, but the runtime How about instead using the emit_probe_local_init() to generate adjustment code?
(In reply to comment #7) > > Barring that, it may be useful to add a probe_type enum to CONTEXT that > > categorizes the probe flavors. > > Perhaps, but the runtime Indeed, think of the runtime! > How about instead using the emit_probe_local_init() to generate > adjustment code? Actually, I'm a little nervous that emit_probe_local_init() may not be playing nicely with the probe-body-duplication checks. I'm contemplating getting rid of that piece, as it only has one minor use right now. If this is always an issue for uprobes, why not just adjust it directly in enter_uprobe_probe?
(In reply to comment #8) > (In reply to comment #7) > > Perhaps, but the runtime > > Indeed, think of the runtime! > > If this is always an issue for uprobes, why not just adjust it directly in > enter_uprobe_probe? That seems the easiest solution. I am testing the following patch which seems to make things (for the testcase from #10454, except of course the last one without debuginfo) just work as expected: diff --git a/tapsets.cxx b/tapsets.cxx index 2d68ddd..8dc76d7 100644 --- a/tapsets.cxx +++ b/tapsets.cxx @@ -4381,7 +4381,18 @@ uprobe_derived_probe_group::emit_module_decls (systemtap_session& s) s.op->newline() << "if (sup->spec_index < 0 ||" << "sup->spec_index >= " << probes.size() << ") return;"; // XXX: should not happen s.op->newline() << "c->regs = regs;"; + + // Make it look like the IP is set as it would in the actual user + // task when calling real probe handler. Reset IP regs on return, so + // we don't confuse uprobes. PR10458 + s.op->newline() << "{"; + s.op->indent(1); + s.op->newline() << "unsigned long uprobes_ip = REG_IP(c->regs);"; + s.op->newline() << "REG_IP(regs) = inst->vaddr;"; s.op->newline() << "(*sups->ph) (c);"; + s.op->newline() << "REG_IP(regs) = uprobes_ip;"; + s.op->newline(-1) << "}"; + common_probe_entryfn_epilogue (s.op); s.op->newline(-1) << "}"; @@ -4393,7 +4404,18 @@ uprobe_derived_probe_group::emit_module_decls (systemtap_session& s) << "sup->spec_index >= " << probes.size() << ") return;"; // XXX: should not happen // XXX: kretprobes saves "c->pi = inst;" too s.op->newline() << "c->regs = regs;"; + + // Make it look like the IP is set as it would in the actual user + // task when calling real probe handler. Reset IP regs on return, so + // we don't confuse uprobes. PR10458 + s.op->newline() << "{"; + s.op->indent(1); + s.op->newline() << "unsigned long uprobes_ip = REG_IP(c->regs);"; + s.op->newline() << "REG_IP(regs) = inst->rp->u.vaddr;"; s.op->newline() << "(*sups->ph) (c);"; + s.op->newline() << "REG_IP(regs) = uprobes_ip;"; + s.op->newline(-1) << "}"; + common_probe_entryfn_epilogue (s.op); s.op->newline(-1) << "}";
That worked very well. The advantage of this is that the pt_regs IP_REG now always points to the actual instruction we are interested in. I added the same for the kprobe variants. Now these act similarly to other probes that provice pt_regs. This also means we can get rid of a anomaly in the unwinder where we would always adjust the instruction by one even for cases where that wasn't necessary (and where effectively we could unwind from the wrong spot just before a function entry). commit 6415dddecb81f59996e422e87e1d3da266d743e8 Author: Mark Wielaard <mjw@redhat.com> Date: Fri Jul 31 18:46:47 2009 +0200 PR10458. User actual breakpoint address for [ku]probe[ret]. Setup the pt_regs REG_IP to the actual breakpoint address before entering a probe handler for [ku]probe[ret] (and restore it after returning). This helps getting symbol resolution and backtraces more correct and makes it more conform with other probe handlers like the iutrace and profile timers that also provide pt_regs (which untill now exhibited off-by-one errors while unwinding). * tapsets.cxx (dwarf_derived_probe_group::emit_module_decls): Setup REG_IP correctly before calling enter_kprobe_probe and enter_kretprobe_probe, and restore afterwards. (uprobe_derived_probe_group::emit_module_decls): Likewise for enter_uprobe_probe and enter_uretprobe_probe. (kprobe_derived_probe_group::emit_module_decls): Likewise for enter_kprobe2_probe and enter_kretprobe2_probe. * runtime/unwind/i386.h (arch_unw_init_frame_info): Initialize info->call_frame to zero. * runtime/unwind/x86_64.h (arch_unw_init_frame_info): Likewise.
Looks good, but keep in mind that the (corrected) IP when you're in a kretprobe or uretprobe handler is just the address of the return-probe trampoline.
(In reply to comment #11) > Looks good, but keep in mind that the (corrected) IP when you're in a kretprobe > or uretprobe handler is just the address of the return-probe trampoline. This was mostly to make sure that we are always at the actual probe address and not accidentally one past (possibly in the middle of the actual instruction, and in the worse case just one past the end of the function). The next layer/level deals with the actual symbol/unwind resolving. But we might indeed always have to special case the [ku]retprobe case by checking CONTEXT->pi being set anyway. If so, then we might indeed never need the actual address in the first place. Thanks for the reminder. I'll think about it.