Re: [patch 2/3] attach-fail-reasons: Say more than ptrace: Operation not permitted.

Pedro Alves
2012-03-13T10:33:40Z

On 03/13/2012 09:43 AM, Jan Kratochvil wrote:

> On Thu, 08 Mar 2012 14:55:22 +0100, Pedro Alves wrote:
>>> +extern pid_t linux_proc_get_tracerpid (int lwpid);
>> ..
>>
>> Would be neater with `pid_t lwpid'.
>
> I chose 'int' specifically for you as this is TID, not PID and specifically

That's flattering. :-)

> you typically use 'int' instead of 'pid_t' in your patches / in gdbserver,

I use int in my patches where I'm touching code that is already using int everywhere.
If I'm touching code that is already using `int' everywhere, I'll make my code use
it too. `int' is what the codebase uses and was already using before I ever
laid my hands on it. Your original code had:

+void
+linux_ptrace_attach_warnings (pid_t pid)
^^^^^^^^^
+{
+ pid_t tracerpid;
+
+ tracerpid = linux_proc_get_tracerpid (pid);
^^^

So the caller had a pid_t handy.

+pid_t
+linux_proc_get_tracerpid (int lwpid)
+{

And this returns pid_t, but actually takes int, although the only caller
has a pid_t handy. It just seems neater to be all around consistent
here. That's all.

> where pid_t is available. I do not address this in reviews as I find it as
> a real nitpick.

I'll appreciate the nits if they're pointing at a similar inconsistency. I'd
never say anything if int's were being used in the surrounding code. Anyway, lots of
words for a handful of characters. :-)

> OK, so changed it to pid_t, thanks.

Thanks.

>> linux_ptrace_attach_warnings has a tiny race, in that by the time you open /proc/pid/... you
>> may be looking at a different process. There's no way to fix that, I think, and this
>> will be right in the large majority of the cases, so we can just live with it, IMO.
>
> I agree, this is affecting all the /proc operations.

Yeah, although if you're already attached to the process, then the risk of /proc/..
hitting the wrong pid is much lower, though it does exist. One way I've considered
to minimize that further down to practically zero, was to open /proc/PID/task/TID/...
instead of /proc/PID/... in most places. It'd like the /proc equivalent of tgkill
(where Linux's tgkill was invented precisely to avoid such races).

> As the code examines

> /proc only in the case the attachment failed so any races have only
> informational impact.

>
>
>> You may have considered the following already, but I haven't seen it mentioned. I think it
>> may be better if the new warning strings are actually part of the thrown error string:
>
> I fully agree, IIRC I considered it a bit but:
>
> (1) Plain C code dealing with dynamic strings in gdbserver has to be ugly.

There's common/buffer.[h|c] for that. It's like a very simple obstack. I wrote it
initially for gdbserver, exactly to avoid pulling in libiberty. At some point, we'll
probably end up eliminating it, but it's there now. I'm okay with your new version though.

> (2b) If introducing a new framework to gdbserver code we should bring in STL

> and not the libiberty/gdb code. But we cannot yet use STL.

Although I'm a C++ supporter, I really don't think such C++ arguments hold
much weight for the current C based code base.

> gdb/
> 2012-03-13 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * common/linux-procfs.c (linux_proc_get_int): New, from
> linux_proc_get_tgid, change its LWPID type to pid_t, add parameter
> field.
> (linux_proc_get_tgid): Only call linux_proc_get_int.
> (linux_proc_get_tracerpid): New.
> (linux_proc_pid_has_state): New, from linux_proc_pid_is_zombie.
> (linux_proc_pid_is_stopped, linux_proc_pid_is_zombie): Only call
> linux_proc_pid_has_state.
> * common/linux-procfs.h (linux_proc_get_tracerpid): New declaration.
> * common/linux-ptrace.c: Include linux-procfs.h.
> (linux_ptrace_attach_warnings): New.
> * common/linux-ptrace.h (linux_ptrace_attach_warnings): New declaration.
> * linux-nat.c: Include exceptions.h and linux-ptrace.h.
> (linux_nat_attach): New variables ex and message. Wrap to_attach by
> TRY_CATCH and call linux_ptrace_attach_warnings.
>
> gdb/gdbserver/
> 2012-03-13 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * linux-low.c (linux_attach_lwp_1): New variable message. Call
> linux_ptrace_attach_warnings.
>
> gdb/testsuite/
> 2012-03-06 Jan Kratochvil <jan.kratochvil@redhat.com>
>
> * gdb.base/attach-twice.c: New files.
> * gdb.base/attach-twice.exp: New files.

Looks good to me. Thanks.

--
Pedro Alves