LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* utrace comments
@ 2006-11-27 16:51 Christoph Hellwig
  2006-12-05  9:51 ` Roland McGrath
  2007-04-30  4:02 ` Roland McGrath
  0 siblings, 2 replies; 36+ messages in thread
From: Christoph Hellwig @ 2006-11-27 16:51 UTC (permalink / raw)
  To: Roland McGrath; +Cc: linux-kernel, linux-arch

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.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2006-11-27 16:51 utrace comments Christoph Hellwig
@ 2006-12-05  9:51 ` Roland McGrath
  2006-12-06 21:58   ` Christoph Hellwig
  2007-04-30  4:02 ` Roland McGrath
  1 sibling, 1 reply; 36+ messages in thread
From: Roland McGrath @ 2006-12-05  9:51 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-kernel, linux-arch

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2006-12-05  9:51 ` Roland McGrath
@ 2006-12-06 21:58   ` Christoph Hellwig
  0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2006-12-06 21:58 UTC (permalink / raw)
  To: Roland McGrath; +Cc: Christoph Hellwig, linux-kernel, linux-arch

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.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2006-11-27 16:51 utrace comments Christoph Hellwig
  2006-12-05  9:51 ` Roland McGrath
@ 2007-04-30  4:02 ` Roland McGrath
  2007-04-30  9:08   ` Christoph Hellwig
  2007-04-30  9:11   ` condingstyle, was " Christoph Hellwig
  1 sibling, 2 replies; 36+ messages in thread
From: Roland McGrath @ 2007-04-30  4:02 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-kernel, linux-arch

I'm sorry I've taken so long to reply to your review comments.
I won't dwell on that, and just dive into the discussion.

> --- 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.

You mean renumber 7,8,9 down to 6,7,8?  I guess there's no reason not to
do that, I was just not changing values I didn't need to.  AFAIK the
only thing that matters about these value is the bits < 16 that there
and the bits >= 16 likewise, for _TIF_ALLWORK_MASK to work right.

> +#define utrace_lock(utrace)	spin_lock(&(utrace)->lock)
> +#define utrace_unlock(utrace)	spin_unlock(&(utrace)->lock)
> 
> 	Please don't introduce such obsfucation macros.

I removed them.

> +/***
> + *** These are the exported entry points for tracing engines to use.
> + ***/
> 
> 	This is not a standard comment format.  It should be:

Ok.  The intention was to clearly set off those declarations from others.
It appears the thing other linux/*.h files do for this is:

/**************************************************************************
 * These are the exported entry points for tracing engines to use.
 **************************************************************************/

> 	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 ;-)

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.

>  	Please try consolidate all the compat code in a single #ifdef block,
> 	preferably at the end of the file.

I did it this way to put each compat_foo near foo so it makes sense in
context, and it's easy to remember to update it in parallel if you change
foo.  Is it really better to move them all to an undifferentiated cluster
at the end?

> 	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));

I agree it's come out ugly.  The reason I complicated it was an attempt
to make the answer testable in #if when it's constant at compile time.
My thinking is there will be situations where you want to avoid building
in some useless dead code and just "if (0)" is not sufficient.  But
primarily I was thinking of avoiding building extra fancy code in lieu
of single-step on machines/configurations where single-step is always
available, and the current #if's don't help with that.  How about if an
asm-arch/tracehook.h either declares arch_has_single_step or else it
#define's one of two things and in a single place I'll add:

	#ifdef ARCH_ALWAYS_HAS_SINGLE_STEP
	#define arch_has_single_step()	1
	#elif defined ARCH_NEVER_HAS_SINGLE_STEP
	#define arch_has_single_step()	0
	#endif

> 	The coding style here is wrong.  The else should be on the line
> 	of the closing brace.  

I can ordinarily ignore syntax, but this is an abomination in the sight
of the Lord and always will be.  Fortunately, it's far from being 100%
consistently used in the kernel already.  People are welcome to change
the code after I submit it, but I just can't make myself write it that
way, sorry.

[wrt utrace_regset_copy*]
> 	This function is far too large to inline it.  Please move it out
> 	of line.

The reason these functions are inlines is that some of their arguments
are always constants, so they get inlined into not a whole lot more than
the inlined memcpy.  Having these interfaces makes it a whole lot easier
to write the machine regset code, which needs pretty simple code, but
with constant arithmetic that's easy to louse up doing it by hand.  As
real functions they would just be more code that always had some runtime
arithmetic and tests it didn't need.

> 	This doesn't follow kernel coding style at all, we always
> 	put the && or || operators at the end of the closing line.

I could swear I've been "corrected" in the opposite direction on this one.
It is not mentioned in Documentation/CodingStyle, and the existing kernel
code is far from consistent on it.  I really don't care which way it is,
but I'd like clear authoritative direction from Linus and Andrew before I
bother with it.

> 	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))

No, that's not equivalent.  UTRACE_EVENT_SIGNAL_ALL is not just one bit,
but a mask of several bits.  

> +#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

I've changed it that way.

> 	Please add a block comment at the top of the file noting the
> 	copyright/license status author and basic desription of the
> 	contents.

I've now done that for all the files I added.

> 	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.:

I will use more gotos.

> +#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.

I don't agree that duplicating intricate boilerplate code is a good
idea.  I will see if I can formulate macros more similar to those common
in the kernel that don't increase error-prone manual duplication much.

> +// XXX copied from signal.c
[...]
> 	Copying and pasting this kind of stuff is a very bad idea.

I know.

> 	Just put a helper like the following into signal.c and use it from
> 	utrace.c:

I'd rather avoid the gratuitous call for some simple constants.  Instead,
I'd just move the macros to linux/signal.h and clean them up a little.
I hadn't done this just to separate misc cleanup from new stuff.  I'll
send a separate patch to do this, so once that's merged, utrace won't need
anything unclean here.

> 	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.

I changed it that way, except that ptrace_may_attach hasn't changed name.
I think it should change name, since it's really used more often for /proc
permissions than for ptrace.  But until the security_ptrace hook changes
name or is replaced with some cleaner hooks, the name makes sense.

> +int getrusage(struct task_struct *, int, struct rusage __user *);
> 
>  	Please never put prototypes like this into .c files.

I was copying existing practice for that function at the time, really just
to avoid comingling unrelated lint patches with my changes.  I've removed
the declaration now that it's in a header file.

> +#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.

I changed it that way.

> +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.

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.


Thanks,
Roland

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  4:02 ` Roland McGrath
@ 2007-04-30  9:08   ` Christoph Hellwig
  2007-04-30  9:18     ` Russell King
  2007-05-10  8:49     ` Roland McGrath
  2007-04-30  9:11   ` condingstyle, was " Christoph Hellwig
  1 sibling, 2 replies; 36+ messages in thread
From: Christoph Hellwig @ 2007-04-30  9:08 UTC (permalink / raw)
  To: Roland McGrath; +Cc: Christoph Hellwig, linux-kernel, linux-arch

On Sun, Apr 29, 2007 at 09:02:13PM -0700, Roland McGrath wrote:
> I'm sorry I've taken so long to reply to your review comments.
> I won't dwell on that, and just dive into the discussion.
> 
> > --- 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.
> 
> You mean renumber 7,8,9 down to 6,7,8?  I guess there's no reason not to
> do that, I was just not changing values I didn't need to.  AFAIK the
> only thing that matters about these value is the bits < 16 that there
> and the bits >= 16 likewise, for _TIF_ALLWORK_MASK to work right.

