Re: [PATCH v2 4/7] Test case for gdb.thread_from_thread_handle

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-04-09 02:07, Kevin Buettner wrote:
> As the title says, this is a test case for
> gdb.thread_from_thread_handle, a python function which will, given a
> thread library dependent thread handle, find the GDB thread which
> corresponds to that thread handle.
>
> The C file for this test case causes the thread handles for the
> main thread and two child threads to be placed into an array.  The
> test case runs to one of the functions (do_something()) at which point,
> it retrieves the thread handles from the array and attempts to find the
> correponding thread in GDB's internal thread list.
>
> I use a barrier to make sure that both threads have actually started;
> execution will stop when one of the threads breaks at do_something.
>
> The one concern I have about what I've written is with the last three
> invocations of gdb_test.  I don't know that we can be certain that
> thrs[1] will always map to GDB thread 2 and that thrs[2] will map to
> GDB thread 3.  It seems likely, but some perverse pthreads 
> implementation
> could change the order in which newly created threads are actually 
> started.
> If anyone thinks this is a problem, I can tweak it so that the test 
> case
> simply verifies that reasonable output is produced and another test can
> verify that the two child thread numbers are actually different.

One case I can think of where thread numbers could be unstable is with a 
remote target.  AFAIK, new threads are only reported to GDB when there 
is a stop.  Depending on the remote target implementation, it could be 
possible that the order in which it reports the threads changes.

When I test the full series with --target_board=native-gdbserver, I 
actually see it.  I added an "info threads" in the test:

  1    Thread 27095.27095 "py-thrhandle" 0x00007ffff7bc457d in 
pthread_join () from target:/usr/lib/libpthread.so.0^M
* 2    Thread 27095.27100 "py-thrhandle" do_something (n=2) at 
/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-thrhandle.c:32^M
  3    Thread 27095.27099 "py-thrhandle" do_something (n=1) at 
/home/simark/src/binutils-gdb/gdb/testsuite/gdb.python/py-thrhandle.c:32^M

And I see FAILs for thrs[1] and [2].

I think a good fix would be to force the thread numbering to be stable 
by hitting a breakpoint after each thread start.  That would force the 
target to report the threads in the order they really appear.  That 
should be done easily by having a barrier of 2, to be hit by the main 
thread and the newly-spawned thread.  You can have a breakpoint in main 
just after that barrier, to which you "continue" as many times as there 
are threads.  You would need to make sure that the previously started 
threads don't exit while you're busy starting other threads, so you can 
make them sleep(30) for example.

> gdb/testsuite/ChangeLog:
>
>     	* gdb.python/py-thrhandle.c, gdb.python/py-thrhandle.exp: New
>     	files.
> ---
>  gdb/testsuite/gdb.python/py-thrhandle.c   | 76 
> +++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.python/py-thrhandle.exp | 52 +++++++++++++++++++++
>  2 files changed, 128 insertions(+)
>
> diff --git a/gdb/testsuite/gdb.python/py-thrhandle.c
> b/gdb/testsuite/gdb.python/py-thrhandle.c
> new file mode 100644
> index 0000000..93d7dee
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-thrhandle.c
> @@ -0,0 +1,76 @@
> +/* This testcase is part of GDB, the GNU debugger.
> +
> +   Copyright 2016 Free Software Foundation, Inc.

2017?

> +
> +   This program is free software; you can redistribute it and/or 
> modify
> +   it under the terms of the GNU General Public License as published 
> by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see  
> <http://www.gnu.org/licenses/>.  */
> +
> +#include <stdio.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +#include <alloca.h>
> +#include <memory.h>
> +
> +#define NTHR 3
> +pthread_t thrs[NTHR+2];

It was not clear to me why the +2.  Maybe you could split it in two 
arrays, threads and invalid_threads?

