From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16537 invoked by alias); 13 Apr 2010 11:14:11 -0000 Received: (qmail 16526 invoked by uid 22791); 13 Apr 2010 11:14:09 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=BAYES_00,TW_QS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 13 Apr 2010 11:14:04 +0000 Received: (qmail 17586 invoked from network); 13 Apr 2010 11:14:02 -0000 Received: from unknown (HELO orlando.localnet) (pedro@127.0.0.2) by mail.codesourcery.com with ESMTPA; 13 Apr 2010 11:14:02 -0000 From: Pedro Alves To: "Pierre Muller" Subject: Re: [RFA-v7] Add windows OS Thread Information Block Date: Tue, 13 Apr 2010 11:14:00 -0000 User-Agent: KMail/1.12.2 (Linux/2.6.31-20-generic; KDE/4.3.2; x86_64; ; ) Cc: "'Eli Zaretskii'" , gdb-patches@sourceware.org References: <000901c9f5ef$4ee06f10$eca14d30$@u-strasbg.fr> <201004121743.44687.pedro@codesourcery.com> <002d01cadae4$ae5b2d10$0b118730$@muller@ics-cnrs.unistra.fr> In-Reply-To: <002d01cadae4$ae5b2d10$0b118730$@muller@ics-cnrs.unistra.fr> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201004131214.00652.pedro@codesourcery.com> X-IsSubscribed: yes 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 X-SW-Source: 2010-04/txt/msg00398.txt.bz2 On Tuesday 13 April 2010 09:38:23, Pierre Muller wrote: > > > + if (the_target->get_tib_address != NULL) > > > + strcat (own_buf, ";qGetTIBAddr+"); > > > > You missed addressing a comment I made to this: > > I guessed that this is the needed counterpart of the fact > that qGetTIBAddr is in the list of features that can be disabled. Not really. Notice how The `qGetTLSAddr' packet, which you're basing your new packet on, doesn't have this equivalent qSupported feature, and you can still disable it with the `set remote get-thread-local-storage-address' command. > > I can't seem to find where this new qsupported feature > > was documented in the manual changes? (and is this needed > > at all? I guess it doesn't hurt.) > > At least, I tested disabling it with 'set remote get-thread-information-block-address off' > and that worked OK. See above. The way it is in your patch currently, this isn't doing what you think it is. It's actually doing nothing. For it to work, you'd need to add a new entry in the `remote_protocol_features' array, with PACKED_DISABLE as default, so that only when the feature is reported would this test: + if (remote_protocol_packets[PACKET_qGetTIBAddr].support != PACKET_DISABLE) + { fail. As is, remote_protocol_packets[PACKET_qGetTIBAddr].support starts as PACKET_SUPPORT_UNKNOWN, and on first use, the packet_ok call below updates it depending on the remote stub reply: + getpkt (&rs->buf, &rs->buf_size, 0); + result = packet_ok (rs->buf, + &remote_protocol_packets[PACKET_qGetTIBAddr]); Again, what I'm saying is: _new qSupported features need to be documented in the manual_. If this stays, it needs to be documented. But if it stays, it needs to be made to work correctly. And hence my earlier question: (is this needed at all?). Please, do try to fully understand all code you write and propose. I think these hints should get you a good start. > Here is one more version. > The main change (apart from the suggestions made by Pedro) > is the addition of a NEWS entry. > > * windows-tdep.h (info_w32_cmdlist): Declare. ^ spurious double space. > Index: NEWS > =================================================================== > RCS file: /cvs/src/src/gdb/NEWS,v > retrieving revision 1.369 > diff -u -p -r1.369 NEWS > --- NEWS 9 Apr 2010 15:26:54 -0000 1.369 > +++ NEWS 13 Apr 2010 07:34:59 -0000 > @@ -3,6 +3,14 @@ > > *** Changes since GDB 7.1 > > +* New Windows OS feature: Thread Information Block. > + > + - GDB now supports disaplying of thread specific information for > + Windows OS executables. This feature is accessible either as a new command > + 'info w32 thread-information-block' or as a new computed internal variable > + named '$_tlb' a thread-specific pointer to the Thread Information Block. > + 'qGetTIBAddr' is a new remote packet associated with this feature. > + Typo "disaplying". Double space after period. Note that the GNU single-quote style is to open a quote with a back-quote (`). Also, you're not adding a new feature to the OS. :-) May I suggest this instead ? : Index: src/gdb/NEWS =================================================================== --- src.orig/gdb/NEWS 2010-04-13 11:34:17.000000000 +0100 +++ src/gdb/NEWS 2010-04-13 11:37:54.000000000 +0100 @@ -3,6 +3,22 @@ *** Changes since GDB 7.1 +* Windows Thread Information Block access. + + On Windows targets, GDB now supports displaying the Windows Thread + Information Block (TIB) structure. This structure is visible either + by using the new command `info w32 thread-information-block' or, by + dereferencing the new convenience variable named `$_tlb', a + thread-specific pointer to the TIB. This feature is also supported + when remote debugging using GDBserver. + +* New remote packets + +qGetTIBAddr + + Return the address of the Windows Thread Information Block of the + current thread. + * New features in the GDB remote stub, GDBserver - GDBserver now support tracepoints. The feature is currently > /* Support for inferring a target description based on the current > architecture and the size of a 'g' packet. While the 'g' packet > can have any size (since optional registers can be left off the > @@ -9890,6 +9934,8 @@ Specify the serial device it is connecte > remote_ops.to_set_circular_trace_buffer = remote_set_circular_trace_buffer; > remote_ops.to_core_of_thread = remote_core_of_thread; > remote_ops.to_verify_memory = remote_verify_memory; > + remote_ops.to_get_tib_address = remote_get_tib_address; > + Spurious new line. > } > +/* Provide thread local base, i.e. Thread Information Block address. > + Returns 1 if ptid is found and thread_local_base is non zero. */ You also need to update the comment... > + > +static int > +windows_get_tib_address (ptid_t ptid, CORE_ADDR *addr) > +{ > + thread_info *th; > + > + th = thread_rec (ptid_get_tid (ptid), 0); > + if (th == NULL) > + return 0; > + > + if (addr != NULL) > + *addr = th->thread_local_base; > + > + return 1; > +} > +static void > +tlb_value_write (struct value *v, struct value *fromval) > +{ > + error (_("Impossible to change tlb")); This is really a nit, but I suggest error (_("Impossible to change the Thread Local Base.")); Or, error (_("Impossible to change the TLB.")); > + tib = alloca (tib_size); > + > + /* This needs to be changed if multi-process support is added. */ This comment is still here. I thought you were going to remove it? > +/* 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). */ > +static void While we're at it, add a new line here too, please. > +display_tib (char * args, int from_tty) > + add_setshow_boolean_cmd ("show-all-tib", class_maintenance, > + &maint_display_all_tib, _("\ > +Set whether to display all non-zero fields of thread information block."), _("\ > +Show whether to display all non-zero fields of thread information block."), _("\ > +Use \"on\" to enable, \"off\" to disable.\n\ > +If enabled, all non-zero fields of thread information block are displayed,\n\ > +even if their meaning is unknown."), > + NULL, > + NULL, If you would please install a real `show' callback instead of this second NULL, I'd appreciate it. The reason is that the default callback when this is left as NULL can't work correctly in non-English. A goal is to have no command left with this callback left as NULL. But if you don't want to do it now (too many iterations already!), it's quite fine. > +@item info w32 thread-information-block > +This command displays thread specific information stored in the > +Thread Information Block for x86 CPU family (readable using @code{$fs} > +selector for 32-bit programs and @code{$gs} for 64-bit programs). Could you instead say: +Thread Information Block (readable on the X86 CPU family using @code{$fs} +selector for 32-bit programs and @code{$gs} for 64-bit programs). > +@kindex maint set show-all-tib > +@kindex maint show show-all-tib > +@item maint set show-all-tib > +@itemx maint show show-all-tib > +Control whether to show all non zero areas within a 1k block starting > +at thread local base, when using @samp{info w32 thread-information-block} > +command. when using _the_ ... command. > +@item qGetTIBAddr:@var{thread-id}: ^ This `:' is spurious; it isn't a part of the packet. Please remove it. -- Pedro Alves