No need to renumber.  You remove TIF_SYSCALL_EMU which is six,
so the newly added TIF_FORCED_TF could reuse that bit.

> > +/***
> > + *** These are the exported entry points for tracing engines to use.
> > + ***/
> > 
> > 	This is not a standard comment format.  It should be:
> 
> Ok.  The intention was to clearly set off those declarations from others.
> It appears the thing other linux/*.h files do for this is:
> 
> /**************************************************************************
>  * These are the exported entry points for tracing engines to use.
>  **************************************************************************/

Not really that much.  Prefered would be just a normal comment which
would stick out if the actual comments were kerneldoc in the source files.

> > 	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 ;-)
> 
> 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.

> 
> >  	Please try consolidate all the compat code in a single #ifdef block,
> > 	preferably at the end of the file.
> 
> I did it this way to put each compat_foo near foo so it makes sense in
> context, and it's easy to remember to update it in parallel if you change
> foo.  Is it really better to move them all to an undifferentiated cluster
> at the end?

That's the normal approach.  If it's big enough it might even make
sense to give it a file of it's own to have the compat code totally
separate.  But IIRC there wasn't a whole lot of compat code in utrace.

> > 	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));
> 
> I agree it's come out ugly.  The reason I complicated it was an attempt
> to make the answer testable in #if when it's constant at compile time.
> My thinking is there will be situations where you want to avoid building
> in some useless dead code and just "if (0)" is not sufficient.  But
> primarily I was thinking of avoiding building extra fancy code in lieu
> of single-step on machines/configurations where single-step is always
> available, and the current #if's don't help with that.  How about if an
> asm-arch/tracehook.h either declares arch_has_single_step or else it
> #define's one of two things and in a single place I'll add:
> 
> 	#ifdef ARCH_ALWAYS_HAS_SINGLE_STEP
> 	#define arch_has_single_step()	1
> 	#elif defined ARCH_NEVER_HAS_SINGLE_STEP
> 	#define arch_has_single_step()	0
> 	#endif

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.

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?

> > 	The coding style here is wrong .  The else should be on the line
> > 	of the closing brace.  
> 
> I can ordinarily ignore syntax, but this is an abomination in the sight
> of the Lord and always will be.  Fortunately, it's far from being 100%
> consistently used in the kernel already.  People are welcome to change
> the code after I submit it, but I just can't make myself write it that
> way, sorry.

It's used pretty consistant in core code not written by you, which
looks similarly horrible.  Please try to fit a consistant style.

> 
> [wrt utrace_regset_copy*]
> > 	This function is far too large to inline it.  Please move it out
> > 	of line.
> 
> The reason these functions are inlines is that some of their arguments
> are always constants, so they get inlined into not a whole lot more than
> the inlined memcpy.  Having these interfaces makes it a whole lot easier
> to write the machine regset code, which needs pretty simple code, but
> with constant arithmetic that's easy to louse up doing it by hand.  As
> real functions they would just be more code that always had some runtime
> arithmetic and tests it didn't need.

Needs comments in the code explaining it.

> > 	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))
> 
> No, that's not equivalent.  UTRACE_EVENT_SIGNAL_ALL is not just one bit,
> but a mask of several bits.  

> > +#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.
> 
> I don't agree that duplicating intricate boilerplate code is a good
> idea.  I will see if I can formulate macros more similar to those common
> in the kernel that don't increase error-prone manual duplication much.

It's just a few uses and expands to two lines of code, so avoiding
the macro probably is a good idea.

> > +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.
> 
> 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.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* condingstyle, was Re: utrace comments
  2007-04-30  4:02 ` Roland McGrath
  2007-04-30  9:08   ` Christoph Hellwig
@ 2007-04-30  9:11   ` Christoph Hellwig
  2007-04-30 17:09     ` Andrew Morton
  1 sibling, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2007-04-30  9:11 UTC (permalink / raw)
  To: Roland McGrath, akpm; +Cc: Christoph Hellwig, linux-kernel, linux-arch

I've separated this out under a new subject because some style issues
that so far aren't documented explicitly are in doubt here, and Roland
wants and Answer from Andrew.

We also should put clauses on this into CodingStyle.


On Sun, Apr 29, 2007 at 09:02:13PM -0700, Roland McGrath wrote:
> > 	The coding style here is wrong.  The else should be on the line
> > 	of the closing brace.  
> 
> I can ordinarily ignore syntax, but this is an abomination in the sight
> of the Lord and always will be.  Fortunately, it's far from being 100%
> consistently used in the kernel already.  People are welcome to change
> the code after I submit it, but I just can't make myself write it that
> way, sorry.

> > 	This doesn't follow kernel coding style at all, we always
> > 	put the && or || operators at the end of the closing line.
> 
> I could swear I've been "corrected" in the opposite direction on this one.
> It is not mentioned in Documentation/CodingStyle, and the existing kernel
> code is far from consistent on it.  I really don't care which way it is,
> but I'd like clear authoritative direction from Linus and Andrew before I
> bother with it.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:08   ` Christoph Hellwig
@ 2007-04-30  9:18     ` Russell King
  2007-04-30  9:22       ` Christoph Hellwig
  2007-05-10  8:49     ` Roland McGrath
  1 sibling, 1 reply; 36+ messages in thread
From: Russell King @ 2007-04-30  9:18 UTC (permalink / raw)
  To: Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

On Mon, Apr 30, 2007 at 10:08:40AM +0100, Christoph Hellwig wrote:
> 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?

Roland's idea of single-stepping is that it *must* be supported by
hardware for utrace to use it.  There are a number of architectures
which can only do single-stepping by modifying the text of the
program being single stepped.  ARM is one such example.

As such, even when utrace is complete, some architectures will never
support in-kernel single step with utrace.  I believe Roland's idea
is to have single step supported on these via some vapourware userspace
library.

My biggest worry is that the architectures which do not support hardware
single stepping are seen by many people as "minority architectures" which
"don't really matter" and as such making support for a feature such a
special case will probably result in the feature effectively being
unavailable on those architectures.

As such I have zero motivation to continue my forrey into utrace from
a couple of months ago.  I'd also like to see utrace become *optional*
for architectures to support, rather than as it currently stands as
a *mandatory* requirement when merged.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:18     ` Russell King
@ 2007-04-30  9:22       ` Christoph Hellwig
  2007-04-30  9:33         ` Russell King
  0 siblings, 1 reply; 36+ messages in thread
From: Christoph Hellwig @ 2007-04-30  9:22 UTC (permalink / raw)
  To: Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

On Mon, Apr 30, 2007 at 10:18:09AM +0100, Russell King wrote:
> Roland's idea of single-stepping is that it *must* be supported by
> hardware for utrace to use it.  There are a number of architectures
> which can only do single-stepping by modifying the text of the
> program being single stepped.  ARM is one such example.
> 
> As such, even when utrace is complete, some architectures will never
> support in-kernel single step with utrace.  I believe Roland's idea
> is to have single step supported on these via some vapourware userspace
> library.

Does the current arm ptrace code support single stepping in kernelspace?
If yes we absolutely need to continue to support it.

> I'd also like to see utrace become *optional*
> for architectures to support, rather than as it currently stands as
> a *mandatory* requirement when merged.

No way we'd keep both the old ptrace mess and utrace in the same tree.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:22       ` Christoph Hellwig
@ 2007-04-30  9:33         ` Russell King
  2007-04-30  9:45           ` Russell King
                             ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Russell King @ 2007-04-30  9:33 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Christoph Hellwig, Roland McGrath, linux-kernel, linux-arch

