[commit/RFA] Avoid switch to invalid ptid during Ada task switch.

[commit/RFA] Avoid switch to invalid ptid during Ada task switch.

[Joel Brobecker]

Hello,

This is a followup on something I mentioned in an earlier email:

    http://www.sourceware.org/ml/gdb-patches/2010-03/msg00272.html

The goal is to prevent an internal error during an Ada task switch. A task
switch is simply a thread switch under the hood. What we do is collect
the info from the Ada Task Control Block, deduce the associated thread
ptid, and then switch to that thread.  If the thread ptid computation
routine has not been implemented for the target, of if there is a bug,
then we end up computing a bogus ptid which GDB does not know about,
which eventually leads to an assertion failure:

    (gdb) task 1
    [New Thread 5715]
    /[...]/gdb/thread.c:595: internal-error: is_thread_state:
     Assertion `tp' failed.
    A problem internal to GDB has been detected,
    further debugging may prove unreliable.
    Quit this debugging session? (y or n)

When this happens, it's just nicer for the user to print an error
message, and cancel the task switch. After this patch is applied,
this is what we get:

    (gdb) task 1
    [New Thread 10250]
    Unable to compute thread ID for task 1.
    Cannot switch to this task.

What do you guys think?  Is the wording OK?

gdb/ChangeLog:

        * ada-tasks.c (task_command_1): Check that the task ptid is valid
        before doing the associated thread switch.

Tested on x86_64-linux. No regression.

I was thinking of applying this to head, and maybe the branch as well.
But I'll wait for a couple of days in care there are some suggestions.

---
 gdb/ada-tasks.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/gdb/ada-tasks.c b/gdb/ada-tasks.c
index be9b8e6..519cfc6 100644
--- a/gdb/ada-tasks.c
+++ b/gdb/ada-tasks.c
@@ -877,6 +877,19 @@ task_command_1 (char *taskno_str, int from_tty)
      to the thread list.  */
   target_find_new_threads ();
 
+  /* Verify that the ptid of the task we want to switch to is valid
+     (in other words, a ptid that GDB knows about).  Otherwise, we will
+     cause an assertion failure later on, when we try to determine
+     the ptid associated thread_info data.  We should normally never
+     encounter such an error, but the wrong ptid can actually easily be
+     computed if target_get_ada_task_ptid has not been implemented for
+     our target (yet).  Rather than cause an assertion error in that case,
+     it's nicer for the user to just refuse to perform the task switch.  */
+  if (!find_thread_ptid (task_info->ptid))
+    error (_("Unable to compute thread ID for task %d.\n"
+             "Cannot switch to this task."),
+           taskno);
+
   switch_to_thread (task_info->ptid);
   ada_find_printable_frame (get_selected_frame (NULL));
   printf_filtered (_("[Switching to task %d]\n"), taskno);
-- 
1.6.3.3

Re: [commit/RFA] Avoid switch to invalid ptid during Ada task switch.

[Kevin Buettner]

On Sun,  7 Mar 2010 19:25:29 +0400
Joel Brobecker <brobecker@adacore.com> wrote:

> When this happens, it's just nicer for the user to print an error
> message, and cancel the task switch. After this patch is applied,
> this is what we get:
> 
>     (gdb) task 1
>     [New Thread 10250]
>     Unable to compute thread ID for task 1.
>     Cannot switch to this task.
> 
> What do you guys think?  Is the wording OK?

I think that the wording that you added is fine, though I find it
confusing to see that there's a new thread, [New Thread 10250], but
that GDB is unable to switch to it.  I think it might be less
confusing if the [New Thread...] message did not appear at all.  I'm
guessing though that suppressing that message would not be easy, and
probably not worthwhile, especially for a case that's not supposed to
happen.

Kevin

Re: [commit/RFA] Avoid switch to invalid ptid during Ada task switch.

[Joel Brobecker]

> I think that the wording that you added is fine, though I find it
> confusing to see that there's a new thread, [New Thread 10250], but
> that GDB is unable to switch to it.  I think it might be less
> confusing if the [New Thread...] message did not appear at all.  I'm
> guessing though that suppressing that message would not be easy, and
> probably not worthwhile, especially for a case that's not supposed to
> happen.

Thanks for the feedback.  About that new-thread message, I don't think
it's avoidable.  On some targets, we "discover" threads only on demand.
What I mean by that is that the kernel does not send a notification
of some kind when a new thread is created or destroyed. It's GDB that
has to actively go look for new threads (target_ops to_find_new_threads).
GDB does not do this update systematically every time we stop - it would
be very expensive. So GDB it only does so when it needs to access that
thread list. For instance, when the user requests the list of threads.

Using the same example I used in my first message, here is the output
I get if I use "info threads" rather than try to perform a task switch:

    (gdb) info threads
    [New Thread 7598]
      3 Thread 7598  0x00007f865d88e5a9 in pthread_cond_wait@@GLIBC_2.3.2 ()
       from /lib/libpthread.so.0
    * 2 Thread 7599  task_switch.break_me () at task_switch.adb:44
      1 Thread 7592  0x00007f865d88e5a9 in pthread_cond_wait@@GLIBC_2.3.2 ()
       from /lib/libpthread.so.0

And if I only now try to perform the task switch, I only get the new
error message:

    (gdb) task 1
    Unable to compute thread ID for task 1.
    Cannot switch to this task.

(no new-thread notification).

-- 
Joel

Re: [commit/RFA] Avoid switch to invalid ptid during Ada task switch.

[Joel Brobecker]

Hello,

> gdb/ChangeLog:
> 
>         * ada-tasks.c (task_command_1): Check that the task ptid is valid
>         before doing the associated thread switch.

FYI: I just checked this in. (head only)

-- 
Joel