From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27332 invoked by alias); 7 Jan 2016 22:28:13 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 27312 invoked by uid 89); 7 Jan 2016 22:28:12 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 spammy=160107 X-HELO: usplmg20.ericsson.net Received: from usplmg20.ericsson.net (HELO usplmg20.ericsson.net) (198.24.6.45) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Thu, 07 Jan 2016 22:28:10 +0000 Received: from EUSAAHC002.ericsson.se (Unknown_Domain [147.117.188.78]) by usplmg20.ericsson.net (Symantec Mail Security) with SMTP id 71.A3.06940.A94EE865; Thu, 7 Jan 2016 23:20:10 +0100 (CET) Received: from [142.133.110.144] (147.117.188.8) by smtp-am.internal.ericsson.com (147.117.188.80) with Microsoft SMTP Server id 14.3.248.2; Thu, 7 Jan 2016 17:28:07 -0500 Subject: Re: [PATCH v3 4/7] Per-inferior/Inferior-qualified thread IDs To: Pedro Alves , , Pierre Muller References: <1452085418-18300-1-git-send-email-palves@redhat.com> <1452085418-18300-5-git-send-email-palves@redhat.com> <568D6242.1030802@ericsson.com> <568EC055.7020209@redhat.com> From: Simon Marchi Message-ID: <568EE677.6050407@ericsson.com> Date: Thu, 07 Jan 2016 22:28:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <568EC055.7020209@redhat.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00122.txt.bz2 On 16-01-07 02:45 PM, Pedro Alves wrote: >> Also, it might be good to mention what the stop condition for the iterat= ion >> is. Maybe as a simple usage example? >=20 > 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 threa= d id, >> or a range. Should it be mentioned here? >=20 > 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 =91displa= y_tib=92: >> /home/emaisin/src/binutils-gdb/gdb/windows-tdep.c:378:7: error: implicit= declaration of function =91find_thread_id=92 [-Werror=3Dimplicit-function-= declaration] >> tp =3D find_thread_id (gdb_id); >> ^ >> /home/emaisin/src/binutils-gdb/gdb/windows-tdep.c:378:10: error: assignm= ent makes pointer from integer without a cast [-Werror] >> tp =3D find_thread_id (gdb_id); >> ^ >> cc1: all warnings being treated as errors >> make: *** [windows-tdep.o] Error 1 >=20 > Whoops, forgot --enable-targets=3Dall... >=20 > But, looking at the code, that's parsing a GDB thread ID, while the comme= nt > 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 resolve= d :). > Here are the changes addressing your review comments. >=20 > From d2f3387ebbed744da6d2e686ebc1f72f94a7e328 Mon Sep 17 00:00:00 2001 > From: Pedro Alves > Date: Thu, 7 Jan 2016 19:26:07 +0000 > Subject: [PATCH] review >=20 > --- > 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(-) >=20 > diff --git a/gdb/testsuite/gdb.multi/tids.c b/gdb/testsuite/gdb.multi/tid= s.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; > } >=20=20 > void * > @@ -34,6 +36,8 @@ thread_function1 (void *arg) >=20=20 > while (1) > sleep (1); > + > + return NULL; > } >=20=20 > int > diff --git a/gdb/testsuite/gdb.multi/tids.exp b/gdb/testsuite/gdb.multi/t= ids.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 > } >=20=20 > -# 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 > } >=20=20 > -# 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" >=20=20 > - # 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" >=20=20 > @@ -133,7 +133,7 @@ with_test_prefix "two inferiors" { >=20=20 > runto_main >=20=20 > - # 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; > } >=20=20 > -/* See tid-parse.h. */ > - > -int > -tid_range_parser_get_tid_range (struct tid_range_parser *parser, int *in= f_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 =3D=3D TID_RANGE_STATE_INFERIOR) > { > @@ -233,11 +236,24 @@ tid_range_parser_get_tid_range (struct tid_range_pa= rser *parser, int *inf_num, >=20=20 > /* See tid-parse.h. */ >=20=20 > -void > +int > +tid_range_parser_get_tid_range (struct tid_range_parser *parser, int *in= f_num, > + int *thr_start, int *thr_end) > +{ > + gdb_assert (inf_num !=3D NULL && thr_start !=3D NULL && thr_end !=3D N= ULL); > + > + 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 !=3D NULL && thr_start !=3D NULL); s/thr_start/thr_num/ > + > + return get_tid_range (parser, inf_num, thr_num, NULL); s/get_tid_range/get_tid_or_range/ > } >=20=20 > /* 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_p= arser *parser, > /* Parse a thread ID or a thread range list. >=20=20 > A range will be of the form > - .- and will represent > - all the threads of inferior inferior_num with number between > - thread_number1 and thread_number2, inclusive. can > - also be omitted, as in -, in which > - case GDB infers the inferior number from the current inferior. >=20=20 > - 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. > + .- > + > + and will represent all the threads of inferior INFERIOR_NUM with > + number between THREAD_NUMBER1 and THREAD_NUMBER2, inclusive. > + can also be omitted, as in > + > + - > + > + 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). >=20=20 > At the beginning of parsing a thread range, the char pointer > PARSER->string will be advanced past 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 . */ > -extern void tid_range_parser_get_tid (struct tid_range_parser *parser, > + range will advance the pointer past . > + > + 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=3D1; *THR_NUM=3D2 (finished=3D=3D0) > + 2nd call: *INF_NUM=3D3; *THR_NUM=3D4 (finished=3D=3D0) > + 3rd call: *INF_NUM=3D3; *THR_NUM=3D5 (finished=3D=3D0) > + 4th call: *INF_NUM=3D3; *THR_NUM=3D6 (finished=3D=3D1) > + > + 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); >=20=20 > -/* 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. >=20=20 > 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. >=20=20 > - list: "1.2 3.4-6" > - 1st call: *INF_NUM=3D1; *THR_START=3D2; *THR_END=3D2 > - 2nd call: *INF_NUM=3D3; *THR_START=3D4; *THR_END=3D4 > + E.g.,. with list: "1.2 3.4-6" >=20=20 > + 1st call: *INF_NUM=3D1; *THR_START=3D2; *THR_END=3D2 (finished=3D= =3D0) > + 2nd call: *INF_NUM=3D3; *THR_START=3D4; *THR_END=3D6 (finished=3D= =3D1) >=20=20 > Returns true if parsed a thread/range successfully, false > otherwise. */ > @@ -135,7 +156,7 @@ extern void tid_range_parser_skip (struct tid_range_p= arser *parser); > extern int tid_range_parser_qualified (struct tid_range_parser *parser); >=20=20 > /* 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. >=20=20 > 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;=20=20 > } >=20=20 > -/* 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. */ >=20=20 > static void > display_tib (char * args, int from_tty) > { > - if (args) > - { > - struct thread_info *tp; > - int gdb_id =3D value_as_long (parse_and_eval (args)); > - > - tp =3D 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); > } >=20=20 >=20 Otherwise, it LGTM.