* [PATCH 0/2] Some additional debug output @ 2022-10-05 9:34 Andrew Burgess 2022-10-05 9:34 ` [PATCH 1/2] gdb: add some additional debug in mark_async_event_handler Andrew Burgess 2022-10-05 9:34 ` [PATCH 2/2] gdb: more infrun debug from breakpoint.c Andrew Burgess 0 siblings, 2 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-05 9:34 UTC (permalink / raw) To: gdb-patches Some extra debug output that I found useful while debugging some issues in GDB. --- Andrew Burgess (2): gdb: add some additional debug in mark_async_event_handler gdb: more infrun debug from breakpoint.c gdb/async-event.c | 6 ++++-- gdb/breakpoint.c | 22 ++++++++++++++++++++-- 2 files changed, 24 insertions(+), 4 deletions(-) -- 2.25.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] gdb: add some additional debug in mark_async_event_handler 2022-10-05 9:34 [PATCH 0/2] Some additional debug output Andrew Burgess @ 2022-10-05 9:34 ` Andrew Burgess 2022-10-05 9:34 ` [PATCH 2/2] gdb: more infrun debug from breakpoint.c Andrew Burgess 1 sibling, 0 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-05 9:34 UTC (permalink / raw) To: gdb-patches Extend the existing debug printf call to include the previous state of the async_event_handler object. --- gdb/async-event.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gdb/async-event.c b/gdb/async-event.c index 12ce62cfa5f..8c6ba099f59 100644 --- a/gdb/async-event.c +++ b/gdb/async-event.c @@ -293,8 +293,10 @@ create_async_event_handler (async_event_handler_func *proc, void mark_async_event_handler (async_event_handler *async_handler_ptr) { - event_loop_debug_printf ("marking async event handler `%s`", - async_handler_ptr->name); + event_loop_debug_printf ("marking async event handler `%s` " + "(previous state was %d)", + async_handler_ptr->name, + async_handler_ptr->ready); async_handler_ptr->ready = 1; } -- 2.25.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] gdb: more infrun debug from breakpoint.c 2022-10-05 9:34 [PATCH 0/2] Some additional debug output Andrew Burgess 2022-10-05 9:34 ` [PATCH 1/2] gdb: add some additional debug in mark_async_event_handler Andrew Burgess @ 2022-10-05 9:34 ` Andrew Burgess 2022-10-05 12:27 ` Simon Marchi 1 sibling, 1 reply; 10+ messages in thread From: Andrew Burgess @ 2022-10-05 9:34 UTC (permalink / raw) To: gdb-patches 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"); } /* Returns true if we need to track moribund locations of LOC's type @@ -13157,7 +13175,7 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable) owner. 1-based indexing. -1 signals NOT FOUND. */ static int -find_loc_num_by_location (bp_location *loc) +find_loc_num_by_location (const bp_location *loc) { if (loc != nullptr && loc->owner != nullptr) { -- 2.25.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gdb: more infrun debug from breakpoint.c 2022-10-05 9:34 ` [PATCH 2/2] gdb: more infrun debug from breakpoint.c Andrew Burgess @ 2022-10-05 12:27 ` Simon Marchi 2022-10-06 8:45 ` Andrew Burgess 0 siblings, 1 reply; 10+ messages in thread From: Simon Marchi @ 2022-10-05 12:27 UTC (permalink / raw) To: Andrew Burgess, gdb-patches 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"); This way, all cases where we decided not to stop are handled as early returns (there are more above in the function). And if we reach the very end of the function, it means we decided to stop. Other than that, this patch and the previous one LGTM. Simon ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gdb: more infrun debug from breakpoint.c 2022-10-05 12:27 ` Simon Marchi @ 2022-10-06 8:45 ` Andrew Burgess 2022-10-06 9:37 ` [PUSHED 0/3] Some additional debug output Andrew Burgess 2022-10-06 11:06 ` [PATCH 2/2] gdb: more infrun debug from breakpoint.c Andrew Burgess 0 siblings, 2 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-06 8:45 UTC (permalink / raw) To: Simon Marchi, gdb-patches 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. Anyway. I've tweaked my original patches slightly and will push them, I'll follow up to this mail with what I'm actually pushing. Thanks, Andrew > > This way, all cases where we decided not to stop are handled as early > returns (there are more above in the function). And if we reach the > very end of the function, it means we decided to stop. > > Other than that, this patch and the previous one LGTM. > > Simon ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PUSHED 0/3] Some additional debug output 2022-10-06 8:45 ` Andrew Burgess @ 2022-10-06 9:37 ` Andrew Burgess 2022-10-06 9:37 ` [PUSHED 1/3] gdb: add some additional debug in mark_async_event_handler Andrew Burgess ` (2 more replies) 2022-10-06 11:06 ` [PATCH 2/2] gdb: more infrun debug from breakpoint.c Andrew Burgess 1 sibling, 3 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-06 9:37 UTC (permalink / raw) To: gdb-patches Some extra debug output. --- Andrew Burgess (3): gdb: add some additional debug in mark_async_event_handler gdb: more infrun debug from breakpoint.c gdb: add missing nullptr checks in bpstat_check_breakpoint_conditions gdb/async-event.c | 6 ++++-- gdb/breakpoint.c | 30 ++++++++++++++++++++++++++---- 2 files changed, 30 insertions(+), 6 deletions(-) -- 2.25.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PUSHED 1/3] gdb: add some additional debug in mark_async_event_handler 2022-10-06 9:37 ` [PUSHED 0/3] Some additional debug output Andrew Burgess @ 2022-10-06 9:37 ` Andrew Burgess 2022-10-06 9:37 ` [PUSHED 2/3] gdb: more infrun debug from breakpoint.c Andrew Burgess 2022-10-06 9:37 ` [PUSHED 3/3] gdb: add missing nullptr checks in bpstat_check_breakpoint_conditions Andrew Burgess 2 siblings, 0 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-06 9:37 UTC (permalink / raw) To: gdb-patches Extend the existing debug printf call to include the previous state of the async_event_handler object. --- gdb/async-event.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gdb/async-event.c b/gdb/async-event.c index 12ce62cfa5f..8c6ba099f59 100644 --- a/gdb/async-event.c +++ b/gdb/async-event.c @@ -293,8 +293,10 @@ create_async_event_handler (async_event_handler_func *proc, void mark_async_event_handler (async_event_handler *async_handler_ptr) { - event_loop_debug_printf ("marking async event handler `%s`", - async_handler_ptr->name); + event_loop_debug_printf ("marking async event handler `%s` " + "(previous state was %d)", + async_handler_ptr->name, + async_handler_ptr->ready); async_handler_ptr->ready = 1; } -- 2.25.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PUSHED 2/3] gdb: more infrun debug from breakpoint.c 2022-10-06 9:37 ` [PUSHED 0/3] Some additional debug output Andrew Burgess 2022-10-06 9:37 ` [PUSHED 1/3] gdb: add some additional debug in mark_async_event_handler Andrew Burgess @ 2022-10-06 9:37 ` Andrew Burgess 2022-10-06 9:37 ` [PUSHED 3/3] gdb: add missing nullptr checks in bpstat_check_breakpoint_conditions Andrew Burgess 2 siblings, 0 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-06 9:37 UTC (permalink / raw) To: gdb-patches 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 | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index c8c34120aa0..29a02945234 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -161,6 +161,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 @@ -5298,6 +5300,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. */ @@ -5312,6 +5316,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. */ @@ -5319,6 +5327,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; } @@ -5329,6 +5340,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; } @@ -5420,16 +5432,26 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) if (cond && !condition_result) { + infrun_debug_printf ("condition_result = false, not stopping"); bs->stop = 0; + return; } 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); - } + return; + } + + if (bs->stop) + infrun_debug_printf ("stopping at this breakpoint"); + else + infrun_debug_printf ("not stopping at this breakpoint"); } /* Returns true if we need to track moribund locations of LOC's type @@ -13142,7 +13164,7 @@ enable_disable_bp_num_loc (int bp_num, int loc_num, bool enable) owner. 1-based indexing. -1 signals NOT FOUND. */ static int -find_loc_num_by_location (bp_location *loc) +find_loc_num_by_location (const bp_location *loc) { if (loc != nullptr && loc->owner != nullptr) { -- 2.25.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PUSHED 3/3] gdb: add missing nullptr checks in bpstat_check_breakpoint_conditions 2022-10-06 9:37 ` [PUSHED 0/3] Some additional debug output Andrew Burgess 2022-10-06 9:37 ` [PUSHED 1/3] gdb: add some additional debug in mark_async_event_handler Andrew Burgess 2022-10-06 9:37 ` [PUSHED 2/3] gdb: more infrun debug from breakpoint.c Andrew Burgess @ 2022-10-06 9:37 ` Andrew Burgess 2 siblings, 0 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-06 9:37 UTC (permalink / raw) To: gdb-patches Add a couple of missing nullptr checks in the function bpstat_check_breakpoint_conditions. No user visible change after this commit. --- gdb/breakpoint.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 29a02945234..a3a154c19a9 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -5358,7 +5358,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) else cond = bl->cond.get (); - if (cond && b->disposition != disp_del_at_next_stop) + if (cond != nullptr && b->disposition != disp_del_at_next_stop) { int within_current_scope = 1; struct watchpoint * w; @@ -5430,7 +5430,7 @@ bpstat_check_breakpoint_conditions (bpstat *bs, thread_info *thread) value_free_to_mark (mark); } - if (cond && !condition_result) + if (cond != nullptr && !condition_result) { infrun_debug_printf ("condition_result = false, not stopping"); bs->stop = 0; -- 2.25.4 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] gdb: more infrun debug from breakpoint.c 2022-10-06 8:45 ` Andrew Burgess 2022-10-06 9:37 ` [PUSHED 0/3] Some additional debug output Andrew Burgess @ 2022-10-06 11:06 ` Andrew Burgess 1 sibling, 0 replies; 10+ messages in thread From: Andrew Burgess @ 2022-10-06 11:06 UTC (permalink / raw) To: Simon Marchi, gdb-patches 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-10-06 11:06 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-05 9:34 [PATCH 0/2] Some additional debug output Andrew Burgess 2022-10-05 9:34 ` [PATCH 1/2] gdb: add some additional debug in mark_async_event_handler Andrew Burgess 2022-10-05 9:34 ` [PATCH 2/2] gdb: more infrun debug from breakpoint.c Andrew Burgess 2022-10-05 12:27 ` Simon Marchi 2022-10-06 8:45 ` Andrew Burgess 2022-10-06 9:37 ` [PUSHED 0/3] Some additional debug output Andrew Burgess 2022-10-06 9:37 ` [PUSHED 1/3] gdb: add some additional debug in mark_async_event_handler Andrew Burgess 2022-10-06 9:37 ` [PUSHED 2/3] gdb: more infrun debug from breakpoint.c Andrew Burgess 2022-10-06 9:37 ` [PUSHED 3/3] gdb: add missing nullptr checks in bpstat_check_breakpoint_conditions Andrew Burgess 2022-10-06 11:06 ` [PATCH 2/2] gdb: more infrun debug from breakpoint.c Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).