> +pthread_barrier_t barrier;
> +pthread_mutex_t mutex;
> +
> +void
> +do_something (int n)
> +{
> +  pthread_mutex_lock (&mutex);
> +  printf ("%d\n", n);
> +  pthread_mutex_unlock (&mutex);

Is this mutex (and printf) actually useful?  I think it would be better 
to keep the test program as simple as it can be.  Getting rid of the 
printf  would also allow getting rid of the alloca in main.

> +}
> +
> +void *
> +do_work (void *data)
> +{
> +  int num = * (int *) data;
> +
> +  pthread_barrier_wait (&barrier);
> +
> +  do_something (num);
> +
> +  pthread_exit (NULL);
> +}
> +
> +int
> +main (int argc, char **argv)
> +{
> +  int i;
> +
> +  pthread_barrier_init (&barrier, NULL, NTHR-1);
> +  pthread_mutex_init (&mutex, NULL);
> +
> +  thrs[0] = pthread_self ();
> +
> +  for (i=1; i< NTHR; i++)

We try to respect the C coding style even for tests, so here that would 
mean spaces around the binary operators ...

> +    {
> +      int *iptr = alloca (sizeof (int));
> +
> +      *iptr = i;
> +      pthread_create (&thrs[i], NULL, do_work, iptr);
> +    }
> +
> +  /* Create two bogus thread handles.  */
> +  memset (&thrs[NTHR], 0, sizeof (pthread_t));
> +  memset (&thrs[NTHR+1], 0xaa, sizeof (pthread_t));

... and here too I guess ...

> +
> +  for (i=1; i< NTHR; i++)

... and here too.  You can also remove the curly braces.

> +    {
> +      pthread_join (thrs[i], NULL);
> +    }
> +  printf ("Done!");
> +}
> diff --git a/gdb/testsuite/gdb.python/py-thrhandle.exp
> b/gdb/testsuite/gdb.python/py-thrhandle.exp
> new file mode 100644
> index 0000000..d542734
> --- /dev/null
> +++ b/gdb/testsuite/gdb.python/py-thrhandle.exp
> @@ -0,0 +1,52 @@
> +# Copyright (C) 2016 Free Software Foundation, Inc.

2017?

> +
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see 
> <http://www.gnu.org/licenses/>.
> +
> +# Please email any bugs, comments, and/or additions to this file to:
> +# bug-gdb@gnu.org
> +
> +# This file verifies that gdb.thread_from_thread_handle works as 
> expected.
> +
> +standard_testfile
> +
> +
> +if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}"
> "${binfile}" executable debug] != "" } {
> +    return -1
> +}
> +
> +clean_restart ${binfile}
> +runto_main
> +
> +gdb_test "break do_something" \
> +    "Breakpoint 2 at .*: file .*${srcfile}, line .*" \
> +         "breakpoint on do_something"

The indentation look inconsistent between these two lines.

Note that you can use gdb_breakpoint also.

> +
> +gdb_test "continue" \
> +	"Breakpoint 2, do_something .*" \
> +	"run to do_something"

gdb_continue_to_breakpoint can be useful here.

> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[0\]')).num" \
> +	"1"

Please use parenthesis with print so that the test works with Python 3.

> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[1\]')).num" \
> +	"2"
> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[2\]')).num" \
> +	"3"
> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[3\]'))" \
> +	"None"
> +
> +gdb_test "python print
> gdb.thread_from_thread_handle(gdb.parse_and_eval('thrs\[4\]'))" \
> +	"None"

Something I realized while testing is that the assert in 
linux-thread-db.c is not ideal, since the user can input any kind of 
value through Python:

> > > print(gdb.thread_from_thread_handle(gdb.parse_and_eval('2')).num)
/home/simark/src/binutils-gdb/gdb/linux-thread-db.c:1423: 
internal-error: thread_info* 
thread_db_thread_handle_to_thread_info(target_ops*, const gdb_byte*, 
int): Assertion `handle_len == sizeof (handle_tid)' failed.
A problem internal to GDB has been detected,
further debugging may prove unreliable.
Quit this debugging session? (y or n)

So perhaps returning NULL would be a better choice if the length doesn't 
match.

While on the subject, instead of passing a buffer and a length to the 
target method, how about passing the struct value directly and checking 
if the type is the right one (pthread_t in this case), returning NULL if 
it isn't?

Thanks,

Simon