New feature: allow thread command to take a LWPID.

New feature: allow thread command to take a LWPID.

[scott.harrison]

[-- Attachment #1: Type: text/plain, Size: 983 bytes --]

All,

We script GDB to attach to our application when it crashes and generate a 
crash report giving lots of valuable information for developers to debug 
the problem.  Before invoking GDB we know the LWPID and the PID so we 
attach to the PID (--pid), however, currently GDB only knows about its own 
threadids, we don't know this before invoking GDB but our script is already 
written, so I have created a small patch to the "thread" command to take a 
%<LWPID> option, and swtich to that thread.  We thought you might be 
interested.

Changelog
---------

2010-02-04  Scott Harrison <scott.harrison@tandberg.com>

	* gdb/thread.c: Add support for LWPID to the thread command.


Patch is attached.  I realise that this patch may not be applicable to all 
platforms that gdb is used on, sorry.

Kind regards,

Scott Harrison

---
TANDBERG Telecom UK Ltd
Registered in England and Wales No: 3390345. Registered address: Unit 2
Pine Trees, Chertsey Lane, Staines, Middlesex, TW18 3HR

[-- Attachment #2: gdb-tid.patch --]
[-- Type: text/plain, Size: 1075 bytes --]

diff -Nru gdb-7.0.1/gdb/thread.c gdb-7.0.1-1/gdb/thread.c
--- gdb-7.0.1/gdb/thread.c	2009-09-13 17:28:28.000000000 +0100
+++ gdb-7.0.1-1/gdb/thread.c	2010-02-03 17:01:26.000000000 +0000
@@ -311,6 +311,18 @@
   return NULL;
 }
 
+struct thread_info *
+find_thread_lwp (int num)
+{
+  struct thread_info *tp;
+
+  for (tp = thread_list; tp; tp = tp->next)
+    if (tp->ptid.lwp == num)
+      return tp;
+
+  return NULL;
+}
+
 /* Find a thread_info by matching PTID.  */
 struct thread_info *
 find_thread_ptid (ptid_t ptid)
@@ -1164,11 +1176,19 @@
 do_captured_thread_select (struct ui_out *uiout, void *tidstr)
 {
   int num;
+  char *tidstring=(char *)tidstr;
   struct thread_info *tp;
 
-  num = value_as_long (parse_and_eval (tidstr));
-
-  tp = find_thread_id (num);
+  if (tidstring[0] == '%')
+  {
+    num = value_as_long (parse_and_eval ((void *)(tidstring+1)));
+    tp = find_thread_lwp (num);
+  }
+  else
+  {
+    num = value_as_long (parse_and_eval (tidstr));
+    tp = find_thread_id (num);
+  }
 
   if (!tp)
     error (_("Thread ID %d not known."), num);

Re: New feature: allow thread command to take a LWPID.

[Tom Tromey]

>>>>> "scott" == scott harrison <scott.harrison@tandberg.com> writes:

Scott> Before invoking GDB we know the LWPID and the PID so we attach to
Scott> the PID (--pid), however, currently GDB only knows about its own
Scott> threadids, we don't know this before invoking GDB but our script
Scott> is already written, so I have created a small patch to the
Scott> "thread" command to take a %<LWPID> option, and swtich to that
Scott> thread.

This seems like a reasonable idea.

I am not super fond of the syntax.  I don't have a better suggestion
though :-(

Scott> Patch is attached.  I realise that this patch may not be applicable to
Scott> all platforms that gdb is used on, sorry.

Do you have a copyright assignment in place?  We'll need one before we
can incorporate this.  If you don't, send me email off-list and I will
get you started.

Mini review:

Scott> +struct thread_info *
Scott> +find_thread_lwp (int num)

New functions should have a header comment describing their purpose,
arguments, and return.  And, this one should be static.

Scott> +    if (tp->ptid.lwp == num)
Scott> +      return tp;

lwp is an long but the `num' argument is an int.  That seems wrong.

Scott> +  char *tidstring=(char *)tidstr;

In the GNU style, there are spaces around operators.  In this case you
also don't need the cast.  So it would look like:

    char *tidstring = tidstr;

Scott> +  if (tidstring[0] == '%')
Scott> +  {

Brace indented two more spaces, then the body two beyond that.

Scott> +    num = value_as_long (parse_and_eval ((void *)(tidstring+1)));

Spaces.  You should probably introduce a new local of 'long' type here.
You don't need the cast to void*, because parse_and_eval actually takes
a char* argument.

thanks,
Tom

Re: New feature: allow thread command to take a LWPID.

[Joel Brobecker]

> I am not super fond of the syntax.  I don't have a better suggestion
> though :-(

I agree about the syntax. How about using a command "switch".
For instance:

    (gdb) thread THREAD_NO
    (gdb) thread /l LWP

We could even think of adding:

    (gdb) thread /t TID

(for the platforms that use the tid rather than the lwp).

-- 
Joel

Re: New feature: allow thread command to take a LWPID.

[scott.harrison]

Joel,

Is there any existing, generic code that I can utilise to do this ?

Regards,

Scott.

On Fri, Feb 05, 2010 at 07:28:01AM +0400, Joel Brobecker wrote:
>> I am not super fond of the syntax.  I don't have a better suggestion
>> though :-(
>
>I agree about the syntax. How about using a command "switch".
>For instance:
>
>    (gdb) thread THREAD_NO
>    (gdb) thread /l LWP
>
>We could even think of adding:
>
>    (gdb) thread /t TID
>
>(for the platforms that use the tid rather than the lwp).
>
>--
>Joel

-- 
TANDBERG Telecom UK Ltd
Registered in England and Wales No: 3390345. Registered address: Unit 2
Pine Trees, Chertsey Lane, Staines, Middlesex, TW18 3HR

Re: New feature: allow thread command to take a LWPID.

[Joel Brobecker]

> Is there any existing, generic code that I can utilise to do this ?

Yes, you can use gdb_buildargv to parse the arguments.

-- 
Joel

Re: New feature: allow thread command to take a LWPID.

[Andreas Schwab]

Joel Brobecker <brobecker@adacore.com> writes:

>> I am not super fond of the syntax.  I don't have a better suggestion
>> though :-(
>
> I agree about the syntax. How about using a command "switch".
> For instance:
>
>     (gdb) thread THREAD_NO
>     (gdb) thread /l LWP

      (gdb) thread lwp LWP

> We could even think of adding:
>
>     (gdb) thread /t TID

      (gdb) thread tid TID

Andreas.

-- 
Andreas Schwab, schwab@redhat.com
GPG Key fingerprint = D4E8 DBE3 3813 BB5D FA84  5EC7 45C6 250E 6F00 984E
"And now for something completely different."

Re: New feature: allow thread command to take a LWPID.

[Joel Brobecker]

> >     (gdb) thread /t TID
> 
>       (gdb) thread tid TID

FWIW, this is equally fine as far as I am concerned.

-- 
Joel

Re: New feature: allow thread command to take a LWPID.

[Tom Tromey]

>>>>> "Joel" == Joel Brobecker <brobecker@adacore.com> writes:

>> >     (gdb) thread /t TID
>> 
>> (gdb) thread tid TID

Joel> FWIW, this is equally fine as far as I am concerned.

Hah, I have no idea why I didn't think of a flag or a subcommand :-)

I think the "/t" form is mostly used for print formats, isn't it?  And
we either use subcommands or the "-switch" in other cases?  Though I
wouldn't be surprised to find some inconsistencies.

Tom

Re: New feature: allow thread command to take a LWPID.

[Joel Brobecker]

> I think the "/t" form is mostly used for print formats, isn't it?  And
> we either use subcommands or the "-switch" in other cases?  Though I
> wouldn't be surprised to find some inconsistencies.

I used the "find" command as the model when coming up with the /t.
I can't remember commands that use -switch, although I'm sure it'll
come back. I agree that we've probably been inconsistent anyway.

That being said, it's not terribly important to me which approach
we choose. I would prefer, however, if we avoided the %lwp syntax,
though.

-- 
Joel