On Mon, Apr 30, 2007 at 11:22:00AM +0200, Christoph Hellwig wrote:
> On Mon, Apr 30, 2007 at 10:18:09AM +0100, Russell King wrote:
> > Roland's idea of single-stepping is that it *must* be supported by
> > hardware for utrace to use it.  There are a number of architectures
> > which can only do single-stepping by modifying the text of the
> > program being single stepped.  ARM is one such example.
> > 
> > As such, even when utrace is complete, some architectures will never
> > support in-kernel single step with utrace.  I believe Roland's idea
> > is to have single step supported on these via some vapourware userspace
> > library.
> 
> Does the current arm ptrace code support single stepping in kernelspace?
> If yes we absolutely need to continue to support it.

single stepping of user space code via standard ptrace calls, yes.

> > I'd also like to see utrace become *optional*
> > for architectures to support, rather than as it currently stands as
> > a *mandatory* requirement when merged.
> 
> No way we'd keep both the old ptrace mess and utrace in the same tree.

Given the stated arguments from yourself and Roland, that only leaves
one solution to that.

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'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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:33         ` Russell King
@ 2007-04-30  9:45           ` Russell King
  2007-04-30 10:32             ` Christoph Hellwig
  2007-04-30 10:18           ` Christoph Hellwig
  2007-06-22  2:40           ` Roland McGrath
  2 siblings, 1 reply; 36+ messages in thread
From: Russell King @ 2007-04-30  9:45 UTC (permalink / raw)
  To: Christoph Hellwig, Christoph Hellwig, Roland McGrath,
	linux-kernel, linux-arch

On Mon, Apr 30, 2007 at 10:33:31AM +0100, Russell King wrote:
> On Mon, Apr 30, 2007 at 11:22:00AM +0200, Christoph Hellwig wrote:
> > On Mon, Apr 30, 2007 at 10:18:09AM +0100, Russell King wrote:
> > > Roland's idea of single-stepping is that it *must* be supported by
> > > hardware for utrace to use it.  There are a number of architectures
> > > which can only do single-stepping by modifying the text of the
> > > program being single stepped.  ARM is one such example.
> > > 
> > > As such, even when utrace is complete, some architectures will never
> > > support in-kernel single step with utrace.  I believe Roland's idea
> > > is to have single step supported on these via some vapourware userspace
> > > library.
> > 
> > Does the current arm ptrace code support single stepping in kernelspace?
> > If yes we absolutely need to continue to support it.
> 
> single stepping of user space code via standard ptrace calls, yes.
> 
> > > I'd also like to see utrace become *optional*
> > > for architectures to support, rather than as it currently stands as
> > > a *mandatory* requirement when merged.
> > 
> > No way we'd keep both the old ptrace mess and utrace in the same tree.
> 
> Given the stated arguments from yourself and Roland, that only leaves
> one solution to that.
> 
> 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'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.

For the sake of avoiding too much rehash, here's Roland's reply to my
initial forrey into utrace:

  http://marc.info/?l=linux-kernel&m=117309251916053&w=2

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:33         ` Russell King
  2007-04-30  9:45           ` Russell King
@ 2007-04-30 10:18           ` Christoph Hellwig
  2007-06-22  2:40           ` Roland McGrath
  2 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2007-04-30 10:18 UTC (permalink / raw)
  To: Christoph Hellwig, Christoph Hellwig, Roland McGrath,
	linux-kernel, linux-arch

On Mon, Apr 30, 2007 at 10:33:31AM +0100, Russell King wrote:
> > Does the current arm ptrace code support single stepping in kernelspace?
> > If yes we absolutely need to continue to support it.
> 
> single stepping of user space code via standard ptrace calls, yes.
> 
> > > I'd also like to see utrace become *optional*
> > > for architectures to support, rather than as it currently stands as
> > > a *mandatory* requirement when merged.
> > 
> > No way we'd keep both the old ptrace mess and utrace in the same tree.
> 
> Given the stated arguments from yourself and Roland, that only leaves
> one solution to that.
> 
> 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.

Umm, no.  A major regression in the ptrace functionality for some
architectures is simply not acceptable.  We can't merge utrace if
we break existing userspace ABIs, and losing single stepping support
is exactly that.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:45           ` Russell King
@ 2007-04-30 10:32             ` Christoph Hellwig
  0 siblings, 0 replies; 36+ messages in thread
From: Christoph Hellwig @ 2007-04-30 10:32 UTC (permalink / raw)
  To: Christoph Hellwig, Christoph Hellwig, Roland McGrath,
	linux-kernel, linux-arch

On Mon, Apr 30, 2007 at 10:45:10AM +0100, Russell King wrote:
> For the sake of avoiding too much rehash, here's Roland's reply to my
> initial forrey into utrace:
> 
>   http://marc.info/?l=linux-kernel&m=117309251916053&w=2

In that mail Roland suggests keeping the singlestep code entirely
in the arm ptrace code.  After a brief look at the arm code this
looks easily possible.  From a brief look the arm software singlestep
consist of the following pieces:

 - PTRACE_SINGLESTEP implementation.  Sets the PT_SINGLESTEP flag,
   clears TIF_SYSCALL_TRACE, sets ->exit_code in the traced code
   to the singlestepping signal and wakes the traced process up.

   This can easily be implemented by putting alsmost equivalent code
   into arch_ptrace.
 - clearing PT_SINGLESTEP and cancelling the breakpoint in ptrace_disable.

   Equivalent code can go into tracehook_disable_single_step.

 - Various places in signal.c that check PT_SINGLESTEP to set/clear
   the special singlestep breakpoint.  This can stay, it just needs
   a different place to store the singlestep flag.

Do I miss something?


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-04-30  9:11   ` condingstyle, was " Christoph Hellwig
@ 2007-04-30 17:09     ` Andrew Morton
  2007-04-30 18:19       ` Jan Engelhardt
                         ` (3 more replies)
  0 siblings, 4 replies; 36+ messages in thread
From: Andrew Morton @ 2007-04-30 17:09 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Roland McGrath, Christoph Hellwig, linux-kernel, linux-arch

On Mon, 30 Apr 2007 10:11:21 +0100 Christoph Hellwig <hch@infradead.org> wrote:

> I've separated this out under a new subject because some style issues
> that so far aren't documented explicitly are in doubt here, and Roland
> wants and Answer from Andrew.
> 
> We also should put clauses on this into CodingStyle.
> 
> 
> On Sun, Apr 29, 2007 at 09:02:13PM -0700, Roland McGrath wrote:
> > > 	The coding style here is wrong.  The else should be on the line
> > > 	of the closing brace.  
> > 
> > I can ordinarily ignore syntax, but this is an abomination in the sight
> > of the Lord and always will be.  Fortunately, it's far from being 100%
> > consistently used in the kernel already.  People are welcome to change
> > the code after I submit it, but I just can't make myself write it that
> > way, sorry.

