[PATCH v3 4/7] Per-inferior/Inferior-qualified thread IDs
Simon Marchi
simon.marchi@ericsson.com
Thu Jan 7 22:28:00 GMT 2016
On 16-01-07 02:45 PM, Pedro Alves wrote:
>> Also, it might be good to mention what the stop condition for the iteration
>> is. Maybe as a simple usage example?
>
> I tried to do that now.
I think it's clear how you phrased it.
>> IIUC, the value of thr_end defines if you want the to get a single thread id,
>> or a range. Should it be mentioned here?
>
> I was leaving that as an internal implementation detail, didn't want to
> have callers rely on that. I "factored out" a helper function instead
> now, and added assertions tid_range_parser_get_tid_range.
It looks nice. A few comments below.
>> windows-tdep.c fails to build with this patch. There is still one usage of find_thread_id:
>>
>> /home/emaisin/src/binutils-gdb/gdb/windows-tdep.c: In function ‘display_tib’:
>> /home/emaisin/src/binutils-gdb/gdb/windows-tdep.c:378:7: error: implicit declaration of function ‘find_thread_id’ [-Werror=implicit-function-declaration]
>> tp = find_thread_id (gdb_id);
>> ^
>> /home/emaisin/src/binutils-gdb/gdb/windows-tdep.c:378:10: error: assignment makes pointer from integer without a cast [-Werror]
>> tp = find_thread_id (gdb_id);
>> ^
>> cc1: all warnings being treated as errors
>> make: *** [windows-tdep.o] Error 1
>
> Whoops, forgot --enable-targets=all...
>
> But, looking at the code, that's parsing a GDB thread ID, while the comment
> says the argument is supposed to be a Windows thread ID, not a GDB ID...
> This argument doesn't seem to be documented in either the manual or
> gdb's online help. It's not hard to fix this, but maybe we can just
> get rid of it? There's always "thread apply TID info w32 tib".
> Pierre, what do you think?
I was also confused by the comment there. I don't mind how this is resolved :).
> Here are the changes addressing your review comments.
>
> From d2f3387ebbed744da6d2e686ebc1f72f94a7e328 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Thu, 7 Jan 2016 19:26:07 +0000
> Subject: [PATCH] review
>
> ---
> gdb/testsuite/gdb.multi/tids.c | 4 +++
> gdb/testsuite/gdb.multi/tids.exp | 10 ++++----
> gdb/tid-parse.c | 30 +++++++++++++++++-----
> gdb/tid-parse.h | 55 +++++++++++++++++++++++++++-------------
> gdb/windows-tdep.c | 23 ++---------------
> 5 files changed, 72 insertions(+), 50 deletions(-)
>
> diff --git a/gdb/testsuite/gdb.multi/tids.c b/gdb/testsuite/gdb.multi/tids.c
> index 00a8298..84f0c56 100644
> --- a/gdb/testsuite/gdb.multi/tids.c
> +++ b/gdb/testsuite/gdb.multi/tids.c
> @@ -25,6 +25,8 @@ thread_function2 (void *arg)
> {
> while (1)
> sleep (1);
> +
> + return NULL;
> }
>
> void *
> @@ -34,6 +36,8 @@ thread_function1 (void *arg)
>
> while (1)
> sleep (1);
> +
> + return NULL;
> }
>
> int
> diff --git a/gdb/testsuite/gdb.multi/tids.exp b/gdb/testsuite/gdb.multi/tids.exp
> index 7b51c80..33f8144 100644
> --- a/gdb/testsuite/gdb.multi/tids.exp
> +++ b/gdb/testsuite/gdb.multi/tids.exp
> @@ -37,8 +37,8 @@ if { ![runto_main] } then {
> return -1
> }
>
> -# Issue "thread apply TID_LIST p 1" and expect EXP_TID_LIST (a list of
> -# thread ids) to be displayed.
> +# Issue "thread apply TID_LIST p 1234" and expect EXP_TID_LIST (a list
> +# of thread ids) to be displayed.
> proc thread_apply {tid_list exp_tid_list {message ""}} {
> global decimal
> set any "\[^\r\n\]*"
> @@ -72,7 +72,7 @@ proc info_threads {tid_list exp_tid_list {message ""}} {
> gdb_test $cmd $r $message
> }
>
> -# Issue both "info threads TID_LIST" and expect INFO_THR output. Then
> +# Issue "info threads TID_LIST" and expect INFO_THR output. Then
> # issue "thread apply TID_LIST" and expect THR_APPLY output. If
> # THR_APPLY is omitted, INFO_THR is expected instead.
> proc thr_apply_info_thr {tid_list info_thr {thr_apply ""}} {
> @@ -122,7 +122,7 @@ with_test_prefix "two inferiors" {
> # Add another inferior.
> gdb_test "add-inferior" "Added inferior 2.*" "add empty inferior 2"
>
> - # Now that we'd added another inferior, thread IDs now show the
> + # Now that we've added another inferior, thread IDs now show the
> # inferior number.
> info_threads "" "1.1"
>
> @@ -133,7 +133,7 @@ with_test_prefix "two inferiors" {
>
> runto_main
>
> - # Now that we'd added another inferior, thread IDs now show the
> + # Now that we've added another inferior, thread IDs now show the
> # inferior number.
> info_threads "" "1.1 2.1" \
> "info threads show inferior numbers"
> diff --git a/gdb/tid-parse.c b/gdb/tid-parse.c
> index 7c30733..500c99d 100644
> --- a/gdb/tid-parse.c
> +++ b/gdb/tid-parse.c
> @@ -159,11 +159,14 @@ tid_range_parser_qualified (struct tid_range_parser *parser)
> return parser->qualified;
> }
>
> -/* See tid-parse.h. */
> -
> -int
> -tid_range_parser_get_tid_range (struct tid_range_parser *parser, int *inf_num,
> - int *thr_start, int *thr_end)
> +/* Helper for tid_range_parser_get_tid and
> + tid_range_parser_get_tid_range. Basically like
> + tid_range_parser_get_tid_range, bue if THR_END is NULL returns a
bue -> but
I think you can drop
"Basically like tid_range_parser_get_tid_range"
and just say:
"Return the next range if THR_END is non-NULL, return a single thread ID otherwise."
> + single thread ID per call, rather than a range. */
> +
> +static int
> +get_tid_or_range (struct tid_range_parser *parser, int *inf_num,
> + int *thr_start, int *thr_end)
> {
> if (parser->state == TID_RANGE_STATE_INFERIOR)
> {
> @@ -233,11 +236,24 @@ tid_range_parser_get_tid_range (struct tid_range_parser *parser, int *inf_num,
>
> /* See tid-parse.h. */
>
> -void
> +int
> +tid_range_parser_get_tid_range (struct tid_range_parser *parser, int *inf_num,
> + int *thr_start, int *thr_end)
> +{
> + gdb_assert (inf_num != NULL && thr_start != NULL && thr_end != NULL);
> +
> + get_tid_or_range (parser, inf_num, thr_start, thr_end);
Missing "return".
> +}
> +
> +/* See tid-parse.h. */
> +
> +int
> tid_range_parser_get_tid (struct tid_range_parser *parser,
> int *inf_num, int *thr_num)
> {
> - tid_range_parser_get_tid_range (parser, inf_num, thr_num, NULL);
> + gdb_assert (inf_num != NULL && thr_start != NULL);
s/thr_start/thr_num/
> +
> + return get_tid_range (parser, inf_num, thr_num, NULL);
s/get_tid_range/get_tid_or_range/
> }
>
> /* See tid-parse.h. */
> diff --git a/gdb/tid-parse.h b/gdb/tid-parse.h
> index 3c79c08..8f963cd 100644
> --- a/gdb/tid-parse.h
> +++ b/gdb/tid-parse.h
> @@ -85,34 +85,55 @@ extern void tid_range_parser_init (struct tid_range_parser *parser,
> /* Parse a thread ID or a thread range list.
>
> A range will be of the form
> - <inferior_num>.<thread_number1>-<thread_number1> and will represent
> - all the threads of inferior inferior_num with number between
> - thread_number1 and thread_number2, inclusive. <inferior_num> can
> - also be omitted, as in <thread_number1>-<thread_number1>, in which
> - case GDB infers the inferior number from the current inferior.
>
> - While processing a thread ID range list, this function is called
> - iteratively; at each call it will return (in the INF_NUM and
> - THR_NUM output parameters) the next thread in the range.
> + <inferior_num>.<thread_number1>-<thread_number2>
> +
> + and will represent all the threads of inferior INFERIOR_NUM with
> + number between THREAD_NUMBER1 and THREAD_NUMBER2, inclusive.
> + <inferior_num> can also be omitted, as in
> +
> + <thread_number1>-<thread_number2>
> +
> + in which case GDB infers the inferior number from the current
> + inferior.
See my other message about current inferior vs default inferior.
> + This function is designed to be called iteratively. While
> + processing a thread ID range list, at each call it will return (in
> + the INF_NUM and THR_NUM output parameters) the next thread ID in
> + the range (irrespective of whether the thread actually exists).
>
> At the beginning of parsing a thread range, the char pointer
> PARSER->string will be advanced past <thread_number1> and left
> pointing at the '-' token. Subsequent calls will not advance the
> pointer until the range is completed. The call that completes the
> - range will advance the pointer past <thread_number2>. */
> -extern void tid_range_parser_get_tid (struct tid_range_parser *parser,
> + range will advance the pointer past <thread_number2>.
> +
> + This function advances through the input string for as long you
> + call it. Once the end of the input string is reached, a call to
> + tid_range_parser_finished returns false (see below).
> +
> + E.g., with list: "1.2 3.4-6":
> +
> + 1st call: *INF_NUM=1; *THR_NUM=2 (finished==0)
> + 2nd call: *INF_NUM=3; *THR_NUM=4 (finished==0)
> + 3rd call: *INF_NUM=3; *THR_NUM=5 (finished==0)
> + 4th call: *INF_NUM=3; *THR_NUM=6 (finished==1)
> +
> + Returns true if parsed a thread/range successfully, false
> + otherwise. */
> +extern int tid_range_parser_get_tid (struct tid_range_parser *parser,
> int *inf_num, int *thr_num);
>
> -/* Like tid_range_parser_get_tid, but return a thread range per call,
> - rather then a single thread.
> +/* Like tid_range_parser_get_tid, but return a thread ID range per
> + call, rather then a single thread ID.
>
> If the next element in the list is a single thread ID, then
> - *THR_START and *THR_END are set to the same value. E.g.:
> + *THR_START and *THR_END are set to the same value.
>
> - list: "1.2 3.4-6"
> - 1st call: *INF_NUM=1; *THR_START=2; *THR_END=2
> - 2nd call: *INF_NUM=3; *THR_START=4; *THR_END=4
> + E.g.,. with list: "1.2 3.4-6"
>
> + 1st call: *INF_NUM=1; *THR_START=2; *THR_END=2 (finished==0)
> + 2nd call: *INF_NUM=3; *THR_START=4; *THR_END=6 (finished==1)
>
> Returns true if parsed a thread/range successfully, false
> otherwise. */
> @@ -135,7 +156,7 @@ extern void tid_range_parser_skip (struct tid_range_parser *parser);
> extern int tid_range_parser_qualified (struct tid_range_parser *parser);
>
> /* Accept a string-form list of thread IDs such as is accepted by
> - tid_range_parser_get_tid. Return TRUE if the INF_NUM.THR.NUM
> + tid_range_parser_get_tid. Return true if the INF_NUM.THR.NUM
> thread is in the list. DEFAULT_INFERIOR is the inferior number to
> assume if a non-qualified thread ID is found in the list.
>
> diff --git a/gdb/windows-tdep.c b/gdb/windows-tdep.c
> index dd8085f..cddbf23 100644
> --- a/gdb/windows-tdep.c
> +++ b/gdb/windows-tdep.c
> @@ -361,31 +361,12 @@ display_one_tib (ptid_t ptid)
> return 1;
> }
>
> -/* Display thread information block of a thread specified by ARGS.
> - If ARGS is empty, display thread information block of current_thread
> - if current_thread is non NULL.
> - Otherwise ARGS is parsed and converted to a integer that should
> - be the windows ThreadID (not the internal GDB thread ID). */
> +/* Display thread information block of the current thread. */
>
> static void
> display_tib (char * args, int from_tty)
> {
> - if (args)
> - {
> - struct thread_info *tp;
> - int gdb_id = value_as_long (parse_and_eval (args));
> -
> - tp = find_thread_id (gdb_id);
> -
> - if (!tp)
> - error (_("Thread ID %d not known."), gdb_id);
> -
> - if (!target_thread_alive (tp->ptid))
> - error (_("Thread ID %d has terminated."), gdb_id);
> -
> - display_one_tib (tp->ptid);
> - }
> - else if (!ptid_equal (inferior_ptid, null_ptid))
> + if (!ptid_equal (inferior_ptid, null_ptid))
> display_one_tib (inferior_ptid);
> }
>
>
Otherwise, it LGTM.
More information about the Gdb-patches
mailing list