Re: /proc visibility patch breaks GDB, etc.

[Kingsley Cheung <kingsley@aurema.com>]

On Thu, Feb 26, 2004 at 12:09:59PM -0800, Andrew Morton wrote:
> David Mosberger <davidm@napali.hpl.hp.com> wrote:
> >
> > >>>>> On Wed, 25 Feb 2004 22:44:10 -0800, Andrew Morton <akpm@osdl.org> said:
> > 
> >   Andrew> Peter Chubb <peter@chubb.wattle.id.au> wrote:
> >   >> 
> >   >> 
> >   >> In fs/proc/base.c:proc_pid_lookup(), the patch
> >   >> 
> >   >> read_unlock(&tasklist_lock); if (!task) goto out; + if
> >   >> (!thread_group_leader(task)) + goto out_drop_task;
> >   >> 
> >   >> inode = proc_pid_make_inode(dir->i_sb, task, PROC_TGID_INO);
> >   >> 
> >   >> means that threads other than the thread group leader don't
> >   >> appear in the /proc top-level directory.  Programs that are
> >   >> informed via pid of events can no longer find the appropriate
> >   >> process -- for example, using gdb on a multi-threaded process, or
> >   >> profiling using perfmon.
> >   >> 
> >   >> The immediate symptom is GDB saying: Could not open
> >   >> /proc/757/status when 757 is a TID not a PID.
> > 
> >   Andrew> What does `ls /proc/757' say?  Presumably no such file or
> >   Andrew> directory?  It's fairly bizare behaviour to be able to open
> >   Andrew> files which don't exist according to readdir, which is why
> >   Andrew> we made that change.
> > 
> > Excuse, but this seems seriously FOOBAR.  I understand that it's
> > interesting to see the thread-leader/thread relationship, but surely
> > that's no reason to break backwards compatibility and the ability to
> > look up _any_ task's info via /proc/PID/.
> 
> Well you can't look them up - you can only open them.  But I take your
> point.  In another life, these things would appear under a special
> /proc/magical_directory_which_has_dopey_semantics.
> 
> > A program that only wants
> > to show "processes" (thread-group leaders) can simply read
> > /proc/PID/status and ignore the entries for which Tgid != PPid.
> > 
> > Perhaps you could put relative symlinks in task/?  Something like
> > this:
> > 
> >  $ ls -l /proc/self/task
> >  dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13494 -> ..
> >  dr-xr-xr-x    3 davidm   users           0 Feb 26 11:37 13495 -> ../../13495
> > 
> > perhaps?
> 
> Well the contents of /proc/pid/task are OK at present.
> 
> I guess we should revert that change.

Ah, so there was some fundamental reason behind that behaviour!
Perhaps then a comment or two in the code to explain that such
behaviour (prior to change) is intended in proc_pid_lookup()?  That
way it will be clear that is intended behaviour.

Am I correct to assume though that the corresponding change in
proc_task_lookup() should stay?  The existing behaviour there was that
one could do say,

cat /proc/<pid>/task/<tid>/stat, where tid could be any thread and not
a part of the thread group pid.  

The patch that broke backwards compatibility for GDB likewise changed
that.  It enforces that tid must be a part of the pid thread group.

--
		Kingsley