Re: [PATCH 2/2] gdb: more infrun debug from breakpoint.c

[Andrew Burgess <aburgess@redhat.com>]

Andrew Burgess <aburgess@redhat.com> writes:

> Simon Marchi <simark@simark.ca> writes:
>
>> On 2022-10-05 05:34, Andrew Burgess via Gdb-patches wrote:
>>> This commit adds additional infrun debug from the breakpoint.c file.
>>> The new debug output all relates to breakpoint condition evaluation.
>>> 
>>> There is already some infrun debug emitted from the breakpoint.c file,
>>> so hopefully, adding more will not be contentious.  I think the
>>> functions being instrumented make sense as part of the infrun process,
>>> the inferior stops, evaluates the condition, and then either stops or
>>> continues.  This new debug gives more insight into that process.
>>> 
>>> I had to make the bp_location* argument to find_loc_num_by_location
>>> const, and add a declaration for find_loc_num_by_location.
>>> 
>>> There should be no user visible changes unless they turn on debug
>>> output.
>>> ---
>>>  gdb/breakpoint.c | 22 ++++++++++++++++++++--
>>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
>>> index 002f4a935b1..38546ea44ba 100644
>>> --- a/gdb/breakpoint.c
>>> +++ b/gdb/breakpoint.c
>>> @@ -165,6 +165,8 @@ static std::vector<symtab_and_line> bkpt_probe_decode_location_spec
>>>  
>>>  static bool bl_address_is_meaningful (bp_location *loc);
>>>  
>>> +static int find_loc_num_by_location (const bp_location *loc);
>>> +
>>>  /* update_global_location_list's modes of operation wrt to whether to
>>>     insert locations now.  */
>>>  enum ugll_insert_mode
>>> @@ -5302,6 +5304,8 @@ bpstat_check_watchpoint (bpstat *bs)
>>>  static void
>>>  bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>>>  {
>>> +  INFRUN_SCOPED_DEBUG_ENTER_EXIT;
>>> +
>>>    const struct bp_location *bl;
>>>    struct breakpoint *b;
>>>    /* Assume stop.  */
>>> @@ -5316,6 +5320,10 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>>>    b = bs->breakpoint_at;
>>>    gdb_assert (b != NULL);
>>>  
>>> +  infrun_debug_printf ("thread = %s, breakpoint %d.%d",
>>> +		       thread->ptid.to_string ().c_str (),
>>> +		       b->number, find_loc_num_by_location (bl));
>>> +
>>>    /* Even if the target evaluated the condition on its end and notified GDB, we
>>>       need to do so again since GDB does not know if we stopped due to a
>>>       breakpoint or a single step breakpoint.  */
>>> @@ -5323,6 +5331,9 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>>>    if (frame_id_p (b->frame_id)
>>>        && !frame_id_eq (b->frame_id, get_stack_frame_id (get_current_frame ())))
>>>      {
>>> +      infrun_debug_printf ("incorrect frame %s not %s, not stopping",
>>> +			   get_stack_frame_id (get_current_frame ()).to_string ().c_str (),
>>> +			   b->frame_id.to_string ().c_str ());
>>>        bs->stop = 0;
>>>        return;
>>>      }
>>> @@ -5333,6 +5344,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>>>    if ((b->thread != -1 && b->thread != thread->global_num)
>>>        || (b->task != 0 && b->task != ada_get_task_number (thread)))
>>>      {
>>> +      infrun_debug_printf ("incorrect thread or task, not stopping");
>>>        bs->stop = 0;
>>>        return;
>>>      }
>>> @@ -5424,16 +5436,22 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread)
>>>  
>>>    if (cond && !condition_result)
>>>      {
>>> +      infrun_debug_printf ("condition_result = false, not stopping");
>>>        bs->stop = 0;
>>>      }
>>>    else if (b->ignore_count > 0)
>>>      {
>>> +      infrun_debug_printf ("ignore count %d, not stopping",
>>> +			   b->ignore_count);
>>>        b->ignore_count--;
>>>        bs->stop = 0;
>>>        /* Increase the hit count even though we don't stop.  */
>>>        ++(b->hit_count);
>>>        gdb::observers::breakpoint_modified.notify (b);
>>> -    }	
>>> +    }
>>> +
>>> +  if (bs->stop)
>>> +    infrun_debug_printf ("stopping at this breakpoint");
>>>  }
>>
>> I'd suggest a little change just for readability:
>>
>>   if (cond != nullptr && !condition_result)
>>     {
>>       ...
>>       return;
>>     }
>>
>>   if (b->ignore_count > 0)
>>     {
>>       ...
>>       return;
>>     }
>>
>>   infrun_debug_printf ("stopping at this breakpoint");
>
> That does look better, but unfortunately, is not correct.  The problem
> is this line, about midway through the function:
>
>   bs->stop = breakpoint_ext_lang_cond_says_stop (b);
>
> which invokes e.g. the Python Breakpoint.stop methods.  It is possible
> that this sets the stop field back to 0.
>
> Having looked at this a little, I now have a bunch of questions about
> this code, like, prior to the quoted line we perform the thread and
> frame checks for the breakpoint, but after the quoted line we perform
> the condition and ignore count checks for the breakpoint.
>
> This means that for a breakpoint in the wrong thread/frame, the Python
> stop method is never run.
>
> But, for a breakpoint with a condition or ignore count, the stop method
> is run, and might choose to return True, indicating GDB should stop.
> We'll then check the condition, or check the ignore count, and decide
> that we should continue.
>
> This doesn't seem right.
>
> I'm tempted to suggest that either we should run the extension language
> hooks first, before any other checks, so the hook is _always_ run, but
> then update the docs to say that GDB might still choose to override a
> stop request for a breakpoint.  Or, we should run the extension language
> hooks after all other checks, so the hook is _only_ run if GDB could
> stop at this breakpoint.

So, after a little more digging, it's not as bad as I thought.  GDB will
mostly stop a user trying to implement an extension language stop
method, and add a condition to a breakpoint.  So we don't need to worry
about that case.

The ignore count case is still open though.  I think over time the
ignore count implementation has drifted a little from the docs.  The
manual says:

  If a breakpoint has a positive ignore count and a condition, the
  condition is not checked.  Once the ignore count reaches zero,
  @value{GDBN} resumes checking the condition.

But I don't think that is true any longer.  We always check the
condition, and the ignore count will override the condition, but only
when the condition would otherwise stop the breakpoint.

Which I think makes sense?  The ignore count is now ignoring the next N
times the breakpoint _would_ have stopped, not just ignoring the
breakpoint the next N times we hit it.

Given this has been this way for a long time now, it's probably not a
good idea to mess with it, and it probably makes sense for the extension
language to match this behaviour.

I think there's probably some documentation improvements that I could
make in this area, but I don't think GDB itself needs to change.

Thanks,
Andrew