I'm a bit lost here.  Are we referring to

	if (expr) {
		...
	} else {
		...
	}

versus

	if (expr) {
		...
	}
	else {
		...
	}

?

If so the former is usual style.  People do add code which does the latter,
but it tends to get fixed up later on when someone else does some work on
the same code.

> > > 	This doesn't follow kernel coding style at all, we always
> > > 	put the && or || operators at the end of the closing line.
> > 
> > I could swear I've been "corrected" in the opposite direction on this one.
> > It is not mentioned in Documentation/CodingStyle, and the existing kernel
> > code is far from consistent on it.  I really don't care which way it is,
> > but I'd like clear authoritative direction from Linus and Andrew before I
> > bother with it.

This is

	if (expr1 &&
		expr2)

versus

	if (expr1
		&& expr2)

the former is more common and is, IMO, more readable.

The latter can be handy sometimes to prevent an 80-col overflow in the
first line.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-04-30 17:09     ` Andrew Morton
@ 2007-04-30 18:19       ` Jan Engelhardt
  2007-04-30 18:39       ` Daniel Hazelton
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 36+ messages in thread
From: Jan Engelhardt @ 2007-04-30 18:19 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch


On Apr 30 2007 10:09, Andrew Morton wrote:
>
>This is
>
>	if (expr1 &&
>		expr2)
>
>versus
>
>	if (expr1
>		&& expr2)
>
>the former is more common and is, IMO, more readable.
>
>The latter can be handy sometimes to prevent an 80-col overflow in the
>first line.

But reads like you would be trying to put
a \ on the expr2 line before expr2 :-/


Jan
-- 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-04-30 17:09     ` Andrew Morton
  2007-04-30 18:19       ` Jan Engelhardt
@ 2007-04-30 18:39       ` Daniel Hazelton
  2007-04-30 18:42       ` Satyam Sharma
  2007-04-30 21:34       ` Luck, Tony
  3 siblings, 0 replies; 36+ messages in thread
From: Daniel Hazelton @ 2007-04-30 18:39 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

On Monday 30 April 2007 13:09:17 Andrew Morton wrote:
> On Mon, 30 Apr 2007 10:11:21 +0100 Christoph Hellwig <hch@infradead.org> 
wrote:
> > I've separated this out under a new subject because some style issues
> > that so far aren't documented explicitly are in doubt here, and Roland
> > wants and Answer from Andrew.
> >
> > We also should put clauses on this into CodingStyle.
> >
> > On Sun, Apr 29, 2007 at 09:02:13PM -0700, Roland McGrath wrote:
> > > > 	The coding style here is wrong.  The else should be on the line
> > > > 	of the closing brace.
> > >
> > > I can ordinarily ignore syntax, but this is an abomination in the sight
> > > of the Lord and always will be.  Fortunately, it's far from being 100%
> > > consistently used in the kernel already.  People are welcome to change
> > > the code after I submit it, but I just can't make myself write it that
> > > way, sorry.
>
> I'm a bit lost here.  Are we referring to
>
> 	if (expr) {
> 		...
> 	} else {
> 		...
> 	}
>
> versus
>
> 	if (expr) {
> 		...
> 	}
> 	else {
> 		...
> 	}
>
> ?

For some strange reason I have a feeling its the former. I remember that one 
of the "Rules of Style" that are pushed by a lot of people is to "Uncuddle 
the Else".

I prefer the '} else {'  style - for the same reason that I prefer the 
'float logf(float x) {' style for functions and similar (where it doesn't go 
beyond 80 columns) in my own code. The reason is that I like trying to keep 
the number of lines with no code on them minimal - helps me to look at the 
output of wc -l to find files where I might have been overly verbose with the 
code and/or comments.

DRH



^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-04-30 17:09     ` Andrew Morton
  2007-04-30 18:19       ` Jan Engelhardt
  2007-04-30 18:39       ` Daniel Hazelton
@ 2007-04-30 18:42       ` Satyam Sharma
  2007-04-30 22:18         ` Stefan Richter
                           ` (2 more replies)
  2007-04-30 21:34       ` Luck, Tony
  3 siblings, 3 replies; 36+ messages in thread
From: Satyam Sharma @ 2007-04-30 18:42 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

Hi,

On 4/30/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Mon, 30 Apr 2007 10:11:21 +0100 Christoph Hellwig <hch@infradead.org> wrote:
>
> > I've separated this out under a new subject because some style issues
> > that so far aren't documented explicitly are in doubt here, and Roland
> > wants and Answer from Andrew.
> >
> > We also should put clauses on this into CodingStyle.
> >
> >
> > On Sun, Apr 29, 2007 at 09:02:13PM -0700, Roland McGrath wrote:
> > > >   The coding style here is wrong.  The else should be on the line
> > > >   of the closing brace.
> > >
> > > I can ordinarily ignore syntax, but this is an abomination in the sight
> > > of the Lord and always will be.  Fortunately, it's far from being 100%
> > > consistently used in the kernel already.  People are welcome to change
> > > the code after I submit it, but I just can't make myself write it that
> > > way, sorry.
>
> I'm a bit lost here.  Are we referring to
>
>         if (expr) {
>                 ...
>         } else {
>                 ...
>         }
>
> versus
>
>         if (expr) {
>                 ...
>         }
>         else {
>                 ...
>         }
>
> ?
>
> If so the former is usual style.  People do add code which does the latter,
> but it tends to get fixed up later on when someone else does some work on
> the same code.

Documentation/CodingStyle is indeed a bit ambiguous w.r.t. this. It does
mention what Andrew is saying here: a closing brace always on a line of its
own _except_ that an "else" may appear immediately after the closing brace
of the previous statement block in if-else statements. Unfortunately, this goes
against the example given in K&R Section 3.2 and the example given to
illustrate this in CodingStyle itself shows a multiple-condition
if-else-if statement,
not a single-condition if-else (and K&R do treat if-else-if's differently than
if-else's for indentation purposes). No harm in continuing to stick to the usual
style, of course, as K&R are not consistent about this themselves.

> > > >   This doesn't follow kernel coding style at all, we always
> > > >   put the && or || operators at the end of the closing line.
> > >
> > > I could swear I've been "corrected" in the opposite direction on this one.
> > > It is not mentioned in Documentation/CodingStyle, and the existing kernel
> > > code is far from consistent on it.  I really don't care which way it is,
> > > but I'd like clear authoritative direction from Linus and Andrew before I
> > > bother with it.
>
> This is
>
>         if (expr1 &&
>                 expr2)
>
> versus
>
>         if (expr1
>                 && expr2)
>
> the former is more common and is, IMO, more readable.

Existing kernel code is all over the place w.r.t. this one. Even a same
person doesn't follow a single convention all the time (see kernel/signal.c
for all sorts of compound-conditions-indentation-abuse :-)

> The latter can be handy sometimes to prevent an 80-col overflow in the
> first line.

Actually, the latter style (one condition per line and the && or ||
operators appearing _before_ the conditions in subsequent lines)
is quite popular for multi-line compound conditions (well, I've seen this
in kernel/workqueue.c, kernel/stop_machine.c, etc at least, and also in
Linus' code, if I'm not mistaken). We also align subsequent lines to the
column of the condition belonging to the same "logical level" on the
previous line using _spaces_ (note that this is alignment, not indentation,
using spaces). The rationale is to make the operator prominent and thus make
the structure of a complex multi-line compound conditional expression more
readable and obvious at first glance itself. For example, consider:

	if (veryverylengthycondition1 &&
		smallcond2 &&
		(conditionnumber3a ||
		condition3b)) {
		...
	}

versus

	if (veryverylengthycondition1
	    && smallcond2
	    && (conditionnumber3a
	        || condition3b)) {
		...
	}

?

Latter wins, doesn't it?

Satyam

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: condingstyle, was Re: utrace comments
  2007-04-30 17:09     ` Andrew Morton
                         ` (2 preceding siblings ...)
  2007-04-30 18:42       ` Satyam Sharma
