From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19928 invoked by alias); 17 Aug 2007 10:24:06 -0000 Received: (qmail 19524 invoked by uid 22791); 17 Aug 2007 10:24:03 -0000 X-Spam-Check-By: sourceware.org Received: from romy.inter.net.il (HELO romy.inter.net.il) (213.8.233.24) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 17 Aug 2007 10:23:57 +0000 Received: from HOME-C4E4A596F7 (IGLD-84-228-207-214.inter.net.il [84.228.207.214]) by romy.inter.net.il (MOS 3.7.3-GA) with ESMTP id IQZ47246 (AUTH halo1); Fri, 17 Aug 2007 13:23:29 +0300 (IDT) Date: Fri, 17 Aug 2007 10:24:00 -0000 Message-Id: From: Eli Zaretskii To: luisgpm@linux.vnet.ibm.com CC: gdb-patches@sourceware.org In-reply-to: <1187298178.5853.11.camel@localhost> (message from Luis Machado on Thu, 16 Aug 2007 18:02:58 -0300) Subject: Re: [patch] Watchpoints: support for thread parameters Reply-to: Eli Zaretskii References: <1187298178.5853.11.camel@localhost> 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: 2007-08/txt/msg00331.txt.bz2 > From: Luis Machado > Date: Thu, 16 Aug 2007 18:02:58 -0300 > > This is a first try on the implementation of the additional "thread > " parameter for the watchpoint command in order to make GDB > stop only on specific threads when a watchpoint is triggered. Thanks. > Basically i started parsing the arguments backwards, trying to locate > tokens that identify a "thread " command. After that i hand > all the left parameters over to the default expression parser. Your code assumes that is a literal number. Do we want to allow an integral expression there, or is a literal number good enough? In any case, this change, if and when accepted, should be accompanied by a suitable change to the user manual. Allow me a few comments on your code: > + toklen = strlen (arg); /* Size of argument list */ > + > + /* Points tok to the end of the argument list */ > + tok = arg + toklen; Did you try your code when the argument has trailing whitespace? Unless I'm missing something, the last line above make `tok' point to the null character that ends the argument, so the while loop after that will end immediately, even if there is whitespace after the thread ID number, and the whole parsing business will not work correctly. > + while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t')) > + tok--; > + while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t')) > + tok--; > + > + /* Go backwards in the parameters list. Skip one more parameter. > + If we're expecting a 'thread ' parameter, we should > + reach a "thread" token. */ > + while (strlen (tok) < toklen && (*tok == ' ' || *tok == '\t')) > + tok--; > + > + end_tok = tok; > + > + while (strlen (tok) < toklen && (*tok != ' ' && *tok != '\t')) > + tok--; You don't need to compute `strlen (tok)' every time through the loops, because each iteration you go back exactly one character. So you can compute it only once, and then decrement its value inside the loop bodies. Alternatively, just check that you didn't yet get to the first character of the argument: while (tok > arg && (*tok != ' ' && *tok != '\t')) tok--; I like this latter alternative better. Actually, given the trailing whitespace problem I mentioned above, this is even better: while (tok > arg && (tok[-1] != ' ' && tok[-1] != '\t')) tok--; (You will need to adjust the rest of code to bump `tok' one position forward after you exit the loop, to account for the -1 above.) > + tmptok = tok; > + thread = strtol (tok, &tok, 0); > + > + if (tok == tmptok) > + error (_("Incorrect parameter after thread keyword.")); There's a much better way of checking whether `tok' is a literal number: look at the pointer returned by `strtol' in its 2nd arg, and if it points to something other than a whitespace character, err out: char *endp; thread = strtol (tok, &endp, 0); if (*endp != ' ' && *endp != '\t') error (_("Invalid thread ID specification %s."), tok); Thanks again for working on this.