Hi Roland,
a while ago you posted a set of patches implementing utrace, a very
promising debugging framework. Unfortunately everything around it got
really silent and you haven't been posting updates for a long time.
Thas is rather unfortunate as beeing silent and only posting updates
on your website is definitly not the way to get things merged.
Especially not something that basically rewrites a core kernel
functionality and requires updates for every single architecture.
Is there a chance you could post regular updates again and outline
a schedule of when you plan to merge things? I suspect we need to
stabilize things in -mm for quite a while and give all the architecture
maintainers a chance to update their port, as ripping out debugging
support for most architectures is most certainly not an option.
To get the ball rolling again I'm posting a first review below.
Note that this mostly focusses on codingstyle and general kernel
integration issues for now. I have some ideas on more fundamental
things, but I want to understand the code a little bit better before
commenting on them.
--- linux-2.6/include/asm-i386/thread_info.h.utrace-ptrace-compat
+++ linux-2.6/include/asm-i386/thread_info.h
@@ -135,13 +135,13 @@ static inline struct thread_info *curren
#define TIF_NEED_RESCHED 3 /* rescheduling necessary */
#define TIF_SINGLESTEP 4 /* restore singlestep on return to user mode */
#define TIF_IRET 5 /* return with iret */
-#define TIF_SYSCALL_EMU 6 /* syscall emulation active */
#define TIF_SYSCALL_AUDIT 7 /* syscall auditing active */
#define TIF_SECCOMP 8 /* secure computing */
#define TIF_RESTORE_SIGMASK 9 /* restore signal mask in do_signal() */
#define TIF_MEMDIE 16
#define TIF_DEBUG 17 /* uses debug registers */
#define TIF_IO_BITMAP 18 /* uses I/O bitmap */
+#define TIF_FORCED_TF 19 /* true if TF in eflags artificially */
I think it would make a lot of sense if you simply reused the
existing flag number instead of leaving a hole.
+#define utrace_lock(utrace) spin_lock(&(utrace)->lock)
+#define utrace_unlock(utrace) spin_unlock(&(utrace)->lock)
Please don't introduce such obsfucation macros.
+/***
+ *** These are the exported entry points for tracing engines to use.
+ ***/
This is not a standard comment format. It should be:
/*
* These are the exported entry points for tracing engines to use.
*/
Same comment style is in various other places that should
be fixed up aswell.
+
+/*
+ * Attach a new tracing engine to a thread, or look up attached engines.
+ * See UTRACE_ATTACH_* flags, above. The caller must ensure that the
+ * target thread does not get freed, i.e. hold a ref or be its parent.
+ */
+struct utrace_attached_engine *utrace_attach(struct task_struct *target,
+ int flags,
+ const struct utrace_engine_ops *,
+ unsigned long data);
Please don't put such long comments in headerfiles. They should be
part of the kerneldoc comments near the actual function body so
we can extract them and autogenerate real documentation for it.
A big thanks for the huge amount of comments, btw - they're just
in the wrong place ;-)
--- linux-2.6/include/linux/ptrace.h.utrace-ptrace-compat
+++ linux-2.6/include/linux/ptrace.h
+#ifdef CONFIG_PTRACE
+#include <asm/tracehook.h>
+struct utrace_attached_engine;
+struct utrace_regset_view;
Please make the include (and the forward-declarations while you're at
it) unconditional. That way we avoid the possiblity of come code
compiling when CONFIG_PTRACE is set but failing if it's not set.
+#ifdef CONFIG_COMPAT
+#include <linux/compat.h>
+
+extern fastcall int arch_compat_ptrace(compat_long_t *request,
+ struct task_struct *child,
+ struct utrace_attached_engine *engine,
+ compat_ulong_t a, compat_ulong_t d,
+ compat_long_t *retval);
+#endif
...
+#ifdef CONFIG_COMPAT
Please try consolidate all the compat code in a single #ifdef block,
preferably at the end of the file.
--- linux-2.6/include/linux/tracehook.h.utrace-ptrace-compat
+++ linux-2.6/include/linux/tracehook.h
@@ -0,0 +1,707 @@
+ * void tracehook_enable_single_step(struct task_struct *tsk);
+ * void tracehook_disable_single_step(struct task_struct *tsk);
+ * int tracehook_single_step_enabled(struct task_struct *tsk);
+ *
+ * If those calls are defined, #define ARCH_HAS_SINGLE_STEP to nonzero.
+ * Do not #define it if these calls are never available in this kernel config.
+ * If defined, the value of ARCH_HAS_SINGLE_STEP can be constant or variable.
+ * It should evaluate to nonzero if the hardware is able to support
+ * tracehook_enable_single_step. If it's a variable expression, it
+ * should be one that can be evaluated in modules, i.e. uses exported symbols.
Please don't do this kind of conditional mess. It leads to really
ugly code like this (later in the patch):
#ifdef ARCH_HAS_SINGLE_STEP
if (! ARCH_HAS_SINGLE_STEP)
#endif
WARN_ON(flags & UTRACE_ACTION_SINGLESTEP);
#ifdef ARCH_HAS_BLOCK_STEP
if (! ARCH_HAS_BLOCK_STEP)
#endif
WARN_ON(flags & UTRACE_ACTION_BLOCKSTEP);
Instead make it a
arch_has_single_step()
which must be defined by every architecture as a boolean value, with
fixes like that the code above will become a lot more readable:
WARN_ON(!arch_has_single_step() && (flags & UTRACE_ACTION_SINGLESTEP));
WARN_ON(!arch_has_block_step() && (flags & UTRACE_ACTION_BLOCKSTEP));
+static inline int
+utrace_regset_copyout(unsigned int *pos, unsigned int *count,
+ void **kbuf, void __user **ubuf,
+ const void *data, int start_pos, int end_pos)
+{
+ if (*count == 0)
+ return 0;
+ BUG_ON(*pos < start_pos);
+ if (end_pos < 0 || *pos < end_pos) {
+ unsigned int copy = (end_pos < 0 ? *count
+ : min(*count, end_pos - *pos));
+ data += *pos - start_pos;
+ if (*kbuf) {
+ memcpy(*kbuf, data, copy);
+ *kbuf += copy;
+ }
+ else if (copy_to_user(*ubuf, data, copy))
+ return -EFAULT;
+ else
+ *ubuf += copy;
The coding style here is wrong. The else should be on the line
of the closing brace. You're making that codingstyle mistake
in a lot of places in this patch, please try to fix all of them
up.
+ *pos += copy;
+ *count -= copy;
+ }
+ return 0;
+}
This function is far too large to inline it. Please move it out
of line.
+static inline int
+utrace_regset_copyin(unsigned int *pos, unsigned int *count,
+ const void **kbuf, const void __user **ubuf,
+ void *data, int start_pos, int end_pos)
+{
Should go out of line aswell.
+static inline int
+utrace_regset_copyout_zero(unsigned int *pos, unsigned int *count,
+ void **kbuf, void __user **ubuf,
+ int start_pos, int end_pos)
Ditto.
+static inline int
+utrace_regset_copyin_ignore(unsigned int *pos, unsigned int *count,
+ const void **kbuf, const void __user **ubuf,
+ int start_pos, int end_pos)
+/*
+ * Called in copy_process when setting up the copied task_struct,
+ * with tasklist_lock held for writing.
+ */
+static inline void tracehook_init_task(struct task_struct *child)
+{
+#ifdef CONFIG_UTRACE
+ child->utrace_flags = 0;
+ child->utrace = NULL;
+#endif
+}
This shows very nicely why the tracehooks vs utrace abstraction
is utter madness. Every tracehook 'abstraction' just conatains
an ifdef block. Just kill CONFIG_UTRACE as there is no point
in making this functionality conditional an opencode the
utrace functionality at the callers (or add a utrace_ helper
for the few cases where it makes sense)
+static inline void tracehook_report_handle_signal(int sig,
+ const struct k_sigaction *ka,
+ const sigset_t *oldset,
+ struct pt_regs *regs)
+{
+#ifdef CONFIG_UTRACE
+ struct task_struct *tsk = current;
+ if ((tsk->utrace_flags & UTRACE_EVENT_SIGNAL_ALL)
+ && (tsk->utrace_flags & (UTRACE_ACTION_SINGLESTEP
+ | UTRACE_ACTION_BLOCKSTEP)))
+ utrace_signal_handler_singlestep(tsk, regs);
This doesn't follow kernel coding style at all, we always
put the && or || operators at the end of the closing line.
if ((tsk->utrace_flags & UTRACE_EVENT_SIGNAL_ALL) &&
(tsk->utrace_flags & (UTRACE_ACTION_SINGLESTEP |
UTRACE_ACTION_BLOCKSTEP)))
Also I'm not sure why you're doing this kind of test,
as you could do a
if (current->utrace_flags & (UTRACE_EVENT_SIGNAL_ALL|
UTRACE_ACTION_SINGLESTEP|UTRACE_ACTION_BLOCKSTEP))
aswell
@@ -1028,6 +1027,9 @@ static struct task_struct *copy_process(
INIT_LIST_HEAD(&p->sibling);
p->vfork_done = NULL;
spin_lock_init(&p->alloc_lock);
+#ifdef CONFIG_PTRACE
+ INIT_LIST_HEAD(&p->ptracees);
+#endif
This should be hidden behing a ptrace_init_task macro
that does nothing in the !CONFIG_PTRACE case
--- linux-2.6/kernel/utrace.c.utrace-ptrace-compat
+++ linux-2.6/kernel/utrace.c
@@ -0,0 +1,1735 @@
+#include <linux/utrace.h>
+#include <linux/tracehook.h>
+#include <linux/err.h>
+#include <linux/sched.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <asm/tracehook.h>
Please add a block comment at the top of the file noting the
copyright/license status author and basic desription of the
contents.
+ if (likely(target->utrace == NULL)) {
+ rcu_assign_pointer(target->utrace, utrace);
+ /*
+ * The task_lock protects us against another thread doing
+ * the same thing. We might still be racing against
+ * tracehook_release_task. It's called with ->exit_state
+ * set to EXIT_DEAD and then checks ->utrace with an
+ * smp_mb() in between. If EXIT_DEAD is set, then
+ * release_task might have checked ->utrace already and saw
+ * it NULL; we can't attach. If we see EXIT_DEAD not yet
+ * set after our barrier, then we know release_task will
+ * see our target->utrace pointer.
+ */
+ smp_mb();
+ if (target->exit_state == EXIT_DEAD) {
+ /*
+ * The target has already been through release_task.
+ */
+ target->utrace = NULL;
+ goto cannot_attach;
+ }
+ task_unlock(target);
+
+ /*
+ * If the thread is already dead when we attach, then its
+ * parent was notified already and we shouldn't repeat the
+ * notification later after a detach or NOREAP flag change.
+ */
+ if (target->exit_state)
+ utrace->u.exit.notified = 1;
+ }
+ else {
+ /*
+ * Another engine attached first, so there is a struct already.
+ * A null return says to restart looking for the existing one.
+ */
+ cannot_attach:
+ ret = NULL;
+ task_unlock(target);
+ utrace_unlock(utrace);
+ kmem_cache_free(utrace_cachep, utrace);
+ }
+
+ return ret;
+}
This is written more than messy. when you use gotos anyway (as
recommended for kernel code), use it throughout to make the
normal path be straight and the error path using gotos, e.g.:
if (unlikely(target->utrace)) {
/*
* Another engine attached first, so there is a struct already.
* A null return says to restart looking for the existing one.
*/
goto cannot_attach;
}
...
return ret;
cannot_attach:
task_unlock(target);
utrace_unlock(utrace);
kmem_cache_free(utrace_cachep, utrace);
return NULL;
}
+struct utrace_attached_engine *
+utrace_attach(struct task_struct *target, int flags,
+ const struct utrace_engine_ops *ops, unsigned long data)
Again, this is pretty bad goto spagetthi without much value,
below is a slightly rewritten variant that should work the same:
struct utrace_attached_engine *
utrace_attach(struct task_struct *target, int flags,
const struct utrace_engine_ops *ops, unsigned long data)
{
struct utrace_attached_engine *engine = NULL;
struct utrace *utrace;
restart:
rcu_read_lock();
utrace = rcu_dereference(target->utrace);
smp_rmb();
if (!utrace) {
rcu_read_unlock();
if (!(flags & UTRACE_ATTACH_CREATE))
return ERR_PTR(-ENOENT);
engine = kmem_cache_alloc(utrace_engine_cachep, SLAB_KERNEL);
if (unlikely(engine == NULL))
return ERR_PTR(-ENOMEM);
engine->flags = 0;
goto first;
}
if (unlikely(target->exit_state == EXIT_DEAD)) {
/*
* The target has already been reaped.
*/
rcu_read_unlock();
return ERR_PTR(-ESRCH);
}
if (!(flags & UTRACE_ATTACH_CREATE)) {
engine = matching_engine(utrace, flags, ops, data);
rcu_read_unlock();
return engine;
}
rcu_read_unlock();
engine = kmem_cache_alloc(utrace_engine_cachep, SLAB_KERNEL);
if (unlikely(engine == NULL))
return ERR_PTR(-ENOMEM);
engine->flags = ops->report_reap ? UTRACE_EVENT(REAP) : 0;
rcu_read_lock();
utrace = rcu_dereference(target->utrace);
if (unlikely(!utrace)) { /* Race with detach. */
rcu_read_unlock();
goto first;
}
utrace_lock(utrace);
if (flags & UTRACE_ATTACH_EXCLUSIVE) {
struct utrace_attached_engine *old;
old = matching_engine(utrace, flags, ops, data);
if (!IS_ERR(old)) {
utrace_unlock(utrace);
rcu_read_unlock();
kmem_cache_free(utrace_engine_cachep, engine);
return ERR_PTR(-EEXIST);
}
}
if (unlikely(rcu_dereference(target->utrace) != utrace)) {
/*
* We lost a race with other CPUs doing a sequence
* of detach and attach before we got in.
*/
utrace_unlock(utrace);
rcu_read_unlock();
kmem_cache_free(utrace_engine_cachep, engine);
goto restart;
}
rcu_read_unlock();
list_add_tail_rcu(&engine->entry, &utrace->engines);
goto out;
first:
utrace = utrace_first_engine(target, engine);
if (IS_ERR(utrace)) {
kmem_cache_free(utrace_engine_cachep, engine);
return ERR_PTR(PTR_ERR(utrace));
}
if (unlikely(!utrace)) /* Race condition. */
goto restart;
out:
engine->ops = ops;
engine->data = data;
utrace_unlock(utrace);
return engine;
}
EXPORT_SYMBOL_GPL(utrace_attach);
There is not modular user of this, so this and the other utrace_
functions should not be exported. Nor do I think that exporting
such a low-level process control is nessecary a good idea, but
we'll have to evaluate that if patches to add users show up.
+#define REPORT(callback, ...) do { \
+ u32 ret = (*rcu_dereference(engine->ops)->callback) \
+ (engine, tsk, ##__VA_ARGS__); \
+ action = update_action(tsk, utrace, engine, ret); \
+ } while (0)
I don not think these kinds of macros are a very good idea.
Just opencode the two lines of code instead of a single
macro.
+// XXX copied from signal.c
+#ifdef SIGEMT
+#define M_SIGEMT M(SIGEMT)
+#else
+#define M_SIGEMT 0
+#endif
+
+#if SIGRTMIN > BITS_PER_LONG
+#define M(sig) (1ULL << ((sig)-1))
+#else
+#define M(sig) (1UL << ((sig)-1))
+#endif
+#define T(sig, mask) (M(sig) & (mask))
+
+#define SIG_KERNEL_ONLY_MASK (\
+ M(SIGKILL) | M(SIGSTOP) )
+
+#define SIG_KERNEL_STOP_MASK (\
+ M(SIGSTOP) | M(SIGTSTP) | M(SIGTTIN) | M(SIGTTOU) )
+
+#define SIG_KERNEL_COREDUMP_MASK (\
+ M(SIGQUIT) | M(SIGILL) | M(SIGTRAP) | M(SIGABRT) | \
+ M(SIGFPE) | M(SIGSEGV) | M(SIGBUS) | M(SIGSYS) | \
+ M(SIGXCPU) | M(SIGXFSZ) | M_SIGEMT )
+
+#define SIG_KERNEL_IGNORE_MASK (\
+ M(SIGCONT) | M(SIGCHLD) | M(SIGWINCH) | M(SIGURG) )
+
+#define sig_kernel_only(sig) \
+ (((sig) < SIGRTMIN) && T(sig, SIG_KERNEL_ONLY_MASK))
+#define sig_kernel_coredump(sig) \
+ (((sig) < SIGRTMIN) && T(sig, SIG_KERNEL_COREDUMP_MASK))
+#define sig_kernel_ignore(sig) \
+ (((sig) < SIGRTMIN) && T(sig, SIG_KERNEL_IGNORE_MASK))
+#define sig_kernel_stop(sig) \
+ (((sig) < SIGRTMIN) && T(sig, SIG_KERNEL_STOP_MASK))
Copying and pasting this kind of stuff is a very bad idea.
Just put a helper like the following into signal.c and use it from
utrace.c:
void sigaction_to_utrace(struct k_sigaction *ka, unsigned long *action,
unsigned long *event)
{
if (ka->sa.sa_handler == SIG_IGN) {
*event = UTRACE_EVENT(SIGNAL_IGN);
*action = UTRACE_SIGNAL_IGN;
} else if (ka->sa.sa_handler != SIG_DFL) {
*event = UTRACE_EVENT(SIGNAL);
*action = UTRACE_ACTION_RESUME;
} else if (sig_kernel_coredump(signal.signr)) {
*event = UTRACE_EVENT(SIGNAL_CORE);
*action = UTRACE_SIGNAL_CORE;
} else if (sig_kernel_ignore(signal.signr)) {
*event = UTRACE_EVENT(SIGNAL_IGN);
*action = UTRACE_SIGNAL_IGN;
} else if (sig_kernel_stop(signal.signr)) {
*event = UTRACE_EVENT(SIGNAL_STOP);
*action = (signal.signr == SIGSTOP ?
UTRACE_SIGNAL_STOP : UTRACE_SIGNAL_TSTP);
} else {
*event = UTRACE_EVENT(SIGNAL_TERM);
*action = UTRACE_SIGNAL_TERM;
}
}
+#ifdef CONFIG_PTRACE
+#include <linux/utrace.h>
+#include <linux/tracehook.h>
+#include <asm/tracehook.h>
+#endif
A really huge NACK for putting #ifdef CONFIG_PTRACE into ptrace.c.
The file should only be compile if CONFIG_PTRACE is set.
sys_ptrace should become a cond_syscall, and ptrace_may_attach
should move into a differnt file and get a more suitable name.
+int getrusage(struct task_struct *, int, struct rusage __user *);
Please never put prototypes like this into .c files.
+#ifdef PTRACE_DEBUG
+ printk("ptrace pid %ld => %p\n", pid, child);
+#endif
Please don't do this kind of ifdef mess. just use pr_debug
instead.
+const struct utrace_regset_view utrace_ppc32_view = {
+ .name = "ppc", .e_machine = EM_PPC,
+ .regsets = ppc32_regsets,
+ .n = sizeof ppc32_regsets / sizeof ppc32_regsets[0],
+};
+EXPORT_SYMBOL_GPL(utrace_ppc32_view);
+#endif
Who would be using this from modular code?
I see this is for all views, but I'd rather move the
accessor out of line than exporting all them if we
really have legitimate modular users.
Thanks very much for your interest in utrace and for your comments.
Unfortunately, I cannot say exactly when I will be able to respond
to them in detail. I broke my arm in September and have had a
difficult recovery, including a second surgery in November, two
weeks ago. I am now immobilized such that I cannot type properly,
and will be unable to type much until some time in January. Issues
in getting utrace merged are indeed my top priority after fixing
known bugs in the current utrace code. My injury came at a quite
inopportune time. I do not want to start the discussion in earnest
before I am again able to participate, and to hack on the code,
with the vigor and verbosity I usually expect of myself.
Thanks for your patience,
Roland
On Tue, Dec 05, 2006 at 01:51:45AM -0800, Roland McGrath wrote:
> Thanks very much for your interest in utrace and for your comments.
> Unfortunately, I cannot say exactly when I will be able to respond
> to them in detail. I broke my arm in September and have had a
> difficult recovery, including a second surgery in November, two
> weeks ago. I am now immobilized such that I cannot type properly,
> and will be unable to type much until some time in January. Issues
> in getting utrace merged are indeed my top priority after fixing
> known bugs in the current utrace code. My injury came at a quite
> inopportune time. I do not want to start the discussion in earnest
> before I am again able to participate, and to hack on the code,
> with the vigor and verbosity I usually expect of myself.
Sure, not problem to wait. I wish you'll get better soon.
> No need to renumber. You remove TIF_SYSCALL_EMU which is six,
> so the newly added TIF_FORCED_TF could reuse that bit.
No, that would be incorrect. As I mentioned earlier, there are magic
semantics to bits < 16, namely that they are in _TIF_ALLWORK_MASK (and
assembly code knows implicitly that these are in the low 16 bits).
TIF_FORCED_TF must not be in _TIF_ALLWORK_MASK, hence must be >= 16.
> > Ok. The kerneldoc stuff is new and strange to me, and I've never
> > personally experienced its benefical features. But I've read
> > Documentation/kernel-doc-nano-HOWTO.txt now and I will try to convert all
> > my documentary comments to that style.
>
> Thanks. If you need some help ping me or Randy.
I've converted the comments to kerneldoc style where I could figure out how
to. I left some unmarked comments in places I couldn't see how to get them
extracted from. I'd appreciate any specific advice you have for the
descriptive comments as they now stand.
> Current gcc (since 3.<something>) can optimize away code under if-branches
> that are determinable at compile time, so having each architecture
> define a (possible trivial) arch_has_single_step() should be fine.
What I had in mind was whole chunks of code you might not build, including
global symbols or whole modules. In particular, I expect to have some
utility code for executing a single copied instruction (in the style of
kprobes), which can also be implemented for machines without single-step
(see kprobes ARM port). That could be employed to simulate single-stepping
by a tracing engine (though not without complexities and caveats, which is
why it won't be built in as the UTRACE_ACTION_SINGLESTEP support). An
engine could support this using only machine-independent interfaces to the
aforementioned utility code, and rely on arch_has_single_step() to know
whether it needs to. That code might be costly to build in and would not
all be dropped at compile time even if always dead at runtime, or perhaps
isn't there at all for a particular machine. But if on that machine you
know at compile time that arch_has_single_step() will never return 0, you
don't need it. I don't want to get too bogged down in those specifics,
because they aren't the point right now. The only point I'm making here is
that #if really is going to be useful.
> Btw, is there a fundamental reason why an architecture would not support
> single-stepping except for a transition period of porting, i.e. are there
> real hardware limitation in any of our ports?
As I understand it, Alpha and ARM hardware in fact have no single-step feature.
There may be others.
> > utrace_native_view is expected to be the only user of the *_view symbols.
> > I inlined it because on single-arch platforms it's just a constant return
> > and I didn't want to add another useless call frame. I admit this is undue
> > microoptimization. Perhaps utrace_native_view should just be non-inlined
> > and exported.
>
> Yeah, the latter probably makes sense.
I've changed it that way now.
Thanks,
Roland
Hi Russell. Your last comments in this thread gave the impression you
thought that ARM's existing PTRACE_SINGLESTEP support would be lost by
converting to the utrace-based ptrace implementation. Christoph Hellwig
posted a reply giving the (correct) details of how this is not the case.
But I gather there is still some misunderstanding circulating on this point.
Here is the section from utrace-arch-porting-howto.txt about this:
7. Software single-step.
A few machines do not have any hardware single-step support, but provide
PTRACE_SINGLESTEP by doing memory breakpoint insertion. If your machine
does this, do not define tracehook_enable_single_step et al. The
tracehook single-step/block-step functions are intended for true
hardware support, or forms of software support that truly work as well
as hardware support does. Simply changing memory has a lot of problems,
notably its incompatibility with multi-threaded debugging.
For ptrace compatibility, just handle PTRACE_SINGLESTEP in your
arch_ptrace function using your existing code. If arch_ptrace needs
to do something that should be undone when ptrace cleans up,
asm/ptrace.h can #define HAVE_ARCH_PTRACE_DETACH and it will
call void arch_ptrace_detach(struct task_struct *) before detaching.
In future, the utrace world will have facilities to do things like
per-thread breakpoints while mitigating the bad side effects of
breakpoint insertion. Then single-stepping will be emulated using
those. Until we have that, your old PTRACE_SINGLESTEP support code is
fine for ptrace, but new utrace-based users will expect not to see side
effects like memory-writing breakpoint insertion and are better off not
falsely thinking there is proper single-step support.
I think that is pretty unambiguous, but please advise me how to make it
more clear. In short, any existing arch with PTRACE_SINGLESTEP support
will keep it. What differs between an arch with hardware support and an
arch doing a breakpoint-insertion hack is just where the old code is moved
to in the newly reorganized code. Hardware support (or anything
indistinguishable from it) goes in tracehook_enable_single_step et al.
Old-style memory-modifying breakpoint-insertion goes into arch_ptrace.
> I have no real problem with a decision being made to drop kernel-based
> single stepping _provided_ we have some replacement strategy in place
> and readily available. At the moment I've not seen such a strategy.
I never intended to suggest in any way that any regression in the behavior
of the ptrace call on any machine would be acceptable. Maintaining the
status quo for ptrace is straightforward.
The challenge for machines without hardware single-step support is for the
future, more-interesting things built on utrace, unrelated to ptrace, to
support instruction-step features in nice ways. I have not gotten into
details of the strategies for that because it is still vaporware barely
tried yet even in contorted prototype forms, and is not directly apropos to
the more immediate goal of integrating the utrace core into the kernel
without regressions in ptrace behavior.
> I'm not sure if Roland's expecting architecture maintainers to
> create such a strategy themselves - which would probably turn out to
> being far worse since you could end up with different implementations
> for each architecture.
Indeed I do not expect any arch to start from scratch, just to do the
narrowly arch-specific parts. There isn't even a prototype of something to
try doing new ports of, so I have not mentioned details of what arch
support entails. Making kprobes work robustly for your arch is useful
related work that you can do now (and someone has done some for ARM), not
that it addresses the userland single-step issue per se, but the arch
implementation details for kprobes inform arch details of some strategies
available. We can talk about it if you like, but that is future work for
new features beyond the status quo. I wouldn't like to conflate it with
discussions about the utrace work that exists now, or let that slow down
review or arch ports for the basic infrastructure.
I hope you will consider taking another crack at the utrace port for your
arch. It really is not so much work. If your arch is supported well by
qemu, and you can point me to either a qemu disk image or an easily
installed distro version that works well for kernel hacking on your arch, I
would be happy to give it a whirl to help write and debug the port.
Thanks,
Roland