@ 2007-04-30 21:34       ` Luck, Tony
  3 siblings, 0 replies; 36+ messages in thread
From: Luck, Tony @ 2007-04-30 21:34 UTC (permalink / raw)
  To: Andrew Morton, Christoph Hellwig
  Cc: Roland McGrath, Christoph Hellwig, linux-kernel, linux-arch

> I'm a bit lost here.  Are we referring to
>
>	if (expr) {
>		...
>	} else {
>		...
>	}
>
> versus
>
>	if (expr) {
>		...
>	}
>	else {
>		...
>	}

This one is already covered by Documentation/CodingStyle (with the Justification
that this is the way that K&R do it, so it must be right).

> This is
>
>	if (expr1 &&
>		expr2)
>
> versus
>
>	if (expr1
>		&& expr2)

This isn't covered ... CodingStyle merely says that long lines
should be broken into "sensible chunks".  Perhaps we should add
some words about breaking long lines leaving binary operators
trailing on the end of the line (my preferred style).  But the
only examples of this I could find leafing through K&R they
put the binary operator on the start of the continuation line
(could the prophets have been wrong here :-)

-Tony

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-04-30 18:42       ` Satyam Sharma
@ 2007-04-30 22:18         ` Stefan Richter
  2007-05-01  9:00         ` Geert Uytterhoeven
  2007-05-01 16:15         ` Stuart MacDonald
  2 siblings, 0 replies; 36+ messages in thread
From: Stefan Richter @ 2007-04-30 22:18 UTC (permalink / raw)
  To: Satyam Sharma
  Cc: Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

Satyam Sharma wrote:
[...]
> The rationale is to make the operator prominent and thus make
> the structure of a complex multi-line compound conditional expression more
> readable and obvious at first glance itself. For example, consider:
> 
>     if (veryverylengthycondition1 &&
>         smallcond2 &&
>         (conditionnumber3a ||
>         condition3b)) {
>         ...
>     }
> 
> versus
> 
>     if (veryverylengthycondition1
>         && smallcond2
>         && (conditionnumber3a
>             || condition3b)) {
>         ...
>     }
> 
> ?
> 
> Latter wins, doesn't it?

I find the latter more sensible too, even though I used the former
myself until recently (because I didn't knew better).
-- 
Stefan Richter
-=====-=-=== -=-= ----=
http://arcgraph.de/sr/

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-04-30 18:42       ` Satyam Sharma
  2007-04-30 22:18         ` Stefan Richter
@ 2007-05-01  9:00         ` Geert Uytterhoeven
  2007-05-01 13:11           ` Scott Preece
  2007-05-01 14:16           ` David Woodhouse
  2007-05-01 16:15         ` Stuart MacDonald
  2 siblings, 2 replies; 36+ messages in thread
From: Geert Uytterhoeven @ 2007-05-01  9:00 UTC (permalink / raw)
  To: Satyam Sharma
  Cc: Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

On Tue, 1 May 2007, Satyam Sharma wrote:
> Actually, the latter style (one condition per line and the && or ||
> operators appearing _before_ the conditions in subsequent lines)
> is quite popular for multi-line compound conditions (well, I've seen this
> in kernel/workqueue.c, kernel/stop_machine.c, etc at least, and also in
> Linus' code, if I'm not mistaken). We also align subsequent lines to the
> column of the condition belonging to the same "logical level" on the
> previous line using _spaces_ (note that this is alignment, not indentation,
> using spaces). The rationale is to make the operator prominent and thus make
> the structure of a complex multi-line compound conditional expression more
> readable and obvious at first glance itself. For example, consider:
> 
> 	if (veryverylengthycondition1 &&
> 		smallcond2 &&
> 		(conditionnumber3a ||
> 		condition3b)) {
> 		...
> 	}
> 
> versus
> 
> 	if (veryverylengthycondition1
> 	    && smallcond2
> 	    && (conditionnumber3a
> 	        || condition3b)) {
> 		...
> 	}
> 
> ?
> 
> Latter wins, doesn't it?

... because you forgot to align subsequent lines to the column of the
condition belonging to the same "logical level" on the previous line.

Consider this:

	if (veryverylengthycondition1 &&
	    smallcond2 &&
	    (conditionnumber3a ||
	     condition3b)) {
		...
	}

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01  9:00         ` Geert Uytterhoeven
@ 2007-05-01 13:11           ` Scott Preece
  2007-05-01 14:16           ` David Woodhouse
  1 sibling, 0 replies; 36+ messages in thread
From: Scott Preece @ 2007-05-01 13:11 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Satyam Sharma, Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

On 5/1/07, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Tue, 1 May 2007, Satyam Sharma wrote:
> > Actually, the latter style (one condition per line and the && or ||
> > operators appearing _before_ the conditions in subsequent lines)
> > is quite popular for multi-line compound conditions (well, I've seen this
> > in kernel/workqueue.c, kernel/stop_machine.c, etc at least, and also in
> > Linus' code, if I'm not mistaken). We also align subsequent lines to the
> > column of the condition belonging to the same "logical level" on the
> > previous line using _spaces_ (note that this is alignment, not indentation,
> > using spaces). The rationale is to make the operator prominent and thus make
> > the structure of a complex multi-line compound conditional expression more
> > readable and obvious at first glance itself. For example, consider:
> >
> >       if (veryverylengthycondition1 &&
> >               smallcond2 &&
> >               (conditionnumber3a ||
> >               condition3b)) {
> >               ...
> >       }
> >
> > versus
> >
> >       if (veryverylengthycondition1
> >           && smallcond2
> >           && (conditionnumber3a
> >               || condition3b)) {
> >               ...
> >       }
> >
> > ?
> >
> > Latter wins, doesn't it?
>
> ... because you forgot to align subsequent lines to the column of the
> condition belonging to the same "logical level" on the previous line.
>
> Consider this:
>
>         if (veryverylengthycondition1 &&
>             smallcond2 &&
>             (conditionnumber3a ||
>              condition3b)) {
>                 ...
>         }
>
> Gr{oetje,eeting}s,
>
>                                                 Geert


I still find the leading-operator style much more readable. The most
important thing in reading a long, complex conditional is
understanding the structure of the operators, not the operands.
Putting the operators at the front of the line emphasizes that
structure, especially since you can line the operators up to match the
logical indenting, whereas the trailing-operator style leaves the
operators scattered and hard to assemble into a logical structure.

