From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 4662 invoked by alias); 17 Aug 2007 15:47:19 -0000 Received: (qmail 4390 invoked by uid 22791); 17 Aug 2007 15:47:16 -0000 X-Spam-Check-By: sourceware.org Received: from igw1.br.ibm.com (HELO igw1.br.ibm.com) (32.104.18.24) by sourceware.org (qpsmtpd/0.31) with ESMTP; Fri, 17 Aug 2007 15:47:05 +0000 Received: from mailhub3.br.ibm.com (mailhub3 [9.18.232.110]) by igw1.br.ibm.com (Postfix) with ESMTP id 8F39D14810E for ; Fri, 17 Aug 2007 12:30:19 -0300 (BRT) Received: from d24av02.br.ibm.com (d24av02.br.ibm.com [9.18.232.47]) by mailhub3.br.ibm.com (8.13.8/8.13.8/NCO v8.5) with ESMTP id l7HFkx801769586 for ; Fri, 17 Aug 2007 12:46:59 -0300 Received: from d24av02.br.ibm.com (loopback [127.0.0.1]) by d24av02.br.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id l7HFkxKp006527 for ; Fri, 17 Aug 2007 12:46:59 -0300 Received: from [9.18.238.24] ([9.18.238.24]) by d24av02.br.ibm.com (8.12.11.20060308/8.12.11) with ESMTP id l7HFkv03006496; Fri, 17 Aug 2007 12:46:59 -0300 Subject: Re: [patch] Watchpoints: support for thread parameters From: Luis Machado Reply-To: luisgpm@linux.vnet.ibm.com To: Eli Zaretskii Cc: gdb-patches@sourceware.org In-Reply-To: References: <1187298178.5853.11.camel@localhost> Content-Type: multipart/mixed; boundary="=-vaOvHE1pkpv/9gNZaFz6" Date: Fri, 17 Aug 2007 15:47:00 -0000 Message-Id: <1187365616.4520.14.camel@localhost> Mime-Version: 1.0 X-Mailer: Evolution 2.6.1 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/msg00332.txt.bz2 --=-vaOvHE1pkpv/9gNZaFz6 Content-Type: text/plain Content-Transfer-Encoding: 7bit Content-length: 2169 Hi, > Your code assumes that is a literal number. Do we want > to allow an integral expression there, or is a literal number good > enough? I believe the thread command assumes a literal number as well. Were you thinking about more complex expressions defining the thread ID? Like a variable or an arithmetic expression? > In any case, this change, if and when accepted, should be accompanied > by a suitable change to the user manual. Agreed. We might as well keep the parameter format the same as for breakpoint commands. > 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. I did try the code with trailing white spaces and it seemed to work, but maybe i didn't pick a specific case that would break the code. Anyway, i've changed that in the new version to start before the null character, so if we have any trailing white spaces or tabs, it'll handle them correctly until the token shows up. > 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. Yes, that was something Bauermann suggested in the first version. That's updated in the new version of the patch. I exchanged those strlen()'s for "tok > arg" conditions. > 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... This was also addressed in the new version. Looks better and cleaner. I've also addressed some coding standard issues with identations and spaces in comments. Thanks for the suggestions Eli. How does this new version look? Best regards, -- Luis Machado Software Engineer IBM Linux Technology Center e-mail: luisgpm@linux.vnet.ibm.com --=-vaOvHE1pkpv/9gNZaFz6 Content-Disposition: attachment; filename=watch_thread_param.diff Content-Type: text/x-patch; name=watch_thread_param.diff; charset=utf-8 Content-Transfer-Encoding: 7bit Content-length: 3534 2007-08-16 Luis Machado * breakpoint.c: (watch_command_1): Parse additional optional "thread" parameter to the watchpoint command and set the "thread" member of the breakpoint struct. Index: gdb/breakpoint.c =================================================================== --- gdb.orig/breakpoint.c 2007-08-17 07:09:37.000000000 -0700 +++ gdb/breakpoint.c 2007-08-17 08:24:19.000000000 -0700 @@ -5826,7 +5826,7 @@ struct frame_info *prev_frame = NULL; char *exp_start = NULL; char *exp_end = NULL; - char *tok, *end_tok; + char *tok, *id_tok_start, *end_tok; int toklen; char *cond_start = NULL; char *cond_end = NULL; @@ -5834,10 +5834,72 @@ int i, other_type_used, target_resources_ok = 0; enum bptype bp_type; int mem_cnt = 0; + int thread = -1; init_sal (&sal); /* initialize to zeroes */ - /* Parse arguments. */ + /* Make sure that we actually have parameters to parse. */ + if (arg != NULL && strlen (arg) >= 1) + { + toklen = strlen (arg); /* Size of argument list. */ + + /* Points tok to the end of the argument list. */ + tok = arg + toklen - 1; + + /* Go backwards in the parameters list. Skip the last parameter. + If we're expecting a 'thread ' parameter, this should + be the thread identifier. */ + while (tok > arg && (*tok == ' ' || *tok == '\t')) + tok--; + while (tok > arg && (*tok != ' ' && *tok != '\t')) + tok--; + + /* Points end_tok to the beginning of the last token. */ + id_tok_start = tok + 1; + + /* Go backwards in the parameters list. Skip one more parameter. + If we're expecting a 'thread ' parameter, we should + reach a "thread" token. */ + while (tok > arg && (*tok == ' ' || *tok == '\t')) + tok--; + + end_tok = tok; + + while (tok > arg && (*tok != ' ' && *tok != '\t')) + tok--; + + /* Move the pointer forward to skip the whitespace and + calculate the length of the token. */ + tok++; + toklen = end_tok - tok; + + if (toklen >= 1 && strncmp (tok, "thread", toklen) == 0) + { + /* At this point we've found a "thread" token, which means + the user is trying to set a watchpoint that triggers + only in a specific thread. */ + char *endp; + + /* Extract the thread ID from the next token. */ + thread = strtol (id_tok_start, &endp, 0); + + /* Check if the user provided a valid numeric value for the + thread ID. */ + if (*endp != ' ' && *endp != '\t' && *endp != '\0') + error (_("Invalid thread ID specification %s."), id_tok_start); + + /* Check if the thread actually exists. */ + if (!valid_thread_id (thread)) + error (_("Unknown thread %d."), thread); + + /* Truncate the string and get rid of the thread + parameter before the parameter list is parsed by the + evaluate_expression() function. */ + *tok = '\0'; + } + } + + /* Parse the rest of the arguments. */ innermost_block = NULL; exp_start = arg; exp = parse_exp_1 (&arg, 0, 0); @@ -5921,6 +5983,7 @@ b = set_raw_breakpoint (sal, bp_type); set_breakpoint_count (breakpoint_count + 1); b->number = breakpoint_count; + b->thread = thread; b->disposition = disp_donttouch; b->exp = exp; b->exp_valid_block = exp_valid_block; --=-vaOvHE1pkpv/9gNZaFz6--