From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17057 invoked by alias); 27 Jul 2012 02:45:16 -0000 Received: (qmail 17028 invoked by uid 22791); 27 Jul 2012 02:45:12 -0000 X-SWARE-Spam-Status: No, hits=-4.0 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL X-Spam-Check-By: sourceware.org Received: from imr3.ericy.com (HELO imr3.ericy.com) (198.24.6.13) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 27 Jul 2012 02:44:47 +0000 Received: from eusaamw0711.eamcs.ericsson.se ([147.117.20.178]) by imr3.ericy.com (8.13.8/8.13.8) with ESMTP id q6R2ih40020691 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=FAIL); Thu, 26 Jul 2012 21:44:44 -0500 Received: from EUSAACMS0703.eamcs.ericsson.se ([169.254.1.135]) by eusaamw0711.eamcs.ericsson.se ([147.117.20.178]) with mapi; Thu, 26 Jul 2012 22:44:43 -0400 From: Marc Khouzam To: Pedro Alves CC: "'Tom Tromey'" , "'gdb-patches@sourceware.org'" Date: Fri, 27 Jul 2012 02:45:00 -0000 Subject: RE: [Patch] Cannot set pending bp if condition set explicitly Message-ID: References: <87zk6nkghi.fsf@fleche.redhat.com> <501194E0.5040109@redhat.com> In-Reply-To: <501194E0.5040109@redhat.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 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: 2012-07/txt/msg00657.txt.bz2 > -----Original Message----- > From: Pedro Alves [mailto:palves@redhat.com]=20 > Sent: Thursday, July 26, 2012 3:05 PM > To: Marc Khouzam > Cc: 'Tom Tromey'; 'gdb-patches@sourceware.org' > Subject: Re: [Patch] Cannot set pending bp if condition set explicitly >=20 > Hello, Thanks Pedro for having taken the time to dig into this. > On 07/25/2012 09:30 PM, Marc Khouzam wrote: > > Index: gdb/breakpoint.c > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > 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 @@ > >=20=20 > > static void tcatch_command (char *arg, int from_tty); > >=20=20 > > +static int all_locations_are_pending (struct bp_location *loc); > > + > > static void detach_single_step_breakpoints (void); > >=20=20 > > static int single_step_breakpoint_inserted_here_p (struct=20 > address_space *, > > @@ -951,7 +953,12 @@ > > /* I don't know if it matters whether this is the=20 > string the user > > typed in or the decompiled expression. */ > > b->cond_string =3D xstrdup (arg); > > - b->condition_not_parsed =3D 0; > > + > > + /* For a pending breakpoint, the condition is not=20 > parsed yet. */ > > + if (all_locations_are_pending (b->loc)) > > + b->condition_not_parsed =3D 1; > > + else > > + b->condition_not_parsed =3D 0; > >=20=20 > > if (is_watchpoint (b)) > > { >=20 > It looks to me the sole reason of b->condition_not_parsed's=20 > existence, is that > when the user does >=20 > break foo 1 =3D=3D 1 thread 3 >=20 > we need to split the "1 =3D=3D 1" part from the "thread 3" part.=20=20 > The expression > that results from this parse is just discarded. >=20 > E.g., see: >=20 > /* 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. */ >=20 > find_condition_and_thread (arg,=20 > 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 =3D rest; >=20 >=20 > So "condition_not_parsed" is just a flag that means=20 > "find_condition_and_thread > not called yet, so I don't know yet where my condition string ends". Ok, I see that now.=20=20 > Unfortunately, the "1 =3D=3D 1" part is language dependent, so it=20 > needs to be parsed > with some context. >=20 > Note that the result of such parsing is always discarded -=20 > the only thing > find_condition_and_thread is interested in, is in advancing=20 > the command arg pointer > past the condition. Later, each location's condition=20 > expression is always reparsed > using each location as context, and that is what is stored in=20 > bloc->cond > or in ((struct watchpoint *)b)->cond_exp, for watchpoints. That makes me wonder why we need to jump through hoops with having "condition_not_parsed"? Why not=20 always call find_condition_and_thread() inside of create_breakpoint() even for pending breakpoints? That way, b->cond_string, b->thread, and b->extra_string would always be set after create_breakpoint() and we wouldn't need to treat pending breakpoints differently in this case. It seems find_condition_and_thread() requires an address, which would explain why it is not called for pending bp. However, after reading your explanation, since find_condition_and_thread()= =20 is only interested in advancing the command arg pointer past the condition, why do we need the address at all? Should work fine for pending breakpoints, no? Maybe it is just a requirement for parse_exp_1() which is the one that actually parse the command argument? > Therefore, it seems to be the patch has unexpected consequences. >=20 > This currently works: >=20 > (gdb) b main if 1 =3D=3D 1 thread 1 > Breakpoint 2 at 0x4572bb: file ../../src/gdb/gdb.c, line 29. >=20 > But this does not: >=20 > (gdb) condition 2 1 =3D=3D 1 thread 1 > Junk at end of expression On a side-note, I see that although there is an error printed and=20 set_breakpoint_condition() is aborted, the bad condition is not cleaned-up, as shown here: (gdb) info b Num Type Disp Enb Address What 1 breakpoint keep y 0x0804851d in main at ../../../src/gdb/test= suite/gdb.base/pending.c:26 stop only if k=3D=3D1 thread 1 Shouldn't there be some kind of cleanup in set_breakpoint_condition()? > However, for pending breakpoints, this will not produce an error: >=20 > (gdb) condition 2 1 =3D=3D 1 thread 1 > (gdb) info breakpoints > Num Type Disp Enb Address What > 2 breakpoint keep y foo > stop only if 1 =3D=3D 1 thread 1 >=20 > 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. >=20 > > @@ -951,7 +953,12 @@ > > /* I don't know if it matters whether this is the=20 > string the user > > typed in or the decompiled expression. */ > > b->cond_string =3D xstrdup (arg); > > - b->condition_not_parsed =3D 0; > > + > > + /* For a pending breakpoint, the condition is not=20 > parsed yet. */ > > + if (all_locations_are_pending (b->loc)) > > + b->condition_not_parsed =3D 1; > > + else > > + b->condition_not_parsed =3D 0; > > >=20 > 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=20 > condition_not_parsed > seems wrong. (if it were right, it would seem better to=20 > 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=20 > the condition > string was!). Got it. > 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: >=20 > Error in re-setting breakpoint 5: No source file named pendshr.c. >=20 > seen here: >=20 > #0 addr_string_to_sals (b=3D0xf9ecc0, addr_string=3D0xe7dbe0=20 > "pendshr.c:24", found=3D0x7fffffffd2ec) at=20 > ../../src/gdb/breakpoint.c:14002 > #1 0x0000000000555483 in breakpoint_re_set_default=20 > (b=3D0xf9ecc0) at ../../src/gdb/breakpoint.c:14076 > #2 0x000000000055306e in bkpt_re_set (b=3D0xf9ecc0) at=20 > ../../src/gdb/breakpoint.c:12812 > #3 0x000000000055577f in breakpoint_re_set_one=20 > (bint=3D0xf9ecc0) at ../../src/gdb/breakpoint.c:14193 > #4 0x00000000005ce097 in catch_errors (func=3D0x555747=20 > , func_args=3D0xf9ecc0,=20 > errstring=3D0x111a190 "Error in re-setting breakpoint 7: ", mask=3D6) > at ../../src/gdb/exceptions.c:546 > #5 0x0000000000555811 in breakpoint_re_set () at=20 > ../../src/gdb/breakpoint.c:14217 > #6 0x00000000006df7c1 in solib_add (pattern=3D0x0, from_tty=3D0,=20 > target=3D0xc42100, readsyms=3D1) at ../../src/gdb/solib.c:930 >=20 > static struct symtabs_and_lines > addr_string_to_sals (struct breakpoint *b, char *addr_string,=20 > int *found) > { > char *s; > struct symtabs_and_lines sals =3D {0}; > volatile struct gdb_exception e; >=20 > gdb_assert (b->ops !=3D NULL); > s =3D addr_string; >=20 > TRY_CATCH (e, RETURN_MASK_ERROR) > { > b->ops->decode_linespec (b, &s, &sals); > } > if (e.reason < 0) > { > int not_found_and_ok =3D 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 =3D=3D NOT_FOUND_ERROR > && (b->condition_not_parsed > ^^^^^^^^^^^^^^^^^^^^^^^ > || (b->loc && b->loc->shlib_disabled) > || (b->loc && b->loc->pspace->executing_startup) > || b->enable_state =3D=3D bp_disabled)) > not_found_and_ok =3D 1; >=20 > And "b->condition_not_parsed" being set results in not_found_and_ok > being set too. >=20 > 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: >=20 > if (e.reason =3D=3D 0 || e.error !=3D NOT_FOUND_ERROR) > { > int i; >=20 > for (i =3D 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; >=20 > find_condition_and_thread (s, sals.sals[0].pc, > &cond_string, &thread, &task, > &extra_string); >=20 >=20 > But s[0] is \0, because s decode_linespace consumes the whole=20 > addr_string, > which was "pendshr.c:24".=20=20 Nice! Thanks for clearing that up. > So find_condition_and_thread is=20 > never called, > meaning the "thread N" never actually works, and,=20 > condition_not_parsed remains > set forever. I am of the opinion that "condition BP thread=20 > N" should _not_ > work (reserve that for a separate command, we have enough=20 > trouble due to > "thread/task" already), so "condition" for non-pending=20 > breakpoints should > remain as it is, and for pending breakpoints, thread "1 =3D=3D 1=20 > thread N" should > be treated as a bogus condition, just like "ninja fu" is an invalid > C condition. Yes, that makes sense. In that case, set_breakpoint_condition()=20 would need to parse the condition even if the bp was pending. This would fit well with needed cleanup of set_breakpoint_condition() mentioned above. Should I write a patch for this? (I'm leaving for vacation on Friday, so this may not happen very soon.) =20 > I think create_breakpoint has a related latent bug. The contract is: >=20 > /* (...) 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. (...) */ >=20 > 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. That is in part my fault. The handling of PARSE_CONDITION_AND_THREAD was not done right for pending breakpoints. I tried to fix it here: http://sourceware.org/ml/gdb-patches/2012-07/msg00418.html but I didn't deal with condition_not_parsed because if I set it to 0 in that case, MI started showing the bug that we're trying to fix in this patch. But since you showed that setting=20 condition_not_parsed to 1 only solves the problem by a side-effect, MI was only working by chance in this case. Another side-note is that when a pending breakpoint is created, b->extra_string doesn't seem set properly, just like b->cond_string was not. I believe extra_string is for dprintf which cannot be set using MI yet (I think), but once it will, this will become a problem. > So given all that, I tried the alternate patch below. IOW, > change how addr_string_to_sals knows a breakpoint is a=20 > pending breakpoint. > It also fixes the new test, and causes no regressions (x86_64=20 > Fedora 17). That makes total sense. > Not sure yet what is the best predicate to put there. > The whole condition looks a bit in need of TLC.=20=20 > all_locations_are_pending would > return true if all locations had been shlib_disabled. The=20 > condition does > also check for shlib_disabled, but only on the first=20 > location. Especially now > that breakpoints can have locations anywhere, I think we=20 > might need to consider > what happens and what should happen to when only a few of the=20 > conditions > are shlib_disabled.. >=20 > WDYT? I'll try to give this predicate a bit more thought=20 > (but I'm running > out of day for today), but these are my thoughts so far. I don't have much of an opinion. I'm not yet grasping all the details of breakpoints with multiple locations. Maybe the breakpoint structure needs a pending flag? > gdb/breakpoint.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) >=20 > 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, >=20 > b->addr_string =3D copy_arg; > if (parse_condition_and_thread) > - b->cond_string =3D NULL; > + { > + b->cond_string =3D NULL; > + b->condition_not_parsed =3D 1; > + } > else > { > /* Create a private copy of condition string. */ > @@ -9572,7 +9575,6 @@ create_breakpoint (struct gdbarch *gdbarch, > b->extra_string =3D NULL; > b->ignore_count =3D ignore_count; > b->disposition =3D tempflag ? disp_del : disp_donttouch; > - b->condition_not_parsed =3D 1; > b->enable_state =3D enabled ? bp_enabled : bp_disabled; > if ((type_wanted !=3D bp_breakpoint > && type_wanted !=3D bp_hardware_breakpoint) || thread !=3D -1) > @@ -14008,7 +14010,7 @@ addr_string_to_sals (struct=20 > breakpoint *b, char *addr_string, int *found) > breakpoint being disabled, and don't want to see more > errors. */ > if (e.error =3D=3D NOT_FOUND_ERROR > - && (b->condition_not_parsed > + && (b->loc =3D=3D NULL > || (b->loc && b->loc->shlib_disabled) > || (b->loc && b->loc->pspace->executing_startup) > || b->enable_state =3D=3D bp_disabled)) This patch works well from the basic testing I did. Please let me know how you suggest I can go forward with the different issues discussed above about bp conditions. Thanks again! Marc