However, there's a lot of difference of opinion on this (perhaps
rooted in differences in cognition and reading behavior). For me it's
not even close - expressions broken so the operators are at the head
of the line snap into focus and those with operators at the ends of
the lines look like undifferentiated goo. Since some of the style
guides I've seen do it the other way, I assume some people have the
opposite perception. I guess that's why indent offers options for
doing it either way...

scott

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01  9:00         ` Geert Uytterhoeven
  2007-05-01 13:11           ` Scott Preece
@ 2007-05-01 14:16           ` David Woodhouse
  2007-05-01 15:00             ` John Anthony Kazos Jr.
                               ` (3 more replies)
  1 sibling, 4 replies; 36+ messages in thread
From: David Woodhouse @ 2007-05-01 14:16 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Satyam Sharma, Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

On Tue, 2007-05-01 at 11:00 +0200, Geert Uytterhoeven wrote:
> 
>         if (veryverylengthycondition1 &&
>             smallcond2 &&
>             (conditionnumber3a ||
>              condition3b)) {
>                 ...
>         }

It's horrid. I'd much rather see

        if (veryverylengthycondition1 &&
            smallcond2 &&
            (conditionnumber3a || condition3b)) {
                ...
        }

Even if that does take it over 80 columns, the 'failure' mode will be
that it gets wrapped round to the beginning of the screen or truncated
at 80 columns -- and for the _majority_ of the time, it doesn't matter.

Unless you're paying particular attention to the logic and debugging the
statement, a 'high-level' view of what's going on is perfectly
sufficient; you don't actually read every character anyway.

-- 
dwmw2


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 14:16           ` David Woodhouse
@ 2007-05-01 15:00             ` John Anthony Kazos Jr.
  2007-05-01 20:12               ` Satyam Sharma
  2007-05-01 15:05             ` Randy Dunlap
                               ` (2 subsequent siblings)
  3 siblings, 1 reply; 36+ messages in thread
From: John Anthony Kazos Jr. @ 2007-05-01 15:00 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Geert Uytterhoeven, Satyam Sharma, Andrew Morton,
	Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

> >         if (veryverylengthycondition1 &&
> >             smallcond2 &&
> >             (conditionnumber3a ||
> >              condition3b)) {
> >                 ...
> >         }
> 
> It's horrid. I'd much rather see
> 
>         if (veryverylengthycondition1 &&
>             smallcond2 &&
>             (conditionnumber3a || condition3b)) {
>                 ...
>         }

	if (veryverylengthycondition1
		&& smallcond2
		&& (conditionnumber3a
			|| condition3b)) {
		...
	}

Clear, crisp, and 80-wide. I also like how the logical operator on the 
following line is indented slightly into the condition of the previous 
line. I think this is much more sensical and sensible than using spaces to 
line them up with the parentheses. Makes clear for each operator the 
condition to which it applies.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 14:16           ` David Woodhouse
  2007-05-01 15:00             ` John Anthony Kazos Jr.
@ 2007-05-01 15:05             ` Randy Dunlap
  2007-05-01 15:06               ` David Woodhouse
  2007-05-01 15:07             ` Geert Uytterhoeven
  2007-05-01 16:07             ` David Howells
  3 siblings, 1 reply; 36+ messages in thread
From: Randy Dunlap @ 2007-05-01 15:05 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Geert Uytterhoeven, Satyam Sharma, Andrew Morton,
	Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

On Tue, 01 May 2007 15:16:13 +0100 David Woodhouse wrote:

> On Tue, 2007-05-01 at 11:00 +0200, Geert Uytterhoeven wrote:
> > 
> >         if (veryverylengthycondition1 &&
> >             smallcond2 &&
> >             (conditionnumber3a ||
> >              condition3b)) {
> >                 ...
> >         }
> 
> It's horrid. I'd much rather see
> 
>         if (veryverylengthycondition1 &&
>             smallcond2 &&
>             (conditionnumber3a || condition3b)) {
>                 ...
>         }
> 
> Even if that does take it over 80 columns, the 'failure' mode will be
> that it gets wrapped round to the beginning of the screen or truncated
> at 80 columns -- and for the _majority_ of the time, it doesn't matter.
> 
> Unless you're paying particular attention to the logic and debugging the
> statement, a 'high-level' view of what's going on is perfectly
> sufficient; you don't actually read every character anyway.

I prefer this format also, but I'm not sure that we can get it
into CodingStyle.  CodingStyle is about (either) concensus or
dictum, but I don't see us close to concensus...

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 15:05             ` Randy Dunlap
@ 2007-05-01 15:06               ` David Woodhouse
  2007-05-01 20:44                 ` Satyam Sharma
  0 siblings, 1 reply; 36+ messages in thread
From: David Woodhouse @ 2007-05-01 15:06 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Geert Uytterhoeven, Satyam Sharma, Andrew Morton,
	Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

On Tue, 2007-05-01 at 08:05 -0700, Randy Dunlap wrote:
> I prefer this format also, but I'm not sure that we can get it
> into CodingStyle.  CodingStyle is about (either) concensus or
> dictum, but I don't see us close to concensus... 

CodingStyle is mostly about consensus. We don't have a consensus, which
is why this particular stuff isn't specified in CodingStyle. :)

-- 
dwmw2


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 14:16           ` David Woodhouse
  2007-05-01 15:00             ` John Anthony Kazos Jr.
  2007-05-01 15:05             ` Randy Dunlap
@ 2007-05-01 15:07             ` Geert Uytterhoeven
  2007-05-01 15:11               ` David Woodhouse
  2007-05-01 16:07             ` David Howells
  3 siblings, 1 reply; 36+ messages in thread
From: Geert Uytterhoeven @ 2007-05-01 15:07 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Satyam Sharma, Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

On Tue, 1 May 2007, David Woodhouse wrote:
> On Tue, 2007-05-01 at 11:00 +0200, Geert Uytterhoeven wrote:
> > 
> >         if (veryverylengthycondition1 &&
> >             smallcond2 &&
> >             (conditionnumber3a ||
> >              condition3b)) {
> >                 ...
> >         }
> 
> It's horrid. I'd much rather see
> 
>         if (veryverylengthycondition1 &&
>             smallcond2 &&
>             (conditionnumber3a || condition3b)) {
>                 ...
>         }

For clarity, if it fits, I prefer that one, too.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 15:07             ` Geert Uytterhoeven
@ 2007-05-01 15:11               ` David Woodhouse
  0 siblings, 0 replies; 36+ messages in thread
From: David Woodhouse @ 2007-05-01 15:11 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Satyam Sharma, Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

On Tue, 2007-05-01 at 17:07 +0200, Geert Uytterhoeven wrote:
> For clarity, if it fits, I prefer that one, too.

I don't think that was under question, was it?
My point was that I prefer it even when it _doesn't_ fit.

-- 
dwmw2


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments 
  2007-05-01 14:16           ` David Woodhouse
                               ` (2 preceding siblings ...)
  2007-05-01 15:07             ` Geert Uytterhoeven
