From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17084 invoked by alias); 26 Jul 2012 19:05:24 -0000 Received: (qmail 17063 invoked by uid 22791); 26 Jul 2012 19:05:22 -0000 X-SWARE-Spam-Status: No, hits=-7.5 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 26 Jul 2012 19:05:08 +0000 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6QJ562c022149 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Thu, 26 Jul 2012 15:05:06 -0400 Received: from [127.0.0.1] (ovpn01.gateway.prod.ext.ams2.redhat.com [10.39.146.11]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q6QJ54xt014806; Thu, 26 Jul 2012 15:05:05 -0400 Message-ID: <501194E0.5040109@redhat.com> Date: Thu, 26 Jul 2012 19:05:00 -0000 From: Pedro Alves User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:14.0) Gecko/20120717 Thunderbird/14.0 MIME-Version: 1.0 To: Marc Khouzam CC: "'Tom Tromey'" , "'gdb-patches@sourceware.org'" Subject: Re: [Patch] Cannot set pending bp if condition set explicitly References: <87zk6nkghi.fsf@fleche.redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: 2012-07/txt/msg00639.txt.bz2 Hello, On 07/25/2012 09:30 PM, Marc Khouzam wrote: > Index: gdb/breakpoint.c > =================================================================== > RCS file: /cvs/src/src/gdb/breakpoint.c,v > retrieving revision 1.695 > diff -u -r1.695 breakpoint.c > --- gdb/breakpoint.c 24 Jul 2012 17:37:56 -0000 1.695 > +++ gdb/breakpoint.c 25 Jul 2012 20:15:20 -0000 > @@ -230,6 +230,8 @@ > > static void tcatch_command (char *arg, int from_tty); > > +static int all_locations_are_pending (struct bp_location *loc); > + > static void detach_single_step_breakpoints (void); > > static int single_step_breakpoint_inserted_here_p (struct address_space *, > @@ -951,7 +953,12 @@ > /* I don't know if it matters whether this is the string the user > typed in or the decompiled expression. */ > b->cond_string = xstrdup (arg); > - b->condition_not_parsed = 0; > + > + /* For a pending breakpoint, the condition is not parsed yet. */ > + if (all_locations_are_pending (b->loc)) > + b->condition_not_parsed = 1; > + else > + b->condition_not_parsed = 0; > > if (is_watchpoint (b)) > { It looks to me the sole reason of b->condition_not_parsed's existence, is that when the user does break foo 1 == 1 thread 3 we need to split the "1 == 1" part from the "thread 3" part. The expression that results from this parse is just discarded. E.g., see: /* Here we only parse 'arg' to separate condition from thread number, so parsing in context of first sal is OK. When setting the breakpoint we'll re-parse it in context of each sal. */ find_condition_and_thread (arg, lsal->sals.sals[0].pc, &cond_string, &thread, &task, &rest); if (cond_string) make_cleanup (xfree, cond_string); if (rest) make_cleanup (xfree, rest); if (rest) extra_string = rest; So "condition_not_parsed" is just a flag that means "find_condition_and_thread not called yet, so I don't know yet where my condition string ends". Unfortunately, the "1 == 1" part is language dependent, so it needs to be parsed with some context. Note that the result of such parsing is always discarded - the only thing find_condition_and_thread is interested in, is in advancing the command arg pointer past the condition. Later, each location's condition expression is always reparsed using each location as context, and that is what is stored in bloc->cond or in ((struct watchpoint *)b)->cond_exp, for watchpoints. Therefore, it seems to be the patch has unexpected consequences. This currently works: (gdb) b main if 1 == 1 thread 1 Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29. But this does not: (gdb) condition 2 1 == 1 thread 1 Junk at end of expression However, for pending breakpoints, this will not produce an error: (gdb) condition 2 1 == 1 thread 1 (gdb) info breakpoints Num Type Disp Enb Address What 2 breakpoint keep y foo stop only if 1 == 1 thread 1 But later on, when the breakpoint's condition is finally parsed, the "thread 1" part will not really be used for anything. More on that below. > @@ -951,7 +953,12 @@ > /* I don't know if it matters whether this is the string the user > typed in or the decompiled expression. */ > b->cond_string = xstrdup (arg); > - b->condition_not_parsed = 0; > + > + /* For a pending breakpoint, the condition is not parsed yet. */ > + if (all_locations_are_pending (b->loc)) > + b->condition_not_parsed = 1; > + else > + b->condition_not_parsed = 0; > So in light of the above, this reads a bit odd. This function is only called from condition_command. The "condition" command only supports a real condition, not the "thread N" part. So the passed in EXP to set_condition_command already _is_ the condition, and we don't need to call find_condition_and_thread. Therefore, setting condition_not_parsed seems wrong. (if it were right, it would seem better to return immediately, rather than continue and try to parse the condition expression for each location, given we had just asserted we didn't know what the condition string was!). The reason setting "condition_not_parsed" fixes the problem, is that when you "run", breakpoints end up being re_set, and that ends up with: Error in re-setting breakpoint 5: No source file named pendshr.c. seen here: #0 addr_string_to_sals (b=0xf9ecc0, addr_string=0xe7dbe0 "pendshr.c:24", found=0x7fffffffd2ec) at ../../src/gdb/breakpoint.c:14002 #1 0x0000000000555483 in breakpoint_re_set_default (b=0xf9ecc0) at ../../src/gdb/breakpoint.c:14076 #2 0x000000000055306e in bkpt_re_set (b=0xf9ecc0) at ../../src/gdb/breakpoint.c:12812 #3 0x000000000055577f in breakpoint_re_set_one (bint=0xf9ecc0) at ../../src/gdb/breakpoint.c:14193 #4 0x00000000005ce097 in catch_errors (func=0x555747 , func_args=0xf9ecc0, errstring=0x111a190 "Error in re-setting breakpoint 7: ", mask=6) at ../../src/gdb/exceptions.c:546 #5 0x0000000000555811 in breakpoint_re_set () at ../../src/gdb/breakpoint.c:14217 #6 0x00000000006df7c1 in solib_add (pattern=0x0, from_tty=0, target=0xc42100, readsyms=1) at ../../src/gdb/solib.c:930 static struct symtabs_and_lines addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found) { char *s; struct symtabs_and_lines sals = {0}; volatile struct gdb_exception e; gdb_assert (b->ops != NULL); s = addr_string; TRY_CATCH (e, RETURN_MASK_ERROR) { b->ops->decode_linespec (b, &s, &sals); } if (e.reason < 0) { int not_found_and_ok = 0; /* For pending breakpoints, it's expected that parsing will fail until the right shared library is loaded. User has already told to create pending breakpoints and don't need extra messages. If breakpoint is in bp_shlib_disabled state, then user already saw the message about that breakpoint being disabled, and don't want to see more errors. */ if (e.error == NOT_FOUND_ERROR && (b->condition_not_parsed ^^^^^^^^^^^^^^^^^^^^^^^ || (b->loc && b->loc->shlib_disabled) || (b->loc && b->loc->pspace->executing_startup) || b->enable_state == bp_disabled)) not_found_and_ok = 1; And "b->condition_not_parsed" being set results in not_found_and_ok being set too. When finally decode_linespec does not throw an error, because the library is loaded in the inferior, and the breakpoint manages to become resolvable, we reach: if (e.reason == 0 || e.error != NOT_FOUND_ERROR) { int i; for (i = 0; i < sals.nelts; ++i) resolve_sal_pc (&sals.sals[i]); if (b->condition_not_parsed && s && s[0]) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ { char *cond_string, *extra_string; int thread, task; find_condition_and_thread (s, sals.sals[0].pc, &cond_string, &thread, &task, &extra_string); But s[0] is \0, because s decode_linespace consumes the whole addr_string, which was "pendshr.c:24". So find_condition_and_thread is never called, meaning the "thread N" never actually works, and, condition_not_parsed remains set forever. I am of the opinion that "condition BP thread N" should _not_ work (reserve that for a separate command, we have enough trouble due to "thread/task" already), so "condition" for non-pending breakpoints should remain as it is, and for pending breakpoints, thread "1 == 1 thread N" should be treated as a bogus condition, just like "ninja fu" is an invalid C condition. I think create_breakpoint has a related latent bug. The contract is: /* (...) This function has two major modes of operations, selected by the PARSE_CONDITION_AND_THREAD parameter. If non-zero, the function will parse arg, extracting breakpoint location, address and thread. Otherwise, ARG is just the location of breakpoint, with condition and thread specified by the COND_STRING and THREAD parameters. (...) */ But when a pending breakpoint is created, condition_not_parsed is left set, even when PARSE_CONDITION_AND_THREAD was false, thus ignoring the THREAD argument. So given all that, I tried the alternate patch below. IOW, change how addr_string_to_sals knows a breakpoint is a pending breakpoint. It also fixes the new test, and causes no regressions (x86_64 Fedora 17). Not sure yet what is the best predicate to put there. The whole condition looks a bit in need of TLC. all_locations_are_pending would return true if all locations had been shlib_disabled. The condition does also check for shlib_disabled, but only on the first location. Especially now that breakpoints can have locations anywhere, I think we might need to consider what happens and what should happen to when only a few of the conditions are shlib_disabled.. WDYT? I'll try to give this predicate a bit more thought (but I'm running out of day for today), but these are my thoughts so far. gdb/breakpoint.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 03719d4..ae21631 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -9558,7 +9558,10 @@ create_breakpoint (struct gdbarch *gdbarch, b->addr_string = copy_arg; if (parse_condition_and_thread) - b->cond_string = NULL; + { + b->cond_string = NULL; + b->condition_not_parsed = 1; + } else { /* Create a private copy of condition string. */ @@ -9572,7 +9575,6 @@ create_breakpoint (struct gdbarch *gdbarch, b->extra_string = NULL; b->ignore_count = ignore_count; b->disposition = tempflag ? disp_del : disp_donttouch; - b->condition_not_parsed = 1; b->enable_state = enabled ? bp_enabled : bp_disabled; if ((type_wanted != bp_breakpoint && type_wanted != bp_hardware_breakpoint) || thread != -1) @@ -14008,7 +14010,7 @@ addr_string_to_sals (struct breakpoint *b, char *addr_string, int *found) breakpoint being disabled, and don't want to see more errors. */ if (e.error == NOT_FOUND_ERROR - && (b->condition_not_parsed + && (b->loc == NULL || (b->loc && b->loc->shlib_disabled) || (b->loc && b->loc->pspace->executing_startup) || b->enable_state == bp_disabled))