Re: [Patch] Cannot set pending bp if condition set explicitly

[Pedro Alves <palves@redhat.com>]

On 07/27/2012 03:44 AM, Marc Khouzam wrote:

>> 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.
> 
> That makes me wonder why we need to jump
> through hoops with having "condition_not_parsed"?  Why not 
> 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() 
> 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?

Yes, we parse the condition in the context of the breakpoint location,
for locals, etc., so we need the location's address.  We assume the condition
would parse more or less the same at any location, as in, gross validation,
and skipping past the condition, so we just pick the first location).

An alternative would be to restrict pending breakpoints conditions
to globals only.  Not sure whether that would fly.

> 
>> 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
> 
> On a side-note, I see that although there is an error printed and 
> 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/testsuite/gdb.base/pending.c:26
>         stop only if k==1 thread 1
> 
> Shouldn't there be some kind of cleanup in set_breakpoint_condition()?

Indeed.  Hmm, that, or disable the location, with a warning.  We parse the condition in the
context of each location.  A condition might well make sense and parse okay
for a location, but not for another (e.g., different locals/parameters).  Disabling
is also what we do if we fail to parse a location later on with "break foo if bar".

> Yes, that makes sense.  In that case, set_breakpoint_condition() 
> 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.)

I've already got something in the works.  Let me see if I can finish it.
And I'm leaving for vacation at the end of the week too.  :-)

-- 
Pedro Alves