@ 2007-05-01 16:07             ` David Howells
  2007-05-02  2:18               ` Eric W. Biederman
  2007-05-02  9:32               ` David Howells
  3 siblings, 2 replies; 36+ messages in thread
From: David Howells @ 2007-05-01 16:07 UTC (permalink / raw)
  To: John Anthony Kazos Jr.
  Cc: David Woodhouse, Geert Uytterhoeven, Satyam Sharma,
	Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

John Anthony Kazos Jr. <jakj@j-a-k-j.com> wrote:

> 	if (veryverylengthycondition1
> 		&& smallcond2
> 		&& (conditionnumber3a
> 			|| condition3b)) {
> 		...
> 	}
> 
> Clear, crisp, and 80-wide. I also like how the logical operator on the 
> following line is indented slightly into the condition of the previous 
> line.

The brain works better when associated things are lined up, so:

	if (veryverylengthycondition1 &&
	    smallcond2 &&
	    (conditionnumber3a ||
	     condition3b)) {

is better as it gives a visual clue to association.

David

^ permalink raw reply	[flat|nested] 36+ messages in thread

* RE: condingstyle, was Re: utrace comments
  2007-04-30 18:42       ` Satyam Sharma
  2007-04-30 22:18         ` Stefan Richter
  2007-05-01  9:00         ` Geert Uytterhoeven
@ 2007-05-01 16:15         ` Stuart MacDonald
  2 siblings, 0 replies; 36+ messages in thread
From: Stuart MacDonald @ 2007-05-01 16:15 UTC (permalink / raw)
  To: 'Satyam Sharma', 'Andrew Morton'
  Cc: 'Christoph Hellwig', 'Roland McGrath',
	'Christoph Hellwig',
	linux-kernel, linux-arch

From: On Behalf Of Satyam Sharma
> readable and obvious at first glance itself. For example, consider:
               ^^^^^^^^^^^^^^^^^^^^^^^
> 
> 	if (veryverylengthycondition1 &&
> 		smallcond2 &&
> 		(conditionnumber3a ||
> 		condition3b)) {
> 		...
> 	}
> 
> versus

Whoops! You've got an unterminated if there, let me fix it up...

> 	if (veryverylengthycondition1) {
> 	    && smallcond2
> 	    && (conditionnumber3a
> 	        || condition3b)) {
> 		...
> 	}> 

From: On Behalf Of Scott Preece
> I still find the leading-operator style much more readable. The most
> important thing in reading a long, complex conditional is
> understanding the structure of the operators, not the operands.

Since there's a mix of pre-, post- and infix, the structure of the
operators _depends on_ the operands.

> However, there's a lot of difference of opinion on this (perhaps
> rooted in differences in cognition and reading behavior). For me it's
> not even close - expressions broken so the operators are at the head
> of the line snap into focus and those with operators at the ends of
> the lines look like undifferentiated goo. Since some of the style

I'm exactly opposite; the "uncorrected" line above is a typo because
there's no continuation item at the end of the line. I don't even see
the following lines because they are, by definition, not part of the
code I'm looking at. Leaving the operators at the end of the line I
easily see that it's a binary operator, and there's no second operand
so the next line must contain it, parse the next line as well.

Hm. I didn't realise this but the unspoken underlying factor for me is
the "one instruction per line" style. If you must break over multiple
lines, there must be a continuation item to indicate that. This is how
I detect missing semicolons.

..Stu


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 15:00             ` John Anthony Kazos Jr.
@ 2007-05-01 20:12               ` Satyam Sharma
  0 siblings, 0 replies; 36+ messages in thread
From: Satyam Sharma @ 2007-05-01 20:12 UTC (permalink / raw)
  To: John Anthony Kazos Jr.
  Cc: David Woodhouse, Geert Uytterhoeven, Andrew Morton,
	Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

On 5/1/07, John Anthony Kazos Jr. <jakj@j-a-k-j.com> wrote:
> > It's horrid. I'd much rather see
> >
> >         if (veryverylengthycondition1 &&
> >             smallcond2 &&
> >             (conditionnumber3a || condition3b)) {
> >                 ...
> >         }
>
>         if (veryverylengthycondition1
>                 && smallcond2
>                 && (conditionnumber3a
>                         || condition3b)) {
>                 ...
>         }
>
> Clear, crisp, and 80-wide. I also like how the logical operator on the
> following line is indented slightly into the condition of the previous
> line. I think this is much more sensical and sensible than using spaces to
> line them up with the parentheses. Makes clear for each operator the
> condition to which it applies.

Hmmm ... I did use the spaces to line up the operators with the
starting column of the _expression_ in the previous lines, not the
parentheses (that would be gross!). Of course, && and || are binary
operators so they apply both to the expression on the previous line as
well as the expression following it on the same line.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 15:06               ` David Woodhouse
@ 2007-05-01 20:44                 ` Satyam Sharma
  0 siblings, 0 replies; 36+ messages in thread
From: Satyam Sharma @ 2007-05-01 20:44 UTC (permalink / raw)
  To: David Woodhouse
  Cc: Randy Dunlap, Geert Uytterhoeven, Andrew Morton,
	Christoph Hellwig, Roland McGrath, Christoph Hellwig,
	linux-kernel, linux-arch

On 5/1/07, David Woodhouse <dwmw2@infradead.org> wrote:
> On Tue, 2007-05-01 at 08:05 -0700, Randy Dunlap wrote:
> > I prefer this format also, but I'm not sure that we can get it
> > into CodingStyle.  CodingStyle is about (either) concensus or
> > dictum, but I don't see us close to concensus...

Yes, some of these styles are too personal and subjective to even try
and standardize. And then often even the same person doesn't follow a
single convention across his own code. More likely you'd succeed
standardizing *religion* than this ...

> CodingStyle is mostly about consensus. We don't have a consensus, which
> is why this particular stuff isn't specified in CodingStyle. :)

Actually, I'm not sure if we really gain much by finding consensus for
this particular stuff. Most compound conditions only contain upto 3-4
operators/expressions, so most of the styles discussed here would be
almost equally readable. And any code that goes beyond 3-4
operators/expressions is probably ugly in many other ways and needs to
fix its logic.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-01 16:07             ` David Howells
@ 2007-05-02  2:18               ` Eric W. Biederman
  2007-05-02  9:32               ` David Howells
  1 sibling, 0 replies; 36+ messages in thread
From: Eric W. Biederman @ 2007-05-02  2:18 UTC (permalink / raw)
  To: David Howells
  Cc: John Anthony Kazos Jr.,
	David Woodhouse, Geert Uytterhoeven, Satyam Sharma,
	Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

David Howells <dhowells@redhat.com> writes:

