[Bug uprobes/10458] New: uaddr() returns one past current instruction for uprobes

[Bug uprobes/10458] New: uaddr() returns one past current instruction for uprobes

[mjw at redhat dot com]

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)

-- 
           Summary: uaddr() returns one past current instruction for uprobes
           Product: systemtap
           Version: unspecified
            Status: NEW
          Severity: normal
          Priority: P2
         Component: uprobes
        AssignedTo: systemtap at sources dot redhat dot com
        ReportedBy: mjw at redhat dot com


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2009-07-29 14:52 -------
It would be nice if uprobes did the same adjustment to regs->ip
as e.g. arch/x86/kernel/kprobes.c does.


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[mhiramat at redhat dot com]

------- Additional Comments From mhiramat at redhat dot com  2009-07-29 16:00 -------
FYI, kprobes on x86 doesn't adjust regs->ip, so regs->ip is always equal to
kp->addr + 1.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[jkenisto at us dot ibm dot com]

------- Additional Comments From jkenisto at us dot ibm dot com  2009-07-29 16:17 -------
(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.




-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[jkenisto at us dot ibm dot com]

------- Additional Comments From jkenisto at us dot ibm dot com  2009-07-29 17:21 -------
Forgot to mention... the probe address is always available as uprobe->vaddr or
uretprobe->u.vaddr.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[mjw at redhat dot com]

------- Additional Comments From mjw at redhat dot com  2009-07-29 22:32 -------
(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?

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2009-07-29 22:38 -------
(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.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[fche at redhat dot com]

------- Additional Comments From fche at redhat dot com  2009-07-29 23:51 -------
> > 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?


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[jistone at redhat dot com]

------- Additional Comments From jistone at redhat dot com  2009-07-30 00:28 -------
(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?

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[mjw at redhat dot com]

------- Additional Comments From mjw at redhat dot com  2009-07-30 14:44 -------
(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) << "}";
 


-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[mjw at redhat dot com]

------- Additional Comments From mjw at redhat dot com  2009-07-31 18:10 -------
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.


-- 
           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[jkenisto at us dot ibm dot com]

------- Additional Comments From jkenisto at us dot ibm dot com  2009-07-31 18:28 -------
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.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

[Bug uprobes/10458] uaddr() returns one past current instruction for uprobes

[mjw at redhat dot com]

------- Additional Comments From mjw at redhat dot com  2009-07-31 18:56 -------
(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.

-- 


http://sourceware.org/bugzilla/show_bug.cgi?id=10458

------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.