From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 45402 invoked by alias); 7 May 2015 17:03:39 -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 45384 invoked by uid 89); 7 May 2015 17:03:38 -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_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 07 May 2015 17:03:36 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t47H3X5w030571 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Thu, 7 May 2015 13:03:33 -0400 Received: from valrhona.uglyboxes.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t47H3W1a017186 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 7 May 2015 13:03:32 -0400 Message-ID: <554B9AE3.5060601@redhat.com> Date: Thu, 07 May 2015 17:03:00 -0000 From: Keith Seitz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Doug Evans CC: gdb-patches@sourceware.org Subject: Re: [PATCH v3 3/9] Explicit locations: use new location API References: <20150217220619.1312.39861.stgit@valrhona.uglyboxes.com> <20150217220643.1312.93245.stgit@valrhona.uglyboxes.com> In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg00147.txt.bz2 On 03/01/2015 12:53 PM, Doug Evans wrote: > Keith Seitz writes: > >> >-/* Set a breakpoint. This function is shared between CLI and MI >> >- functions for setting a breakpoint. This function has two major >> >- modes of operations, selected by the PARSE_ARG parameter. If >> >- non-zero, the function will parse ARG, extracting location, >> >- condition, thread and extra string. Otherwise, ARG is just the >> >- breakpoint's location, with condition, thread, and extra string >> >- specified by the COND_STRING, THREAD and EXTRA_STRING parameters. >> >+/* Set a breakpoint. This function is shared between CLI and MI functions >> >+ for setting a breakpoint at LOCATION. >> >+ >> >+ This function has two major modes of operations, selected by the PARSE_ARG >> >+ parameter. >> >+ >> >+ If PARSE_ARG is zero, LOCATION is just the breakpoint's location, >> >+ with condition, thread, and extra string specified by the COND_STRING, >> >+ THREAD, and EXTRA_STRING parameters. >> >+ >> >+ If PARSE_ARG is non-zero and LOCATION is a linespec location, >> >+ this function will attempt to extract the location, condition, thread, >> >+ and extra string from the linespec stored in LOCATION. >> >+ For non-linespec locations EXTRA_STRING is parsed for condition, thread, >> >+ and extra string. >> >+ >> > If INTERNAL is non-zero, the breakpoint number will be allocated >> >- from the internal breakpoint count. Returns true if any breakpoint >> >- was created; false otherwise. */ >> >+ from the internal breakpoint count. >> >+ >> >+ Returns true if any breakpoint was created; false otherwise. */ >> > >> > int >> > create_breakpoint (struct gdbarch *gdbarch, >> >- char *arg, char *cond_string, >> >+ const struct event_location *location, char *cond_string, >> > int thread, char *extra_string, >> > int parse_arg, > ==== > Rename parse_arg to parse_location or parse_extra? > ["parse_arg" made sense when "arg" was a parameter, but it's confusing now] Sure. I've picked parse_extra and updated the comment. That also works well with your next comment: >> >@@ -10082,6 +10130,8 @@ create_breakpoint (struct gdbarch *gdbarch, >> > breakpoint. */ >> > if (!pending) >> > { >> >+ char *arg = extra_string; > ==== > I gather having "arg" is here to minimize changes, but it reduces > readability a bit. I've simply removed the local variable `arg' and passed extra_string to find_condition_and_thread. >> >@@ -10159,7 +10222,15 @@ create_breakpoint (struct gdbarch *gdbarch, >> > } >> > b->cond_string = cond_string; >> > } >> >- b->extra_string = NULL; >> >+ >> >+ /* Make a private copy of extra_string for the breakpoint. */ >> >+ if (extra_string != NULL) >> >+ { >> >+ b->extra_string = xstrdup (extra_string); >> >+ make_cleanup (xfree, b->extra_string); > ==== > This cleanup is odd. I would have expected a cleanup for "b" to > completely clean itself up. However, it looks like no cleanup for "b" > is ever created. So that's an existing bug which I wouldn't impose > on this patchset to fix, but OTOH this cleanup still feels wrong. I've rewritten/rearranged this a little bit so that it more closely mimics what is done with cond_string immediately before it. [And, you're right -- `b' doesn't get "cleaned up" in the way you'd expect. It took me a couple of read-throughs to figure this out, too.] > Also, the docs of extra_string in breakpoint.h are lacking. Docs for create_breakpoint don't even exist in breakpoint.h! :-) I've moved them there. > It's not something that has to be fixed with this patchset, > but I'm having to dig too deep to figure out how extra_string > is different from cond_string. Can I trouble you to clarify things? I have made an attempt at it. [It turns out that this needed rewriting anyway -- I apparently forgot to fully update the comment on this last revision.] >> >@@ -10206,16 +10277,21 @@ break_command_1 (char *arg, int flag, int from_tty) >> > : bp_breakpoint); >> > struct breakpoint_ops *ops; >> > const char *arg_cp = arg; >> >+ struct event_location *location; >> >+ struct cleanup *cleanup; >> >+ >> >+ location = string_to_event_location (&arg, current_language); >> >+ cleanup = make_cleanup_delete_event_location (location); >> > >> > /* Matching breakpoints on probes. */ >> >- if (arg && probe_linespec_to_ops (&arg_cp) != NULL) >> >+ if (arg_cp != NULL && probe_linespec_to_ops (&arg_cp) != NULL) > ==== > Extra space before &&. > Fixed. >> >@@ -14582,21 +14691,37 @@ location_to_sals (struct breakpoint *b, char *addr_string, int *found) >> > >> > for (i = 0; i < sals.nelts; ++i) >> > resolve_sal_pc (&sals.sals[i]); >> >- if (b->condition_not_parsed && s && s[0]) >> >+ if (b->condition_not_parsed) >> > { >> >- char *cond_string, *extra_string; >> >- int thread, task; >> >+ const char *s = b->extra_string; >> > >> >- find_condition_and_thread (s, sals.sals[0].pc, >> >- &cond_string, &thread, &task, >> >- &extra_string); >> >- if (cond_string) >> >- b->cond_string = cond_string; >> >- b->thread = thread; >> >- b->task = task; >> >- if (extra_string) >> >- b->extra_string = extra_string; >> >- b->condition_not_parsed = 0; >> >+ if (s != NULL && *s != '\0') > ==== > If we have a rule that b->extra_string is never "" (only NULL) then > we can simplify this, and even remove this extra nested "if". > E.g., just have > > + if (b->condition_not_parsed && b->extra_string != NULL) > Yes, good catch! >> >@@ -433,10 +434,9 @@ linespec_lexer_lex_keyword (const char *p) >> > int len = strlen (linespec_keywords[i]); >> > >> > /* If P begins with one of the keywords and the next >> >- character is not a valid identifier character, >> >- we have found a keyword. */ >> >+ character is whitespace, we have found a keyword. */ >> > if (strncmp (p, linespec_keywords[i], len) == 0 >> >- && !(isalnum (p[len]) || p[len] == '_')) >> >+ && isspace (p[len])) > ==== > I think I'm ok with this change, but it seems separate from this patchset. > Submit it separately? Some time ago, I committed a more complete patch for this. >> >@@ -1765,21 +1765,29 @@ linespec_parse_basic (linespec_parser *parser) >> > STATE->canonical. */ >> > >> > static void >> >-canonicalize_linespec (struct linespec_state *state, linespec_p ls) >> >+canonicalize_linespec (struct linespec_state *state, const linespec_p ls) >> > { >> >+ char *tmp; >> >+ >> > /* If canonicalization was not requested, no need to do anything. */ >> > if (!state->canonical) >> > return; >> > >> > /* Shortcut expressions, which can only appear by themselves. */ >> > if (ls->expression != NULL) >> >- state->canonical->addr_string = xstrdup (ls->expression); >> >+ { >> >+ tmp = ASTRDUP (ls->expression); > ==== > No need to make this change now (assuming it's doable), > but I'm curious if the parser needs to modify its argument > or whether we can pass a const char * and remove the ASTRDUP here > (and elsewhere). > s/its argument/its state/ ? Otherwise I'm not entirely sure what you mean. The linespec parser takes all const arguments already (parse_linespec [now] takes const char *, decode_line* take const struct event_location *). I could change canonicalize_linespec to take the canonical location instead of the entire parser state, though, yes, that's doable, but it still needs to be mutable. >> >@@ -1999,8 +2001,10 @@ spu_catch_start (struct objfile *objfile) >> > >> > /* Use a numerical address for the set_breakpoint command to avoid having >> > the breakpoint re-set incorrectly. */ >> >- xsnprintf (buf, sizeof buf, "*%s", core_addr_to_string (pc)); >> >- create_breakpoint (get_objfile_arch (objfile), buf /* arg */, >> >+ p = ASTRDUP (core_addr_to_string (pc)); > ==== > The leading '*' got dropped. > I can imagine this is just temporary anyway, as a later patch > (address locations) redoes this again. > I'm ok with leaving it as is, but I'd be remiss if I didn't > point it out. Yes, it did drop the '*'! For completeness, I've put it back, even if temporary. Keith