> John Anthony Kazos Jr. <jakj@j-a-k-j.com> wrote:
>
>> 	if (veryverylengthycondition1
>> 		&& smallcond2
>> 		&& (conditionnumber3a
>> 			|| condition3b)) {
>> 		...
>> 	}
>> 
>> Clear, crisp, and 80-wide. I also like how the logical operator on the 
>> following line is indented slightly into the condition of the previous 
>> line.
>
> The brain works better when associated things are lined up, so:
>
> 	if (veryverylengthycondition1 &&
> 	    smallcond2 &&
> 	    (conditionnumber3a ||
> 	     condition3b)) {
>
> is better as it gives a visual clue to association.

Not lining up with the code following the if statement is also
a plus.  Because it clearly delineates the conditions from the code.

Of course in many cases conditions this complex are candidates for

if (!condition1)
	continue;
if (!condition2)
	continue;
if (!condition3a && !condition3b)
	continue;

Eric

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments 
  2007-05-01 16:07             ` David Howells
  2007-05-02  2:18               ` Eric W. Biederman
@ 2007-05-02  9:32               ` David Howells
  2007-05-02 11:55                 ` Eric W. Biederman
  2007-05-02 12:05                 ` David Howells
  1 sibling, 2 replies; 36+ messages in thread
From: David Howells @ 2007-05-02  9:32 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: John Anthony Kazos Jr.,
	David Woodhouse, Geert Uytterhoeven, Satyam Sharma,
	Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

Eric W. Biederman <ebiederm@xmission.com> wrote:

> Not lining up with the code following the if statement is also
> a plus.  Because it clearly delineates the conditions from the code.

But the condition doesn't line up with the code:

	if (veryverylengthycondition1 &&
	    smallcond2 &&
	    (conditionnumber3a ||
	     condition3b)) {
		this_is_some_code();
		this_is_some_more_code();
	}

Personally, for complicated conditions like this, I prefer:

	if (veryverylengthycondition1 &&
	    smallcond2 &&
	    (conditionnumber3a ||
	     condition3b)
	    ) {
		this_is_some_code();
		this_is_some_more_code();
	}

But that seems to offend Andrew for some reason (or was it Christoph? or
both?).

David

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments
  2007-05-02  9:32               ` David Howells
@ 2007-05-02 11:55                 ` Eric W. Biederman
  2007-05-02 12:05                 ` David Howells
  1 sibling, 0 replies; 36+ messages in thread
From: Eric W. Biederman @ 2007-05-02 11:55 UTC (permalink / raw)
  To: David Howells
  Cc: John Anthony Kazos Jr.,
	David Woodhouse, Geert Uytterhoeven, Satyam Sharma,
	Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

David Howells <dhowells@redhat.com> writes:

> Eric W. Biederman <ebiederm@xmission.com> wrote:
>
>> Not lining up with the code following the if statement is also
>> a plus.  Because it clearly delineates the conditions from the code.
>
> But the condition doesn't line up with the code:

Exactly.  The condition not lining up with the following code helps
code helps separate the two.

> 	if (veryverylengthycondition1 &&
> 	    smallcond2 &&
> 	    (conditionnumber3a ||
> 	     condition3b)) {
> 		this_is_some_code();
> 		this_is_some_more_code();
> 	}
>
> Personally, for complicated conditions like this, I prefer:
>
> 	if (veryverylengthycondition1 &&
> 	    smallcond2 &&
> 	    (conditionnumber3a ||
> 	     condition3b)
> 	    ) {
> 		this_is_some_code();
> 		this_is_some_more_code();
> 	}
>
> But that seems to offend Andrew for some reason (or was it Christoph? or
> both?).

Yes. 

Although I suspect simply not tucking the trailing brace is as
good or better.  I believe not putting the beginning brace at
the beginning of the line is a violation of coding style.

	if (veryverylengthycondition1 &&
	    smallcond2 &&
	    (conditionnumber3a ||
	     condition3b))
	{
		this_is_some_code();
		this_is_some_more_code();
	}


However there is the practical way to solve this if you have
a sufficiently large conditional, or the conditional appears
several times.

	static inline int test_func()
	{
		if (!veryverylengthycondition1)
                	return 0;
  		if (!smallcond2)
			return 0;
		if (conditionnumber3A)
			return 1;
		if (condition3b)
			return 1;
		return 0;
	}

	if (test_func()) {
        	this_is_some_code();
		this_is_some_more_code();
        }	

Eric

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: condingstyle, was Re: utrace comments 
  2007-05-02  9:32               ` David Howells
  2007-05-02 11:55                 ` Eric W. Biederman
@ 2007-05-02 12:05                 ` David Howells
  1 sibling, 0 replies; 36+ messages in thread
From: David Howells @ 2007-05-02 12:05 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: John Anthony Kazos Jr.,
	David Woodhouse, Geert Uytterhoeven, Satyam Sharma,
	Andrew Morton, Christoph Hellwig, Roland McGrath,
	Christoph Hellwig, linux-kernel, linux-arch

Eric W. Biederman <ebiederm@xmission.com> wrote:

> > But the condition doesn't line up with the code:
> 
> Exactly.  The condition not lining up with the following code helps
> code helps separate the two.

Sorry about that: I realised you were agreeing with me about 5s after I sent
the message.

> However there is the practical way to solve this if you have
> a sufficiently large conditional, or the conditional appears
> several times.

That doesn't necessarily work - for instance if the condition has side effects
on local variables (eg: postinc).  OTOH, large complex conditions with side
effects have to be abused with care and preferably avoided.  goto is your
friend:-)

David

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:08   ` Christoph Hellwig
  2007-04-30  9:18     ` Russell King
@ 2007-05-10  8:49     ` Roland McGrath
  1 sibling, 0 replies; 36+ messages in thread
From: Roland McGrath @ 2007-05-10  8:49 UTC (permalink / raw)
  To: Christoph Hellwig; +Cc: linux-kernel, linux-arch

> 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

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: utrace comments
  2007-04-30  9:33         ` Russell King
  2007-04-30  9:45           ` Russell King
  2007-04-30 10:18           ` Christoph Hellwig
@ 2007-06-22  2:40           ` Roland McGrath
  2 siblings, 0 replies; 36+ messages in thread
From: Roland McGrath @ 2007-06-22  2:40 UTC (permalink / raw)
  To: Russell King; +Cc: Christoph Hellwig, linux-kernel, linux-arch

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

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2007-06-22  2:40 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-11-27 16:51 utrace comments Christoph Hellwig
2006-12-05  9:51 ` Roland McGrath
2006-12-06 21:58   ` Christoph Hellwig
2007-04-30  4:02 ` Roland McGrath
2007-04-30  9:08   ` Christoph Hellwig
2007-04-30  9:18     ` Russell King
2007-04-30  9:22       ` Christoph Hellwig
2007-04-30  9:33         ` Russell King
2007-04-30  9:45           ` Russell King
2007-04-30 10:32             ` Christoph Hellwig
2007-04-30 10:18           ` Christoph Hellwig
2007-06-22  2:40           ` Roland McGrath
2007-05-10  8:49     ` Roland McGrath
2007-04-30  9:11   ` condingstyle, was " Christoph Hellwig
2007-04-30 17:09     ` Andrew Morton
2007-04-30 18:19       ` Jan Engelhardt
2007-04-30 18:39       ` Daniel Hazelton
2007-04-30 18:42       ` Satyam Sharma
2007-04-30 22:18         ` Stefan Richter
2007-05-01  9:00         ` Geert Uytterhoeven
2007-05-01 13:11           ` Scott Preece
2007-05-01 14:16           ` David Woodhouse
2007-05-01 15:00             ` John Anthony Kazos Jr.
2007-05-01 20:12               ` Satyam Sharma
2007-05-01 15:05             ` Randy Dunlap
2007-05-01 15:06               ` David Woodhouse
2007-05-01 20:44                 ` Satyam Sharma
2007-05-01 15:07             ` Geert Uytterhoeven
2007-05-01 15:11               ` David Woodhouse
2007-05-01 16:07             ` David Howells
2007-05-02  2:18               ` Eric W. Biederman
2007-05-02  9:32               ` David Howells
2007-05-02 11:55                 ` Eric W. Biederman
2007-05-02 12:05                 ` David Howells
2007-05-01 16:15         ` Stuart MacDonald
2007-04-30 21:34       ` Luck, Tony

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).