* [PATCH v4 0/9] enable target-async by default
@ 2013-10-22 17:59 Tom Tromey
2013-10-22 17:59 ` [PATCH v4 5/9] PR gdb/13860: make "-exec-foo"'s MI output equal to "foo"'s MI output Tom Tromey
` (8 more replies)
0 siblings, 9 replies; 53+ messages in thread
From: Tom Tromey @ 2013-10-22 17:59 UTC (permalink / raw)
To: gdb-patches
This is version 4 of the patch series to enable target-async by
default. It is very similar to version 3, submitted here:
https://sourceware.org/ml/gdb-patches/2013-08/msg00102.html
I've merely rebased it onto the new git repository and fixed up
various merge issues.
I built and regtested this on x86-64 Fedora 18, both native and using
the native-gdbserver target board.
I plan to check it in soon (next couple of days) unless there are more
comments.
thanks,
Tom
^ permalink raw reply [flat|nested] 53+ messages in thread* [PATCH v4 5/9] PR gdb/13860: make "-exec-foo"'s MI output equal to "foo"'s MI output. 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey @ 2013-10-22 17:59 ` Tom Tromey 2013-10-22 17:59 ` [PATCH v4 1/9] fix latent bugs in ui-out.c Tom Tromey ` (7 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Tom Tromey @ 2013-10-22 17:59 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves From: Pedro Alves <palves@redhat.com> Part of PR gdb/13860 is about the mi-solib.exp test's output being different in sync vs async modes. sync: >./gdb -nx -q ./testsuite/gdb.mi/solib-main -ex "set stop-on-solib-events 1" -ex "set target-async off" -i=mi =thread-group-added,id="i1" ~"Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/solib-main..." ~"done.\n" (gdb) &"start\n" ~"Temporary breakpoint 1 at 0x400608: file ../../../src/gdb/testsuite/gdb.mi/solib-main.c, line 21.\n" =breakpoint-created,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x0000000000400608",func="main",file="../../../src/gdb/testsuite/gdb.mi/solib-main.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/solib-main.c",line="21",times="0",original-location="main"} ~"Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/solib-main \n" =thread-group-started,id="i1",pid="17724" =thread-created,id="1",group-id="i1" ^running *running,thread-id="all" (gdb) =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1" ~"Stopped due to shared library event (no libraries added or removed)\n" *stopped,reason="solib-event",frame={addr="0x000000379180f990",func="_dl_debug_state",args=[],from="/lib64/ld-linux-x86-64.so.2"},thread-id="1",stopped-threads="all",core="3" (gdb) async: >./gdb -nx -q ./testsuite/gdb.mi/solib-main -ex "set stop-on-solib-events 1" -ex "set target-async on" -i=mi =thread-group-added,id="i1" ~"Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/solib-main..." ~"done.\n" (gdb) start &"start\n" ~"Temporary breakpoint 1 at 0x400608: file ../../../src/gdb/testsuite/gdb.mi/solib-main.c, line 21.\n" =breakpoint-created,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x0000000000400608",func="main",file="../../../src/gdb/testsuite/gdb.mi/solib-main.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/solib-main.c",line="21",times="0",original-location="main"} ~"Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/solib-main \n" =thread-group-started,id="i1",pid="17729" =thread-created,id="1",group-id="i1" ^running *running,thread-id="all" =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1" (gdb) *stopped,reason="solib-event",thread-id="1",stopped-threads="all",core="1" For now, let's focus only on the *stopped event. We see that the async output is missing frame info. And this causes a test failure in async mode, as "mi_expect_stop solib-event" wants to see the frame info. However, if we compare the event output when a real MI execution command is used, compared to a CLI command (e.g., run vs -exec-run, next vs -exec-next, etc.), we see: >./gdb -nx -q ./testsuite/gdb.mi/solib-main -ex "set stop-on-solib-events 1" -ex "set target-async off" -i=mi =thread-group-added,id="i1" ~"Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/solib-main..." ~"done.\n" (gdb) r &"r\n" ~"Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/solib-main \n" =thread-group-started,id="i1",pid="17751" =thread-created,id="1",group-id="i1" ^running *running,thread-id="all" (gdb) =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1" ~"Stopped due to shared library event (no libraries added or removed)\n" *stopped,reason="solib-event",frame={addr="0x000000379180f990",func="_dl_debug_state",args=[],from="/lib64/ld-linux-x86-64.so.2"},thread-id="1",stopped-threads="all",core="3" (gdb) -exec-run =thread-exited,id="1",group-id="i1" =thread-group-exited,id="i1" =library-unloaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",thread-group="i1" =thread-group-started,id="i1",pid="17754" =thread-created,id="1",group-id="i1" ^running *running,thread-id="all" (gdb) =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1" *stopped,reason="solib-event",thread-id="1",stopped-threads="all",core="1" =thread-selected,id="1" (gdb) As seen above, with MI commands, the *stopped event _doesn't_ have frame info. This is because normal_stop, as commanded by the result of bpstat_print, skips printing frame info in this case (it's an "event", not a "breakpoint"), and when the interpreter is MI, mi_on_normal_stop skips calling print_stack_frame, as the normal_stop call was already done with the MI uiout. This explains why the async output is different even with a CLI command. Its because in async mode, the mi_on_normal_stop path is always taken; it is always reached with the MI uiout, because the stop is handled from the event loop, instead of from within `proceed -> wait_for_inferior -> normal_stop' with the interpreter overridden, as in sync mode. This patch fixes the issue by making all cases output the same *stopped event, by factoring out the print code from normal_stop, and using it from mi_on_normal_stop as well. I chose the *stopped output without a frame, mainly because that is what you already get if you use MI execution commands, the commands frontends are supposed to use (except when implementing a console). This patch makes it simpler to tweak the MI output differently if desired, as we only have to change the centralized print_stop_event (taking into account whether the uiout is MI-like), and all different modes will change accordingly. Tested on x86_64 Fedora 16, no regressions. The mi-solib.exp test no longer fails in async mode with this patch, so the patch removes the kfail. 2012-05-09 Pedro Alves <palves@redhat.com> PR gdb/13860 gdb/ * inferior.h (print_stop_event): Declare. * infrun.c (print_stop_event): New, factored out from ... (normal_stop): ... this. * mi/mi-interp.c (mi_on_normal_stop): Use print_stop_event instead of bpstat_print/print_stack_frame. gdb/testsuite/ * gdb.mi/mi-solib.exp: Remove gdb/13860 kfail. * lib/mi-support.exp (mi_expect_stop): Add special handling for solib-event. --- gdb/inferior.h | 2 + gdb/infrun.c | 118 ++++++++++++++++++++------------------ gdb/mi/mi-interp.c | 3 +- gdb/testsuite/gdb.mi/mi-solib.exp | 4 -- gdb/testsuite/lib/mi-support.exp | 18 +++++- 5 files changed, 82 insertions(+), 63 deletions(-) diff --git a/gdb/inferior.h b/gdb/inferior.h index fff072b..cd6d962 100644 --- a/gdb/inferior.h +++ b/gdb/inferior.h @@ -205,6 +205,8 @@ extern void start_remote (int from_tty); extern void normal_stop (void); +extern void print_stop_event (struct target_waitstatus *ws); + extern int signal_stop_state (int); extern int signal_print_state (int); diff --git a/gdb/infrun.c b/gdb/infrun.c index b1d2dc5..49e8ab5 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -5964,6 +5964,68 @@ print_no_history_reason (void) ui_out_text (current_uiout, "\nNo more reverse-execution history.\n"); } +/* Print current location without a level number, if we have changed + functions or hit a breakpoint. Print source line if we have one. + bpstat_print contains the logic deciding in detail what to print, + based on the event(s) that just occurred. */ + +void +print_stop_event (struct target_waitstatus *ws) +{ + int bpstat_ret; + int source_flag; + int do_frame_printing = 1; + struct thread_info *tp = inferior_thread (); + + bpstat_ret = bpstat_print (tp->control.stop_bpstat, ws->kind); + switch (bpstat_ret) + { + case PRINT_UNKNOWN: + /* FIXME: cagney/2002-12-01: Given that a frame ID does (or + should) carry around the function and does (or should) use + that when doing a frame comparison. */ + if (tp->control.stop_step + && frame_id_eq (tp->control.step_frame_id, + get_frame_id (get_current_frame ())) + && step_start_function == find_pc_function (stop_pc)) + { + /* Finished step, just print source line. */ + source_flag = SRC_LINE; + } + else + { + /* Print location and source line. */ + source_flag = SRC_AND_LOC; + } + break; + case PRINT_SRC_AND_LOC: + /* Print location and source line. */ + source_flag = SRC_AND_LOC; + break; + case PRINT_SRC_ONLY: + source_flag = SRC_LINE; + break; + case PRINT_NOTHING: + /* Something bogus. */ + source_flag = SRC_LINE; + do_frame_printing = 0; + break; + default: + internal_error (__FILE__, __LINE__, _("Unknown value.")); + } + + /* The behavior of this routine with respect to the source + flag is: + SRC_LINE: Print only source line + LOCATION: Print only location + SRC_AND_LOC: Print location and source line. */ + if (do_frame_printing) + print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1); + + /* Display the auto-display expressions. */ + do_displays (); +} + /* Here to return control to GDB when the inferior stops for real. Print appropriate messages, remove breakpoints, give terminal our modes. @@ -6086,65 +6148,11 @@ normal_stop (void) { select_frame (get_current_frame ()); - /* Print current location without a level number, if - we have changed functions or hit a breakpoint. - Print source line if we have one. - bpstat_print() contains the logic deciding in detail - what to print, based on the event(s) that just occurred. */ - /* If --batch-silent is enabled then there's no need to print the current source location, and to try risks causing an error message about missing source files. */ if (stop_print_frame && !batch_silent) - { - int bpstat_ret; - int source_flag; - int do_frame_printing = 1; - struct thread_info *tp = inferior_thread (); - - bpstat_ret = bpstat_print (tp->control.stop_bpstat, last.kind); - switch (bpstat_ret) - { - case PRINT_UNKNOWN: - /* FIXME: cagney/2002-12-01: Given that a frame ID does - (or should) carry around the function and does (or - should) use that when doing a frame comparison. */ - if (tp->control.stop_step - && frame_id_eq (tp->control.step_frame_id, - get_frame_id (get_current_frame ())) - && step_start_function == find_pc_function (stop_pc)) - source_flag = SRC_LINE; /* Finished step, just - print source line. */ - else - source_flag = SRC_AND_LOC; /* Print location and - source line. */ - break; - case PRINT_SRC_AND_LOC: - source_flag = SRC_AND_LOC; /* Print location and - source line. */ - break; - case PRINT_SRC_ONLY: - source_flag = SRC_LINE; - break; - case PRINT_NOTHING: - source_flag = SRC_LINE; /* something bogus */ - do_frame_printing = 0; - break; - default: - internal_error (__FILE__, __LINE__, _("Unknown value.")); - } - - /* The behavior of this routine with respect to the source - flag is: - SRC_LINE: Print only source line - LOCATION: Print only location - SRC_AND_LOC: Print location and source line. */ - if (do_frame_printing) - print_stack_frame (get_selected_frame (NULL), 0, source_flag, 1); - - /* Display the auto-display expressions. */ - do_displays (); - } + print_stop_event (&last); } /* Save the function value return registers, if we care. diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index 44ebd21..57d997c 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -446,9 +446,8 @@ mi_on_normal_stop (struct bpstats *bs, int print_frame) current_uiout = mi_uiout; get_last_target_status (&last_ptid, &last); - bpstat_print (bs, last.kind); + print_stop_event (&last); - print_stack_frame (get_selected_frame (NULL), 0, SRC_AND_LOC, 1); current_uiout = saved_uiout; } diff --git a/gdb/testsuite/gdb.mi/mi-solib.exp b/gdb/testsuite/gdb.mi/mi-solib.exp index a760f92..cd400ea 100644 --- a/gdb/testsuite/gdb.mi/mi-solib.exp +++ b/gdb/testsuite/gdb.mi/mi-solib.exp @@ -60,8 +60,4 @@ mi_gdb_test "777-gdb-set stop-on-solib-events 1" "777\\^done" \ # commands still cause the correct MI output to be generated. mi_run_with_cli -global async -if { $async } { - setup_kfail gdb/13860 *-*-* -} mi_expect_stop solib-event .* .* .* .* .* "check for solib event" diff --git a/gdb/testsuite/lib/mi-support.exp b/gdb/testsuite/lib/mi-support.exp index 3d55609..6a67718 100644 --- a/gdb/testsuite/lib/mi-support.exp +++ b/gdb/testsuite/lib/mi-support.exp @@ -1013,6 +1013,8 @@ proc mi_expect_stop { reason func args file line extra test } { global thread_selected_re global breakpoint_re + set any "\[^\n\]*" + set after_stopped "" set after_reason "" if { [llength $extra] == 2 } { @@ -1055,6 +1057,20 @@ proc mi_expect_stop { reason func args file line extra test } { return } + if { $reason == "solib-event" } { + set pattern "\\*stopped,reason=\"solib-event\",thread-id=\"$decimal\",stopped-threads=$any\r\n($thread_selected_re|$breakpoint_re)*$prompt_re" + verbose -log "mi_expect_stop: expecting: $pattern" + gdb_expect { + -re "$pattern" { + pass "$test" + } + timeout { + fail "$test (unknown output after running)" + } + } + return + } + set args "\\\[$args\\\]" set bn "" @@ -1072,8 +1088,6 @@ proc mi_expect_stop { reason func args file line extra test } { set a $after_reason - set any "\[^\n\]*" - verbose -log "mi_expect_stop: expecting: \\*stopped,${r}${a}${bn}frame=\{addr=\"$hex\",func=\"$func\",args=$args,(?:file=\"$any$file\",fullname=\"${fullname_syntax}$file\",line=\"$line\"|from=\"$file\")\}$after_stopped,thread-id=\"$decimal\",stopped-threads=$any\r\n($thread_selected_re|$breakpoint_re)*$prompt_re" gdb_expect { -re "\\*stopped,${r}${a}${bn}frame=\{addr=\"$hex\",func=\"$func\",args=$args,(?:file=\"$any$file\",fullname=\"${fullname_syntax}$file\",line=\"($line)\"|from=\"$file\")\}$after_stopped,thread-id=\"$decimal\",stopped-threads=$any\r\n($thread_selected_re|$breakpoint_re)*$prompt_re" { -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 1/9] fix latent bugs in ui-out.c 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey 2013-10-22 17:59 ` [PATCH v4 5/9] PR gdb/13860: make "-exec-foo"'s MI output equal to "foo"'s MI output Tom Tromey @ 2013-10-22 17:59 ` Tom Tromey 2013-10-28 15:20 ` Pedro Alves 2013-10-22 17:59 ` [PATCH v4 3/9] add target method delegation Tom Tromey ` (6 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-22 17:59 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey The destructor code in ui-out.c has a latent bug, which is hidden by the fact that nothing uses this right now. This patch fixes the problem. The bug is that we don't always clear a pointer in the ui-out object, leading to bad a free. * ui-out.c (clear_table, ui_out_new): Clear uiout->table.id. --- gdb/ui-out.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/gdb/ui-out.c b/gdb/ui-out.c index 03b1240..3c6c529 100644 --- a/gdb/ui-out.c +++ b/gdb/ui-out.c @@ -807,8 +807,8 @@ uo_table_header (struct ui_out *uiout, int width, enum ui_align align, static void clear_table (struct ui_out *uiout) { - if (uiout->table.id) - xfree (uiout->table.id); + xfree (uiout->table.id); + uiout->table.id = NULL; clear_header_list (uiout); } @@ -1114,6 +1114,7 @@ ui_out_new (struct ui_out_impl *impl, void *data, current->field_count = 0; VEC_safe_push (ui_out_level_p, uiout->levels, current); + uiout->table.id = NULL; uiout->table.header_first = NULL; uiout->table.header_last = NULL; uiout->table.header_next = NULL; -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 1/9] fix latent bugs in ui-out.c 2013-10-22 17:59 ` [PATCH v4 1/9] fix latent bugs in ui-out.c Tom Tromey @ 2013-10-28 15:20 ` Pedro Alves 2013-10-28 17:36 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-10-28 15:20 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/22/2013 06:59 PM, Tom Tromey wrote: > The destructor code in ui-out.c has a latent bug, which is hidden by > the fact that nothing uses this right now. This patch fixes the > problem. The bug is that we don't always clear a pointer in the > ui-out object, leading to bad a free. "to a bad free" > > * ui-out.c (clear_table, ui_out_new): Clear uiout->table.id. Looks good to me. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 1/9] fix latent bugs in ui-out.c 2013-10-28 15:20 ` Pedro Alves @ 2013-10-28 17:36 ` Tom Tromey 0 siblings, 0 replies; 53+ messages in thread From: Tom Tromey @ 2013-10-28 17:36 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> On 10/22/2013 06:59 PM, Tom Tromey wrote: >> The destructor code in ui-out.c has a latent bug, which is hidden by >> the fact that nothing uses this right now. This patch fixes the >> problem. The bug is that we don't always clear a pointer in the >> ui-out object, leading to bad a free. Pedro> "to a bad free" Thanks, I've fixed this on my branch. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 3/9] add target method delegation 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey 2013-10-22 17:59 ` [PATCH v4 5/9] PR gdb/13860: make "-exec-foo"'s MI output equal to "foo"'s MI output Tom Tromey 2013-10-22 17:59 ` [PATCH v4 1/9] fix latent bugs in ui-out.c Tom Tromey @ 2013-10-22 17:59 ` Tom Tromey 2013-10-28 16:05 ` Pedro Alves 2013-10-22 17:59 ` [PATCH v4 4/9] PR gdb/13860: make -interpreter-exec console "list" behave more like "list" Tom Tromey ` (5 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-22 17:59 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey This patch replaces some code in the record targets with target method delegation. Right now there are two latent problems in the record target. First, record-full.c stores pointers to many target methods when the record target is pushed. Then it later delegates some calls via these. This is wrong because it violates the target stack contract. In particular it is ok to unpush a target at any stratum, but record-full does not keep track of this, so it could potentially call into an unpushed target. Second, RECORD_IS_USED and some other spots look at current_target.to_stratum to determine whether a record target is in use. This is bad because arch_stratum is greater than record_stratum. To fix the first problem, this patch introduces a handful of target_delegate_* functions, which forward calls further down the target stack. To fix the second problem, this patch adds find_target_at to determine whether a target appears at a given stratum. This may seem like overkill somehow, but I have a subsequent patch series (see archer.git tromey/multi-target) that uses it more heavily. * record-full.c (record_full_beneath_to_resume_ops) (record_full_beneath_to_resume, record_full_beneath_to_wait_ops) (record_full_beneath_to_wait) (record_full_beneath_to_store_registers_ops) (record_full_beneath_to_store_registers) (record_full_beneath_to_xfer_partial_ops) (record_full_beneath_to_xfer_partial) (record_full_beneath_to_insert_breakpoint) (record_full_beneath_to_remove_breakpoint) (record_full_beneath_to_stopped_by_watchpoint) (record_full_beneath_to_stopped_data_address) (record_full_beneath_to_async, tmp_to_resume_ops, tmp_to_resume) (tmp_to_wait_ops, tmp_to_wait, tmp_to_store_registers_ops) (tmp_to_store_registers, tmp_to_xfer_partial_ops) (tmp_to_xfer_partial, tmp_to_insert_breakpoint) (tmp_to_remove_breakpoint, tmp_to_stopped_by_watchpoint) (tmp_to_stopped_data_address, tmp_to_async): Remove. (record_full_open_1, record_full_open): Update. Use RECORD_IS_USED. (record_full_resume, record_full_wait_1) (record_full_stopped_by_watchpoint, record_full_stopped_data_address) (record_full_store_registers, record_full_xfer_partial) (record_full_insert_breakpoint, record_full_remove_breakpoint) (record_full_async, record_full_can_async_p, record_full_is_async_p) (record_full_core_xfer_partial): Use target delegation. * record.c (find_record_target): Use find_target_at. * record.h (RECORD_IS_USED): Use find_target_at. * target.c (update_current_target): Use target_delegate_xfer_partial. (target_delegate_xfer_partial): Now public. Renamed from... (current_xfer_partial): ...this. Remove. (target_delegate_async, target_delegate_is_async_p) (target_delegate_can_async_p, target_delegate_insert_breakpoint) (target_delegate_remove_breakpoint, target_delegate_wait) (target_delegate_resume, find_target_at) (target_delegate_store_registers) (target_delegate_stopped_by_watchpoint) (target_delegate_stopped_data_address): New functions. * target.h (target_delegate_async, target_delegate_is_async_p) (target_delegate_can_async_p, target_delegate_insert_breakpoint) (target_delegate_remove_breakpoint, target_delegate_wait) (target_delegate_resume, find_target_at) (target_delegate_store_registers) (target_delegate_stopped_by_watchpoint) (target_delegate_stopped_data_address) (target_delegate_xfer_partial): Declare. --- gdb/record-full.c | 209 ++++++------------------------------------------------ gdb/record.c | 8 +-- gdb/record.h | 2 +- gdb/target.c | 206 +++++++++++++++++++++++++++++++++++++++++++++++++---- gdb/target.h | 76 ++++++++++++++++++-- 5 files changed, 287 insertions(+), 214 deletions(-) diff --git a/gdb/record-full.c b/gdb/record-full.c index 6555f2d..8e577d3 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -215,43 +215,6 @@ static struct cmd_list_element *show_record_full_cmdlist; /* Command list for "record full". */ static struct cmd_list_element *record_full_cmdlist; -/* The beneath function pointers. */ -static struct target_ops *record_full_beneath_to_resume_ops; -static void (*record_full_beneath_to_resume) (struct target_ops *, ptid_t, int, - enum gdb_signal); -static struct target_ops *record_full_beneath_to_wait_ops; -static ptid_t (*record_full_beneath_to_wait) (struct target_ops *, ptid_t, - struct target_waitstatus *, - int); -static struct target_ops *record_full_beneath_to_store_registers_ops; -static void (*record_full_beneath_to_store_registers) (struct target_ops *, - struct regcache *, - int regno); -static struct target_ops *record_full_beneath_to_xfer_partial_ops; -static LONGEST - (*record_full_beneath_to_xfer_partial) (struct target_ops *ops, - enum target_object object, - const char *annex, - gdb_byte *readbuf, - const gdb_byte *writebuf, - ULONGEST offset, - LONGEST len); -static int - (*record_full_beneath_to_insert_breakpoint) (struct target_ops *, - struct gdbarch *, - struct bp_target_info *); -static int - (*record_full_beneath_to_remove_breakpoint) (struct target_ops *, - struct gdbarch *, - struct bp_target_info *); -static int (*record_full_beneath_to_stopped_by_watchpoint) (struct target_ops *); -static int (*record_full_beneath_to_stopped_data_address) (struct target_ops *, - CORE_ADDR *); -static void - (*record_full_beneath_to_async) (struct target_ops *, - void (*) (enum inferior_event_type, void *), - void *); - static void record_full_goto_insn (struct record_full_entry *entry, enum exec_direction_kind dir); static void record_full_save (const char *recfilename); @@ -798,36 +761,6 @@ record_full_exec_insn (struct regcache *regcache, } } -static struct target_ops *tmp_to_resume_ops; -static void (*tmp_to_resume) (struct target_ops *, ptid_t, int, - enum gdb_signal); -static struct target_ops *tmp_to_wait_ops; -static ptid_t (*tmp_to_wait) (struct target_ops *, ptid_t, - struct target_waitstatus *, - int); -static struct target_ops *tmp_to_store_registers_ops; -static void (*tmp_to_store_registers) (struct target_ops *, - struct regcache *, - int regno); -static struct target_ops *tmp_to_xfer_partial_ops; -static LONGEST (*tmp_to_xfer_partial) (struct target_ops *ops, - enum target_object object, - const char *annex, - gdb_byte *readbuf, - const gdb_byte *writebuf, - ULONGEST offset, - LONGEST len); -static int (*tmp_to_insert_breakpoint) (struct target_ops *, - struct gdbarch *, - struct bp_target_info *); -static int (*tmp_to_remove_breakpoint) (struct target_ops *ops, - struct gdbarch *, - struct bp_target_info *); -static int (*tmp_to_stopped_by_watchpoint) (struct target_ops *); -static int (*tmp_to_stopped_data_address) (struct target_ops *, CORE_ADDR *); -static void (*tmp_to_async) (struct target_ops *, - void (*) (enum inferior_event_type, void *), void *); - static void record_full_restore (void); /* Asynchronous signal handle registered as event loop source for when @@ -890,26 +823,6 @@ record_full_open_1 (char *name, int from_tty) error (_("Process record: the current architecture doesn't support " "record function.")); - if (!tmp_to_resume) - error (_("Could not find 'to_resume' method on the target stack.")); - if (!tmp_to_wait) - error (_("Could not find 'to_wait' method on the target stack.")); - if (!tmp_to_store_registers) - error (_("Could not find 'to_store_registers' " - "method on the target stack.")); - if (!tmp_to_insert_breakpoint) - error (_("Could not find 'to_insert_breakpoint' " - "method on the target stack.")); - if (!tmp_to_remove_breakpoint) - error (_("Could not find 'to_remove_breakpoint' " - "method on the target stack.")); - if (!tmp_to_stopped_by_watchpoint) - error (_("Could not find 'to_stopped_by_watchpoint' " - "method on the target stack.")); - if (!tmp_to_stopped_data_address) - error (_("Could not find 'to_stopped_data_address' " - "method on the target stack.")); - push_target (&record_full_ops); } @@ -926,83 +839,16 @@ record_full_open (char *name, int from_tty) fprintf_unfiltered (gdb_stdlog, "Process record: record_full_open\n"); /* Check if record target is already running. */ - if (current_target.to_stratum == record_stratum) + if (RECORD_IS_USED) error (_("Process record target already running. Use \"record stop\" to " "stop record target first.")); - /* Reset the tmp beneath pointers. */ - tmp_to_resume_ops = NULL; - tmp_to_resume = NULL; - tmp_to_wait_ops = NULL; - tmp_to_wait = NULL; - tmp_to_store_registers_ops = NULL; - tmp_to_store_registers = NULL; - tmp_to_xfer_partial_ops = NULL; - tmp_to_xfer_partial = NULL; - tmp_to_insert_breakpoint = NULL; - tmp_to_remove_breakpoint = NULL; - tmp_to_stopped_by_watchpoint = NULL; - tmp_to_stopped_data_address = NULL; - tmp_to_async = NULL; - - /* Set the beneath function pointers. */ - for (t = current_target.beneath; t != NULL; t = t->beneath) - { - if (!tmp_to_resume) - { - tmp_to_resume = t->to_resume; - tmp_to_resume_ops = t; - } - if (!tmp_to_wait) - { - tmp_to_wait = t->to_wait; - tmp_to_wait_ops = t; - } - if (!tmp_to_store_registers) - { - tmp_to_store_registers = t->to_store_registers; - tmp_to_store_registers_ops = t; - } - if (!tmp_to_xfer_partial) - { - tmp_to_xfer_partial = t->to_xfer_partial; - tmp_to_xfer_partial_ops = t; - } - if (!tmp_to_insert_breakpoint) - tmp_to_insert_breakpoint = t->to_insert_breakpoint; - if (!tmp_to_remove_breakpoint) - tmp_to_remove_breakpoint = t->to_remove_breakpoint; - if (!tmp_to_stopped_by_watchpoint) - tmp_to_stopped_by_watchpoint = t->to_stopped_by_watchpoint; - if (!tmp_to_stopped_data_address) - tmp_to_stopped_data_address = t->to_stopped_data_address; - if (!tmp_to_async) - tmp_to_async = t->to_async; - } - if (!tmp_to_xfer_partial) - error (_("Could not find 'to_xfer_partial' method on the target stack.")); - /* Reset */ record_full_insn_num = 0; record_full_insn_count = 0; record_full_list = &record_full_first; record_full_list->next = NULL; - /* Set the tmp beneath pointers to beneath pointers. */ - record_full_beneath_to_resume_ops = tmp_to_resume_ops; - record_full_beneath_to_resume = tmp_to_resume; - record_full_beneath_to_wait_ops = tmp_to_wait_ops; - record_full_beneath_to_wait = tmp_to_wait; - record_full_beneath_to_store_registers_ops = tmp_to_store_registers_ops; - record_full_beneath_to_store_registers = tmp_to_store_registers; - record_full_beneath_to_xfer_partial_ops = tmp_to_xfer_partial_ops; - record_full_beneath_to_xfer_partial = tmp_to_xfer_partial; - record_full_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint; - record_full_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint; - record_full_beneath_to_stopped_by_watchpoint = tmp_to_stopped_by_watchpoint; - record_full_beneath_to_stopped_data_address = tmp_to_stopped_data_address; - record_full_beneath_to_async = tmp_to_async; - if (core_bfd) record_full_core_open_1 (name, from_tty); else @@ -1126,8 +972,7 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step, /* Make sure the target beneath reports all signals. */ target_pass_signals (0, NULL); - record_full_beneath_to_resume (record_full_beneath_to_resume_ops, - ptid, step, signal); + target_delegate_resume (ops, ptid, step, signal); } /* We are about to start executing the inferior (or simulate it), @@ -1217,8 +1062,7 @@ record_full_wait_1 (struct target_ops *ops, if (record_full_resume_step) { /* This is a single step. */ - return record_full_beneath_to_wait (record_full_beneath_to_wait_ops, - ptid, status, options); + return target_delegate_wait (ops, ptid, status, options); } else { @@ -1229,8 +1073,7 @@ record_full_wait_1 (struct target_ops *ops, while (1) { - ret = record_full_beneath_to_wait - (record_full_beneath_to_wait_ops, ptid, status, options); + ret = target_delegate_wait (ops, ptid, status, options); if (status->kind == TARGET_WAITKIND_IGNORE) { if (record_debug) @@ -1314,9 +1157,7 @@ record_full_wait_1 (struct target_ops *ops, "Process record: record_full_wait " "issuing one more step in the " "target beneath\n"); - record_full_beneath_to_resume - (record_full_beneath_to_resume_ops, ptid, step, - GDB_SIGNAL_0); + target_delegate_resume (ops, ptid, step, GDB_SIGNAL_0); continue; } } @@ -1520,7 +1361,7 @@ record_full_stopped_by_watchpoint (struct target_ops *ops) if (RECORD_FULL_IS_REPLAY) return record_full_hw_watchpoint; else - return record_full_beneath_to_stopped_by_watchpoint (find_target_beneath (ops)); + return target_delegate_stopped_by_watchpoint (ops); } static int @@ -1529,7 +1370,7 @@ record_full_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p) if (RECORD_FULL_IS_REPLAY) return 0; else - return record_full_beneath_to_stopped_data_address (ops, addr_p); + return target_delegate_stopped_data_address (ops, addr_p); } /* Record registers change (by user or by GDB) to list as an instruction. */ @@ -1632,8 +1473,7 @@ record_full_store_registers (struct target_ops *ops, record_full_registers_change (regcache, regno); } - record_full_beneath_to_store_registers - (record_full_beneath_to_store_registers_ops, regcache, regno); + target_delegate_store_registers (ops, regcache, regno); } /* "to_xfer_partial" method. Behavior is conditional on @@ -1698,9 +1538,8 @@ record_full_xfer_partial (struct target_ops *ops, enum target_object object, record_full_insn_num++; } - return record_full_beneath_to_xfer_partial - (record_full_beneath_to_xfer_partial_ops, object, annex, - readbuf, writebuf, offset, len); + return target_delegate_xfer_partial (ops, object, annex, + readbuf, writebuf, offset, len); } /* This structure represents a breakpoint inserted while the record @@ -1781,8 +1620,7 @@ record_full_insert_breakpoint (struct target_ops *ops, int ret; old_cleanups = record_full_gdb_operation_disable_set (); - ret = record_full_beneath_to_insert_breakpoint (find_target_beneath (ops), - gdbarch, bp_tgt); + ret = target_delegate_insert_breakpoint (ops, gdbarch, bp_tgt); do_cleanups (old_cleanups); if (ret != 0) @@ -1823,8 +1661,7 @@ record_full_remove_breakpoint (struct target_ops *ops, int ret; old_cleanups = record_full_gdb_operation_disable_set (); - ret = record_full_beneath_to_remove_breakpoint (find_target_beneath (ops), - gdbarch, bp_tgt); + ret = target_delegate_remove_breakpoint (ops, gdbarch, bp_tgt); do_cleanups (old_cleanups); if (ret != 0) @@ -1906,22 +1743,19 @@ record_full_async (struct target_ops *ops, /* If we're on top of a line target (e.g., linux-nat, remote), then set it to async mode as well. Will be NULL if we're sitting on top of the core target, for "record restore". */ - if (record_full_beneath_to_async != NULL) - record_full_beneath_to_async (find_target_beneath (ops), callback, context); + target_delegate_async (ops, callback, context); } static int record_full_can_async_p (struct target_ops *ops) { - /* We only enable async when the user specifically asks for it. */ - return target_async_permitted; + return target_delegate_can_async_p (ops); } static int record_full_is_async_p (struct target_ops *ops) { - /* We only enable async when the user specifically asks for it. */ - return target_async_permitted; + return target_delegate_is_async_p (ops); } static enum exec_direction_kind @@ -2247,10 +2081,10 @@ record_full_core_xfer_partial (struct target_ops *ops, else { if (!entry) - return record_full_beneath_to_xfer_partial - (record_full_beneath_to_xfer_partial_ops, - object, annex, readbuf, writebuf, - offset, len); + return target_delegate_xfer_partial (ops, + object, annex, + readbuf, writebuf, + offset, len); memcpy (readbuf, entry->buf + sec_offset, (size_t) len); @@ -2266,9 +2100,8 @@ record_full_core_xfer_partial (struct target_ops *ops, error (_("You can't do that without a process to debug.")); } - return record_full_beneath_to_xfer_partial - (record_full_beneath_to_xfer_partial_ops, object, annex, - readbuf, writebuf, offset, len); + return target_delegate_xfer_partial (ops, object, annex, + readbuf, writebuf, offset, len); } /* "to_insert_breakpoint" method for prec over corefile. */ diff --git a/gdb/record.c b/gdb/record.c index 4078def..1d113cb 100644 --- a/gdb/record.c +++ b/gdb/record.c @@ -62,13 +62,7 @@ struct cmd_list_element *info_record_cmdlist = NULL; static struct target_ops * find_record_target (void) { - struct target_ops *t; - - for (t = current_target.beneath; t != NULL; t = t->beneath) - if (t->to_stratum == record_stratum) - return t; - - return NULL; + return find_target_at (record_stratum); } /* Check that recording is active. Throw an error, if it isn't. */ diff --git a/gdb/record.h b/gdb/record.h index 65d508f..5f0dfbd 100644 --- a/gdb/record.h +++ b/gdb/record.h @@ -22,7 +22,7 @@ struct cmd_list_element; -#define RECORD_IS_USED (current_target.to_stratum == record_stratum) +#define RECORD_IS_USED (find_target_at (record_stratum) != NULL) extern unsigned int record_debug; diff --git a/gdb/target.c b/gdb/target.c index 1b6fe5c..81bf55b 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -75,12 +75,6 @@ static LONGEST default_xfer_partial (struct target_ops *ops, const gdb_byte *writebuf, ULONGEST offset, LONGEST len); -static LONGEST current_xfer_partial (struct target_ops *ops, - enum target_object object, - const char *annex, gdb_byte *readbuf, - const gdb_byte *writebuf, - ULONGEST offset, LONGEST len); - static struct gdbarch *default_thread_architecture (struct target_ops *ops, ptid_t ptid); @@ -852,7 +846,7 @@ update_current_target (void) de_fault (to_stop, (void (*) (ptid_t)) target_ignore); - current_target.to_xfer_partial = current_xfer_partial; + current_target.to_xfer_partial = target_delegate_xfer_partial; de_fault (to_rcmd, (void (*) (char *, struct ui_file *)) tcomplain); @@ -1996,14 +1990,14 @@ default_xfer_partial (struct target_ops *ops, enum target_object object, return -1; } -/* The xfer_partial handler for the topmost target. Unlike the default, - it does not need to handle memory specially; it just passes all - requests down the stack. */ +/* See target.h. */ -static LONGEST -current_xfer_partial (struct target_ops *ops, enum target_object object, - const char *annex, gdb_byte *readbuf, - const gdb_byte *writebuf, ULONGEST offset, LONGEST len) +LONGEST +target_delegate_xfer_partial (struct target_ops *ops, + enum target_object object, + const char *annex, gdb_byte *readbuf, + const gdb_byte *writebuf, + ULONGEST offset, LONGEST len) { if (ops->beneath != NULL) return ops->beneath->to_xfer_partial (ops->beneath, object, annex, @@ -2012,6 +2006,53 @@ current_xfer_partial (struct target_ops *ops, enum target_object object, return -1; } +/* See target.h. */ + +void +target_delegate_async (struct target_ops *self, + void (*callback) (enum inferior_event_type, void *), + void *datum) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_async) + { + t->to_async (t, callback, datum); + break; + } + } +} + +/* See target.h. */ + +int +target_delegate_is_async_p (struct target_ops *self) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + if (t->to_is_async_p != NULL) + return t->to_is_async_p (t); + + gdb_assert_not_reached (_("reached end of target stack during delegation")); +} + +/* See target.h. */ + +int +target_delegate_can_async_p (struct target_ops *self) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + if (t->to_can_async_p != NULL) + return t->to_can_async_p (t); + + gdb_assert_not_reached (_("reached end of target stack during delegation")); +} + /* Target vector read/write partial wrapper functions. */ static LONGEST @@ -2463,6 +2504,24 @@ target_insert_breakpoint (struct gdbarch *gdbarch, gdbarch, bp_tgt); } +/* See target.h. */ + +int +target_delegate_insert_breakpoint (struct target_ops *self, + struct gdbarch *gdbarch, + struct bp_target_info *bp_tgt) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_insert_breakpoint) + return t->to_insert_breakpoint (t, gdbarch, bp_tgt); + } + + gdb_assert_not_reached (_("reached end of target stack during delegation")); +} + int target_remove_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) @@ -2481,6 +2540,24 @@ target_remove_breakpoint (struct gdbarch *gdbarch, gdbarch, bp_tgt); } +/* See target.h. */ + +int +target_delegate_remove_breakpoint (struct target_ops *self, + struct gdbarch *gdbarch, + struct bp_target_info *bp_tgt) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_remove_breakpoint) + return t->to_remove_breakpoint (t, gdbarch, bp_tgt); + } + + gdb_assert_not_reached (_("reached end of target stack during delegation")); +} + static void target_info (char *args, int from_tty) { @@ -2688,6 +2765,24 @@ target_wait (ptid_t ptid, struct target_waitstatus *status, int options) noprocess (); } +/* See target.h. */ + +ptid_t +target_delegate_wait (struct target_ops *self, + ptid_t ptid, struct target_waitstatus *status, + int options) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_wait) + return t->to_wait (t, ptid, status, options); + } + + gdb_assert_not_reached (_("reached end of target stack during delegation")); +} + char * target_pid_to_str (ptid_t ptid) { @@ -2745,6 +2840,24 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal) noprocess (); } +/* See target.h. */ + +void +target_delegate_resume (struct target_ops *self, + ptid_t ptid, int step, enum gdb_signal signal) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_resume) + { + t->to_resume (t, ptid, step, signal); + break; + } + } +} + void target_pass_signals (int numsigs, unsigned char *pass_signals) { @@ -3641,6 +3754,20 @@ find_target_beneath (struct target_ops *t) return t->beneath; } +/* See target.h. */ + +struct target_ops * +find_target_at (enum strata stratum) +{ + struct target_ops *t; + + for (t = current_target.beneath; t != NULL; t = t->beneath) + if (t->to_stratum == stratum) + return t; + + return NULL; +} + \f /* The inferior process has died. Long live the inferior! */ @@ -3994,6 +4121,24 @@ target_store_registers (struct regcache *regcache, int regno) noprocess (); } +/* See target.h. */ + +void +target_delegate_store_registers (struct target_ops *self, + struct regcache *regcache, int regno) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_store_registers) + { + t->to_store_registers (t, regcache, regno); + break; + } + } +} + int target_core_of_thread (ptid_t ptid) { @@ -4127,6 +4272,39 @@ target_ranged_break_num_registers (void) /* See target.h. */ int +target_delegate_stopped_by_watchpoint (struct target_ops *self) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_stopped_by_watchpoint) + return t->to_stopped_by_watchpoint (t); + } + + gdb_assert_not_reached (_("reached end of target stack during delegation")); +} + +/* See target.h. */ + +int +target_delegate_stopped_data_address (struct target_ops *self, + CORE_ADDR *addr_p) +{ + struct target_ops *t; + + for (t = self->beneath; t != NULL; t = t->beneath) + { + if (t->to_stopped_data_address) + return t->to_stopped_data_address (t, addr_p); + } + + gdb_assert_not_reached (_("reached end of target stack during delegation")); +} + +/* See target.h. */ + +int target_supports_btrace (void) { struct target_ops *t; diff --git a/gdb/target.h b/gdb/target.h index 027e0f8..b1a9a69 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -972,6 +972,12 @@ extern void target_disconnect (char *, int); extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal); +/* Delegate "target_resume" to a target beneath SELF. */ + +extern void target_delegate_resume (struct target_ops *self, + ptid_t ptid, int step, + enum gdb_signal signal); + /* Wait for process pid to do something. PTID = -1 to wait for any pid to do something. Return pid of child, or -1 in case of error; store status through argument pointer STATUS. Note that it is @@ -984,6 +990,13 @@ extern void target_resume (ptid_t ptid, int step, enum gdb_signal signal); extern ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status, int options); +/* Delegate "target_wait" to a target beneath SELF. */ + +extern ptid_t target_delegate_wait (struct target_ops *self, + ptid_t ptid, + struct target_waitstatus *status, + int options); + /* Fetch at least register REGNO, or all regs if regno == -1. No result. */ extern void target_fetch_registers (struct regcache *regcache, int regno); @@ -994,6 +1007,12 @@ extern void target_fetch_registers (struct regcache *regcache, int regno); extern void target_store_registers (struct regcache *regcache, int regs); +/* Delegate "target_store_registers" to a target beneath SELF. */ + +extern void target_delegate_store_registers (struct target_ops *self, + struct regcache *regcache, + int regno); + /* Get ready to modify the registers array. On machines which store individual registers, this doesn't need to do anything. On machines which store all the registers in one fell swoop, this makes sure @@ -1126,12 +1145,24 @@ int target_write_memory_blocks (VEC(memory_write_request_s) *requests, extern int target_insert_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt); +/* Delegate "target_insert_breakpoint" to a target beneath SELF. */ + +extern int target_delegate_insert_breakpoint (struct target_ops *self, + struct gdbarch *gdbarch, + struct bp_target_info *bp_tgt); + /* Remove a breakpoint at address BP_TGT->placed_address in the target machine. Result is 0 for success, non-zero for error. */ extern int target_remove_breakpoint (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt); +/* Delegate "target_remove_breakpoint" to a target beneath SELF. */ + +extern int target_delegate_remove_breakpoint (struct target_ops *self, + struct gdbarch *gdbarch, + struct bp_target_info *bp_tgt); + /* Initialize the terminal settings we record for the inferior, before we actually run the inferior. */ @@ -1408,16 +1439,30 @@ extern int default_child_has_execution (struct target_ops *ops, extern int target_async_permitted; /* Can the target support asynchronous execution? */ -#define target_can_async_p() (current_target.to_can_async_p (¤t_target)) +#define target_can_async_p() (target_delegate_can_async_p (¤t_target)) + +/* Delegate "target_can_async_p" to a target beneath SELF. */ + +extern int target_delegate_can_async_p (struct target_ops *self); /* Is the target in asynchronous execution mode? */ -#define target_is_async_p() (current_target.to_is_async_p (¤t_target)) +#define target_is_async_p() (target_delegate_is_async_p (¤t_target)) + +/* Delegate "target_is_async_p" to a target beneath SELF. */ + +extern int target_delegate_is_async_p (struct target_ops *self); int target_supports_non_stop (void); /* Put the target in async mode with the specified callback function. */ #define target_async(CALLBACK,CONTEXT) \ - (current_target.to_async (¤t_target, (CALLBACK), (CONTEXT))) + (target_delegate_async (¤t_target, (CALLBACK), (CONTEXT))) + +/* Delegate "target_async" to a target beneath SELF. */ + +extern void target_delegate_async (struct target_ops *, + void (*) (enum inferior_event_type, void *), + void *); #define target_execution_direction() \ (current_target.to_execution_direction ()) @@ -1492,7 +1537,11 @@ extern char *target_thread_name (struct thread_info *); write). Only the INFERIOR_PTID task is being queried. */ #define target_stopped_by_watchpoint() \ - ((*current_target.to_stopped_by_watchpoint) (¤t_target)) + (target_delegate_stopped_by_watchpoint (¤t_target)) + +/* Delegate "target_stopped_by_watchpoint" to a target beneath SELF. */ + +extern int target_delegate_stopped_by_watchpoint (struct target_ops *self); /* Non-zero if we have steppable watchpoints */ @@ -1567,6 +1616,11 @@ extern int target_ranged_break_num_registers (void); #define target_stopped_data_address(target, addr_p) \ (*target.to_stopped_data_address) (target, addr_p) +/* Delegate "target_stopped_data_address" to a target beneath SELF. */ + +extern int target_delegate_stopped_data_address (struct target_ops *self, + CORE_ADDR *addr_p); + /* Return non-zero if ADDR is within the range of a watchpoint spanning LENGTH bytes beginning at START. */ #define target_watchpoint_addr_within_range(target, addr, start, length) \ @@ -1897,6 +1951,11 @@ extern void find_default_create_inferior (struct target_ops *, extern struct target_ops *find_target_beneath (struct target_ops *); +/* Find the target at STRATUM. If no target is at that stratum, + return NULL. */ + +struct target_ops *find_target_at (enum strata stratum); + /* Read OS data object of type TYPE from the target, and return it in XML format. The result is NUL-terminated and returned as a string, allocated using xmalloc. If an error occurs or the transfer is @@ -1999,4 +2058,13 @@ extern void target_call_history_from (ULONGEST begin, int size, int flags); /* See to_call_history_range. */ extern void target_call_history_range (ULONGEST begin, ULONGEST end, int flags); +/* Delegate "target_xfer_partial" to a target beneath SELF. */ + +extern LONGEST target_delegate_xfer_partial (struct target_ops *ops, + enum target_object object, + const char *annex, + gdb_byte *readbuf, + const gdb_byte *writebuf, + ULONGEST offset, LONGEST len); + #endif /* !defined (TARGET_H) */ -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-10-22 17:59 ` [PATCH v4 3/9] add target method delegation Tom Tromey @ 2013-10-28 16:05 ` Pedro Alves 2013-10-28 17:51 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-10-28 16:05 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/22/2013 06:59 PM, Tom Tromey wrote: > This patch replaces some code in the record targets with target method > delegation. > > Right now there are two latent problems in the record target. > > First, record-full.c stores pointers to many target methods when the > record target is pushed. Then it later delegates some calls via > these. This is wrong because it violates the target stack contract. > In particular it is ok to unpush a target at any stratum, but > record-full does not keep track of this, so it could potentially call > into an unpushed target. > > Second, RECORD_IS_USED and some other spots look at > current_target.to_stratum to determine whether a record target is in > use. This is bad because arch_stratum is greater than record_stratum. > > To fix the first problem, this patch introduces a handful of > target_delegate_* functions, which forward calls further down the > target stack. > > To fix the second problem, this patch adds find_target_at to determine > whether a target appears at a given stratum. This may seem like > overkill somehow, but I have a subsequent patch series (see archer.git > tromey/multi-target) that uses it more heavily. Hmm, I think this is trying to do too much at once. Could you please split out the patch for the second problem? I think it'll be a small one. Then changes like these: > #define target_stopped_by_watchpoint() \ > - ((*current_target.to_stopped_by_watchpoint) (¤t_target)) > + (target_delegate_stopped_by_watchpoint (¤t_target)) > + > +/* Delegate "target_stopped_by_watchpoint" to a target beneath SELF. */ > + > +extern int target_delegate_stopped_by_watchpoint (struct target_ops *self); switch from using the inheritance scheme to the run-time walk mechanism, but leave update_current_target still doing the INHERIT/de_fault business. What's the plan for the existing target methods that currently already do a similar beneath lookup? I'd like it that there's be at least a plan, so we don't end up with yet another way of doing things, two incomplete transitions, and no clear direction. > + gdb_assert_not_reached (_("reached end of target stack during delegation")); > +} This appears in several places. Whenever I see the same string repeated over and over, I tend to think it'd be good to add a utility helper function: static void assert_end_of_stack_not_reached_or_something (void) { gdb_assert_not_reached (_("reached end of target stack during delegation")); } Some of the delegation methods have that assert, while others don't have anything at the tail end. What's the story there? > + > +int > +target_delegate_insert_breakpoint (struct target_ops *self, > + struct gdbarch *gdbarch, > + struct bp_target_info *bp_tgt) > +{ > + struct target_ops *t; > + > + for (t = self->beneath; t != NULL; t = t->beneath) > + { > + if (t->to_insert_breakpoint) if (t->to_insert_breakpoint != NULL) -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-10-28 16:05 ` Pedro Alves @ 2013-10-28 17:51 ` Tom Tromey 2013-10-28 17:53 ` Tom Tromey 2013-11-08 16:34 ` Pedro Alves 0 siblings, 2 replies; 53+ messages in thread From: Tom Tromey @ 2013-10-28 17:51 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> Could you please split out the patch for the second problem? I Pedro> think it'll be a small one. Sure. Pedro> What's the plan for the existing target methods that Pedro> currently already do a similar beneath lookup? I'd like it that Pedro> there's be at least a plan, so we don't end up with yet another Pedro> way of doing things, two incomplete transitions, and no clear direction. Do you mean things like target_detach? If so, then I think these are two different things. target.h declares things both for users of the target API and for the implementation of targets. Something like target_detach is a public API. My understanding of the current scheme is that a public-facing method can either be implemented by a function like target_detach, which encodes all the needed logic; or by a macro that just calls into the target_ops, in which case the method must use the inherit/default machinery. On the other hand, target_delegate_* functions are there for the target implementations to use. They just encapsulate a bit of common code for the case where a target wants to delegate a request to the next lower target on the stack. >> + gdb_assert_not_reached (_("reached end of target stack during >> delegation")); >> +} Pedro> This appears in several places. Whenever I see the same string Pedro> repeated over and over, I tend to think it'd be good to add a Pedro> utility helper function: Will do. Pedro> Some of the delegation methods have that assert, while others don't Pedro> have anything at the tail end. What's the story there? I don't remember. It does seem a bit random right now. It seems to me that the best approach is to make a target_delegate_* function assert only when there is no de_fault for the corresponding method. >> + if (t->to_insert_breakpoint) Pedro> if (t->to_insert_breakpoint != NULL) I fixed all the instances of it. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-10-28 17:51 ` Tom Tromey @ 2013-10-28 17:53 ` Tom Tromey 2013-10-29 20:55 ` Tom Tromey 2013-11-08 16:34 ` Pedro Alves 1 sibling, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-28 17:53 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes: Tom> It seems to me that the best approach is to make a target_delegate_* Tom> function assert only when there is no de_fault for the corresponding Tom> method. I mean, when there *is* a de_fault. The idea is that if there is a de_fault, then the slot cannot be NULL. So, delegation must never fail. On the other hand, if there is no de_fault, then the slot can be NULL, and what exactly to do depends on the method in question. But looking more closely at the code on the branch, there is an assertion in those methods returning something other than void. I'll think about it some more. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-10-28 17:53 ` Tom Tromey @ 2013-10-29 20:55 ` Tom Tromey 2013-11-08 17:44 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-29 20:55 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Tom> But looking more closely at the code on the branch, there is an Tom> assertion in those methods returning something other than void. Tom> I'll think about it some more. I looked at all the delegation functions today. I think it would be fine to make nearly all of them assert. The two exceptions are target_delegate_xfer_partial (already does not assert) and target_delegate_wait (which does assert but which I think should not). In all other cases there is either a de_fault call for the method, or the dummy target implements the method. target_delegate_wait is a tricky one, as it returns a value. Perhaps just throwing an exception is best. The current code isn't much of a guide because it throws the exception when the record target is pushed -- but as noted in the thread, this is not robust as the target stack can change even after a target is pushed. Your comments on this would be much appreciated. Some thoughts the target vector. I think the underlying problem here is complex, so it is reasonable if the model is as well. That is, I think it's fine to combine inheritance (e.g., the various linux-* vectors) with delegation (the whole stack itself plus special hacks in record and maybe elsewhere). That in itself is tractable. However, I think the combination of using INHERIT, plus de_fault, plus the dummy target, plus special wrappers for some target APIs leads to madness. It's much too hard to navigate this. I think we should adopt some simpler rule. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-10-29 20:55 ` Tom Tromey @ 2013-11-08 17:44 ` Pedro Alves 2013-12-11 22:03 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-11-08 17:44 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/29/2013 08:55 PM, Tom Tromey wrote: > Tom> But looking more closely at the code on the branch, there is an > Tom> assertion in those methods returning something other than void. > > Tom> I'll think about it some more. > > I looked at all the delegation functions today. > > I think it would be fine to make nearly all of them assert. > > The two exceptions are target_delegate_xfer_partial (already does not > assert) and target_delegate_wait (which does assert but which I think > should not). > > In all other cases there is either a de_fault call for the method, or > the dummy target implements the method. The de_fault only applies to current_target. As the delegation always starts at ops->beneath, the de_fault shouldn't ever come into play. So target methods that do the beneath walk either have the choice of having a default in the target method itself, or installing it in the dummy target. Off hand, I don't think there's a real behavioral difference. Looks like the sort of thing that could be normalized. > > target_delegate_wait is a tricky one, as it returns a value. Perhaps > just throwing an exception is best. The current code isn't much of a > guide because it throws the exception when the record target is pushed > -- but as noted in the thread, this is not robust as the target stack > can change even after a target is pushed. So to take that example, if we made dummy_target.to_wait be the current to_wait default, which is to call noprocess(), then it'd be clear that target_delegate_wait shouldn't ever go past the loop, and then it'd be clear that an assertion is appropriate. target_wait would then be: ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status, int options) { struct target_ops *t; ptid_t retval; retval = target_delegate_wait (¤t_traget, ptid, status, options); if (targetdebug) { char *status_string; char *options_string; status_string = target_waitstatus_to_string (status); options_string = target_options_to_string (options); fprintf_unfiltered (gdb_stdlog, "target_wait (%d, status, options={%s})" " = %d, %s\n", ptid_get_pid (ptid), options_string, ptid_get_pid (retval), status_string); xfree (status_string); xfree (options_string); } } WDYT? > > Your comments on this would be much appreciated. > > > Some thoughts the target vector. > > I think the underlying problem here is complex, so it is reasonable if > the model is as well. That is, I think it's fine to combine inheritance > (e.g., the various linux-* vectors) with delegation (the whole stack > itself plus special hacks in record and maybe elsewhere). That in > itself is tractable. > > However, I think the combination of using INHERIT, plus de_fault, plus > the dummy target, plus special wrappers for some target APIs leads to > madness. > > It's much too hard to navigate this. I think we should adopt some > simpler rule. Yes, agreed. That's why I'm trying to see if we can reuse the delegation with the public API. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-11-08 17:44 ` Pedro Alves @ 2013-12-11 22:03 ` Tom Tromey 2013-12-12 2:46 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-11 22:03 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> So to take that example, if we made dummy_target.to_wait be the Pedro> current to_wait default, which is to call noprocess(), then Pedro> it'd be clear that target_delegate_wait shouldn't ever go past Pedro> the loop, and then it'd be clear that an assertion is appropriate. Pedro> target_wait would then be: [...] Pedro> WDYT? It seems good to me. I'll make a patch to normalize the delegation across all the target methods. Too bad for me I already touched all these methods on the multi-target branch -- ouch. Testing revealed that we can't put the assert into target_delegate_async. This is a funny one. The dummy target has: dummy_target.to_can_async_p = find_default_can_async_p; ... but this means that it is possible for to_can_async_p to return true while the target does not actually implement to_async. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-11 22:03 ` Tom Tromey @ 2013-12-12 2:46 ` Tom Tromey 2013-12-13 22:07 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-12 2:46 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro> target_wait would then be: Tom> [...] Pedro> WDYT? Tom> It seems good to me. I tinkered with this a little and, not liking the result much, spent some more time thinking about it. Specifically, I considered how I would write this in C++. There I think what I would do is have a pure-virtual target base class. This corresponds to target_ops. Then, I'd have a "delegator" subclass from which all ordinary targets would derive. This class would implement each delegatable method by unconditionally forwarding to "beneath": struct target_delegator : public target_ops { // e.g. int has_all_memory () { return beneath->has_all_memory(); } }; I'd make the dummy target implement all methods using some baseline behavior. struct dummy_target : public target_ops { int has_all_memory () { return 0; } }; Finally, individual targets would derive from target_delegator and override methods as appropriate. Translating back to gdb, rather than implement target_delegate_* functions that search through the target stack, what if we implement the appropriate dummy fallback functions, and have delegation functions that call via "beneath"? We can fill in the fields of target_ops in complete_target_initialization. Since this is very repetitive I would consider doing it via a ".defs" file and then some macrology to reduce the amount of boilerplate. Let me know what you think. I'll experiment with it a bit tomorrow. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-12 2:46 ` Tom Tromey @ 2013-12-13 22:07 ` Tom Tromey 2013-12-16 13:07 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-13 22:07 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 2611 bytes --] >>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes: Tom> Since this is very repetitive I would consider doing it via a ".defs" Tom> file and then some macrology to reduce the amount of boilerplate. I tried this a bit and the result was pretty ugly. Instead I wrote a little script that can parse just enough of "struct target_ops" to auto-generate delegation methods when appropriate. See patch #1 attached below. This patch just converts the methods used by the record targets -- that is, it's just a replacement for patch #3 in this series. Note that the patch isn't ready as-is. The log entry is wrong and it does the wrong thing for a couple of the target methods. I'm curious to know what you think about the overall shape of it. As an example of the benefit of this approach, patch #2 converts to_detach to the new style. When examining this patch recall that target-delegates.c is auto-generated. You can see from this that the result is less code to maintain. I then went a little further. It seems to me that we also need a lot of boilerplate (not to mention the questionable casts there right now) to handle the dummy target methods. Patch #3 makes these into simple annotations in the target.h file, getting rid of the list of handled methods from make-target-delegates as well. Before reaching judgment on patch #3, see patch #4, which converts to_attach. I've written a few more like it. The long-term vision for this approach is to regularize all target methods according to these rules: 1. Each method will take a "struct target_ops *" as its first argument. 2. Nearly every method will be defaulted to the appropriate delegate_ method. There may be exceptions, coded into complete_target_initialization. 3. de_fault will be completely eliminated. INHERIT will be limited to the smallest possible list of fields, like to_shortname. Nearly every current_target method will simply delegate. 4. Target API users will call top-level target_* functions that will in most cases just call into current_target. 5. Any given to_* method implementation will be guaranteed to be called with the target_ops with which it was registered. (This rule is handy on multi-target, where I split out the target_ops vtable from the object itself.) Since I'm on a cleanup kick at the moment, and since a good amount of this is relevant to the multi-target work, I think it would be good to start the conversion shortly after 7.7 is branched. This is a bit difficult since I don't have access to many targets. I'm not sure what to do about this aspect. Tom [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: patch #1 --] [-- Type: text/x-patch, Size: 41183 bytes --] From eb626875b5fce4e5d297da897379a0f0427fab1d Mon Sep 17 00:00:00 2001 From: Tom Tromey <tromey@redhat.com> Date: Fri, 12 Jul 2013 13:00:34 -0600 Subject: [PATCH 05/11] add target method delegation MUST UPDATE This patch replaces some code in the record targets with target method delegation. record-full.c stores pointers to many target methods when the record target is pushed. Then it later delegates some calls via these. This is wrong because it violates the target stack contract. In particular it is ok to unpush a target at any stratum, but record-full does not keep track of this, so it could potentially call into an unpushed target. To fix the problem, this patch introduces a handful of target_delegate_* functions, which forward calls further down the target stack. 2013-10-30 Tom Tromey <tromey@redhat.com> * record-full.c (record_full_beneath_to_resume_ops) (record_full_beneath_to_resume, record_full_beneath_to_wait_ops) (record_full_beneath_to_wait) (record_full_beneath_to_store_registers_ops) (record_full_beneath_to_store_registers) (record_full_beneath_to_xfer_partial_ops) (record_full_beneath_to_xfer_partial) (record_full_beneath_to_insert_breakpoint) (record_full_beneath_to_remove_breakpoint) (record_full_beneath_to_stopped_by_watchpoint) (record_full_beneath_to_stopped_data_address) (record_full_beneath_to_async, tmp_to_resume_ops, tmp_to_resume) (tmp_to_wait_ops, tmp_to_wait, tmp_to_store_registers_ops) (tmp_to_store_registers, tmp_to_xfer_partial_ops) (tmp_to_xfer_partial, tmp_to_insert_breakpoint) (tmp_to_remove_breakpoint, tmp_to_stopped_by_watchpoint) (tmp_to_stopped_data_address, tmp_to_async): Remove. (record_full_open_1, record_full_open): Update. Use RECORD_IS_USED. (record_full_resume, record_full_wait_1) (record_full_stopped_by_watchpoint, record_full_stopped_data_address) (record_full_store_registers, record_full_xfer_partial) (record_full_insert_breakpoint, record_full_remove_breakpoint) (record_full_async, record_full_can_async_p, record_full_is_async_p) (record_full_core_xfer_partial): Use target delegation. * target.c (update_current_target): Use target_delegate_xfer_partial. (target_delegate_xfer_partial): Now public. Renamed from... (current_xfer_partial): ...this. Remove. (target_delegate_async, target_delegate_is_async_p) (target_delegate_can_async_p, target_delegate_insert_breakpoint) (target_delegate_remove_breakpoint, target_delegate_wait) (target_delegate_resume, target_delegate_store_registers) (target_delegate_stopped_by_watchpoint) (target_delegate_stopped_data_address) (assert_delegation_failed): New functions. * target.h (target_delegate_async, target_delegate_is_async_p) (target_delegate_can_async_p, target_delegate_insert_breakpoint) (target_delegate_remove_breakpoint, target_delegate_wait) (target_delegate_resume, target_delegate_store_registers) (target_delegate_stopped_by_watchpoint) (target_delegate_stopped_data_address) (target_delegate_xfer_partial): Declare. --- gdb/ChangeLog | 44 +++++++++ gdb/make-target-delegates | 174 +++++++++++++++++++++++++++++++++ gdb/record-full.c | 239 ++++------------------------------------------ gdb/target-delegates.c | 108 +++++++++++++++++++++ gdb/target.c | 196 ++++++++++++++++++------------------- gdb/target.h | 5 +- 6 files changed, 444 insertions(+), 322 deletions(-) create mode 100755 gdb/make-target-delegates create mode 100644 gdb/target-delegates.c diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 001cce3..f87d015 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,49 @@ 2013-10-30 Tom Tromey <tromey@redhat.com> + * record-full.c (record_full_beneath_to_resume_ops) + (record_full_beneath_to_resume, record_full_beneath_to_wait_ops) + (record_full_beneath_to_wait) + (record_full_beneath_to_store_registers_ops) + (record_full_beneath_to_store_registers) + (record_full_beneath_to_xfer_partial_ops) + (record_full_beneath_to_xfer_partial) + (record_full_beneath_to_insert_breakpoint) + (record_full_beneath_to_remove_breakpoint) + (record_full_beneath_to_stopped_by_watchpoint) + (record_full_beneath_to_stopped_data_address) + (record_full_beneath_to_async, tmp_to_resume_ops, tmp_to_resume) + (tmp_to_wait_ops, tmp_to_wait, tmp_to_store_registers_ops) + (tmp_to_store_registers, tmp_to_xfer_partial_ops) + (tmp_to_xfer_partial, tmp_to_insert_breakpoint) + (tmp_to_remove_breakpoint, tmp_to_stopped_by_watchpoint) + (tmp_to_stopped_data_address, tmp_to_async): Remove. + (record_full_open_1, record_full_open): Update. Use RECORD_IS_USED. + (record_full_resume, record_full_wait_1) + (record_full_stopped_by_watchpoint, record_full_stopped_data_address) + (record_full_store_registers, record_full_xfer_partial) + (record_full_insert_breakpoint, record_full_remove_breakpoint) + (record_full_async, record_full_can_async_p, record_full_is_async_p) + (record_full_core_xfer_partial): Use target delegation. + * target.c (update_current_target): Use target_delegate_xfer_partial. + (target_delegate_xfer_partial): Now public. Renamed from... + (current_xfer_partial): ...this. Remove. + (target_delegate_async, target_delegate_is_async_p) + (target_delegate_can_async_p, target_delegate_insert_breakpoint) + (target_delegate_remove_breakpoint, target_delegate_wait) + (target_delegate_resume, target_delegate_store_registers) + (target_delegate_stopped_by_watchpoint) + (target_delegate_stopped_data_address) + (assert_delegation_failed): New functions. + * target.h (target_delegate_async, target_delegate_is_async_p) + (target_delegate_can_async_p, target_delegate_insert_breakpoint) + (target_delegate_remove_breakpoint, target_delegate_wait) + (target_delegate_resume, target_delegate_store_registers) + (target_delegate_stopped_by_watchpoint) + (target_delegate_stopped_data_address) + (target_delegate_xfer_partial): Declare. + +2013-10-30 Tom Tromey <tromey@redhat.com> + * record.c (find_record_target): Use find_target_at. * record.h (RECORD_IS_REPLAY): Use find_target_at. * target.c (find_target_at): New function. diff --git a/gdb/make-target-delegates b/gdb/make-target-delegates new file mode 100755 index 0000000..720ca13 --- /dev/null +++ b/gdb/make-target-delegates @@ -0,0 +1,174 @@ +#!/usr/bin/perl + +# For the time being we only allow certain methods. +%OK = ( + 'to_resume' => 1, + 'to_wait' => 1, + 'to_store_registers' => 1, + 'to_insert_breakpoint' => 1, + 'to_remove_breakpoint' => 1, + 'to_can_async_p' => 1, + 'to_is_async_p' => 1, + 'to_async' => 1, + 'to_stopped_by_watchpoint' => 1, + 'to_stopped_data_address' => 1, + 'to_xfer_partial' => 1 +); + +# The line we search for in target.h that marks where we should start +# looking for methods. +$TRIGGER = qr,^struct target_ops$,; +# The end of the methods part. +$ENDER = qr,^\s*};$,; + +$SYMBOL = qr,[a-zA-Z0-9_]+,; +$NAME_PART = qr,\(\*(?<name>${SYMBOL}+)\)\s,; +$ARGS_PART = qr,(?<args>\(.*)$,; +$INTRO_PART = qr,^\s*,; + +$SIMPLE_RETURN_PART = qr,[^\(]+,; +$VEC_RETURN_PART = qr,VEC\s*\([^\)]+\)[^\(]*,; + +$METHOD = ($INTRO_PART . "(?<return_type>" . $SIMPLE_RETURN_PART + . "|" . $VEC_RETURN_PART . ")" + . $NAME_PART . $ARGS_PART); + +sub trim($) { + my ($result) = @_; + $result =~ s,^\s*(\S*)\s*$,\1,; + return $result; +} + +# Read from the input files until we find the trigger line. +# Die if not found. +sub find_trigger() { + while (<>) { + chomp; + return if m/$TRIGGER/; + } + + die "could not find trigger line\n"; +} + +# Parse arguments into a list. +sub parse_argtypes($) { + my ($typestr) = @_; + + $typestr =~ s/^\((.*)\);$/\1/; + + my (@typelist) = split (/,\s*/, $typestr); + my (@result, $iter, $onetype); + + foreach $iter (@typelist) { + if ($iter =~ m/^(enum\s+${SYMBOL}\s*)(${SYMBOL})?$/) { + $onetype = $1; + } elsif ($iter =~ m/^(.*(enum\s+)?${SYMBOL}.*(\s|\*))${SYMBOL}+$/) { + $onetype = $1; + } elsif ($iter eq 'void') { + next; + } else { + $onetype = $iter; + } + push @result, trim ($onetype); + } + + return @result; +} + +sub dname($) { + my ($name) = @_; + $name =~ s/to_/delegate_/; + return $name; +} + +# Write out a delegation function. +sub write_delegator($$@) { + my ($name, $return_type, @argtypes) = @_; + + print "static " . $return_type . "\n"; + print dname ($name) . ' ('; + + my $iter; + my @argdecls; + my @actuals; + my $i = 0; + foreach $iter (@argtypes) { + my $val = $iter; + + if ($iter !~ m,\*$,) { + $val .= ' '; + } + + my $vname; + if ($i == 0) { + # Just a random nicety. + $vname = 'self'; + } else { + $vname .= "arg$i"; + } + $val .= $vname; + + push @argdecls, $val; + push @actuals, $vname; + ++$i; + } + + print join (', ', @argdecls) . ")\n"; + print "{\n"; + print " self = self->beneath;\n"; + print " "; + if ($return_type ne 'void') { + print "return "; + } + print "self->" . $name . " ("; + print join (', ', @actuals); + print ");\n"; + print "}\n\n"; +} + +print "/* THIS FILE IS GENERATED -*- buffer-read-only: t -*- */\n"; +print "/* vi:set ro: */\n\n"; +print "/* To regenerate this file, run:*/\n"; +print "/* make-target-delegates target.h > target-delegates.c */\n"; + +find_trigger(); + +@delegators = (); +$current_args = ''; +while (<>) { + chomp; + last if m/$ENDER/; + + if ($current_args ne '') { + s/^\s*//; + $current_args .= $_; + } elsif (m/$METHOD/) { + $name = $+{name}; + $current_args = $+{args}; + $return_type = trim ($+{return_type}); + } + + if ($current_args =~ /\);\s*$/) { + if (defined $OK{$name}) { + @argtypes = parse_argtypes ($current_args); + + # The first argument must be "this" to be delegatable. + if ($argtypes[0] =~ /\s*struct\s+target_ops\s*\*\s*/) { + write_delegator ($name, $return_type, @argtypes); + + push @delegators, $name; + } + } + + $current_args = ''; + } +} + +# Now the delegation code. +print "static void\ninstall_delegators (struct target_ops *ops)\n{\n"; + +for $iter (@delegators) { + print " if (ops->" . $iter . " == NULL)\n"; + print " ops->" . $iter . " = " . dname ($iter) . ";\n"; +} +print "}\n"; diff --git a/gdb/record-full.c b/gdb/record-full.c index f4d760a..1d66c6e 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -215,43 +215,6 @@ static struct cmd_list_element *show_record_full_cmdlist; /* Command list for "record full". */ static struct cmd_list_element *record_full_cmdlist; -/* The beneath function pointers. */ -static struct target_ops *record_full_beneath_to_resume_ops; -static void (*record_full_beneath_to_resume) (struct target_ops *, ptid_t, int, - enum gdb_signal); -static struct target_ops *record_full_beneath_to_wait_ops; -static ptid_t (*record_full_beneath_to_wait) (struct target_ops *, ptid_t, - struct target_waitstatus *, - int); -static struct target_ops *record_full_beneath_to_store_registers_ops; -static void (*record_full_beneath_to_store_registers) (struct target_ops *, - struct regcache *, - int regno); -static struct target_ops *record_full_beneath_to_xfer_partial_ops; -static LONGEST - (*record_full_beneath_to_xfer_partial) (struct target_ops *ops, - enum target_object object, - const char *annex, - gdb_byte *readbuf, - const gdb_byte *writebuf, - ULONGEST offset, - LONGEST len); -static int - (*record_full_beneath_to_insert_breakpoint) (struct target_ops *, - struct gdbarch *, - struct bp_target_info *); -static int - (*record_full_beneath_to_remove_breakpoint) (struct target_ops *, - struct gdbarch *, - struct bp_target_info *); -static int (*record_full_beneath_to_stopped_by_watchpoint) (struct target_ops *); -static int (*record_full_beneath_to_stopped_data_address) (struct target_ops *, - CORE_ADDR *); -static void - (*record_full_beneath_to_async) (struct target_ops *, - void (*) (enum inferior_event_type, void *), - void *); - static void record_full_goto_insn (struct record_full_entry *entry, enum exec_direction_kind dir); static void record_full_save (const char *recfilename); @@ -798,36 +761,6 @@ record_full_exec_insn (struct regcache *regcache, } } -static struct target_ops *tmp_to_resume_ops; -static void (*tmp_to_resume) (struct target_ops *, ptid_t, int, - enum gdb_signal); -static struct target_ops *tmp_to_wait_ops; -static ptid_t (*tmp_to_wait) (struct target_ops *, ptid_t, - struct target_waitstatus *, - int); -static struct target_ops *tmp_to_store_registers_ops; -static void (*tmp_to_store_registers) (struct target_ops *, - struct regcache *, - int regno); -static struct target_ops *tmp_to_xfer_partial_ops; -static LONGEST (*tmp_to_xfer_partial) (struct target_ops *ops, - enum target_object object, - const char *annex, - gdb_byte *readbuf, - const gdb_byte *writebuf, - ULONGEST offset, - LONGEST len); -static int (*tmp_to_insert_breakpoint) (struct target_ops *, - struct gdbarch *, - struct bp_target_info *); -static int (*tmp_to_remove_breakpoint) (struct target_ops *ops, - struct gdbarch *, - struct bp_target_info *); -static int (*tmp_to_stopped_by_watchpoint) (struct target_ops *); -static int (*tmp_to_stopped_data_address) (struct target_ops *, CORE_ADDR *); -static void (*tmp_to_async) (struct target_ops *, - void (*) (enum inferior_event_type, void *), void *); - static void record_full_restore (void); /* Asynchronous signal handle registered as event loop source for when @@ -890,26 +823,6 @@ record_full_open_1 (char *name, int from_tty) error (_("Process record: the current architecture doesn't support " "record function.")); - if (!tmp_to_resume) - error (_("Could not find 'to_resume' method on the target stack.")); - if (!tmp_to_wait) - error (_("Could not find 'to_wait' method on the target stack.")); - if (!tmp_to_store_registers) - error (_("Could not find 'to_store_registers' " - "method on the target stack.")); - if (!tmp_to_insert_breakpoint) - error (_("Could not find 'to_insert_breakpoint' " - "method on the target stack.")); - if (!tmp_to_remove_breakpoint) - error (_("Could not find 'to_remove_breakpoint' " - "method on the target stack.")); - if (!tmp_to_stopped_by_watchpoint) - error (_("Could not find 'to_stopped_by_watchpoint' " - "method on the target stack.")); - if (!tmp_to_stopped_data_address) - error (_("Could not find 'to_stopped_data_address' " - "method on the target stack.")); - push_target (&record_full_ops); } @@ -926,83 +839,16 @@ record_full_open (char *name, int from_tty) fprintf_unfiltered (gdb_stdlog, "Process record: record_full_open\n"); /* Check if record target is already running. */ - if (current_target.to_stratum == record_stratum) + if (RECORD_IS_USED) error (_("Process record target already running. Use \"record stop\" to " "stop record target first.")); - /* Reset the tmp beneath pointers. */ - tmp_to_resume_ops = NULL; - tmp_to_resume = NULL; - tmp_to_wait_ops = NULL; - tmp_to_wait = NULL; - tmp_to_store_registers_ops = NULL; - tmp_to_store_registers = NULL; - tmp_to_xfer_partial_ops = NULL; - tmp_to_xfer_partial = NULL; - tmp_to_insert_breakpoint = NULL; - tmp_to_remove_breakpoint = NULL; - tmp_to_stopped_by_watchpoint = NULL; - tmp_to_stopped_data_address = NULL; - tmp_to_async = NULL; - - /* Set the beneath function pointers. */ - for (t = current_target.beneath; t != NULL; t = t->beneath) - { - if (!tmp_to_resume) - { - tmp_to_resume = t->to_resume; - tmp_to_resume_ops = t; - } - if (!tmp_to_wait) - { - tmp_to_wait = t->to_wait; - tmp_to_wait_ops = t; - } - if (!tmp_to_store_registers) - { - tmp_to_store_registers = t->to_store_registers; - tmp_to_store_registers_ops = t; - } - if (!tmp_to_xfer_partial) - { - tmp_to_xfer_partial = t->to_xfer_partial; - tmp_to_xfer_partial_ops = t; - } - if (!tmp_to_insert_breakpoint) - tmp_to_insert_breakpoint = t->to_insert_breakpoint; - if (!tmp_to_remove_breakpoint) - tmp_to_remove_breakpoint = t->to_remove_breakpoint; - if (!tmp_to_stopped_by_watchpoint) - tmp_to_stopped_by_watchpoint = t->to_stopped_by_watchpoint; - if (!tmp_to_stopped_data_address) - tmp_to_stopped_data_address = t->to_stopped_data_address; - if (!tmp_to_async) - tmp_to_async = t->to_async; - } - if (!tmp_to_xfer_partial) - error (_("Could not find 'to_xfer_partial' method on the target stack.")); - /* Reset */ record_full_insn_num = 0; record_full_insn_count = 0; record_full_list = &record_full_first; record_full_list->next = NULL; - /* Set the tmp beneath pointers to beneath pointers. */ - record_full_beneath_to_resume_ops = tmp_to_resume_ops; - record_full_beneath_to_resume = tmp_to_resume; - record_full_beneath_to_wait_ops = tmp_to_wait_ops; - record_full_beneath_to_wait = tmp_to_wait; - record_full_beneath_to_store_registers_ops = tmp_to_store_registers_ops; - record_full_beneath_to_store_registers = tmp_to_store_registers; - record_full_beneath_to_xfer_partial_ops = tmp_to_xfer_partial_ops; - record_full_beneath_to_xfer_partial = tmp_to_xfer_partial; - record_full_beneath_to_insert_breakpoint = tmp_to_insert_breakpoint; - record_full_beneath_to_remove_breakpoint = tmp_to_remove_breakpoint; - record_full_beneath_to_stopped_by_watchpoint = tmp_to_stopped_by_watchpoint; - record_full_beneath_to_stopped_data_address = tmp_to_stopped_data_address; - record_full_beneath_to_async = tmp_to_async; - if (core_bfd) record_full_core_open_1 (name, from_tty); else @@ -1126,8 +972,7 @@ record_full_resume (struct target_ops *ops, ptid_t ptid, int step, /* Make sure the target beneath reports all signals. */ target_pass_signals (0, NULL); - record_full_beneath_to_resume (record_full_beneath_to_resume_ops, - ptid, step, signal); + ops->beneath->to_resume (ops->beneath, ptid, step, signal); } /* We are about to start executing the inferior (or simulate it), @@ -1217,8 +1062,7 @@ record_full_wait_1 (struct target_ops *ops, if (record_full_resume_step) { /* This is a single step. */ - return record_full_beneath_to_wait (record_full_beneath_to_wait_ops, - ptid, status, options); + return ops->beneath->to_wait (ops->beneath, ptid, status, options); } else { @@ -1229,8 +1073,7 @@ record_full_wait_1 (struct target_ops *ops, while (1) { - ret = record_full_beneath_to_wait - (record_full_beneath_to_wait_ops, ptid, status, options); + ret = ops->beneath->to_wait (ops->beneath, ptid, status, options); if (status->kind == TARGET_WAITKIND_IGNORE) { if (record_debug) @@ -1314,9 +1157,8 @@ record_full_wait_1 (struct target_ops *ops, "Process record: record_full_wait " "issuing one more step in the " "target beneath\n"); - record_full_beneath_to_resume - (record_full_beneath_to_resume_ops, ptid, step, - GDB_SIGNAL_0); + ops->beneath->to_resume (ops->beneath, ptid, step, + GDB_SIGNAL_0); continue; } } @@ -1520,11 +1362,7 @@ record_full_stopped_by_watchpoint (struct target_ops *ops) if (RECORD_FULL_IS_REPLAY) return record_full_hw_watchpoint; else - { - struct target_ops *beneath = find_target_beneath (ops); - - return record_full_beneath_to_stopped_by_watchpoint (beneath); - } + return ops->beneath->to_stopped_by_watchpoint (ops->beneath); } static int @@ -1533,7 +1371,7 @@ record_full_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p) if (RECORD_FULL_IS_REPLAY) return 0; else - return record_full_beneath_to_stopped_data_address (ops, addr_p); + return ops->beneath->to_stopped_data_address (ops->beneath, addr_p); } /* Record registers change (by user or by GDB) to list as an instruction. */ @@ -1636,8 +1474,7 @@ record_full_store_registers (struct target_ops *ops, record_full_registers_change (regcache, regno); } - record_full_beneath_to_store_registers - (record_full_beneath_to_store_registers_ops, regcache, regno); + ops->beneath->to_store_registers (ops->beneath, regcache, regno); } /* "to_xfer_partial" method. Behavior is conditional on @@ -1702,9 +1539,9 @@ record_full_xfer_partial (struct target_ops *ops, enum target_object object, record_full_insn_num++; } - return record_full_beneath_to_xfer_partial - (record_full_beneath_to_xfer_partial_ops, object, annex, - readbuf, writebuf, offset, len); + return ops->beneath->to_xfer_partial (ops->beneath, object, annex, + readbuf, writebuf, offset, + len); } /* This structure represents a breakpoint inserted while the record @@ -1785,8 +1622,7 @@ record_full_insert_breakpoint (struct target_ops *ops, int ret; old_cleanups = record_full_gdb_operation_disable_set (); - ret = record_full_beneath_to_insert_breakpoint (find_target_beneath (ops), - gdbarch, bp_tgt); + ret = ops->beneath->to_insert_breakpoint (ops->beneath, gdbarch, bp_tgt); do_cleanups (old_cleanups); if (ret != 0) @@ -1827,8 +1663,8 @@ record_full_remove_breakpoint (struct target_ops *ops, int ret; old_cleanups = record_full_gdb_operation_disable_set (); - ret = record_full_beneath_to_remove_breakpoint (find_target_beneath (ops), - gdbarch, bp_tgt); + ret = ops->beneath->to_remove_breakpoint (ops->beneath, gdbarch, + bp_tgt); do_cleanups (old_cleanups); if (ret != 0) @@ -1902,32 +1738,6 @@ record_full_goto_bookmark (gdb_byte *raw_bookmark, int from_tty) return; } -static void -record_full_async (struct target_ops *ops, - void (*callback) (enum inferior_event_type event_type, - void *context), void *context) -{ - /* If we're on top of a line target (e.g., linux-nat, remote), then - set it to async mode as well. Will be NULL if we're sitting on - top of the core target, for "record restore". */ - if (record_full_beneath_to_async != NULL) - record_full_beneath_to_async (find_target_beneath (ops), callback, context); -} - -static int -record_full_can_async_p (struct target_ops *ops) -{ - /* We only enable async when the user specifically asks for it. */ - return target_async_permitted; -} - -static int -record_full_is_async_p (struct target_ops *ops) -{ - /* We only enable async when the user specifically asks for it. */ - return target_async_permitted; -} - static enum exec_direction_kind record_full_execution_direction (void) { @@ -2092,9 +1902,6 @@ init_record_full_ops (void) /* Add bookmark target methods. */ record_full_ops.to_get_bookmark = record_full_get_bookmark; record_full_ops.to_goto_bookmark = record_full_goto_bookmark; - record_full_ops.to_async = record_full_async; - record_full_ops.to_can_async_p = record_full_can_async_p; - record_full_ops.to_is_async_p = record_full_is_async_p; record_full_ops.to_execution_direction = record_full_execution_direction; record_full_ops.to_info_record = record_full_info; record_full_ops.to_save_record = record_full_save; @@ -2251,10 +2058,10 @@ record_full_core_xfer_partial (struct target_ops *ops, else { if (!entry) - return record_full_beneath_to_xfer_partial - (record_full_beneath_to_xfer_partial_ops, - object, annex, readbuf, writebuf, - offset, len); + return ops->beneath->to_xfer_partial (ops->beneath, + object, annex, + readbuf, writebuf, + offset, len); memcpy (readbuf, entry->buf + sec_offset, (size_t) len); @@ -2270,9 +2077,8 @@ record_full_core_xfer_partial (struct target_ops *ops, error (_("You can't do that without a process to debug.")); } - return record_full_beneath_to_xfer_partial - (record_full_beneath_to_xfer_partial_ops, object, annex, - readbuf, writebuf, offset, len); + return ops->beneath->to_xfer_partial (ops->beneath, object, annex, + readbuf, writebuf, offset, len); } /* "to_insert_breakpoint" method for prec over corefile. */ @@ -2334,9 +2140,6 @@ init_record_full_core_ops (void) /* Add bookmark target methods. */ record_full_core_ops.to_get_bookmark = record_full_get_bookmark; record_full_core_ops.to_goto_bookmark = record_full_goto_bookmark; - record_full_core_ops.to_async = record_full_async; - record_full_core_ops.to_can_async_p = record_full_can_async_p; - record_full_core_ops.to_is_async_p = record_full_is_async_p; record_full_core_ops.to_execution_direction = record_full_execution_direction; record_full_core_ops.to_info_record = record_full_info; diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c new file mode 100644 index 0000000..60a28ca --- /dev/null +++ b/gdb/target-delegates.c @@ -0,0 +1,108 @@ +/* THIS FILE IS GENERATED -*- buffer-read-only: t -*- */ +/* vi:set ro: */ + +/* To regenerate this file, run:*/ +/* make-target-delegates target.h > target-delegates.c */ +static void +delegate_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal arg3) +{ + self = self->beneath; + self->to_resume (self, arg1, arg2, arg3); +} + +static ptid_t +delegate_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *arg2, int arg3) +{ + self = self->beneath; + return self->to_wait (self, arg1, arg2, arg3); +} + +static void +delegate_store_registers (struct target_ops *self, struct regcache *arg1, int arg2) +{ + self = self->beneath; + self->to_store_registers (self, arg1, arg2); +} + +static int +delegate_insert_breakpoint (struct target_ops *self, struct gdbarch *arg1, struct bp_target_info *arg2) +{ + self = self->beneath; + return self->to_insert_breakpoint (self, arg1, arg2); +} + +static int +delegate_remove_breakpoint (struct target_ops *self, struct gdbarch *arg1, struct bp_target_info *arg2) +{ + self = self->beneath; + return self->to_remove_breakpoint (self, arg1, arg2); +} + +static int +delegate_stopped_by_watchpoint (struct target_ops *self) +{ + self = self->beneath; + return self->to_stopped_by_watchpoint (self); +} + +static int +delegate_stopped_data_address (struct target_ops *self, CORE_ADDR *arg1) +{ + self = self->beneath; + return self->to_stopped_data_address (self, arg1); +} + +static int +delegate_can_async_p (struct target_ops *self) +{ + self = self->beneath; + return self->to_can_async_p (self); +} + +static int +delegate_is_async_p (struct target_ops *self) +{ + self = self->beneath; + return self->to_is_async_p (self); +} + +static void +delegate_async (struct target_ops *self, async_callback_ftype *arg1, void *arg2) +{ + self = self->beneath; + self->to_async (self, arg1, arg2); +} + +static LONGEST +delegate_xfer_partial (struct target_ops *self, enum target_object arg1, const char *arg2, gdb_byte *arg3, const gdb_byte *arg4, ULONGEST arg5, LONGEST arg6) +{ + self = self->beneath; + return self->to_xfer_partial (self, arg1, arg2, arg3, arg4, arg5, arg6); +} + +static void +install_delegators (struct target_ops *ops) +{ + if (ops->to_resume == NULL) + ops->to_resume = delegate_resume; + if (ops->to_wait == NULL) + ops->to_wait = delegate_wait; + if (ops->to_store_registers == NULL) + ops->to_store_registers = delegate_store_registers; + if (ops->to_insert_breakpoint == NULL) + ops->to_insert_breakpoint = delegate_insert_breakpoint; + if (ops->to_remove_breakpoint == NULL) + ops->to_remove_breakpoint = delegate_remove_breakpoint; + if (ops->to_stopped_by_watchpoint == NULL) + ops->to_stopped_by_watchpoint = delegate_stopped_by_watchpoint; + if (ops->to_stopped_data_address == NULL) + ops->to_stopped_data_address = delegate_stopped_data_address; + if (ops->to_can_async_p == NULL) + ops->to_can_async_p = delegate_can_async_p; + if (ops->to_is_async_p == NULL) + ops->to_is_async_p = delegate_is_async_p; + if (ops->to_async == NULL) + ops->to_async = delegate_async; + if (ops->to_xfer_partial == NULL) + ops->to_xfer_partial = delegate_xfer_partial; +} diff --git a/gdb/target.c b/gdb/target.c index d2a40ee..e765700 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -45,6 +45,8 @@ #include "gdb/fileio.h" #include "agent.h" +#include "target-delegates.c" + static void target_info (char *, int); static void default_terminal_info (const char *, int); @@ -76,12 +78,6 @@ static LONGEST default_xfer_partial (struct target_ops *ops, const gdb_byte *writebuf, ULONGEST offset, LONGEST len); -static LONGEST current_xfer_partial (struct target_ops *ops, - enum target_object object, - const char *annex, gdb_byte *readbuf, - const gdb_byte *writebuf, - ULONGEST offset, LONGEST len); - static struct gdbarch *default_thread_architecture (struct target_ops *ops, ptid_t ptid); @@ -352,6 +348,8 @@ complete_target_initialization (struct target_ops *t) if (t->to_has_execution == NULL) t->to_has_execution = (int (*) (struct target_ops *, ptid_t)) return_zero; + + install_delegators (t); } /* Add possible target architecture T to the list and add a new @@ -559,6 +557,9 @@ update_current_target (void) /* First, reset current's contents. */ memset (¤t_target, 0, sizeof (current_target)); + /* The newest approach is to auto-delegate some methods. */ + install_delegators (¤t_target); + #define INHERIT(FIELD, TARGET) \ if (!current_target.FIELD) \ current_target.FIELD = (TARGET)->FIELD @@ -582,8 +583,8 @@ update_current_target (void) INHERIT (to_prepare_to_store, t); INHERIT (deprecated_xfer_memory, t); INHERIT (to_files_info, t); - INHERIT (to_insert_breakpoint, t); - INHERIT (to_remove_breakpoint, t); + /* Do not inherit to_insert_breakpoint. */ + /* Do not inherit to_remove_breakpoint. */ INHERIT (to_can_use_hw_breakpoint, t); INHERIT (to_insert_hw_breakpoint, t); INHERIT (to_remove_hw_breakpoint, t); @@ -592,10 +593,10 @@ update_current_target (void) INHERIT (to_remove_watchpoint, t); /* Do not inherit to_insert_mask_watchpoint. */ /* Do not inherit to_remove_mask_watchpoint. */ - INHERIT (to_stopped_data_address, t); + /* Do not inherit to_stopped_data_address. */ INHERIT (to_have_steppable_watchpoint, t); INHERIT (to_have_continuable_watchpoint, t); - INHERIT (to_stopped_by_watchpoint, t); + /* Do not inherit to_stopped_by_watchpoint. */ INHERIT (to_watchpoint_addr_within_range, t); INHERIT (to_region_ok_for_hw_watchpoint, t); INHERIT (to_can_accel_watchpoint_condition, t); @@ -640,9 +641,9 @@ update_current_target (void) /* Do not inherit to_has_registers. */ /* Do not inherit to_has_execution. */ INHERIT (to_has_thread_control, t); - INHERIT (to_can_async_p, t); - INHERIT (to_is_async_p, t); - INHERIT (to_async, t); + /* Do not inherit to_can_async_p. */ + /* Do not inherit to_is_async_p. */ + /* Do not inherit to_async. */ INHERIT (to_find_memory_regions, t); INHERIT (to_make_corefile_notes, t); INHERIT (to_get_bookmark, t); @@ -742,12 +743,6 @@ update_current_target (void) de_fault (to_remove_watchpoint, (int (*) (CORE_ADDR, int, int, struct expression *)) return_minus_one); - de_fault (to_stopped_by_watchpoint, - (int (*) (struct target_ops *)) - return_zero); - de_fault (to_stopped_data_address, - (int (*) (struct target_ops *, CORE_ADDR *)) - return_zero); de_fault (to_watchpoint_addr_within_range, default_watchpoint_addr_within_range); de_fault (to_region_ok_for_hw_watchpoint, @@ -813,18 +808,12 @@ update_current_target (void) de_fault (to_stop, (void (*) (ptid_t)) target_ignore); - current_target.to_xfer_partial = current_xfer_partial; de_fault (to_rcmd, (void (*) (char *, struct ui_file *)) tcomplain); de_fault (to_pid_to_exec_file, (char *(*) (int)) return_zero); - de_fault (to_async, - (void (*) (struct target_ops *, - void (*) (enum inferior_event_type, void*), - void*)) - tcomplain); de_fault (to_thread_architecture, default_thread_architecture); current_target.to_read_description = NULL; @@ -1996,27 +1985,12 @@ default_xfer_partial (struct target_ops *ops, enum target_object object, else return -1; } - else if (ops->beneath != NULL) - return ops->beneath->to_xfer_partial (ops->beneath, object, annex, - readbuf, writebuf, offset, len); - else - return -1; -} - -/* The xfer_partial handler for the topmost target. Unlike the default, - it does not need to handle memory specially; it just passes all - requests down the stack. */ - -static LONGEST -current_xfer_partial (struct target_ops *ops, enum target_object object, - const char *annex, gdb_byte *readbuf, - const gdb_byte *writebuf, ULONGEST offset, LONGEST len) -{ - if (ops->beneath != NULL) - return ops->beneath->to_xfer_partial (ops->beneath, object, annex, - readbuf, writebuf, offset, len); else - return -1; + { + gdb_assert (ops->beneath != NULL); + return ops->beneath->to_xfer_partial (ops->beneath, object, annex, + readbuf, writebuf, offset, len); + } } /* Target vector read/write partial wrapper functions. */ @@ -2665,34 +2639,26 @@ ptid_t target_wait (ptid_t ptid, struct target_waitstatus *status, int options) { struct target_ops *t; + ptid_t retval = (current_target.to_wait) (¤t_target, ptid, + status, options); - for (t = current_target.beneath; t != NULL; t = t->beneath) + if (targetdebug) { - if (t->to_wait != NULL) - { - ptid_t retval = (*t->to_wait) (t, ptid, status, options); - - if (targetdebug) - { - char *status_string; - char *options_string; - - status_string = target_waitstatus_to_string (status); - options_string = target_options_to_string (options); - fprintf_unfiltered (gdb_stdlog, - "target_wait (%d, status, options={%s})" - " = %d, %s\n", - ptid_get_pid (ptid), options_string, - ptid_get_pid (retval), status_string); - xfree (status_string); - xfree (options_string); - } + char *status_string; + char *options_string; - return retval; - } + status_string = target_waitstatus_to_string (status); + options_string = target_options_to_string (options); + fprintf_unfiltered (gdb_stdlog, + "target_wait (%d, status, options={%s})" + " = %d, %s\n", + ptid_get_pid (ptid), options_string, + ptid_get_pid (retval), status_string); + xfree (status_string); + xfree (options_string); } - noprocess (); + return retval; } char * @@ -2730,26 +2696,17 @@ target_resume (ptid_t ptid, int step, enum gdb_signal signal) target_dcache_invalidate (); - for (t = current_target.beneath; t != NULL; t = t->beneath) - { - if (t->to_resume != NULL) - { - t->to_resume (t, ptid, step, signal); - if (targetdebug) - fprintf_unfiltered (gdb_stdlog, "target_resume (%d, %s, %s)\n", - ptid_get_pid (ptid), - step ? "step" : "continue", - gdb_signal_to_name (signal)); - - registers_changed_ptid (ptid); - set_executing (ptid, 1); - set_running (ptid, 1); - clear_inline_frame_state (ptid); - return; - } - } + current_target.to_resume (¤t_target, ptid, step, signal); + if (targetdebug) + fprintf_unfiltered (gdb_stdlog, "target_resume (%d, %s, %s)\n", + ptid_get_pid (ptid), + step ? "step" : "continue", + gdb_signal_to_name (signal)); - noprocess (); + registers_changed_ptid (ptid); + set_executing (ptid, 1); + set_running (ptid, 1); + clear_inline_frame_state (ptid); } void @@ -3146,7 +3103,7 @@ find_default_can_async_p (struct target_ops *ignore) configured with a native debugger, and target remote isn't connected yet. */ t = find_default_run_target (NULL); - if (t && t->to_can_async_p) + if (t) return (t->to_can_async_p) (t); return 0; } @@ -3161,7 +3118,7 @@ find_default_is_async_p (struct target_ops *ignore) configured with a native debugger, and target remote isn't connected yet. */ t = find_default_run_target (NULL); - if (t && t->to_is_async_p) + if (t) return (t->to_is_async_p) (t); return 0; } @@ -3716,6 +3673,46 @@ dummy_pid_to_str (struct target_ops *ops, ptid_t ptid) return normal_pid_to_str (ptid); } +static void +dummy_resume (struct target_ops *self, ptid_t ptid, int step, + enum gdb_signal signal) +{ + noprocess (); +} + +static ptid_t +dummy_wait (struct target_ops *self, ptid_t ptid, + struct target_waitstatus *status, int options) +{ + noprocess (); +} + +static void +dummy_store_registers (struct target_ops *self, + struct regcache *regcache, + int regno) +{ + noprocess (); +} + +static LONGEST +dummy_xfer_partial (struct target_ops *ops, + enum target_object object, + const char *annex, gdb_byte *readbuf, + const gdb_byte *writebuf, + ULONGEST offset, LONGEST len) +{ + return -1; +} + +static void +dummy_async (struct target_ops *self, + void (*callback) (enum inferior_event_type, void *), + void *datum) +{ + tcomplain (); +} + /* Error-catcher for target_find_memory_regions. */ static int dummy_find_memory_regions (find_memory_region_ftype ignore1, void *ignore2) @@ -3760,8 +3757,11 @@ init_dummy_target (void) dummy_target.to_detach = (void (*)(struct target_ops *, const char *, int))target_ignore; dummy_target.to_create_inferior = find_default_create_inferior; + dummy_target.to_resume = dummy_resume; + dummy_target.to_wait = dummy_wait; dummy_target.to_can_async_p = find_default_can_async_p; dummy_target.to_is_async_p = find_default_is_async_p; + dummy_target.to_async = dummy_async; dummy_target.to_supports_non_stop = find_default_supports_non_stop; dummy_target.to_supports_disable_randomization = find_default_supports_disable_randomization; @@ -3771,13 +3771,14 @@ init_dummy_target (void) dummy_target.to_make_corefile_notes = dummy_make_corefile_notes; dummy_target.to_get_bookmark = dummy_get_bookmark; dummy_target.to_goto_bookmark = dummy_goto_bookmark; - dummy_target.to_xfer_partial = default_xfer_partial; + dummy_target.to_xfer_partial = dummy_xfer_partial; dummy_target.to_has_all_memory = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_memory = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_stack = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_execution = (int (*) (struct target_ops *, ptid_t)) return_zero; + dummy_target.to_store_registers = dummy_store_registers; dummy_target.to_stopped_by_watchpoint = (int (*) (struct target_ops *)) return_zero; dummy_target.to_stopped_data_address = @@ -3999,20 +4000,11 @@ target_store_registers (struct regcache *regcache, int regno) if (!may_write_registers) error (_("Writing to registers is not allowed (regno %d)"), regno); - for (t = current_target.beneath; t != NULL; t = t->beneath) + current_target.to_store_registers (¤t_target, regcache, regno); + if (targetdebug) { - if (t->to_store_registers != NULL) - { - t->to_store_registers (t, regcache, regno); - if (targetdebug) - { - debug_print_register ("target_store_registers", regcache, regno); - } - return; - } + debug_print_register ("target_store_registers", regcache, regno); } - - noprocess (); } int diff --git a/gdb/target.h b/gdb/target.h index b219cff..804e8e4 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -341,6 +341,8 @@ extern ULONGEST get_target_memory_unsigned (struct target_ops *ops, \f struct thread_info; /* fwd decl for parameter list below: */ +typedef void async_callback_ftype (enum inferior_event_type, void *); + struct target_ops { struct target_ops *beneath; /* To the target under this one. */ @@ -484,8 +486,7 @@ struct target_ops /* ASYNC target controls */ int (*to_can_async_p) (struct target_ops *); int (*to_is_async_p) (struct target_ops *); - void (*to_async) (struct target_ops *, - void (*) (enum inferior_event_type, void *), void *); + void (*to_async) (struct target_ops *, async_callback_ftype *, void *); int (*to_supports_non_stop) (void); /* find_memory_regions support method for gcore */ int (*to_find_memory_regions) (find_memory_region_ftype func, void *data); -- 1.8.1.4 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #3: patch #2 --] [-- Type: text/x-patch, Size: 2430 bytes --] From 91fe0229e03f82921be927a66762bdc7d4e08d68 Mon Sep 17 00:00:00 2001 From: Tom Tromey <tromey@redhat.com> Date: Fri, 13 Dec 2013 13:33:08 -0700 Subject: [PATCH 07/11] convert to_detach --- gdb/make-target-delegates | 3 ++- gdb/target-delegates.c | 9 +++++++++ gdb/target.c | 17 ++++------------- 3 files changed, 15 insertions(+), 14 deletions(-) diff --git a/gdb/make-target-delegates b/gdb/make-target-delegates index 296ea5e..89a0920 100755 --- a/gdb/make-target-delegates +++ b/gdb/make-target-delegates @@ -13,7 +13,8 @@ 'to_stopped_by_watchpoint' => 1, 'to_stopped_data_address' => 1, 'to_xfer_partial' => 1, - 'to_supports_btrace' => 1 + 'to_supports_btrace' => 1, + 'to_detach' => 1 ); # The line we search for in target.h that marks where we should start diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index b4a35cb..1bfbde6 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -4,6 +4,13 @@ /* To regenerate this file, run:*/ /* make-target-delegates target.h > target-delegates.c */ static void +delegate_detach (struct target_ops *self, const char *arg1, int arg2) +{ + self = self->beneath; + self->to_detach (self, arg1, arg2); +} + +static void delegate_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal arg3) { self = self->beneath; @@ -90,6 +97,8 @@ delegate_supports_btrace (struct target_ops *self) static void install_delegators (struct target_ops *ops) { + if (ops->to_detach == NULL) + ops->to_detach = delegate_detach; if (ops->to_resume == NULL) ops->to_resume = delegate_resume; if (ops->to_wait == NULL) diff --git a/gdb/target.c b/gdb/target.c index c992cf3..dc14c60 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -2597,19 +2597,10 @@ target_detach (const char *args, int from_tty) prepare_for_detach (); - for (t = current_target.beneath; t != NULL; t = t->beneath) - { - if (t->to_detach != NULL) - { - t->to_detach (t, args, from_tty); - if (targetdebug) - fprintf_unfiltered (gdb_stdlog, "target_detach (%s, %d)\n", - args, from_tty); - return; - } - } - - internal_error (__FILE__, __LINE__, _("could not find a target to detach")); + current_target.to_detach (¤t_target, args, from_tty); + if (targetdebug) + fprintf_unfiltered (gdb_stdlog, "target_detach (%s, %d)\n", + args, from_tty); } void -- 1.8.1.4 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #4: patch #3 --] [-- Type: text/x-patch, Size: 18687 bytes --] From a0d428f0ac10c6acc8a41bd69912e4bf26b6066e Mon Sep 17 00:00:00 2001 From: Tom Tromey <tromey@redhat.com> Date: Fri, 13 Dec 2013 14:14:29 -0700 Subject: [PATCH 08/11] add default implementation code --- gdb/make-target-delegates | 107 ++++++++++++++++++++++++++++++++-------------- gdb/target-delegates.c | 72 +++++++++++++++++++++++++++++++ gdb/target.c | 65 ++++------------------------ gdb/target.h | 50 ++++++++++++++++------ 4 files changed, 192 insertions(+), 102 deletions(-) diff --git a/gdb/make-target-delegates b/gdb/make-target-delegates index 89a0920..ed982c1 100755 --- a/gdb/make-target-delegates +++ b/gdb/make-target-delegates @@ -1,29 +1,12 @@ #!/usr/bin/perl -# For the time being we only allow certain methods. -%OK = ( - 'to_resume' => 1, - 'to_wait' => 1, - 'to_store_registers' => 1, - 'to_insert_breakpoint' => 1, - 'to_remove_breakpoint' => 1, - 'to_can_async_p' => 1, - 'to_is_async_p' => 1, - 'to_async' => 1, - 'to_stopped_by_watchpoint' => 1, - 'to_stopped_data_address' => 1, - 'to_xfer_partial' => 1, - 'to_supports_btrace' => 1, - 'to_detach' => 1 -); - # The line we search for in target.h that marks where we should start # looking for methods. $TRIGGER = qr,^struct target_ops$,; # The end of the methods part. $ENDER = qr,^\s*};$,; -$SYMBOL = qr,[a-zA-Z0-9_]+,; +$SYMBOL = qr,[a-zA-Z_][a-zA-Z0-9_]*,; $NAME_PART = qr,\(\*(?<name>${SYMBOL}+)\)\s,; $ARGS_PART = qr,(?<args>\(.*)$,; $INTRO_PART = qr,^\s*,; @@ -31,10 +14,14 @@ $INTRO_PART = qr,^\s*,; $SIMPLE_RETURN_PART = qr,[^\(]+,; $VEC_RETURN_PART = qr,VEC\s*\([^\)]+\)[^\(]*,; +$TARGET_DEFAULT_PART = qr,TARGET_DEFAULT\s*\((?<default_arg>.+)\),; + $METHOD = ($INTRO_PART . "(?<return_type>" . $SIMPLE_RETURN_PART . "|" . $VEC_RETURN_PART . ")" . $NAME_PART . $ARGS_PART); +$METHOD_TRAILER = qr,(?<args>\([^\)]+\))\s*${TARGET_DEFAULT_PART};$,; + sub trim($) { my ($result) = @_; $result =~ s,^\s*(\S*)\s*$,\1,; @@ -56,7 +43,7 @@ sub find_trigger() { sub parse_argtypes($) { my ($typestr) = @_; - $typestr =~ s/^\((.*)\);$/\1/; + $typestr =~ s/^\((.*)\)$/\1/; my (@typelist) = split (/,\s*/, $typestr); my (@result, $iter, $onetype); @@ -83,12 +70,13 @@ sub dname($) { return $name; } -# Write out a delegation function. -sub write_delegator($$@) { +# Write function header given name, return type, and argtypes. +# Returns a list of actual argument names. +sub write_function_header($$@) { my ($name, $return_type, @argtypes) = @_; print "static " . $return_type . "\n"; - print dname ($name) . ' ('; + print $name . ' ('; my $iter; my @argdecls; @@ -117,17 +105,56 @@ sub write_delegator($$@) { print join (', ', @argdecls) . ")\n"; print "{\n"; - print " self = self->beneath;\n"; + + return @actuals; +} + +# Write out a delegation function. +sub write_delegator($$@) { + my ($name, $return_type, @argtypes) = @_; + + my (@names) = write_function_header (dname ($name), $return_type, + @argtypes); + + print " $names[0] = $names[0]->beneath;\n"; print " "; if ($return_type ne 'void') { print "return "; } - print "self->" . $name . " ("; - print join (', ', @actuals); + print "$names[0]->" . $name . " ("; + print join (', ', @names); print ");\n"; print "}\n\n"; } +sub tdname ($) { + my ($name) = @_; + $name =~ s/to_/tdefault_/; + return $name; +} + +# Write out a default function. +sub write_tdefault($$$@) { + my ($content, $name, $return_type, @argtypes) = @_; + + if ($content =~ /^${SYMBOL}$/) { + return $content; + } + + write_function_header (tdname ($name), $return_type, @argtypes); + print " "; + + if ($content !~ /^${SYMBOL}/) { + print "return $content;\n"; + } else { + print "$content;\n"; + } + + print "}\n\n"; + + return tdname ($name); +} + print "/* THIS FILE IS GENERATED -*- buffer-read-only: t -*- */\n"; print "/* vi:set ro: */\n\n"; print "/* To regenerate this file, run:*/\n"; @@ -135,23 +162,27 @@ print "/* make-target-delegates target.h > target-delegates.c */\n"; find_trigger(); +%tdefault_names = (); @delegators = (); -$current_args = ''; +$current_line = ''; while (<>) { chomp; last if m/$ENDER/; - if ($current_args ne '') { + if ($current_line ne '') { s/^\s*//; - $current_args .= $_; + $current_line .= $_; } elsif (m/$METHOD/) { $name = $+{name}; - $current_args = $+{args}; + $current_line = $+{args}; $return_type = trim ($+{return_type}); } - if ($current_args =~ /\);\s*$/) { - if (defined $OK{$name}) { + if ($current_line =~ /\);\s*$/) { + if ($current_line =~ m,$METHOD_TRAILER,) { + $current_args = $+{args}; + $tdefault = $+{default_arg}; + @argtypes = parse_argtypes ($current_args); # The first argument must be "this" to be delegatable. @@ -159,10 +190,14 @@ while (<>) { write_delegator ($name, $return_type, @argtypes); push @delegators, $name; + + $tdefault_names{$name} = write_tdefault ($tdefault, + $name, $return_type, + @argtypes); } } - $current_args = ''; + $current_line = ''; } } @@ -173,4 +208,12 @@ for $iter (@delegators) { print " if (ops->" . $iter . " == NULL)\n"; print " ops->" . $iter . " = " . dname ($iter) . ";\n"; } +print "}\n\n"; + +# Now the default method code. +print "static void\ninstall_dummy_methods (struct target_ops *ops)\n{\n"; + +for $iter (@delegators) { + print " ops->" . $iter . " = " . $tdefault_names{$iter} . ";\n"; +} print "}\n"; diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 1bfbde6..84429a4 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -11,12 +11,24 @@ delegate_detach (struct target_ops *self, const char *arg1, int arg2) } static void +tdefault_detach (struct target_ops *self, const char *arg1, int arg2) +{ + target_ignore (); +} + +static void delegate_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal arg3) { self = self->beneath; self->to_resume (self, arg1, arg2, arg3); } +static void +tdefault_resume (struct target_ops *self, ptid_t arg1, int arg2, enum gdb_signal arg3) +{ + noprocess (); +} + static ptid_t delegate_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *arg2, int arg3) { @@ -24,6 +36,12 @@ delegate_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *a return self->to_wait (self, arg1, arg2, arg3); } +static ptid_t +tdefault_wait (struct target_ops *self, ptid_t arg1, struct target_waitstatus *arg2, int arg3) +{ + noprocess (); +} + static void delegate_store_registers (struct target_ops *self, struct regcache *arg1, int arg2) { @@ -31,6 +49,12 @@ delegate_store_registers (struct target_ops *self, struct regcache *arg1, int ar self->to_store_registers (self, arg1, arg2); } +static void +tdefault_store_registers (struct target_ops *self, struct regcache *arg1, int arg2) +{ + noprocess (); +} + static int delegate_insert_breakpoint (struct target_ops *self, struct gdbarch *arg1, struct bp_target_info *arg2) { @@ -53,6 +77,12 @@ delegate_stopped_by_watchpoint (struct target_ops *self) } static int +tdefault_stopped_by_watchpoint (struct target_ops *self) +{ + return 0; +} + +static int delegate_stopped_data_address (struct target_ops *self, CORE_ADDR *arg1) { self = self->beneath; @@ -60,6 +90,12 @@ delegate_stopped_data_address (struct target_ops *self, CORE_ADDR *arg1) } static int +tdefault_stopped_data_address (struct target_ops *self, CORE_ADDR *arg1) +{ + return 0; +} + +static int delegate_can_async_p (struct target_ops *self) { self = self->beneath; @@ -80,6 +116,12 @@ delegate_async (struct target_ops *self, async_callback_ftype *arg1, void *arg2) self->to_async (self, arg1, arg2); } +static void +tdefault_async (struct target_ops *self, async_callback_ftype *arg1, void *arg2) +{ + tcomplain (); +} + static LONGEST delegate_xfer_partial (struct target_ops *self, enum target_object arg1, const char *arg2, gdb_byte *arg3, const gdb_byte *arg4, ULONGEST arg5, LONGEST arg6) { @@ -87,6 +129,12 @@ delegate_xfer_partial (struct target_ops *self, enum target_object arg1, const return self->to_xfer_partial (self, arg1, arg2, arg3, arg4, arg5, arg6); } +static LONGEST +tdefault_xfer_partial (struct target_ops *self, enum target_object arg1, const char *arg2, gdb_byte *arg3, const gdb_byte *arg4, ULONGEST arg5, LONGEST arg6) +{ + return -1; +} + static int delegate_supports_btrace (struct target_ops *self) { @@ -94,6 +142,12 @@ delegate_supports_btrace (struct target_ops *self) return self->to_supports_btrace (self); } +static int +tdefault_supports_btrace (struct target_ops *self) +{ + return 0; +} + static void install_delegators (struct target_ops *ops) { @@ -124,3 +178,21 @@ install_delegators (struct target_ops *ops) if (ops->to_supports_btrace == NULL) ops->to_supports_btrace = delegate_supports_btrace; } + +static void +install_dummy_methods (struct target_ops *ops) +{ + ops->to_detach = tdefault_detach; + ops->to_resume = tdefault_resume; + ops->to_wait = tdefault_wait; + ops->to_store_registers = tdefault_store_registers; + ops->to_insert_breakpoint = memory_insert_breakpoint; + ops->to_remove_breakpoint = memory_remove_breakpoint; + ops->to_stopped_by_watchpoint = tdefault_stopped_by_watchpoint; + ops->to_stopped_data_address = tdefault_stopped_data_address; + ops->to_can_async_p = find_default_can_async_p; + ops->to_is_async_p = find_default_is_async_p; + ops->to_async = tdefault_async; + ops->to_xfer_partial = tdefault_xfer_partial; + ops->to_supports_btrace = tdefault_supports_btrace; +} diff --git a/gdb/target.c b/gdb/target.c index dc14c60..b6d8c85 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -45,8 +45,6 @@ #include "gdb/fileio.h" #include "agent.h" -#include "target-delegates.c" - static void target_info (char *, int); static void default_terminal_info (const char *, int); @@ -81,6 +79,12 @@ static LONGEST default_xfer_partial (struct target_ops *ops, static struct gdbarch *default_thread_architecture (struct target_ops *ops, ptid_t ptid); +static int find_default_can_async_p (struct target_ops *ignore); + +static int find_default_is_async_p (struct target_ops *ignore); + +#include "target-delegates.c" + static void init_dummy_target (void); static struct target_ops debug_target; @@ -3664,46 +3668,6 @@ dummy_pid_to_str (struct target_ops *ops, ptid_t ptid) return normal_pid_to_str (ptid); } -static void -dummy_resume (struct target_ops *self, ptid_t ptid, int step, - enum gdb_signal signal) -{ - noprocess (); -} - -static ptid_t -dummy_wait (struct target_ops *self, ptid_t ptid, - struct target_waitstatus *status, int options) -{ - noprocess (); -} - -static void -dummy_store_registers (struct target_ops *self, - struct regcache *regcache, - int regno) -{ - noprocess (); -} - -static LONGEST -dummy_xfer_partial (struct target_ops *ops, - enum target_object object, - const char *annex, gdb_byte *readbuf, - const gdb_byte *writebuf, - ULONGEST offset, LONGEST len) -{ - return -1; -} - -static void -dummy_async (struct target_ops *self, - void (*callback) (enum inferior_event_type, void *), - void *datum) -{ - tcomplain (); -} - /* Error-catcher for target_find_memory_regions. */ static int dummy_find_memory_regions (find_memory_region_ftype ignore1, void *ignore2) @@ -3745,14 +3709,7 @@ init_dummy_target (void) dummy_target.to_longname = "None"; dummy_target.to_doc = ""; dummy_target.to_attach = find_default_attach; - dummy_target.to_detach = - (void (*)(struct target_ops *, const char *, int))target_ignore; dummy_target.to_create_inferior = find_default_create_inferior; - dummy_target.to_resume = dummy_resume; - dummy_target.to_wait = dummy_wait; - dummy_target.to_can_async_p = find_default_can_async_p; - dummy_target.to_is_async_p = find_default_is_async_p; - dummy_target.to_async = dummy_async; dummy_target.to_supports_non_stop = find_default_supports_non_stop; dummy_target.to_supports_disable_randomization = find_default_supports_disable_randomization; @@ -3762,21 +3719,15 @@ init_dummy_target (void) dummy_target.to_make_corefile_notes = dummy_make_corefile_notes; dummy_target.to_get_bookmark = dummy_get_bookmark; dummy_target.to_goto_bookmark = dummy_goto_bookmark; - dummy_target.to_xfer_partial = dummy_xfer_partial; dummy_target.to_has_all_memory = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_memory = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_stack = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_execution = (int (*) (struct target_ops *, ptid_t)) return_zero; - dummy_target.to_store_registers = dummy_store_registers; - dummy_target.to_stopped_by_watchpoint - = (int (*) (struct target_ops *)) return_zero; - dummy_target.to_stopped_data_address = - (int (*) (struct target_ops *, CORE_ADDR *)) return_zero; - dummy_target.to_supports_btrace - = (int (*) (struct target_ops *)) return_zero; dummy_target.to_magic = OPS_MAGIC; + + install_dummy_methods (&dummy_target); } \f static void diff --git a/gdb/target.h b/gdb/target.h index 4b364ef..20bb33a 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -343,6 +343,17 @@ struct thread_info; /* fwd decl for parameter list below: */ typedef void async_callback_ftype (enum inferior_event_type, void *); +/* This is used to indicate the default implementation for a target + method. It is handled by make-target-delegates. The argument can be: + . A simple constant, like "0", in which case the default method + returns the constant. + . A function call, like "tcomplain ()", in which case the default + method simply calls the function. Ordinarily such functions must + therefore be "noreturn". + . The name of a function, like "memory_insert_breakpoint", in which + case the named function is used as the default method. */ +#define TARGET_DEFAULT(ARG) + struct target_ops { struct target_ops *beneath; /* To the target under this one. */ @@ -365,13 +376,17 @@ struct target_ops void (*to_close) (void); void (*to_attach) (struct target_ops *ops, char *, int); void (*to_post_attach) (int); - void (*to_detach) (struct target_ops *ops, const char *, int); + void (*to_detach) (struct target_ops *ops, const char *, int) + TARGET_DEFAULT (target_ignore ()); void (*to_disconnect) (struct target_ops *, char *, int); - void (*to_resume) (struct target_ops *, ptid_t, int, enum gdb_signal); + void (*to_resume) (struct target_ops *, ptid_t, int, enum gdb_signal) + TARGET_DEFAULT (noprocess ()); ptid_t (*to_wait) (struct target_ops *, - ptid_t, struct target_waitstatus *, int); + ptid_t, struct target_waitstatus *, int) + TARGET_DEFAULT (noprocess ()); void (*to_fetch_registers) (struct target_ops *, struct regcache *, int); - void (*to_store_registers) (struct target_ops *, struct regcache *, int); + void (*to_store_registers) (struct target_ops *, struct regcache *, int) + TARGET_DEFAULT (noprocess ()); void (*to_prepare_to_store) (struct regcache *); /* Transfer LEN bytes of memory between GDB address MYADDR and @@ -402,9 +417,11 @@ struct target_ops void (*to_files_info) (struct target_ops *); int (*to_insert_breakpoint) (struct target_ops *, struct gdbarch *, - struct bp_target_info *); + struct bp_target_info *) + TARGET_DEFAULT (memory_insert_breakpoint); int (*to_remove_breakpoint) (struct target_ops *, struct gdbarch *, - struct bp_target_info *); + struct bp_target_info *) + TARGET_DEFAULT (memory_remove_breakpoint); int (*to_can_use_hw_breakpoint) (int, int, int); int (*to_ranged_break_num_registers) (struct target_ops *); int (*to_insert_hw_breakpoint) (struct gdbarch *, struct bp_target_info *); @@ -419,10 +436,12 @@ struct target_ops CORE_ADDR, CORE_ADDR, int); int (*to_remove_mask_watchpoint) (struct target_ops *, CORE_ADDR, CORE_ADDR, int); - int (*to_stopped_by_watchpoint) (struct target_ops *); + int (*to_stopped_by_watchpoint) (struct target_ops *) + TARGET_DEFAULT (0); int to_have_steppable_watchpoint; int to_have_continuable_watchpoint; - int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *); + int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *) + TARGET_DEFAULT (0); int (*to_watchpoint_addr_within_range) (struct target_ops *, CORE_ADDR, CORE_ADDR, int); @@ -484,9 +503,12 @@ struct target_ops int to_has_thread_control; /* control thread execution */ int to_attach_no_wait; /* ASYNC target controls */ - int (*to_can_async_p) (struct target_ops *); - int (*to_is_async_p) (struct target_ops *); - void (*to_async) (struct target_ops *, async_callback_ftype *, void *); + int (*to_can_async_p) (struct target_ops *) + TARGET_DEFAULT (find_default_can_async_p); + int (*to_is_async_p) (struct target_ops *) + TARGET_DEFAULT (find_default_is_async_p); + void (*to_async) (struct target_ops *, async_callback_ftype *, void *) + TARGET_DEFAULT (tcomplain ()); int (*to_supports_non_stop) (void); /* find_memory_regions support method for gcore */ int (*to_find_memory_regions) (find_memory_region_ftype func, void *data); @@ -537,7 +559,8 @@ struct target_ops LONGEST (*to_xfer_partial) (struct target_ops *ops, enum target_object object, const char *annex, gdb_byte *readbuf, const gdb_byte *writebuf, - ULONGEST offset, LONGEST len); + ULONGEST offset, LONGEST len) + TARGET_DEFAULT (-1); /* Returns the memory map for the target. A return value of NULL means that no memory map is available. If a memory address @@ -817,7 +840,8 @@ struct target_ops int (*to_can_use_agent) (void); /* Check whether the target supports branch tracing. */ - int (*to_supports_btrace) (struct target_ops *); + int (*to_supports_btrace) (struct target_ops *) + TARGET_DEFAULT (0); /* Enable branch tracing for PTID and allocate a branch trace target information struct for reading and for disabling branch trace. */ -- 1.8.1.4 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #5: patch #4 --] [-- Type: text/x-patch, Size: 3280 bytes --] From 35f46ca676e8e766fe7e384a7d1be89842a713bd Mon Sep 17 00:00:00 2001 From: Tom Tromey <tromey@redhat.com> Date: Fri, 13 Dec 2013 14:16:57 -0700 Subject: [PATCH 09/11] update to_attach --- gdb/target-delegates.c | 10 ++++++++++ gdb/target.c | 21 ++++----------------- gdb/target.h | 3 ++- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/gdb/target-delegates.c b/gdb/target-delegates.c index 84429a4..0847d09 100644 --- a/gdb/target-delegates.c +++ b/gdb/target-delegates.c @@ -4,6 +4,13 @@ /* To regenerate this file, run:*/ /* make-target-delegates target.h > target-delegates.c */ static void +delegate_attach (struct target_ops *self, char *arg1, int arg2) +{ + self = self->beneath; + self->to_attach (self, arg1, arg2); +} + +static void delegate_detach (struct target_ops *self, const char *arg1, int arg2) { self = self->beneath; @@ -151,6 +158,8 @@ tdefault_supports_btrace (struct target_ops *self) static void install_delegators (struct target_ops *ops) { + if (ops->to_attach == NULL) + ops->to_attach = delegate_attach; if (ops->to_detach == NULL) ops->to_detach = delegate_detach; if (ops->to_resume == NULL) @@ -182,6 +191,7 @@ install_delegators (struct target_ops *ops) static void install_dummy_methods (struct target_ops *ops) { + ops->to_attach = find_default_attach; ops->to_detach = tdefault_detach; ops->to_resume = tdefault_resume; ops->to_wait = tdefault_wait; diff --git a/gdb/target.c b/gdb/target.c index b6d8c85..a5c5505 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -3708,7 +3708,6 @@ init_dummy_target (void) dummy_target.to_shortname = "None"; dummy_target.to_longname = "None"; dummy_target.to_doc = ""; - dummy_target.to_attach = find_default_attach; dummy_target.to_create_inferior = find_default_create_inferior; dummy_target.to_supports_non_stop = find_default_supports_non_stop; dummy_target.to_supports_disable_randomization @@ -3755,22 +3754,10 @@ target_close (struct target_ops *targ) void target_attach (char *args, int from_tty) { - struct target_ops *t; - - for (t = current_target.beneath; t != NULL; t = t->beneath) - { - if (t->to_attach != NULL) - { - t->to_attach (t, args, from_tty); - if (targetdebug) - fprintf_unfiltered (gdb_stdlog, "target_attach (%s, %d)\n", - args, from_tty); - return; - } - } - - internal_error (__FILE__, __LINE__, - _("could not find a target to attach")); + current_target.to_attach (¤t_target, args, from_tty); + if (targetdebug) + fprintf_unfiltered (gdb_stdlog, "target_attach (%s, %d)\n", + args, from_tty); } int diff --git a/gdb/target.h b/gdb/target.h index 20bb33a..fc53f56 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -374,7 +374,8 @@ struct target_ops to xfree everything (including the "struct target_ops"). */ void (*to_xclose) (struct target_ops *targ); void (*to_close) (void); - void (*to_attach) (struct target_ops *ops, char *, int); + void (*to_attach) (struct target_ops *ops, char *, int) + TARGET_DEFAULT (find_default_attach); void (*to_post_attach) (int); void (*to_detach) (struct target_ops *ops, const char *, int) TARGET_DEFAULT (target_ignore ()); -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-13 22:07 ` Tom Tromey @ 2013-12-16 13:07 ` Pedro Alves 2013-12-16 21:21 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-12-16 13:07 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 12/13/2013 10:07 PM, Tom Tromey wrote: >>>>>> "Tom" == Tom Tromey <tromey@redhat.com> writes: > > Tom> Since this is very repetitive I would consider doing it via a ".defs" > Tom> file and then some macrology to reduce the amount of boilerplate. > > I tried this a bit and the result was pretty ugly. > > Instead I wrote a little script that can parse just enough of "struct > target_ops" to auto-generate delegation methods when appropriate. > > See patch #1 attached below. This patch just converts the methods used > by the record targets -- that is, it's just a replacement for patch #3 > in this series. > > Note that the patch isn't ready as-is. The log entry is wrong and it > does the wrong thing for a couple of the target methods. I'm curious to > know what you think about the overall shape of it. I like it a lot. > > > As an example of the benefit of this approach, patch #2 converts > to_detach to the new style. When examining this patch recall that > target-delegates.c is auto-generated. You can see from this that the > result is less code to maintain. > > > I then went a little further. It seems to me that we also need a lot of > boilerplate (not to mention the questionable casts there right now) to > handle the dummy target methods. Patch #3 makes these into simple > annotations in the target.h file, getting rid of the list of handled > methods from make-target-delegates as well. Nice. > > > Before reaching judgment on patch #3, see patch #4, which converts > to_attach. I've written a few more like it. > > > The long-term vision for this approach is to regularize all target > methods according to these rules: > > 1. Each method will take a "struct target_ops *" as its first argument. > > 2. Nearly every method will be defaulted to the appropriate delegate_ > method. There may be exceptions, coded into > complete_target_initialization. > > 3. de_fault will be completely eliminated. INHERIT will be limited to > the smallest possible list of fields, like to_shortname. Nearly > every current_target method will simply delegate. Sounds like the squashed target will end up mostly unused? It sounds to me that even for those (data fields) we wouldn't need INHERIT. As all targets have a name, e.g., target_shortname could be made a method that returns the name of the currently topmost target in the stack (not the squashed target). > > 4. Target API users will call top-level target_* functions that will in > most cases just call into current_target. > > 5. Any given to_* method implementation will be guaranteed to be called > with the target_ops with which it was registered. (This rule is > handy on multi-target, where I split out the target_ops vtable from > the object itself.) > > > Since I'm on a cleanup kick at the moment, and since a good amount of > this is relevant to the multi-target work, I think it would be good to > start the conversion shortly after 7.7 is branched. > > This is a bit difficult since I don't have access to many targets. I'm > not sure what to do about this aspect. What I've done in that past for this sort of thing is just go ahead and do the almost mechanical adjustment across the board, and don't fret about it if something breaks. Publish git test branches, IOW, make it very easy for people to test, report issues and hopefully help with targets they care about should help drive this through faster. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-16 13:07 ` Pedro Alves @ 2013-12-16 21:21 ` Tom Tromey 2013-12-17 16:17 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-16 21:21 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: >> 3. de_fault will be completely eliminated. INHERIT will be limited to >> the smallest possible list of fields, like to_shortname. Nearly >> every current_target method will simply delegate. Pedro> Sounds like the squashed target will end up mostly unused? It sounds Pedro> to me that even for those (data fields) we wouldn't need INHERIT. As Pedro> all targets have a name, e.g., target_shortname could be made a method Pedro> that returns the name of the currently topmost target in the stack Pedro> (not the squashed target). Yeah. The squashed target may still need to exist, or the debug target would have to be recast as a new stratum. >> 5. Any given to_* method implementation will be guaranteed to be called >> with the target_ops with which it was registered. (This rule is >> handy on multi-target, where I split out the target_ops vtable from >> the object itself.) FWIW I wrote a script to add all the missing "this" arguments -- not just to target.h but to every method implementation in the tree. This yields 103 patches -- one per converted method. (I may zap a few of these, the bulk of them seem to make sense...) I'm not quite sure how to submit it. I mean, email is doable, but it seems a bit large. >> This is a bit difficult since I don't have access to many targets. I'm >> not sure what to do about this aspect. Pedro> What I've done in that past for this sort of thing is just go ahead Pedro> and do the almost mechanical adjustment across the board, and don't Pedro> fret about it if something breaks. Publish git test branches, IOW, Pedro> make it very easy for people to test, report issues and hopefully Pedro> help with targets they care about should help drive this through Pedro> faster. Yeah. It doesn't quite sit right but it does seem like the best way. The nice thing about these refactorings is that the needed changes are usually trivial anyhow. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-16 21:21 ` Tom Tromey @ 2013-12-17 16:17 ` Pedro Alves 2013-12-18 18:29 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-12-17 16:17 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 12/16/2013 09:21 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > >>> 3. de_fault will be completely eliminated. INHERIT will be limited to >>> the smallest possible list of fields, like to_shortname. Nearly >>> every current_target method will simply delegate. > > Pedro> Sounds like the squashed target will end up mostly unused? It sounds > Pedro> to me that even for those (data fields) we wouldn't need INHERIT. As > Pedro> all targets have a name, e.g., target_shortname could be made a method > Pedro> that returns the name of the currently topmost target in the stack > Pedro> (not the squashed target). > > Yeah. The squashed target may still need to exist, or the debug target > would have to be recast as a new stratum. Yeah. It's always been silly that "set debug target" only works if specified before opening/pushing the target. >>> 5. Any given to_* method implementation will be guaranteed to be called >>> with the target_ops with which it was registered. (This rule is >>> handy on multi-target, where I split out the target_ops vtable from >>> the object itself.) > > FWIW I wrote a script to add all the missing "this" arguments -- not > just to target.h but to every method implementation in the tree. Nice! > This yields 103 patches -- one per converted method. (I may zap a few > of these, the bulk of them seem to make sense...) I'm not quite sure > how to submit it. I mean, email is doable, but it seems a bit large. > Yeah, not sure either... squash a few; post only the non-trivial ones as email; post all of them at once anyway; split in 4 or 5 batches. Whatever really is fine with me. > The nice thing about these refactorings is that the needed changes are > usually trivial anyhow. Yeah. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-17 16:17 ` Pedro Alves @ 2013-12-18 18:29 ` Tom Tromey 2013-12-18 22:06 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-18 18:29 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> Yeah. It's always been silly that "set debug target" only works if Pedro> specified before opening/pushing the target. Yeah, good point. It would be nice if we could auto-generate the debug code as well. Not sure if I'll try to tackle that, this patch series is already absurd. Tom> This yields 103 patches -- one per converted method. (I may zap a few Tom> of these, the bulk of them seem to make sense...) I'm not quite sure Tom> how to submit it. I mean, email is doable, but it seems a bit large. Pedro> Yeah, not sure either... squash a few; post only the non-trivial Pedro> ones as email; post all of them at once anyway; split in 4 or Pedro> 5 batches. Whatever really is fine with me. I'm inclined to send the series as-is because it would be more hilarious. I distantly wonder if this means I have lost my grip. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-18 18:29 ` Tom Tromey @ 2013-12-18 22:06 ` Tom Tromey 2013-12-19 16:03 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-18 22:06 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro> Yeah, not sure either... squash a few; post only the non-trivial Pedro> ones as email; post all of them at once anyway; split in 4 or Pedro> 5 batches. Whatever really is fine with me. In case you want to see my progress, I've pushed the current branch to my gitorious repository: git@gitorious.org:binutils-gdb/gdb.git The branch is "target-cleanup". I've built the branch but barely tested it. I'm sure it breaks in some configurations -- the fixes are trivial but I did have to do a number of them as my refactoring script, while fun, could not perfectly fix everything. Aside from a few patches at the beginning of the series I think it's all split and ordered pretty much as I'd like it to be. I converted all the easy target methods to the new inheritance scheme. This works out to be nearly all of them. There are a few more that aren't too bad to convert (actually, looking again now I see I missed a few more easy ones), and then a handful of tricky ones. It's over the 200 patch mark... not sure I want to write those ChangeLogs (my refactoring script only did the first 100 or so for me...) or even re-read them all for sanity. I hope you will agree that the result is much cleaner. It's simpler to explain and understand; and it is obvious which model one ought to choose when adding new target methods. It's also a good setup for the multi-target work. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-18 22:06 ` Tom Tromey @ 2013-12-19 16:03 ` Pedro Alves 2013-12-19 16:15 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-12-19 16:03 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 12/18/2013 10:06 PM, Tom Tromey wrote: > Pedro> Yeah, not sure either... squash a few; post only the non-trivial > Pedro> ones as email; post all of them at once anyway; split in 4 or > Pedro> 5 batches. Whatever really is fine with me. > > In case you want to see my progress, I've pushed the current branch to > my gitorious repository: > > git@gitorious.org:binutils-gdb/gdb.git I guess that only works for you. I used: git://gitorious.org/binutils-gdb/gdb.git I like what I see a lot. > > The branch is "target-cleanup". > > I've built the branch but barely tested it. I'm sure it breaks in some > configurations -- the fixes are trivial but I did have to do a number of > them as my refactoring script, while fun, could not perfectly fix > everything. > > Aside from a few patches at the beginning of the series I think it's all > split and ordered pretty much as I'd like it to be. > > I converted all the easy target methods to the new inheritance scheme. > This works out to be nearly all of them. There are a few more that > aren't too bad to convert (actually, looking again now I see I missed a > few more easy ones), and then a handful of tricky ones. > > It's over the 200 patch mark... not sure I want to write those > ChangeLogs (my refactoring script only did the first 100 or so for > me...) or even re-read them all for sanity. > > I hope you will agree that the result is much cleaner. It's simpler to > explain and understand; and it is obvious which model one ought to > choose when adding new target methods. It's also a good setup for the > multi-target work. Yes, I definitely agree. Only thing I wonder is whether: > /* This is used to indicate the default implementation for a target > method. It is handled by make-target-delegates. The argument can be: > . A simple constant, like "0", in which case the default method > returns the constant. > . A function call, like "tcomplain ()", in which case the default > method simply calls the function. Ordinarily such functions must > therefore be "noreturn". > . The name of a function, like "memory_insert_breakpoint", in which > case the named function is used as the default method. */ > #define TARGET_DEFAULT(ARG) wouldn't be better be made less smart, and require explicit selection. I'm thinking of grepability, but mainly, to be able to distinguish returning enums vs forwarding. As in, for example, how do you tell that: TARGET_DEFAULT(TARGET_XFER_E_IO) is not meant to forward to a TARGET_XFER_E_IO target-like function? We could look at whether the symbol is uppercase, but then that seems to be getting fragile and harder to explain/read/'quickly grasp what happens if you're not familiar with the target symbols'. #define TARGET_DEFAULT_FORWARD(function_to_forward_to) e.g.: TARGET_DEFAULT_FORWARD(memory_insert_breakpoint) #define TARGET_DEFAULT_RET(expression) // constant, function call, etc. e.g.: TARGET_DEFAULT_RET(0) TARGET_DEFAULT_RET(TARGET_XFER_E_IO) TARGET_DEFAULT_RET(return_zero ()) #define TARGET_DEFAULT_NO_RET(noreturn function/expression) e.g.: TARGET_DEFAULT_NO_RET(noprocess ()) (I'm hating the idea of such a change causing a bunch of redo-series pain, so even if it proves better to be explicit, I'm fine with leaving it as is for now.) -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-19 16:03 ` Pedro Alves @ 2013-12-19 16:15 ` Tom Tromey 2013-12-20 19:24 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-19 16:15 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> wouldn't be better be made less smart, and require explicit Pedro> selection. I'm thinking of grepability, but mainly, to be Pedro> able to distinguish returning enums vs forwarding. Yeah, it's a good idea. Pedro> (I'm hating the idea of such a change causing a bunch of Pedro> redo-series pain, so even if it proves better to be explicit, Pedro> I'm fine with leaving it as is for now.) It's no trouble. I'm enamored of automating this kind of refactoring right now anyway. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-12-19 16:15 ` Tom Tromey @ 2013-12-20 19:24 ` Tom Tromey 0 siblings, 0 replies; 53+ messages in thread From: Tom Tromey @ 2013-12-20 19:24 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro> (I'm hating the idea of such a change causing a bunch of Pedro> redo-series pain, so even if it proves better to be explicit, Pedro> I'm fine with leaving it as is for now.) Tom> It's no trouble. I'm enamored of automating this kind of refactoring Tom> right now anyway. Well, automation didn't work out too well, but still, N rebases later I've force-pushed a new branch that has the change. I cleaned up a few of the earlier patches as well. Many ChangeLogs still aren't written and I haven't done the testing, but it's still worth taking a glance at. It's 229 patches now. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 3/9] add target method delegation 2013-10-28 17:51 ` Tom Tromey 2013-10-28 17:53 ` Tom Tromey @ 2013-11-08 16:34 ` Pedro Alves 1 sibling, 0 replies; 53+ messages in thread From: Pedro Alves @ 2013-11-08 16:34 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/28/2013 05:51 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > > Pedro> Could you please split out the patch for the second problem? I > Pedro> think it'll be a small one. > > Sure. > > Pedro> What's the plan for the existing target methods that > Pedro> currently already do a similar beneath lookup? I'd like it that > Pedro> there's be at least a plan, so we don't end up with yet another > Pedro> way of doing things, two incomplete transitions, and no clear direction. > > Do you mean things like target_detach? Yes. > > If so, then I think these are two different things. > > target.h declares things both for users of the target API and for the > implementation of targets. > > Something like target_detach is a public API. My understanding of the > current scheme is that a public-facing method can either be implemented > by a function like target_detach, which encodes all the needed logic; or > by a macro that just calls into the target_ops, in which case the method > must use the inherit/default machinery. Right. I'm thinking the public functions that do the run time target beneath walk, instead of the inherit/default mechanism, and about having them defer to the delegation version, avoiding the need to implement basically the same twice. Like, to take a simple example: int target_has_all_memory_1 (void) { struct target_ops *t; for (t = current_target.beneath; t != NULL; t = t->beneath) if (t->to_has_all_memory (t)) return 1; return 0; } becomes: #define target_has_all_memory_1 \ target_delegate_has_all_memory (¤t_target) target_detach becomes (completely untested): void target_detach (char *args, int from_tty) { struct target_ops* t; if (gdbarch_has_global_breakpoints (target_gdbarch ())) /* Don't remove global breakpoints here. They're removed on disconnection from the target. */ ; else /* If we're in breakpoints-always-inserted mode, have to remove them before detaching. */ remove_breakpoints_pid (ptid_get_pid (inferior_ptid)); prepare_for_detach (); target_delegate_detach (¤t_target, args, from_tty); if (targetdebug) fprintf_unfiltered (gdb_stdlog, "target_detach (%s, %d)\n", args, from_tty); } etc. So basically, whether that was in the plan. Is there something about the delegate mechanism that would make this not work? If so, could we make it work? Having this considered was the sort of plan thing I had in mind. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 4/9] PR gdb/13860: make -interpreter-exec console "list" behave more like "list". 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey ` (2 preceding siblings ...) 2013-10-22 17:59 ` [PATCH v4 3/9] add target method delegation Tom Tromey @ 2013-10-22 17:59 ` Tom Tromey 2013-10-22 17:59 ` [PATCH v4 2/9] add "this" pointers to more target APIs Tom Tromey ` (4 subsequent siblings) 8 siblings, 0 replies; 53+ messages in thread From: Tom Tromey @ 2013-10-22 17:59 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves From: Pedro Alves <palves@redhat.com> Patch 3 in this series made me notice that "list" behaves differently in CLI vs MI. Particularly: >./gdb -nx -q ./testsuite/gdb.mi/mi-cli Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/mi-cli...done. (gdb) start Temporary breakpoint 1 at 0x40054d: file ../../../src/gdb/testsuite/gdb.mi/basics.c, line 62. Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/mi-cli Temporary breakpoint 1, main () at ../../../src/gdb/testsuite/gdb.mi/basics.c:62 62 callee1 (2, "A string argument.", 3.5); (gdb) list 57 { 58 } 59 60 main () 61 { 62 callee1 (2, "A string argument.", 3.5); 63 callee1 (2, "A string argument.", 3.5); 64 65 do_nothing (); /* Hello, World! */ 66 (gdb) Note the list started at line 57. IOW, the program stopped at line 62, and GDB centered the list on that. compare with: >./gdb -nx -q ./testsuite/gdb.mi/mi-cli -i=mi =thread-group-added,id="i1" ~"Reading symbols from /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/mi-cli..." ~"done.\n" (gdb) start &"start\n" ~"Temporary breakpoint 1 at 0x40054d: file ../../../src/gdb/testsuite/gdb.mi/basics.c, line 62.\n" =breakpoint-created,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x000000000040054d",func="main",file="../../../src/gdb/testsuite/gdb.mi/basics.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/basics.c",line="62",times="0",original-location="main"} ~"Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/mi-cli \n" =thread-group-started,id="i1",pid="14221" =thread-created,id="1",group-id="i1" ^running *running,thread-id="all" (gdb) =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1" =library-loaded,id="/lib64/libm.so.6",target-name="/lib64/libm.so.6",host-name="/lib64/libm.so.6",symbols-loaded="0",thread-group="i1" =library-loaded,id="/lib64/libc.so.6",target-name="/lib64/libc.so.6",host-name="/lib64/libc.so.6",symbols-loaded="0",thread-group="i1" =breakpoint-modified,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x000000000040054d",func="main",file="../../../src/gdb/testsuite/gdb.mi/basics.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/basics.c",line="62",times="1",original-location="main"} ~"\nTemporary breakpoint " ~"1, main () at ../../../src/gdb/testsuite/gdb.mi/basics.c:62\n" ~"62\t callee1 (2, \"A string argument.\", 3.5);\n" *stopped,reason="breakpoint-hit",disp="del",bkptno="1",frame={addr="0x000000000040054d",func="main",args=[],file="../../../src/gdb/testsuite/gdb.mi/basics.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/basics.c",line="62"},thread-id="1",stopped-threads="all",core="0" =breakpoint-deleted,id="1" (gdb) -interpreter-exec console list ~"62\t callee1 (2, \"A string argument.\", 3.5);\n" ~"63\t callee1 (2, \"A string argument.\", 3.5);\n" ~"64\t\n" ~"65\t do_nothing (); /* Hello, World! */\n" ~"66\t\n" ~"67\t callme (1);\n" ~"68\t callme (2);\n" ~"69\t\n" ~"70\t return 0;\n" ~"71\t}\n" ^done (gdb) Here the list starts at line 62, where the program was stopped. This happens because print_stack_frame, called from both normal_stop and mi_on_normal_stop, is the function responsible for setting the current sal from the selected frame, overrides the PRINT_WHAT argument, and only after that does it decide whether to center the current sal line or not, based on the overriden value, and it will always decide false. (The print_stack_frame call in mi_on_normal_stop is a little different from the call in normal_stop, in that it is an unconditional SRC_AND_LOC call. The next patch will make those uniform.) Tested on x86_64 Fedora 16, no regressions. gdb/ 2012-05-09 Pedro Alves <palves@redhat.com> * stack.c (print_stack_frame): Compute CENTER before overriding PRINT_WHAT. gdb/testsuite/ 2012-05-09 Pedro Alves <palves@redhat.com> * gdb.mi/mi-cli.exp: Adjust expected output of "list". --- gdb/stack.c | 4 ++-- gdb/testsuite/gdb.mi/mi-cli.exp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/gdb/stack.c b/gdb/stack.c index cd4ac7a..c325cc2 100644 --- a/gdb/stack.c +++ b/gdb/stack.c @@ -159,14 +159,14 @@ print_stack_frame (struct frame_info *frame, int print_level, { volatile struct gdb_exception e; + int center = (print_what == SRC_LINE || print_what == SRC_AND_LOC); + /* For mi, alway print location and address. */ if (ui_out_is_mi_like_p (current_uiout)) print_what = LOC_AND_ADDRESS; TRY_CATCH (e, RETURN_MASK_ERROR) { - int center = (print_what == SRC_LINE || print_what == SRC_AND_LOC); - print_frame_info (frame, print_level, print_what, 1 /* print_args */, set_current_sal); if (set_current_sal) diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp index 59af58b..5b809e2 100644 --- a/gdb/testsuite/gdb.mi/mi-cli.exp +++ b/gdb/testsuite/gdb.mi/mi-cli.exp @@ -91,7 +91,7 @@ mi_gdb_test "-interpreter-exec console \"set listsize 1\"" \ # {.*\~"32[ \t(\\t)]*callee1.*\\n".*\^done } mi_gdb_test "-interpreter-exec console \"list\"" \ - ".*\~\"$line_main_body\[\\\\t \]*callee1.*;\\\\n\".*\\^done" \ + ".*\~\"57\\\\t\{\\\\n\".*\\^done" \ "-interpreter-exec console \"list\"" mi_execute_to "exec-continue" "breakpoint-hit" "callee4" "" ".*basics.c" $line_callee4_body \ -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 2/9] add "this" pointers to more target APIs 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey ` (3 preceding siblings ...) 2013-10-22 17:59 ` [PATCH v4 4/9] PR gdb/13860: make -interpreter-exec console "list" behave more like "list" Tom Tromey @ 2013-10-22 17:59 ` Tom Tromey 2013-10-28 16:04 ` Pedro Alves 2013-10-22 18:11 ` [PATCH v4 8/9] fix py-finish-breakpoint.exp with always-async Tom Tromey ` (3 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-22 17:59 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey A subsequent pass introduces delegation helper functions to the target API. This delegation is much cleaner if the target_ops pointer is directly available at delegation time. This patch adds the "this" pointer to various to_* methods for this purpose. This updates a number of ports which I am unable to test. Please give them a look-over. Any possible problem here is trivial, though, as all that is required is adding an argument to a function. * aarch64-linux-nat.c (aarch64_linux_stopped_by_watchpoint): Add 'ops' argument. * arm-linux-nat.c (arm_linux_stopped_by_watchpoint): Add 'ops' argument. * corelow.c (ignore): Add 'ops' argument. * exec.c (ignore): Add 'ops' argument. * i386-nat.c (i386_stopped_by_watchpoint): Add 'ops' argument. * ia64-linux-nat.c (ia64_linux_stopped_by_watchpoint): Add 'ops' argument. * inf-ttrace.c (inf_ttrace_stopped_by_watchpoint): Add 'ops' argument. * linux-nat.c (save_sigtrap): Update. (linux_nat_stopped_by_watchpoint, linux_nat_is_async_p) (linux_nat_can_async_p, linux_nat_async): Add 'ops' argument. (linux_nat_close): Update. * mem-break.c (memory_insert_breakpoint, memory_remove_breakpoint): Add 'ops' argument. * mips-linux-nat.c (mips_linux_stopped_by_watchpoint): Add 'ops' argument. * monitor.c (monitor_insert_breakpoint, monitor_remove_breakpoint): Add 'ops' argument. * nto-procfs.c (procfs_insert_breakpoint, procfs_remove_breakpoint) (procfs_stopped_by_watchpoint): Add 'ops' argument. * ppc-linux-nat.c (ppc_linux_stopped_by_watchpoint): Add 'ops' argument. * procfs.c (procfs_stopped_by_watchpoint): Add 'ops' argument. * record-full.c (record_full_beneath_to_insert_breakpoint) (record_full_beneath_to_remove_breakpoint) (record_full_beneath_to_stopped_by_watchpoint) (record_full_beneath_to_async, tmp_to_insert_breakpoint) (tmp_to_remove_breakpoint, tmp_to_stopped_by_watchpoint) (tmp_to_async): Add 'ops' argument. (record_full_stopped_by_watchpoint, record_full_insert_breakpoint) (record_full_remove_breakpoint, record_full_async) (record_full_can_async_p, record_full_is_async_p) (record_full_core_insert_breakpoint) (record_full_core_remove_breakpoint): Add 'ops' argument. * remote-m32r-sdi.c (m32r_insert_breakpoint, m32r_remove_breakpoint) (m32r_stopped_by_watchpoint): Add 'ops' argument. * remote-mips.c (mips_insert_breakpoint, mips_remove_breakpoint) (mips_stopped_by_watchpoint): Add 'ops' argument. * remote.c (remote_insert_breakpoint, remote_remove_breakpoint) (remote_stopped_by_watchpoint_p, remote_can_async_p) (remote_is_async_p, remote_async): Add 'ops' argument. * s390-nat.c (s390_stopped_by_watchpoint): Add 'ops' argument. * target.c (update_current_target) (target_insert_breakpoint, target_remove_breakpoint) (find_default_can_async_p, find_default_is_async_p): Update. (init_dummy_target): Update. (debug_to_insert_breakpoint, debug_to_remove_breakpoint) (debug_to_stopped_by_watchpoint): Add 'ops' argument. * target.h (struct target_ops) <to_insert_breakpoint, to_remove_breakpoint, to_stopped_by_watchpoint, to_can_async_p, to_is_async_p, to_async>: Add 'ops' argument. (target_can_async_p, target_is_async_p, target_async) (target_stopped_by_watchpoint): Update. (memory_remove_breakpoint, memory_insert_breakpoint): Add 'ops' argument. --- gdb/aarch64-linux-nat.c | 4 ++-- gdb/arm-linux-nat.c | 4 ++-- gdb/corelow.c | 3 ++- gdb/exec.c | 3 ++- gdb/i386-nat.c | 4 ++-- gdb/ia64-linux-nat.c | 4 ++-- gdb/inf-ttrace.c | 2 +- gdb/linux-nat.c | 22 +++++++++----------- gdb/mem-break.c | 6 ++++-- gdb/mips-linux-nat.c | 2 +- gdb/monitor.c | 6 ++++-- gdb/nto-procfs.c | 10 +++++---- gdb/ppc-linux-nat.c | 4 ++-- gdb/procfs.c | 2 +- gdb/record-full.c | 54 ++++++++++++++++++++++++++++++------------------- gdb/remote-m32r-sdi.c | 8 +++++--- gdb/remote-mips.c | 12 ++++++----- gdb/remote.c | 34 ++++++++++++++++++------------- gdb/s390-nat.c | 2 +- gdb/target.c | 45 ++++++++++++++++++++--------------------- gdb/target.h | 31 ++++++++++++++++------------ 21 files changed, 147 insertions(+), 115 deletions(-) diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c index 256725b..5f4baa5 100644 --- a/gdb/aarch64-linux-nat.c +++ b/gdb/aarch64-linux-nat.c @@ -1473,11 +1473,11 @@ aarch64_linux_stopped_data_address (struct target_ops *target, /* Implement the "to_stopped_by_watchpoint" target_ops method. */ static int -aarch64_linux_stopped_by_watchpoint (void) +aarch64_linux_stopped_by_watchpoint (struct target_ops *ops) { CORE_ADDR addr; - return aarch64_linux_stopped_data_address (¤t_target, &addr); + return aarch64_linux_stopped_data_address (ops, &addr); } /* Implement the "to_watchpoint_addr_within_range" target_ops method. */ diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c index 96573d9..bc037ad 100644 --- a/gdb/arm-linux-nat.c +++ b/gdb/arm-linux-nat.c @@ -1171,10 +1171,10 @@ arm_linux_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p) /* Has the target been stopped by hitting a watchpoint? */ static int -arm_linux_stopped_by_watchpoint (void) +arm_linux_stopped_by_watchpoint (struct target_ops *ops) { CORE_ADDR addr; - return arm_linux_stopped_data_address (¤t_target, &addr); + return arm_linux_stopped_data_address (ops, &addr); } static int diff --git a/gdb/corelow.c b/gdb/corelow.c index d1e7f6a..adc7bf6 100644 --- a/gdb/corelow.c +++ b/gdb/corelow.c @@ -844,7 +844,8 @@ core_xfer_partial (struct target_ops *ops, enum target_object object, breakpoint_init_inferior). */ static int -ignore (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) +ignore (struct target_ops *ops, struct gdbarch *gdbarch, + struct bp_target_info *bp_tgt) { return 0; } diff --git a/gdb/exec.c b/gdb/exec.c index 758cdc1..1eb6f39 100644 --- a/gdb/exec.c +++ b/gdb/exec.c @@ -756,7 +756,8 @@ exec_set_section_address (const char *filename, int index, CORE_ADDR address) breakpoint_init_inferior). */ static int -ignore (struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) +ignore (struct target_ops *ops, struct gdbarch *gdbarch, + struct bp_target_info *bp_tgt) { return 0; } diff --git a/gdb/i386-nat.c b/gdb/i386-nat.c index 0a5deb0..8d54ae0 100644 --- a/gdb/i386-nat.c +++ b/gdb/i386-nat.c @@ -756,10 +756,10 @@ i386_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p) } static int -i386_stopped_by_watchpoint (void) +i386_stopped_by_watchpoint (struct target_ops *ops) { CORE_ADDR addr = 0; - return i386_stopped_data_address (¤t_target, &addr); + return i386_stopped_data_address (ops, &addr); } /* Insert a hardware-assisted breakpoint at BP_TGT->placed_address. diff --git a/gdb/ia64-linux-nat.c b/gdb/ia64-linux-nat.c index 9d2f75c..b174041 100644 --- a/gdb/ia64-linux-nat.c +++ b/gdb/ia64-linux-nat.c @@ -669,10 +669,10 @@ ia64_linux_stopped_data_address (struct target_ops *ops, CORE_ADDR *addr_p) } static int -ia64_linux_stopped_by_watchpoint (void) +ia64_linux_stopped_by_watchpoint (struct target_ops *ops) { CORE_ADDR addr; - return ia64_linux_stopped_data_address (¤t_target, &addr); + return ia64_linux_stopped_data_address (ops, &addr); } static int diff --git a/gdb/inf-ttrace.c b/gdb/inf-ttrace.c index 3ba830f..37667bc 100644 --- a/gdb/inf-ttrace.c +++ b/gdb/inf-ttrace.c @@ -374,7 +374,7 @@ inf_ttrace_region_ok_for_hw_watchpoint (CORE_ADDR addr, int len) by hitting a "hardware" watchpoint. */ static int -inf_ttrace_stopped_by_watchpoint (void) +inf_ttrace_stopped_by_watchpoint (struct target_ops *ops) { pid_t pid = ptid_get_pid (inferior_ptid); lwpid_t lwpid = ptid_get_lwp (inferior_ptid); diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index 4784a5e..fb2dbb4 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -268,10 +268,6 @@ async_file_mark (void) be awakened anyway. */ } -static void linux_nat_async (void (*callback) - (enum inferior_event_type event_type, - void *context), - void *context); static int kill_lwp (int lwpid, int signo); static int stop_callback (struct lwp_info *lp, void *data); @@ -2487,7 +2483,7 @@ save_sigtrap (struct lwp_info *lp) old_chain = save_inferior_ptid (); inferior_ptid = lp->ptid; - lp->stopped_by_watchpoint = linux_ops->to_stopped_by_watchpoint (); + lp->stopped_by_watchpoint = linux_ops->to_stopped_by_watchpoint (linux_ops); if (lp->stopped_by_watchpoint) { @@ -2505,7 +2501,7 @@ save_sigtrap (struct lwp_info *lp) /* See save_sigtrap. */ static int -linux_nat_stopped_by_watchpoint (void) +linux_nat_stopped_by_watchpoint (struct target_ops *ops) { struct lwp_info *lp = find_lwp_pid (inferior_ptid); @@ -4501,7 +4497,7 @@ linux_trad_target (CORE_ADDR (*register_u_offset)(struct gdbarch *, int, int)) /* target_is_async_p implementation. */ static int -linux_nat_is_async_p (void) +linux_nat_is_async_p (struct target_ops *ops) { /* NOTE: palves 2008-03-21: We're only async when the user requests it explicitly with the "set target-async" command. @@ -4512,7 +4508,7 @@ linux_nat_is_async_p (void) /* target_can_async_p implementation. */ static int -linux_nat_can_async_p (void) +linux_nat_can_async_p (struct target_ops *ops) { /* NOTE: palves 2008-03-21: We're only async when the user requests it explicitly with the "set target-async" command. @@ -4672,8 +4668,10 @@ linux_async_pipe (int enable) /* target_async implementation. */ static void -linux_nat_async (void (*callback) (enum inferior_event_type event_type, - void *context), void *context) +linux_nat_async (struct target_ops *ops, + void (*callback) (enum inferior_event_type event_type, + void *context), + void *context) { if (callback != NULL) { @@ -4758,8 +4756,8 @@ static void linux_nat_close (void) { /* Unregister from the event loop. */ - if (linux_nat_is_async_p ()) - linux_nat_async (NULL, 0); + if (linux_nat_is_async_p (linux_ops)) + linux_nat_async (linux_ops, NULL, 0); if (linux_ops->to_close) linux_ops->to_close (); diff --git a/gdb/mem-break.c b/gdb/mem-break.c index 74fd8db..cb23e2a 100644 --- a/gdb/mem-break.c +++ b/gdb/mem-break.c @@ -77,14 +77,16 @@ default_memory_remove_breakpoint (struct gdbarch *gdbarch, int -memory_insert_breakpoint (struct gdbarch *gdbarch, +memory_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { return gdbarch_memory_insert_breakpoint (gdbarch, bp_tgt); } int -memory_remove_breakpoint (struct gdbarch *gdbarch, +memory_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { return gdbarch_memory_remove_breakpoint (gdbarch, bp_tgt); diff --git a/gdb/mips-linux-nat.c b/gdb/mips-linux-nat.c index 9246741..2285b4b 100644 --- a/gdb/mips-linux-nat.c +++ b/gdb/mips-linux-nat.c @@ -553,7 +553,7 @@ mips_linux_can_use_hw_breakpoint (int type, int cnt, int ot) register triggered. */ static int -mips_linux_stopped_by_watchpoint (void) +mips_linux_stopped_by_watchpoint (struct target_ops *ops) { int n; int num_valid; diff --git a/gdb/monitor.c b/gdb/monitor.c index 08153dd..bceafa8 100644 --- a/gdb/monitor.c +++ b/gdb/monitor.c @@ -2095,7 +2095,8 @@ monitor_mourn_inferior (struct target_ops *ops) /* Tell the monitor to add a breakpoint. */ static int -monitor_insert_breakpoint (struct gdbarch *gdbarch, +monitor_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { CORE_ADDR addr = bp_tgt->placed_address; @@ -2132,7 +2133,8 @@ monitor_insert_breakpoint (struct gdbarch *gdbarch, /* Tell the monitor to remove a breakpoint. */ static int -monitor_remove_breakpoint (struct gdbarch *gdbarch, +monitor_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { CORE_ADDR addr = bp_tgt->placed_address; diff --git a/gdb/nto-procfs.c b/gdb/nto-procfs.c index 1e6ec74..f7290b5 100644 --- a/gdb/nto-procfs.c +++ b/gdb/nto-procfs.c @@ -75,7 +75,7 @@ static int procfs_insert_hw_watchpoint (CORE_ADDR addr, int len, int type, static int procfs_remove_hw_watchpoint (CORE_ADDR addr, int len, int type, struct expression *cond); -static int procfs_stopped_by_watchpoint (void); +static int procfs_stopped_by_watchpoint (struct target_ops *ops); /* These two globals are only ever set in procfs_open(), but are referenced elsewhere. 'nto_procfs_node' is a flag used to say @@ -922,14 +922,16 @@ procfs_breakpoint (CORE_ADDR addr, int type, int size) } static int -procfs_insert_breakpoint (struct gdbarch *gdbarch, +procfs_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { return procfs_breakpoint (bp_tgt->placed_address, _DEBUG_BREAK_EXEC, 0); } static int -procfs_remove_breakpoint (struct gdbarch *gdbarch, +procfs_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { return procfs_breakpoint (bp_tgt->placed_address, _DEBUG_BREAK_EXEC, -1); @@ -1508,7 +1510,7 @@ procfs_insert_hw_watchpoint (CORE_ADDR addr, int len, int type, } static int -procfs_stopped_by_watchpoint (void) +procfs_stopped_by_watchpoint (struct target_ops *ops) { return 0; } diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c index 66e8062..bca0cfc 100644 --- a/gdb/ppc-linux-nat.c +++ b/gdb/ppc-linux-nat.c @@ -2283,10 +2283,10 @@ ppc_linux_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p) } static int -ppc_linux_stopped_by_watchpoint (void) +ppc_linux_stopped_by_watchpoint (struct target_ops *ops) { CORE_ADDR addr; - return ppc_linux_stopped_data_address (¤t_target, &addr); + return ppc_linux_stopped_data_address (ops, &addr); } static int diff --git a/gdb/procfs.c b/gdb/procfs.c index 5a425ed..365b1ea 100644 --- a/gdb/procfs.c +++ b/gdb/procfs.c @@ -4863,7 +4863,7 @@ procfs_can_use_hw_breakpoint (int type, int cnt, int othertype) fault, else returns zero. */ static int -procfs_stopped_by_watchpoint (void) +procfs_stopped_by_watchpoint (struct target_ops *ops) { procinfo *pi; diff --git a/gdb/record-full.c b/gdb/record-full.c index f9af408..6555f2d 100644 --- a/gdb/record-full.c +++ b/gdb/record-full.c @@ -237,16 +237,19 @@ static LONGEST ULONGEST offset, LONGEST len); static int - (*record_full_beneath_to_insert_breakpoint) (struct gdbarch *, + (*record_full_beneath_to_insert_breakpoint) (struct target_ops *, + struct gdbarch *, struct bp_target_info *); static int - (*record_full_beneath_to_remove_breakpoint) (struct gdbarch *, + (*record_full_beneath_to_remove_breakpoint) (struct target_ops *, + struct gdbarch *, struct bp_target_info *); -static int (*record_full_beneath_to_stopped_by_watchpoint) (void); +static int (*record_full_beneath_to_stopped_by_watchpoint) (struct target_ops *); static int (*record_full_beneath_to_stopped_data_address) (struct target_ops *, CORE_ADDR *); static void - (*record_full_beneath_to_async) (void (*) (enum inferior_event_type, void *), + (*record_full_beneath_to_async) (struct target_ops *, + void (*) (enum inferior_event_type, void *), void *); static void record_full_goto_insn (struct record_full_entry *entry, @@ -814,14 +817,16 @@ static LONGEST (*tmp_to_xfer_partial) (struct target_ops *ops, const gdb_byte *writebuf, ULONGEST offset, LONGEST len); -static int (*tmp_to_insert_breakpoint) (struct gdbarch *, +static int (*tmp_to_insert_breakpoint) (struct target_ops *, + struct gdbarch *, struct bp_target_info *); -static int (*tmp_to_remove_breakpoint) (struct gdbarch *, +static int (*tmp_to_remove_breakpoint) (struct target_ops *ops, + struct gdbarch *, struct bp_target_info *); -static int (*tmp_to_stopped_by_watchpoint) (void); +static int (*tmp_to_stopped_by_watchpoint) (struct target_ops *); static int (*tmp_to_stopped_data_address) (struct target_ops *, CORE_ADDR *); -static int (*tmp_to_stopped_data_address) (struct target_ops *, CORE_ADDR *); -static void (*tmp_to_async) (void (*) (enum inferior_event_type, void *), void *); +static void (*tmp_to_async) (struct target_ops *, + void (*) (enum inferior_event_type, void *), void *); static void record_full_restore (void); @@ -1510,12 +1515,12 @@ record_full_wait (struct target_ops *ops, } static int -record_full_stopped_by_watchpoint (void) +record_full_stopped_by_watchpoint (struct target_ops *ops) { if (RECORD_FULL_IS_REPLAY) return record_full_hw_watchpoint; else - return record_full_beneath_to_stopped_by_watchpoint (); + return record_full_beneath_to_stopped_by_watchpoint (find_target_beneath (ops)); } static int @@ -1758,7 +1763,8 @@ record_full_init_record_breakpoints (void) when recording. */ static int -record_full_insert_breakpoint (struct gdbarch *gdbarch, +record_full_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { struct record_full_breakpoint *bp; @@ -1775,7 +1781,8 @@ record_full_insert_breakpoint (struct gdbarch *gdbarch, int ret; old_cleanups = record_full_gdb_operation_disable_set (); - ret = record_full_beneath_to_insert_breakpoint (gdbarch, bp_tgt); + ret = record_full_beneath_to_insert_breakpoint (find_target_beneath (ops), + gdbarch, bp_tgt); do_cleanups (old_cleanups); if (ret != 0) @@ -1795,7 +1802,8 @@ record_full_insert_breakpoint (struct gdbarch *gdbarch, /* "to_remove_breakpoint" method for process record target. */ static int -record_full_remove_breakpoint (struct gdbarch *gdbarch, +record_full_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { struct record_full_breakpoint *bp; @@ -1815,7 +1823,8 @@ record_full_remove_breakpoint (struct gdbarch *gdbarch, int ret; old_cleanups = record_full_gdb_operation_disable_set (); - ret = record_full_beneath_to_remove_breakpoint (gdbarch, bp_tgt); + ret = record_full_beneath_to_remove_breakpoint (find_target_beneath (ops), + gdbarch, bp_tgt); do_cleanups (old_cleanups); if (ret != 0) @@ -1890,25 +1899,26 @@ record_full_goto_bookmark (gdb_byte *raw_bookmark, int from_tty) } static void -record_full_async (void (*callback) (enum inferior_event_type event_type, +record_full_async (struct target_ops *ops, + void (*callback) (enum inferior_event_type event_type, void *context), void *context) { /* If we're on top of a line target (e.g., linux-nat, remote), then set it to async mode as well. Will be NULL if we're sitting on top of the core target, for "record restore". */ if (record_full_beneath_to_async != NULL) - record_full_beneath_to_async (callback, context); + record_full_beneath_to_async (find_target_beneath (ops), callback, context); } static int -record_full_can_async_p (void) +record_full_can_async_p (struct target_ops *ops) { /* We only enable async when the user specifically asks for it. */ return target_async_permitted; } static int -record_full_is_async_p (void) +record_full_is_async_p (struct target_ops *ops) { /* We only enable async when the user specifically asks for it. */ return target_async_permitted; @@ -2264,7 +2274,8 @@ record_full_core_xfer_partial (struct target_ops *ops, /* "to_insert_breakpoint" method for prec over corefile. */ static int -record_full_core_insert_breakpoint (struct gdbarch *gdbarch, +record_full_core_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { return 0; @@ -2273,7 +2284,8 @@ record_full_core_insert_breakpoint (struct gdbarch *gdbarch, /* "to_remove_breakpoint" method for prec over corefile. */ static int -record_full_core_remove_breakpoint (struct gdbarch *gdbarch, +record_full_core_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { return 0; diff --git a/gdb/remote-m32r-sdi.c b/gdb/remote-m32r-sdi.c index 81fea53..21502d4 100644 --- a/gdb/remote-m32r-sdi.c +++ b/gdb/remote-m32r-sdi.c @@ -1145,7 +1145,8 @@ m32r_mourn_inferior (struct target_ops *ops) } static int -m32r_insert_breakpoint (struct gdbarch *gdbarch, +m32r_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { CORE_ADDR addr = bp_tgt->placed_address; @@ -1189,7 +1190,8 @@ m32r_insert_breakpoint (struct gdbarch *gdbarch, } static int -m32r_remove_breakpoint (struct gdbarch *gdbarch, +m32r_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { CORE_ADDR addr = bp_tgt->placed_address; @@ -1476,7 +1478,7 @@ m32r_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p) } static int -m32r_stopped_by_watchpoint (void) +m32r_stopped_by_watchpoint (struct target_ops *ops) { CORE_ADDR addr; diff --git a/gdb/remote-mips.c b/gdb/remote-mips.c index bf6cce5..3ede0ac 100644 --- a/gdb/remote-mips.c +++ b/gdb/remote-mips.c @@ -2364,27 +2364,29 @@ mips_mourn_inferior (struct target_ops *ops) target contents. */ static int -mips_insert_breakpoint (struct gdbarch *gdbarch, +mips_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { if (monitor_supports_breakpoints) return mips_set_breakpoint (bp_tgt->placed_address, MIPS_INSN32_SIZE, BREAK_FETCH); else - return memory_insert_breakpoint (gdbarch, bp_tgt); + return memory_insert_breakpoint (ops, gdbarch, bp_tgt); } /* Remove a breakpoint. */ static int -mips_remove_breakpoint (struct gdbarch *gdbarch, +mips_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { if (monitor_supports_breakpoints) return mips_clear_breakpoint (bp_tgt->placed_address, MIPS_INSN32_SIZE, BREAK_FETCH); else - return memory_remove_breakpoint (gdbarch, bp_tgt); + return memory_remove_breakpoint (ops, gdbarch, bp_tgt); } /* Tell whether this target can support a hardware breakpoint. CNT @@ -2451,7 +2453,7 @@ mips_remove_watchpoint (CORE_ADDR addr, int len, int type, if not. */ static int -mips_stopped_by_watchpoint (void) +mips_stopped_by_watchpoint (struct target_ops *ops) { return hit_watchpoint; } diff --git a/gdb/remote.c b/gdb/remote.c index 7d8a4de..b143014 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -126,12 +126,14 @@ static void remote_kill (struct target_ops *ops); static int tohex (int nib); -static int remote_can_async_p (void); +static int remote_can_async_p (struct target_ops *); -static int remote_is_async_p (void); +static int remote_is_async_p (struct target_ops *); -static void remote_async (void (*callback) (enum inferior_event_type event_type, - void *context), void *context); +static void remote_async (struct target_ops *ops, + void (*callback) (enum inferior_event_type event_type, + void *context), + void *context); static void remote_detach (struct target_ops *ops, char *args, int from_tty); @@ -8194,7 +8196,8 @@ remote_add_target_side_commands (struct gdbarch *gdbarch, which don't, we insert a traditional memory breakpoint. */ static int -remote_insert_breakpoint (struct gdbarch *gdbarch, +remote_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { /* Try the "Z" s/w breakpoint packet if it is not already disabled. @@ -8250,11 +8253,12 @@ remote_insert_breakpoint (struct gdbarch *gdbarch, } } - return memory_insert_breakpoint (gdbarch, bp_tgt); + return memory_insert_breakpoint (ops, gdbarch, bp_tgt); } static int -remote_remove_breakpoint (struct gdbarch *gdbarch, +remote_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { CORE_ADDR addr = bp_tgt->placed_address; @@ -8284,7 +8288,7 @@ remote_remove_breakpoint (struct gdbarch *gdbarch, return (rs->buf[0] == 'E'); } - return memory_remove_breakpoint (gdbarch, bp_tgt); + return memory_remove_breakpoint (ops, gdbarch, bp_tgt); } static int @@ -8438,7 +8442,7 @@ remote_check_watch_resources (int type, int cnt, int ot) } static int -remote_stopped_by_watchpoint (void) +remote_stopped_by_watchpoint (struct target_ops *ops) { struct remote_state *rs = get_remote_state (); @@ -8451,7 +8455,7 @@ remote_stopped_data_address (struct target_ops *target, CORE_ADDR *addr_p) struct remote_state *rs = get_remote_state (); int rc = 0; - if (remote_stopped_by_watchpoint ()) + if (remote_stopped_by_watchpoint (target)) { *addr_p = rs->remote_watch_data_address; rc = 1; @@ -11614,7 +11618,7 @@ Specify the serial device it is connected to (e.g. /dev/ttya)."; } static int -remote_can_async_p (void) +remote_can_async_p (struct target_ops *ops) { struct remote_state *rs = get_remote_state (); @@ -11627,7 +11631,7 @@ remote_can_async_p (void) } static int -remote_is_async_p (void) +remote_is_async_p (struct target_ops *ops) { struct remote_state *rs = get_remote_state (); @@ -11662,8 +11666,10 @@ remote_async_inferior_event_handler (gdb_client_data data) } static void -remote_async (void (*callback) (enum inferior_event_type event_type, - void *context), void *context) +remote_async (struct target_ops *ops, + void (*callback) (enum inferior_event_type event_type, + void *context), + void *context) { struct remote_state *rs = get_remote_state (); diff --git a/gdb/s390-nat.c b/gdb/s390-nat.c index 19bc607..b6a67ce 100644 --- a/gdb/s390-nat.c +++ b/gdb/s390-nat.c @@ -433,7 +433,7 @@ struct watch_area static struct watch_area *watch_base = NULL; static int -s390_stopped_by_watchpoint (void) +s390_stopped_by_watchpoint (struct target_ops *ops) { per_lowcore_bits per_lowcore; ptrace_area parea; diff --git a/gdb/target.c b/gdb/target.c index 22d7fb6..1b6fe5c 100644 --- a/gdb/target.c +++ b/gdb/target.c @@ -94,12 +94,6 @@ static void debug_to_prepare_to_store (struct regcache *); static void debug_to_files_info (struct target_ops *); -static int debug_to_insert_breakpoint (struct gdbarch *, - struct bp_target_info *); - -static int debug_to_remove_breakpoint (struct gdbarch *, - struct bp_target_info *); - static int debug_to_can_use_hw_breakpoint (int, int, int); static int debug_to_insert_hw_breakpoint (struct gdbarch *, @@ -114,8 +108,6 @@ static int debug_to_insert_watchpoint (CORE_ADDR, int, int, static int debug_to_remove_watchpoint (CORE_ADDR, int, int, struct expression *); -static int debug_to_stopped_by_watchpoint (void); - static int debug_to_stopped_data_address (struct target_ops *, CORE_ADDR *); static int debug_to_watchpoint_addr_within_range (struct target_ops *, @@ -790,7 +782,7 @@ update_current_target (void) (int (*) (CORE_ADDR, int, int, struct expression *)) return_minus_one); de_fault (to_stopped_by_watchpoint, - (int (*) (void)) + (int (*) (struct target_ops *)) return_zero); de_fault (to_stopped_data_address, (int (*) (struct target_ops *, CORE_ADDR *)) @@ -868,7 +860,9 @@ update_current_target (void) (char *(*) (int)) return_zero); de_fault (to_async, - (void (*) (void (*) (enum inferior_event_type, void*), void*)) + (void (*) (struct target_ops *, + void (*) (enum inferior_event_type, void*), + void*)) tcomplain); de_fault (to_thread_architecture, default_thread_architecture); @@ -2465,7 +2459,8 @@ target_insert_breakpoint (struct gdbarch *gdbarch, return 1; } - return (*current_target.to_insert_breakpoint) (gdbarch, bp_tgt); + return (*current_target.to_insert_breakpoint) (¤t_target, + gdbarch, bp_tgt); } int @@ -2482,7 +2477,8 @@ target_remove_breakpoint (struct gdbarch *gdbarch, return 1; } - return (*current_target.to_remove_breakpoint) (gdbarch, bp_tgt); + return (*current_target.to_remove_breakpoint) (¤t_target, + gdbarch, bp_tgt); } static void @@ -3134,7 +3130,7 @@ find_default_create_inferior (struct target_ops *ops, } static int -find_default_can_async_p (void) +find_default_can_async_p (struct target_ops *ignore) { struct target_ops *t; @@ -3144,12 +3140,12 @@ find_default_can_async_p (void) connected yet. */ t = find_default_run_target (NULL); if (t && t->to_can_async_p) - return (t->to_can_async_p) (); + return (t->to_can_async_p) (t); return 0; } static int -find_default_is_async_p (void) +find_default_is_async_p (struct target_ops *ignore) { struct target_ops *t; @@ -3159,7 +3155,7 @@ find_default_is_async_p (void) connected yet. */ t = find_default_run_target (NULL); if (t && t->to_is_async_p) - return (t->to_is_async_p) (); + return (t->to_is_async_p) (t); return 0; } @@ -3761,7 +3757,8 @@ init_dummy_target (void) dummy_target.to_has_registers = (int (*) (struct target_ops *)) return_zero; dummy_target.to_has_execution = (int (*) (struct target_ops *, ptid_t)) return_zero; - dummy_target.to_stopped_by_watchpoint = return_zero; + dummy_target.to_stopped_by_watchpoint + = (int (*) (struct target_ops *)) return_zero; dummy_target.to_stopped_data_address = (int (*) (struct target_ops *, CORE_ADDR *)) return_zero; dummy_target.to_magic = OPS_MAGIC; @@ -4513,12 +4510,13 @@ debug_to_files_info (struct target_ops *target) } static int -debug_to_insert_breakpoint (struct gdbarch *gdbarch, +debug_to_insert_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { int retval; - retval = debug_target.to_insert_breakpoint (gdbarch, bp_tgt); + retval = debug_target.to_insert_breakpoint (&debug_target, gdbarch, bp_tgt); fprintf_unfiltered (gdb_stdlog, "target_insert_breakpoint (%s, xxx) = %ld\n", @@ -4528,12 +4526,13 @@ debug_to_insert_breakpoint (struct gdbarch *gdbarch, } static int -debug_to_remove_breakpoint (struct gdbarch *gdbarch, +debug_to_remove_breakpoint (struct target_ops *ops, + struct gdbarch *gdbarch, struct bp_target_info *bp_tgt) { int retval; - retval = debug_target.to_remove_breakpoint (gdbarch, bp_tgt); + retval = debug_target.to_remove_breakpoint (&debug_target, gdbarch, bp_tgt); fprintf_unfiltered (gdb_stdlog, "target_remove_breakpoint (%s, xxx) = %ld\n", @@ -4590,11 +4589,11 @@ debug_to_can_accel_watchpoint_condition (CORE_ADDR addr, int len, int rw, } static int -debug_to_stopped_by_watchpoint (void) +debug_to_stopped_by_watchpoint (struct target_ops *ops) { int retval; - retval = debug_target.to_stopped_by_watchpoint (); + retval = debug_target.to_stopped_by_watchpoint (&debug_target); fprintf_unfiltered (gdb_stdlog, "target_stopped_by_watchpoint () = %ld\n", diff --git a/gdb/target.h b/gdb/target.h index 56ca40c..027e0f8 100644 --- a/gdb/target.h +++ b/gdb/target.h @@ -395,8 +395,10 @@ struct target_ops struct target_ops *target); void (*to_files_info) (struct target_ops *); - int (*to_insert_breakpoint) (struct gdbarch *, struct bp_target_info *); - int (*to_remove_breakpoint) (struct gdbarch *, struct bp_target_info *); + int (*to_insert_breakpoint) (struct target_ops *, struct gdbarch *, + struct bp_target_info *); + int (*to_remove_breakpoint) (struct target_ops *, struct gdbarch *, + struct bp_target_info *); int (*to_can_use_hw_breakpoint) (int, int, int); int (*to_ranged_break_num_registers) (struct target_ops *); int (*to_insert_hw_breakpoint) (struct gdbarch *, struct bp_target_info *); @@ -411,7 +413,7 @@ struct target_ops CORE_ADDR, CORE_ADDR, int); int (*to_remove_mask_watchpoint) (struct target_ops *, CORE_ADDR, CORE_ADDR, int); - int (*to_stopped_by_watchpoint) (void); + int (*to_stopped_by_watchpoint) (struct target_ops *); int to_have_steppable_watchpoint; int to_have_continuable_watchpoint; int (*to_stopped_data_address) (struct target_ops *, CORE_ADDR *); @@ -476,9 +478,10 @@ struct target_ops int to_has_thread_control; /* control thread execution */ int to_attach_no_wait; /* ASYNC target controls */ - int (*to_can_async_p) (void); - int (*to_is_async_p) (void); - void (*to_async) (void (*) (enum inferior_event_type, void *), void *); + int (*to_can_async_p) (struct target_ops *); + int (*to_is_async_p) (struct target_ops *); + void (*to_async) (struct target_ops *, + void (*) (enum inferior_event_type, void *), void *); int (*to_supports_non_stop) (void); /* find_memory_regions support method for gcore */ int (*to_find_memory_regions) (find_memory_region_ftype func, void *data); @@ -1405,16 +1408,16 @@ extern int default_child_has_execution (struct target_ops *ops, extern int target_async_permitted; /* Can the target support asynchronous execution? */ -#define target_can_async_p() (current_target.to_can_async_p ()) +#define target_can_async_p() (current_target.to_can_async_p (¤t_target)) /* Is the target in asynchronous execution mode? */ -#define target_is_async_p() (current_target.to_is_async_p ()) +#define target_is_async_p() (current_target.to_is_async_p (¤t_target)) int target_supports_non_stop (void); /* Put the target in async mode with the specified callback function. */ #define target_async(CALLBACK,CONTEXT) \ - (current_target.to_async ((CALLBACK), (CONTEXT))) + (current_target.to_async (¤t_target, (CALLBACK), (CONTEXT))) #define target_execution_direction() \ (current_target.to_execution_direction ()) @@ -1488,8 +1491,8 @@ extern char *target_thread_name (struct thread_info *); /* Returns non-zero if we were stopped by a hardware watchpoint (memory read or write). Only the INFERIOR_PTID task is being queried. */ -#define target_stopped_by_watchpoint \ - (*current_target.to_stopped_by_watchpoint) +#define target_stopped_by_watchpoint() \ + ((*current_target.to_stopped_by_watchpoint) (¤t_target)) /* Non-zero if we have steppable watchpoints */ @@ -1864,10 +1867,12 @@ extern struct target_section_table *target_get_section_table /* From mem-break.c */ -extern int memory_remove_breakpoint (struct gdbarch *, +extern int memory_remove_breakpoint (struct target_ops *, + struct gdbarch *, struct bp_target_info *); -extern int memory_insert_breakpoint (struct gdbarch *, +extern int memory_insert_breakpoint (struct target_ops *, + struct gdbarch *, struct bp_target_info *); extern int default_memory_remove_breakpoint (struct gdbarch *, -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-10-22 17:59 ` [PATCH v4 2/9] add "this" pointers to more target APIs Tom Tromey @ 2013-10-28 16:04 ` Pedro Alves 2013-10-28 16:37 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-10-28 16:04 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/22/2013 06:59 PM, Tom Tromey wrote: > @@ -4758,8 +4756,8 @@ static void > linux_nat_close (void) > { > /* Unregister from the event loop. */ > - if (linux_nat_is_async_p ()) > - linux_nat_async (NULL, 0); > + if (linux_nat_is_async_p (linux_ops)) > + linux_nat_async (linux_ops, NULL, 0); It doesn't matter much since linux_nat_async doesn't use it's argument, but FYI, this one here doesn't look 100% correct. "linux_ops" is the single-threaded target linux-nat itself delegates some work to (it's never pushed anywhere, etc.). Ideally, to_close would be passed in a "this" pointer too. > static int > -record_full_stopped_by_watchpoint (void) > +record_full_stopped_by_watchpoint (struct target_ops *ops) > { > if (RECORD_FULL_IS_REPLAY) > return record_full_hw_watchpoint; > else > - return record_full_beneath_to_stopped_by_watchpoint (); > + return record_full_beneath_to_stopped_by_watchpoint (find_target_beneath (ops)); > } > Line too long. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-10-28 16:04 ` Pedro Alves @ 2013-10-28 16:37 ` Tom Tromey 2013-10-28 16:44 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-28 16:37 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> On 10/22/2013 06:59 PM, Tom Tromey wrote: >> @@ -4758,8 +4756,8 @@ static void >> linux_nat_close (void) >> { >> /* Unregister from the event loop. */ >> - if (linux_nat_is_async_p ()) >> - linux_nat_async (NULL, 0); >> + if (linux_nat_is_async_p (linux_ops)) >> + linux_nat_async (linux_ops, NULL, 0); Pedro> It doesn't matter much since linux_nat_async doesn't use it's Pedro> argument, but FYI, this one here doesn't look 100% correct. Pedro> "linux_ops" is the single-threaded target linux-nat itself Pedro> delegates some work to (it's never pushed anywhere, etc.). Pedro> Ideally, to_close would be passed in a "this" pointer too. The multi-target branch has a more invasive transformation of the target API. It doesn't add a this pointer to to_close, but it does implement a target_ops / target object split, and it changes more targets to use to_xclose, which does take a "this". In this particular spot, I don't remember why I used linux_ops. It is all moot, I think. There is no reason for linux-nat to ever call linux_nat_is_async_p any more. I think we can drop all the dead code instead. I noted this in the first submission and said I will do it in a followup; but I think I'll just tack it on to this series instead. >> static int >> -record_full_stopped_by_watchpoint (void) >> +record_full_stopped_by_watchpoint (struct target_ops *ops) >> { >> if (RECORD_FULL_IS_REPLAY) >> return record_full_hw_watchpoint; >> else >> - return record_full_beneath_to_stopped_by_watchpoint (); >> + return record_full_beneath_to_stopped_by_watchpoint (find_target_beneath (ops)); >> } >> Pedro> Line too long. Fixed. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-10-28 16:37 ` Tom Tromey @ 2013-10-28 16:44 ` Pedro Alves 2013-10-28 16:52 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-10-28 16:44 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/28/2013 04:37 PM, Tom Tromey wrote: > > It is all moot, I think. There is no reason for linux-nat to ever call > linux_nat_is_async_p any more. I think we can drop all the dead code > instead. I noted this in the first submission and said I will do it in > a followup; but I think I'll just tack it on to this series instead. I'd rather keep the code to allow forcing sync mode for a while, to make it easier to debug problems and compare modes. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-10-28 16:44 ` Pedro Alves @ 2013-10-28 16:52 ` Tom Tromey 2013-11-08 18:04 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-28 16:52 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> On 10/28/2013 04:37 PM, Tom Tromey wrote: >> >> It is all moot, I think. There is no reason for linux-nat to ever call >> linux_nat_is_async_p any more. I think we can drop all the dead code >> instead. I noted this in the first submission and said I will do it in >> a followup; but I think I'll just tack it on to this series instead. Pedro> I'd rather keep the code to allow forcing sync mode for a while, Pedro> to make it easier to debug problems and compare modes. With this series, there's no way to force sync mode. I think maybe it could be done by adding a new "maint" setting. We can't reuse "set target-async" due to the MI misuse, unless we're willing to change the default setting of this parameter based on the current interpreter. In fact an earlier version of my patch series did just this, but IIRC I thought it was too hackish. While we're here, I wonder now whether the distinction between "can async" and "is async" makes sense any more. I'm inclined to remove one of them. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-10-28 16:52 ` Tom Tromey @ 2013-11-08 18:04 ` Pedro Alves 2013-11-08 21:53 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-11-08 18:04 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/28/2013 04:52 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > > Pedro> On 10/28/2013 04:37 PM, Tom Tromey wrote: >>> >>> It is all moot, I think. There is no reason for linux-nat to ever call >>> linux_nat_is_async_p any more. I think we can drop all the dead code >>> instead. I noted this in the first submission and said I will do it in >>> a followup; but I think I'll just tack it on to this series instead. > > Pedro> I'd rather keep the code to allow forcing sync mode for a while, > Pedro> to make it easier to debug problems and compare modes. > > With this series, there's no way to force sync mode. That'll really make our lives complicated. We'll definitely hit async specific problems, and not being able to easily compare how sync behaves will be a nuisance. Also, given most targets don't support async, I think it'll be very valuable to easily check how sync mode works on native GNU/Linux as proxy for those targets -- consider patches changing run control and execution commands code. Heck, I've gone through the trouble of implementing software single-step on x86 just to be able to use that as proxy for sss targets. :-) > I think maybe it could be done by adding a new "maint" setting. Yeah, "set target-async" used to be a set of maint commands. https://sourceware.org/ml/gdb-patches/2008-08/msg00423.html We've come full circle. :-) > > We can't reuse "set target-async" due to the MI misuse, unless we're > willing to change the default setting of this parameter based on the > current interpreter. In fact an earlier version of my patch series did > just this, but IIRC I thought it was too hackish. Yeah, making the setting be MI specific is better. > While we're here, I wonder now whether the distinction between "can > async" and "is async" makes sense any more. I'm inclined to remove one > of them. Yeah, probably doesn't. We used to have this target_async_mask mechanism, that got replaced by TARGET_WNOHANG: https://sourceware.org/ml/gdb-patches/2009-05/msg00459.html and forced waits: https://sourceware.org/ml/gdb-patches/2011-06/msg00086.html Before that, the target could support async, but have it masked (so "can" would be true, "is" would be "false"). -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-11-08 18:04 ` Pedro Alves @ 2013-11-08 21:53 ` Tom Tromey 2013-11-09 3:35 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-11-08 21:53 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Tom> With this series, there's no way to force sync mode. Pedro> That'll really make our lives complicated. We'll definitely Pedro> hit async specific problems, and not being able to easily Pedro> compare how sync behaves will be a nuisance. Also, given most Pedro> targets don't support async, I think it'll be very valuable Pedro> to easily check how sync mode works on native GNU/Linux as proxy Pedro> for those targets -- consider patches changing run control and Pedro> execution commands code. Heck, I've gone through the trouble Pedro> of implementing software single-step on x86 just to be able Pedro> to use that as proxy for sss targets. :-) Ok. I will add it back under "maint". I think the in the long run it would be better if all targets were async. I think this is preferable because async is an enabling feature for other features, and because removing sync mode would simplify one of the more complicated parts of gdb. This is different from software single-step because, IIUC, SSS is an intrinsic feature of some ports; whereas sync targets are purely internal issues to gdb. >> While we're here, I wonder now whether the distinction between "can >> async" and "is async" makes sense any more. Pedro> Yeah, probably doesn't. I'll remove "is_async". Unless you'd rather I remove "can_async". Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-11-08 21:53 ` Tom Tromey @ 2013-11-09 3:35 ` Pedro Alves 2013-12-06 17:40 ` Tom Tromey 2013-12-06 18:23 ` Tom Tromey 0 siblings, 2 replies; 53+ messages in thread From: Pedro Alves @ 2013-11-09 3:35 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 11/08/2013 08:10 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > > Tom> With this series, there's no way to force sync mode. > > Pedro> That'll really make our lives complicated. We'll definitely > Pedro> hit async specific problems, and not being able to easily > Pedro> compare how sync behaves will be a nuisance. Also, given most > Pedro> targets don't support async, I think it'll be very valuable > Pedro> to easily check how sync mode works on native GNU/Linux as proxy > Pedro> for those targets -- consider patches changing run control and > Pedro> execution commands code. Heck, I've gone through the trouble > Pedro> of implementing software single-step on x86 just to be able > Pedro> to use that as proxy for sss targets. :-) > > Ok. I will add it back under "maint". Thanks. > > I think the in the long run it would be better if all targets were > async. Yes, of course. It requires per-target work, however... I'm not seeing that happen anytime soon. (and djgpp might be a challenge.) > I think this is preferable because async is an enabling feature > for other features, and because removing sync mode would simplify one of > the more complicated parts of gdb. Certainly. >>> While we're here, I wonder now whether the distinction between "can >>> async" and "is async" makes sense any more. > > Pedro> Yeah, probably doesn't. > > I'll remove "is_async". Unless you'd rather I remove "can_async". No, that's fine. "is_async" would be my choice as well. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-11-09 3:35 ` Pedro Alves @ 2013-12-06 17:40 ` Tom Tromey 2013-12-06 18:35 ` Pedro Alves 2013-12-06 18:23 ` Tom Tromey 1 sibling, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-06 17:40 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Tom> I think the in the long run it would be better if all targets were Tom> async. Pedro> Yes, of course. It requires per-target work, however... I'm not Pedro> seeing that happen anytime soon. (and djgpp might be a challenge.) I wonder if we could simplify gdb by only providing async at the target API level, and then letting some targets still work synchronously under the hood, just using the async callback to report the event that was found. It wouldn't let "&" work but it might simplify the internals. I can't tell if this makes sense. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-12-06 17:40 ` Tom Tromey @ 2013-12-06 18:35 ` Pedro Alves 0 siblings, 0 replies; 53+ messages in thread From: Pedro Alves @ 2013-12-06 18:35 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 12/06/2013 05:40 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > > Tom> I think the in the long run it would be better if all targets were > Tom> async. > > Pedro> Yes, of course. It requires per-target work, however... I'm not > Pedro> seeing that happen anytime soon. (and djgpp might be a challenge.) > > I wonder if we could simplify gdb by only providing async at the target > API level, and then letting some targets still work synchronously under > the hood, just using the async callback to report the event that was > found. It wouldn't let "&" work but it might simplify the internals. I > can't tell if this makes sense. It's really hard for me to say. It's the sort of thing that requires experimentation, I think. On the target side, supporting target_wait(0, ...) in addition to target_wait(..., TARGET_WHANG) doesn't look that complicated to me. I think GDB will always need to do blocking target waits on occasion, even on async targets. I never imagined it tackled at that level. Hmmm. I think there are several alternative levels/layers where such a sync vs async normalization could occur. Could be like you say, or it could be one level up, that is, have infrun.c call blocking target_wait, and infrun itself push the resulting event in a local pending event queue that the event loop would react to. Or it could be at the command levels, making the sync paths also install the continuations and run then when the target stops, though now that I spell it out, the latter one doesn't sound like it'd remove as much differences as the former ones, I think. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-11-09 3:35 ` Pedro Alves 2013-12-06 17:40 ` Tom Tromey @ 2013-12-06 18:23 ` Tom Tromey 2013-12-06 19:06 ` Pedro Alves 1 sibling, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-06 18:23 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Tom> I'll remove "is_async". Unless you'd rather I remove "can_async". Pedro> No, that's fine. "is_async" would be my choice as well. I looked deeper than the two are still subtly different. Specifically, remote.c delegates to serial: return serial_can_async_p (rs->remote_desc); return serial_is_async_p (rs->remote_desc); And these really do differ: int serial_can_async_p (struct serial *scb) { return (scb->ops->async != NULL); } int serial_is_async_p (struct serial *scb) { return (scb->ops->async != NULL) && (scb->async_handler != NULL); } I find it a bit odd that the upper layers rely on the serial layer to do this bookkeeping. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 2/9] add "this" pointers to more target APIs 2013-12-06 18:23 ` Tom Tromey @ 2013-12-06 19:06 ` Pedro Alves 0 siblings, 0 replies; 53+ messages in thread From: Pedro Alves @ 2013-12-06 19:06 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 12/06/2013 06:23 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > > Tom> I'll remove "is_async". Unless you'd rather I remove "can_async". > > Pedro> No, that's fine. "is_async" would be my choice as well. > > I looked deeper than the two are still subtly different. > Specifically, remote.c delegates to serial: > > return serial_can_async_p (rs->remote_desc); > return serial_is_async_p (rs->remote_desc); > > And these really do differ: > > int > serial_can_async_p (struct serial *scb) > { > return (scb->ops->async != NULL); > } > > int > serial_is_async_p (struct serial *scb) > { > return (scb->ops->async != NULL) && (scb->async_handler != NULL); > } > > > I find it a bit odd that the upper layers rely on the serial layer to do > this bookkeeping. Hmm, indeed. So target_is_async only returns true while target_async (inferior_event_handler) is active. Guess we need to go through target_is_async_p calls and see how to eliminate them. The remote.c ones seem simply replaceable with checking whether we're doing a blocking wait, but the others look more tricky. (below's untested) --- gdb/remote.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gdb/remote.c b/gdb/remote.c index 2ac8c36..a164455 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -6001,7 +6001,7 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options) int ret; int is_notif; - if (!target_is_async_p ()) + if ((options & TARGET_WNOHANG) == 0) { ofunc = signal (SIGINT, sync_remote_interrupt); /* If the user hit C-c before this packet, or between packets, @@ -6020,7 +6020,7 @@ remote_wait_as (ptid_t ptid, struct target_waitstatus *status, int options) ret = getpkt_or_notif_sane (&rs->buf, &rs->buf_size, wait_forever_enabled_p, &is_notif); - if (!target_is_async_p ()) + if ((options & TARGET_WNOHANG) == 0) signal (SIGINT, ofunc); /* GDB gets a notification. Return to core as this event is -- 1.7.11.7 ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 8/9] fix py-finish-breakpoint.exp with always-async 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey ` (4 preceding siblings ...) 2013-10-22 17:59 ` [PATCH v4 2/9] add "this" pointers to more target APIs Tom Tromey @ 2013-10-22 18:11 ` Tom Tromey 2013-11-11 19:51 ` Pedro Alves 2013-10-22 18:26 ` [PATCH v4 6/9] PR gdb/13860: don't lose '-interpreter-exec console EXECUTION_COMMAND''s output in async mode Tom Tromey ` (2 subsequent siblings) 8 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-22 18:11 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey With target async enabled, py-finish-breakpoint.exp will trigger an assertion failure. The failure occurs because execute_command re-enters the event loop in some circumstances, and in this case resets the sync_execution flag. Then later gdb reaches this assertion in normal_stop: gdb_assert (sync_execution || !target_can_async_p ()); execute_command has a comment explaining why it dispatches events: /* If the interpreter is in sync mode (we're running a user command's list, running command hooks or similars), and we just ran a synchronous command that started the target, wait for that command to end. */ However, the code did not follow this comment -- it didn't check to see if the command started the target, just whether the target was executing a sync command at this point. This patch fixes the problem by noting whether the target was executing in sync_execution mode before running the command, and then augmenting the condition to test this as well. Built and regtested on x86-64 Fedora 18. PR gdb/14135: * top.c (execute_command): Only dispatch events if command started target. --- gdb/top.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gdb/top.c b/gdb/top.c index c473d8c..b563eca 100644 --- a/gdb/top.c +++ b/gdb/top.c @@ -415,6 +415,8 @@ execute_command (char *p, int from_tty) { const char *cmd = p; char *arg; + int was_sync = sync_execution; + line = p; /* If trace-commands is set then this will print this command. */ @@ -470,7 +472,7 @@ execute_command (char *p, int from_tty) command's list, running command hooks or similars), and we just ran a synchronous command that started the target, wait for that command to end. */ - if (!interpreter_async && sync_execution) + if (!interpreter_async && !was_sync && sync_execution) { while (gdb_do_one_event () >= 0) if (!sync_execution) -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 8/9] fix py-finish-breakpoint.exp with always-async 2013-10-22 18:11 ` [PATCH v4 8/9] fix py-finish-breakpoint.exp with always-async Tom Tromey @ 2013-11-11 19:51 ` Pedro Alves 2013-12-09 17:53 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-11-11 19:51 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/22/2013 06:59 PM, Tom Tromey wrote: > With target async enabled, py-finish-breakpoint.exp will trigger an > assertion failure. > > The failure occurs because execute_command re-enters the event loop in > some circumstances, and in this case resets the sync_execution flag. > Then later gdb reaches this assertion in normal_stop: > > gdb_assert (sync_execution || !target_can_async_p ()); > > execute_command has a comment explaining why it dispatches events: > > /* If the interpreter is in sync mode (we're running a user > command's list, running command hooks or similars), and we > just ran a synchronous command that started the target, wait > for that command to end. */ > > However, the code did not follow this comment -- it didn't check to > see if the command started the target, just whether the target was > executing a sync command at this point. Can you explain this a little better, please? IIUC (I haven't really stepped through the code): - A synchronous execution command is run. sync_execution is set. - A python breakpoint is hit, and the corresponding stop method is executed. While python commands are executed, interpreter_async is forced to 0. - The Python stop method happens to execute a not-execution-related gdb command ("where 1"). - Seeing that sync_execution is set, GDB nests a new event loop, although that wasn't necessary. - Some event that causes a stop triggers in the inferior, and normal_stop is called. - the nested event loop unwinds/ends, and normal_stop is called again. (IOW, normal_stop was called twice for the same event.) The assertion triggers. Is that accurate? What happens if the Python stop method actually does run an execution command? -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 8/9] fix py-finish-breakpoint.exp with always-async 2013-11-11 19:51 ` Pedro Alves @ 2013-12-09 17:53 ` Tom Tromey 0 siblings, 0 replies; 53+ messages in thread From: Tom Tromey @ 2013-12-09 17:53 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Tom> However, the code did not follow this comment -- it didn't check to Tom> see if the command started the target, just whether the target was Tom> executing a sync command at this point. Pedro> Can you explain this a little better, please? We'll see :) Pedro> IIUC (I haven't really stepped through the code): Pedro> - A synchronous execution command is run. sync_execution is set. Pedro> - A python breakpoint is hit, and the corresponding stop Pedro> method is executed. While python commands are executed, Pedro> interpreter_async is forced to 0. Pedro> - The Python stop method happens to execute a not-execution-related Pedro> gdb command ("where 1"). Pedro> - Seeing that sync_execution is set, GDB nests a new event loop, Pedro> although that wasn't necessary. Pedro> - Some event that causes a stop triggers in the inferior, and Pedro> normal_stop is called. Ok to here. And this step is where I think the bug lies -- the comment in execute_command explains the logic here, but the code doesn't faithfully implement it. Pedro> - the nested event loop unwinds/ends, and normal_stop is called Pedro> again. (IOW, normal_stop was called Pedro> twice for the same event.) The assertion triggers. I think the event is not handled twice. Instead the second time the event is TARGET_WAITKIND_NO_RESUMED: infrun: target_wait (-1, status) = infrun: -1 [process -1], infrun: status->kind = no-resumed infrun: TARGET_WAITKIND_NO_RESUMED infrun: stop_stepping No unwaited-for children left. infrun: BPSTAT_WHAT_STOP_NOISY infrun: stop_stepping Pedro> Is that accurate? Pedro> What happens if the Python stop method actually does run an Pedro> execution command? I can find out -- but note that we explicitly disavow this in the manual, so in a sense it doesn't matter. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 6/9] PR gdb/13860: don't lose '-interpreter-exec console EXECUTION_COMMAND''s output in async mode. 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey ` (5 preceding siblings ...) 2013-10-22 18:11 ` [PATCH v4 8/9] fix py-finish-breakpoint.exp with always-async Tom Tromey @ 2013-10-22 18:26 ` Tom Tromey 2013-10-22 18:26 ` [PATCH v4 9/9] enable target-async Tom Tromey 2013-10-22 19:00 ` [PATCH v4 7/9] make dprintf.exp pass in always-async mode Tom Tromey 8 siblings, 0 replies; 53+ messages in thread From: Tom Tromey @ 2013-10-22 18:26 UTC (permalink / raw) To: gdb-patches; +Cc: Pedro Alves From: Pedro Alves <palves@redhat.com> The other part of PR gdb/13860 is about console execution commands in MI getting their output half lost. E.g., take the finish command, executed on a frontend's GDB console: sync: finish &"finish\n" ~"Run till exit from #0 usleep (useconds=10) at ../sysdeps/unix/sysv/linux/usleep.c:27\n" ^running *running,thread-id="1" (gdb) ~"0x00000000004004d7 in foo () at stepinf.c:6\n" ~"6\t usleep (10);\n" ~"Value returned is $1 = 0\n" *stopped,reason="function-finished",frame={addr="0x00000000004004d7",func="foo",args=[],file="stepinf.c",fullname="/home/pedro/gdb/tests/stepinf.c",line="6"},thread-id="1",stopped-threads="all",core="1" async: finish &"finish\n" ~"Run till exit from #0 usleep (useconds=10) at ../sysdeps/unix/sysv/linux/usleep.c:27\n" ^running *running,thread-id="1" (gdb) *stopped,reason="function-finished",frame={addr="0x00000000004004d7",func="foo",args=[],file="stepinf.c",fullname="/home/pedro/gdb/tests/stepinf.c",line="6"},gdb-result-var="$1",return-value="0",thread-id="1",stopped-threads="all",core="0" Note how all the "Value returned" etc. output is missing in async mode. The same happens with e.g., catchpoints: =breakpoint-modified,bkpt={number="1",type="catchpoint",disp="keep",enabled="y",what="22016",times="1"} ~"\nCatchpoint " ~"1 (forked process 22016), 0x0000003791cbd8a6 in __libc_fork () at ../nptl/sysdeps/unix/sysv/linux/fork.c:131\n" ~"131\t pid = ARCH_FORK ();\n" *stopped,reason="fork",disp="keep",bkptno="1",newpid="22016",frame={addr="0x0000003791cbd8a6",func="__libc_fork",args=[],file="../nptl/sysdeps/unix/sysv/linux/fork.c",fullname="/usr/src/debug/glibc-2.14-394-g8f3b1ff/nptl/sysdeps/unix/sysv/linux/fork.c",line="131"},thread-id="1",stopped-threads="all",core="0" where all those ~ lines are missing in async mode, or just the "step" current line indication: s &"s\n" ^running *running,thread-id="all" (gdb) ~"13\t foo ();\n" *stopped,frame={addr="0x00000000004004ef",func="main",args=[{name="argc",value="1"},{name="argv",value="0x7fffffffdd78"}],file="stepinf.c",fullname="/home/pedro/gdb/tests/stepinf.c",line="13"},thread-id="1",stopped-threads="all",core="3" (gdb) Or in the case of the PRs example, the "Stopped due to shared library event" note: start &"start\n" ~"Temporary breakpoint 1 at 0x400608: file ../../../src/gdb/testsuite/gdb.mi/solib-main.c, line 21.\n" =breakpoint-created,bkpt={number="1",type="breakpoint",disp="del",enabled="y",addr="0x0000000000400608",func="main",file="../../../src/gdb/testsuite/gdb.mi/solib-main.c",fullname="/home/pedro/gdb/mygit/src/gdb/testsuite/gdb.mi/solib-main.c",line="21",times="0",original-location="main"} ~"Starting program: /home/pedro/gdb/mygit/build/gdb/testsuite/gdb.mi/solib-main \n" =thread-group-started,id="i1",pid="21990" =thread-created,id="1",group-id="i1" ^running *running,thread-id="all" (gdb) =library-loaded,id="/lib64/ld-linux-x86-64.so.2",target-name="/lib64/ld-linux-x86-64.so.2",host-name="/lib64/ld-linux-x86-64.so.2",symbols-loaded="0",thread-group="i1" ~"Stopped due to shared library event (no libraries added or removed)\n" *stopped,reason="solib-event",thread-id="1",stopped-threads="all",core="3" (gdb) IMO, if you're typing execution commands in a frontend's console, you expect to see their output. Indeed it's what you get in sync mode. I think async mode should do the same. That's what this patch does. Notes: - mi->out is the same as gdb_stdout when MI is the current interpreter. I think that referring to that directly is cleaner. An earlier revision of this patch made the changes that are now done in mi_on_normal_stop directly in infrun.c:normal_stop, and so not having an obvious place to put the new uiout by then, and not wanting to abuse CLI's uiout, I made a temporary uiout when necessary. - Hopefuly the rest of the patch is more or less obvious given the comments I added. Tested on x86_64 Fedora 16, no regressions. 2012-05-09 Pedro Alves <palves@redhat.com> PR gdb/13860 * gdbthread.h (struct thread_control_state): New field `command_interp'. * infrun.c (follow_fork): Copy the new thread control field to the child fork thread. (clear_proceed_status_thread): Clear the new thread control field. (proceed): Set the new thread control field. * interps.h (command_interp): Declare. * interps.c (command_interpreter): New global. (command_interp): New function. (interp_exec): Set `command_interpreter' while here. * cli-out.c (cli_uiout_dtor): New function. (cli_ui_out_impl): Install it. * mi/mi-interp.c: Include cli-out.h. (mi_cmd_interpreter_exec): Add comment. (restore_current_uiout_cleanup): New function. (ui_out_free_cleanup): New function. (mi_on_normal_stop): In async mode, if finishing an execution command started by a CLI command, or any kind of breakpoint-like event triggered, print the stop event to the output (CLI) stream. * mi/mi-out.c (mi_ui_out_impl): Install NULL `dtor' handler. gdb/testsuite/ * gdb.mi/mi-cli.exp: Also expect the new source line to be output after a "next", in async mode. Make it a pass/fail test. * gdb.mi/mi-solib.exp: Test that the CLI solib event note is output. --- gdb/cli-out.c | 13 +++++++- gdb/gdbthread.h | 5 +++ gdb/infrun.c | 14 +++++++++ gdb/interps.c | 36 ++++++++++++++++++++- gdb/interps.h | 2 ++ gdb/mi/mi-interp.c | 66 +++++++++++++++++++++++++++++++++++++++ gdb/testsuite/gdb.mi/mi-cli.exp | 13 +++++--- gdb/testsuite/gdb.mi/mi-solib.exp | 11 +++++++ 8 files changed, 154 insertions(+), 6 deletions(-) diff --git a/gdb/cli-out.c b/gdb/cli-out.c index 380352b..cedd3af 100644 --- a/gdb/cli-out.c +++ b/gdb/cli-out.c @@ -40,6 +40,17 @@ static void out_field_fmt (struct ui_out *uiout, int fldno, const char *fldname, const char *format,...) ATTRIBUTE_PRINTF (4, 5); +/* The destructor. */ + +static void +cli_uiout_dtor (struct ui_out *ui_out) +{ + cli_out_data *data = ui_out_data (ui_out); + + VEC_free (ui_filep, data->streams); + xfree (data); +} + /* These are the CLI output functions */ /* Mark beginning of a table */ @@ -370,7 +381,7 @@ struct ui_out_impl cli_ui_out_impl = cli_wrap_hint, cli_flush, cli_redirect, - 0, + cli_uiout_dtor, 0, /* Does not need MI hacks (i.e. needs CLI hacks). */ }; diff --git a/gdb/gdbthread.h b/gdb/gdbthread.h index 81dc7ed..6879cbb 100644 --- a/gdb/gdbthread.h +++ b/gdb/gdbthread.h @@ -122,6 +122,11 @@ struct thread_control_state /* Chain containing status of breakpoint(s) the thread stopped at. */ bpstat stop_bpstat; + + /* The interpreter that issued the execution command. NULL if the + thread was resumed as a result of a command applied to some other + thread (e.g., "next" with scheduler-locking off). */ + struct interp *command_interp; }; /* Inferior thread specific part of `struct infcall_suspend_state'. diff --git a/gdb/infrun.c b/gdb/infrun.c index 49e8ab5..a58b6fe 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -432,6 +432,7 @@ follow_fork (void) CORE_ADDR step_range_start = 0; CORE_ADDR step_range_end = 0; struct frame_id step_frame_id = { 0 }; + struct interp *command_interp = NULL; if (!non_stop) { @@ -483,6 +484,7 @@ follow_fork (void) step_frame_id = tp->control.step_frame_id; exception_resume_breakpoint = clone_momentary_breakpoint (tp->control.exception_resume_breakpoint); + command_interp = tp->control.command_interp; /* For now, delete the parent's sr breakpoint, otherwise, parent/child sr breakpoints are considered duplicates, @@ -494,6 +496,7 @@ follow_fork (void) tp->control.step_range_end = 0; tp->control.step_frame_id = null_frame_id; delete_exception_resume_breakpoint (tp); + tp->control.command_interp = NULL; } parent = inferior_ptid; @@ -538,6 +541,7 @@ follow_fork (void) tp->control.step_frame_id = step_frame_id; tp->control.exception_resume_breakpoint = exception_resume_breakpoint; + tp->control.command_interp = command_interp; } else { @@ -1997,6 +2001,8 @@ clear_proceed_status_thread (struct thread_info *tp) tp->control.proceed_to_finish = 0; + tp->control.command_interp = NULL; + /* Discard any remaining commands or status from previous stop. */ bpstat_clear (&tp->control.stop_bpstat); } @@ -2198,6 +2204,14 @@ proceed (CORE_ADDR addr, enum gdb_signal siggnal, int step) regcache_write_pc (regcache, addr); } + /* Record the interpreter that issued the execution command that + caused this thread to resume. If the top level interpreter is + MI/async, and the execution command was a CLI command + (next/step/etc.), we'll want to print stop event output to the MI + console channel (the stepped-to line, etc.), as if the user + entered the execution command on a real GDB console. */ + inferior_thread ()->control.command_interp = command_interp (); + if (debug_infrun) fprintf_unfiltered (gdb_stdlog, "infrun: proceed (addr=%s, signal=%d, step=%d)\n", diff --git a/gdb/interps.c b/gdb/interps.c index 7f19385..ed1b32e 100644 --- a/gdb/interps.c +++ b/gdb/interps.c @@ -318,6 +318,29 @@ current_interp_display_prompt_p (void) data); } +/* The interpreter that is active while `interp_exec' is active, NULL + at all other times. */ +static struct interp *command_interpreter; + +/* The interpreter that was active when a command was executed. + Normally that'd always be CURRENT_INTERPRETER, except that MI's + -interpreter-exec command doesn't actually flip the current + interpreter when running its sub-command. The + `command_interpreter' global tracks when interp_exec is called + (IOW, when -interpreter-exec is called). If that is set, it is + INTERP in '-interpreter-exec INTERP "CMD"' or in 'interpreter-exec + INTERP "CMD". Otherwise, interp_exec isn't active, and so the + interpreter running the command is the current interpreter. */ + +struct interp * +command_interp (void) +{ + if (command_interpreter != NULL) + return command_interpreter; + else + return current_interpreter; +} + /* Run the current command interpreter's main loop. */ void current_interp_command_loop (void) @@ -358,7 +381,18 @@ interp_exec (struct interp *interp, const char *command_str) { if (interp->procs->exec_proc != NULL) { - return interp->procs->exec_proc (interp->data, command_str); + struct gdb_exception ex; + struct interp *save_command_interp; + + /* See `command_interp' for why we do this. */ + save_command_interp = command_interpreter; + command_interpreter = interp; + + ex = interp->procs->exec_proc (interp->data, command_str); + + command_interpreter = save_command_interp; + + return ex; } return exception_none; } diff --git a/gdb/interps.h b/gdb/interps.h index 58ac6b2..6a45a7b 100644 --- a/gdb/interps.h +++ b/gdb/interps.h @@ -97,6 +97,8 @@ extern int current_interp_set_logging (int start_log, struct ui_file *out, extern void *top_level_interpreter_data (void); extern struct interp *top_level_interpreter (void); +extern struct interp *command_interp (void); + /* True if the current interpreter is in async mode, false if in sync mode. If in sync mode, running a synchronous execution command (with execute_command, e.g, "next") will not return until the diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index 57d997c..527e4f1 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -37,6 +37,7 @@ #include "gdb.h" #include "objfiles.h" #include "tracepoint.h" +#include "cli-out.h" /* These are the interpreter setup, etc. functions for the MI interpreter. */ @@ -236,6 +237,10 @@ mi_cmd_interpreter_exec (char *command, char **argv, int argc) "does not support command execution"), argv[0]); + /* Note that unlike the CLI version of this command, we don't + actually set INTERP_TO_USE as the current interpreter, as we + still want gdb_stdout, etc. to point at MI streams. */ + /* Insert the MI out hooks, making sure to also call the interpreter's hooks if it has any. */ /* KRS: We shouldn't need this... Events should be installed and @@ -420,6 +425,26 @@ mi_inferior_removed (struct inferior *inf) gdb_flush (mi->event_channel); } +/* Cleanup that restores a previous current uiout. */ + +static void +restore_current_uiout_cleanup (void *arg) +{ + struct ui_out *saved_uiout = arg; + + current_uiout = saved_uiout; +} + +/* Cleanup that destroys the a ui_out object. */ + +static void +ui_out_free_cleanup (void *arg) +{ + struct ui_out *uiout = arg; + + ui_out_destroy (uiout); +} + static void mi_on_normal_stop (struct bpstats *bs, int print_frame) { @@ -450,6 +475,47 @@ mi_on_normal_stop (struct bpstats *bs, int print_frame) current_uiout = saved_uiout; } + /* Otherwise, frame information has already been printed by + normal_stop. */ + else if (target_can_async_p ()) + { + /* However, CLI execution commands (-interpreter-exec + console "next", for example) in async mode have the + opposite issue. normal_stop has already printed frame + information to MI uiout, but nothing has printed the same + information to the CLI channel. We should print the + source line to the console when stepping or other similar + commands, iff the step was started by a console command + (but not if it was started with -exec-step or similar). + Breakpoint hits should always be mirrored to the + console. */ + struct thread_info *tp = inferior_thread (); + + if ((tp->control.command_interp != NULL + && tp->control.command_interp != top_level_interpreter ()) + || (!tp->control.stop_step + && !tp->control.proceed_to_finish)) + { + struct mi_interp *mi = top_level_interpreter_data (); + struct target_waitstatus last; + ptid_t last_ptid; + struct ui_out *cli_uiout; + struct cleanup *old_chain; + + /* Sets the current uiout to a new temporary CLI uiout + assigned to STREAM. */ + cli_uiout = cli_out_new (mi->out); + old_chain = make_cleanup (ui_out_free_cleanup, cli_uiout); + + make_cleanup (restore_current_uiout_cleanup, current_uiout); + current_uiout = cli_uiout; + + get_last_target_status (&last_ptid, &last); + print_stop_event (&last); + + do_cleanups (old_chain); + } + } ui_out_field_int (mi_uiout, "thread-id", pid_to_thread_id (inferior_ptid)); diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp index 5b809e2..bee296d 100644 --- a/gdb/testsuite/gdb.mi/mi-cli.exp +++ b/gdb/testsuite/gdb.mi/mi-cli.exp @@ -166,10 +166,15 @@ mi_gdb_test "34 next" \ ".*34\\\^running.*\\*running,thread-id=\"all\"" \ "34 next: run" -if {!$async} { - gdb_expect { - -re "~\[^\r\n\]+\r\n" { - } +# Test that the new current source line is output, given we executed +# the console 'next' command, not -exec-next. +set test "34 next: CLI output" +gdb_expect { + -re "~\"67\[^\r\n\]+\r\n" { + pass $test + } + timeout { + fail "$test (timeout)" } } diff --git a/gdb/testsuite/gdb.mi/mi-solib.exp b/gdb/testsuite/gdb.mi/mi-solib.exp index cd400ea..0a8b43b 100644 --- a/gdb/testsuite/gdb.mi/mi-solib.exp +++ b/gdb/testsuite/gdb.mi/mi-solib.exp @@ -60,4 +60,15 @@ mi_gdb_test "777-gdb-set stop-on-solib-events 1" "777\\^done" \ # commands still cause the correct MI output to be generated. mi_run_with_cli +# Also test that the CLI solib event note is output. +set test "CLI prints solib event" +gdb_expect { + -re "~\"Stopped due to shared library event \\(no libraries added or removed\\)\\\\n" { + pass "$test" + } + timeout { + fail "$test (timeout)" + } +} + mi_expect_stop solib-event .* .* .* .* .* "check for solib event" -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 9/9] enable target-async 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey ` (6 preceding siblings ...) 2013-10-22 18:26 ` [PATCH v4 6/9] PR gdb/13860: don't lose '-interpreter-exec console EXECUTION_COMMAND''s output in async mode Tom Tromey @ 2013-10-22 18:26 ` Tom Tromey 2013-10-22 20:15 ` Eli Zaretskii ` (2 more replies) 2013-10-22 19:00 ` [PATCH v4 7/9] make dprintf.exp pass in always-async mode Tom Tromey 8 siblings, 3 replies; 53+ messages in thread From: Tom Tromey @ 2013-10-22 18:26 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey This enables target-async by default. Unlike the CLI, MI chose to treat target-async specially -- setting it changes the default behavior of commands. So, we can't get rid of the option. Instead we have to make it MI-only. The hardest part of this patch, to my surprise, was getting the MI prompt to work properly. It was reasonably easy, and clean, to get close to what the test suite expects; but to fix the last remaining failure (mi-async.exp), I had to resort to a hack. It seems to me that the MI grammar was never updated to account for changes implied by async. Perhaps some future MI can dispense with the prompt entirely. Built and regtested on x86-64 Fedora 18. * infrun.c (set_observer_mode): Don't set target_async_permitted. * linux-nat.c (linux_nat_is_async_p): Always return 1. (linux_nat_can_async_p): Likewise. * mi/mi-interp.c (thread_command_not_mi): New function. (mi_interpreter_prompt_p): Maybe print the MI prompt. (mi_execute_command_input_handler): Conditionally print prompt. (mi_on_resume): Check sync_execution before printing prompt. * mi/mi-main.c (mi_target_can_async_p): New function. (exec_continue): Maybe call async_disable_stdin. (run_one_inferior, mi_cmd_exec_run, mi_cmd_list_target_features): Use mi_target_can_async_p. (mi_execute_async_cli_command): Use mi_target_can_async_p. * remote.c (remote_open_1, remote_terminal_inferior) (remote_terminal_ours, remote_can_async_p, remote_is_async_p): Don't check target_async_permitted. * gdb.texinfo (Non-Stop Mode): Remove "set target-async 1" from example. (Background Execution): Move target-async docs... (Asynchronous and non-stop modes): ... here. Rewrite to MI form. * gdb.mi/mi-cli.exp: Don't check "$async". --- gdb/doc/gdb.texinfo | 29 ++++++++++--------------- gdb/infrun.c | 1 - gdb/linux-nat.c | 10 ++------- gdb/mi/mi-interp.c | 43 ++++++++++++++++++++++++++++++------ gdb/mi/mi-main.c | 26 ++++++++++++++++------ gdb/remote.c | 48 +++++++++++------------------------------ gdb/testsuite/gdb.mi/mi-cli.exp | 15 +------------ gdb/tui/tui-interp.c | 1 + 8 files changed, 85 insertions(+), 88 deletions(-) diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo index 44fb174..770e7cb 100644 --- a/gdb/doc/gdb.texinfo +++ b/gdb/doc/gdb.texinfo @@ -5672,9 +5672,6 @@ To enter non-stop mode, use this sequence of commands before you run or attach to your program: @smallexample -# Enable the async interface. -set target-async 1 - # If using the CLI, pagination breaks non-stop. set pagination off @@ -5744,21 +5741,6 @@ the program to report that some thread has stopped before prompting for another command. In background execution, @value{GDBN} immediately gives a command prompt so that you can issue other commands while your program runs. -You need to explicitly enable asynchronous mode before you can use -background execution commands. You can use these commands to -manipulate the asynchronous mode setting: - -@table @code -@kindex set target-async -@item set target-async on -Enable asynchronous mode. -@item set target-async off -Disable asynchronous mode. -@kindex show target-async -@item show target-async -Show the current target-async setting. -@end table - If the target doesn't support async mode, @value{GDBN} issues an error message if you attempt to use the background execution commands. @@ -28679,6 +28661,17 @@ frontend has started the executable or attached to the target, it can find if asynchronous execution is enabled using the @code{-list-target-features} command. +@table @code +@kindex -gdb-set target-async +@item -gdb-set target-async on +Enable asynchronous mode. This mode only affects MI commands. +@item -gdb-set target-async off +Disable asynchronous mode. +@kindex -gdb-show target-async +@item -gdb-show target-async +Show the current target-async setting. +@end table + Even if @value{GDBN} can accept a command while target is running, many commands that access the target do not work when the target is running. Therefore, asynchronous command execution is most useful diff --git a/gdb/infrun.c b/gdb/infrun.c index a58b6fe..b5e81a8 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -246,7 +246,6 @@ set_observer_mode (char *args, int from_tty, going out we leave it that way. */ if (observer_mode) { - target_async_permitted = 1; pagination_enabled = 0; non_stop = non_stop_1 = 1; } diff --git a/gdb/linux-nat.c b/gdb/linux-nat.c index fb2dbb4..a772772 100644 --- a/gdb/linux-nat.c +++ b/gdb/linux-nat.c @@ -4499,10 +4499,7 @@ linux_trad_target (CORE_ADDR (*register_u_offset)(struct gdbarch *, int, int)) static int linux_nat_is_async_p (struct target_ops *ops) { - /* NOTE: palves 2008-03-21: We're only async when the user requests - it explicitly with the "set target-async" command. - Someday, linux will always be async. */ - return target_async_permitted; + return 1; } /* target_can_async_p implementation. */ @@ -4510,10 +4507,7 @@ linux_nat_is_async_p (struct target_ops *ops) static int linux_nat_can_async_p (struct target_ops *ops) { - /* NOTE: palves 2008-03-21: We're only async when the user requests - it explicitly with the "set target-async" command. - Someday, linux will always be async. */ - return target_async_permitted; + return 1; } static int diff --git a/gdb/mi/mi-interp.c b/gdb/mi/mi-interp.c index 527e4f1..76db63b 100644 --- a/gdb/mi/mi-interp.c +++ b/gdb/mi/mi-interp.c @@ -207,11 +207,33 @@ mi_interpreter_exec (void *data, const char *command) return exception_none; } +/* A thread iterator callback function that checks to see if a given + thread's command interpreter was not the MI interpreter. */ + +static int +thread_command_not_mi (struct thread_info *thr, void *ignore) +{ + return thr->control.command_interp != top_level_interpreter (); +} + /* Never display the default GDB prompt in MI case. */ static int mi_interpreter_prompt_p (void *data) { + if (!interp_quiet_p (NULL)) + { + if (!target_is_async_p () + || (!sync_execution + && (!target_async_permitted + || iterate_over_threads (thread_command_not_mi, + NULL) == NULL))) + { + fputs_unfiltered ("(gdb) \n", raw_stdout); + gdb_flush (raw_stdout); + } + } + return 0; } @@ -308,8 +330,17 @@ mi_execute_command_input_handler (char *cmd) { mi_execute_command_wrapper (cmd); - fputs_unfiltered ("(gdb) \n", raw_stdout); - gdb_flush (raw_stdout); + /* MI generally prints a prompt after a command. However, if target + is async, and a synchronous command was issued, then we will + print the prompt elsewhere, after printing "*running". + target_is_async_p checks whether the target is async; + sync_execution checks whether a synchronous command was + issued. */ + if (!target_is_async_p () || !sync_execution) + { + fputs_unfiltered ("(gdb) \n", raw_stdout); + gdb_flush (raw_stdout); + } } static void @@ -821,10 +852,10 @@ mi_on_resume (ptid_t ptid) running_result_record_printed = 1; /* This is what gdb used to do historically -- printing prompt even if it cannot actually accept any input. This will be surely removed - for MI3, and may be removed even earler. */ - /* FIXME: review the use of target_is_async_p here -- is that - what we want? */ - if (!target_is_async_p ()) + for MI3, and may be removed even earler. SYNC_EXECUTION is + checked here because we only need to emit a prompt if a + synchronous command was issued when the target is async. */ + if (!target_is_async_p () || sync_execution) fputs_unfiltered ("(gdb) \n", raw_stdout); } gdb_flush (raw_stdout); diff --git a/gdb/mi/mi-main.c b/gdb/mi/mi-main.c index 24ac1e0..704a13b 100644 --- a/gdb/mi/mi-main.c +++ b/gdb/mi/mi-main.c @@ -106,6 +106,15 @@ static int register_changed_p (int regnum, struct regcache *, static void output_register (struct frame_info *, int regnum, int format, int skip_unavailable); +/* A wrapper for target_can_async_p that takes the MI setting into + account. */ + +static int +mi_target_can_async_p (void) +{ + return target_async_permitted && target_can_async_p (); +} + /* Command implementations. FIXME: Is this libgdb? No. This is the MI layer that calls libgdb. Any operation used in the below should be formalized. */ @@ -262,6 +271,11 @@ exec_continue (char **argv, int argc) { struct cleanup *back_to = make_cleanup_restore_integer (&sched_multi); + /* If MI is in sync mode but the target is async, then + normal_stop enabled stdin. We undo the change here. */ + if (!target_async_permitted && target_can_async_p ()) + async_disable_stdin (); + if (current_context->all) { sched_multi = 1; @@ -396,8 +410,8 @@ run_one_inferior (struct inferior *inf, void *arg) switch_to_thread (null_ptid); set_current_program_space (inf->pspace); } - mi_execute_cli_command (run_cmd, target_can_async_p (), - target_can_async_p () ? "&" : NULL); + mi_execute_cli_command (run_cmd, mi_target_can_async_p (), + mi_target_can_async_p () ? "&" : NULL); return 0; } @@ -451,8 +465,8 @@ mi_cmd_exec_run (char *command, char **argv, int argc) { const char *run_cmd = start_p ? "start" : "run"; - mi_execute_cli_command (run_cmd, target_can_async_p (), - target_can_async_p () ? "&" : NULL); + mi_execute_cli_command (run_cmd, mi_target_can_async_p (), + mi_target_can_async_p () ? "&" : NULL); } } @@ -1837,7 +1851,7 @@ mi_cmd_list_target_features (char *command, char **argv, int argc) struct ui_out *uiout = current_uiout; cleanup = make_cleanup_ui_out_list_begin_end (uiout, "features"); - if (target_can_async_p ()) + if (mi_target_can_async_p ()) ui_out_field_string (uiout, NULL, "async"); if (target_can_execute_reverse) ui_out_field_string (uiout, NULL, "reverse"); @@ -2252,7 +2266,7 @@ mi_execute_async_cli_command (char *cli_command, char **argv, int argc) struct cleanup *old_cleanups; char *run; - if (target_can_async_p ()) + if (mi_target_can_async_p ()) run = xstrprintf ("%s %s&", cli_command, argc ? *argv : ""); else run = xstrprintf ("%s %s", cli_command, argc ? *argv : ""); diff --git a/gdb/remote.c b/gdb/remote.c index b143014..db7f7ae 100644 --- a/gdb/remote.c +++ b/gdb/remote.c @@ -4275,8 +4275,7 @@ remote_open_1 (char *name, int from_tty, "(e.g. /dev/ttyS0, /dev/ttya, COM1, etc.).")); /* See FIXME above. */ - if (!target_async_permitted) - wait_forever_enabled_p = 1; + wait_forever_enabled_p = 1; /* If we're connected to a running target, target_preopen will kill it. Ask this question first, before target_preopen has a chance to kill @@ -4362,20 +4361,17 @@ remote_open_1 (char *name, int from_tty, rs->use_threadinfo_query = 1; rs->use_threadextra_query = 1; - if (target_async_permitted) - { - /* With this target we start out by owning the terminal. */ - remote_async_terminal_ours_p = 1; + /* With this target we start out by owning the terminal. */ + remote_async_terminal_ours_p = 1; - /* FIXME: cagney/1999-09-23: During the initial connection it is - assumed that the target is already ready and able to respond to - requests. Unfortunately remote_start_remote() eventually calls - wait_for_inferior() with no timeout. wait_forever_enabled_p gets - around this. Eventually a mechanism that allows - wait_for_inferior() to expect/get timeouts will be - implemented. */ - wait_forever_enabled_p = 0; - } + /* FIXME: cagney/1999-09-23: During the initial connection it is + assumed that the target is already ready and able to respond to + requests. Unfortunately remote_start_remote() eventually calls + wait_for_inferior() with no timeout. wait_forever_enabled_p gets + around this. Eventually a mechanism that allows + wait_for_inferior() to expect/get timeouts will be + implemented. */ + wait_forever_enabled_p = 0; /* First delete any symbols previously loaded from shared libraries. */ no_shared_libraries (NULL, 0); @@ -4411,14 +4407,12 @@ remote_open_1 (char *name, int from_tty, already before throwing the exception. */ if (rs->remote_desc != NULL) remote_unpush_target (); - if (target_async_permitted) - wait_forever_enabled_p = 1; + wait_forever_enabled_p = 1; throw_exception (ex); } } - if (target_async_permitted) - wait_forever_enabled_p = 1; + wait_forever_enabled_p = 1; } /* This takes a program previously attached to and detaches it. After @@ -5209,10 +5203,6 @@ Give up (and stop debugging it)? "))) static void remote_terminal_inferior (void) { - if (!target_async_permitted) - /* Nothing to do. */ - return; - /* FIXME: cagney/1999-09-27: Make calls to target_terminal_*() idempotent. The event-loop GDB talking to an asynchronous target with a synchronous command calls this function from both @@ -5232,10 +5222,6 @@ remote_terminal_inferior (void) static void remote_terminal_ours (void) { - if (!target_async_permitted) - /* Nothing to do. */ - return; - /* See FIXME in remote_terminal_inferior. */ if (remote_async_terminal_ours_p) return; @@ -11622,10 +11608,6 @@ remote_can_async_p (struct target_ops *ops) { struct remote_state *rs = get_remote_state (); - if (!target_async_permitted) - /* We only enable async when the user specifically asks for it. */ - return 0; - /* We're async whenever the serial device is. */ return serial_can_async_p (rs->remote_desc); } @@ -11635,10 +11617,6 @@ remote_is_async_p (struct target_ops *ops) { struct remote_state *rs = get_remote_state (); - if (!target_async_permitted) - /* We only enable async when the user specifically asks for it. */ - return 0; - /* We're async whenever the serial device is. */ return serial_is_async_p (rs->remote_desc); } diff --git a/gdb/testsuite/gdb.mi/mi-cli.exp b/gdb/testsuite/gdb.mi/mi-cli.exp index bee296d..08e443e 100644 --- a/gdb/testsuite/gdb.mi/mi-cli.exp +++ b/gdb/testsuite/gdb.mi/mi-cli.exp @@ -134,20 +134,7 @@ mi_gdb_test "500-stack-select-frame 0" \ {500\^done} \ "-stack-select-frame 0" -# When a CLI command is entered in MI session, the respose is different in -# sync and async modes. In sync mode normal_stop is called when current -# interpreter is CLI. So: -# - print_stop_reason prints stop reason in CLI uiout, and we don't show it -# in MI -# - The stop position is printed, and appears in MI 'console' channel. -# -# In async mode the stop event is processed when we're back to MI interpreter, -# so the stop reason is printed into MI uiout an. -if {$async} { - set reason "end-stepping-range" -} else { - set reason "" -} +set reason "end-stepping-range" mi_execute_to "interpreter-exec console step" $reason "callee4" "" ".*basics.c" $line_callee4_next \ "" "check *stopped from CLI command" diff --git a/gdb/tui/tui-interp.c b/gdb/tui/tui-interp.c index 1095220..659f1cd 100644 --- a/gdb/tui/tui-interp.c +++ b/gdb/tui/tui-interp.c @@ -30,6 +30,7 @@ #include "tui/tui.h" #include "tui/tui-io.h" #include "exceptions.h" +#include "target.h" /* Set to 1 when the TUI mode must be activated when we first start gdb. */ -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-10-22 18:26 ` [PATCH v4 9/9] enable target-async Tom Tromey @ 2013-10-22 20:15 ` Eli Zaretskii 2013-11-11 19:54 ` Pedro Alves 2013-11-12 20:53 ` Pedro Alves 2 siblings, 0 replies; 53+ messages in thread From: Eli Zaretskii @ 2013-10-22 20:15 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches, tromey > From: Tom Tromey <tromey@redhat.com> > Cc: Tom Tromey <tromey@redhat.com> > Date: Tue, 22 Oct 2013 11:59:29 -0600 > > * infrun.c (set_observer_mode): Don't set target_async_permitted. > * linux-nat.c (linux_nat_is_async_p): Always return 1. > (linux_nat_can_async_p): Likewise. > * mi/mi-interp.c (thread_command_not_mi): New function. > (mi_interpreter_prompt_p): Maybe print the MI prompt. > (mi_execute_command_input_handler): Conditionally print prompt. > (mi_on_resume): Check sync_execution before printing prompt. > * mi/mi-main.c (mi_target_can_async_p): New function. > (exec_continue): Maybe call async_disable_stdin. > (run_one_inferior, mi_cmd_exec_run, mi_cmd_list_target_features): > Use mi_target_can_async_p. > (mi_execute_async_cli_command): Use mi_target_can_async_p. > * remote.c (remote_open_1, remote_terminal_inferior) > (remote_terminal_ours, remote_can_async_p, remote_is_async_p): > Don't check target_async_permitted. > > * gdb.texinfo (Non-Stop Mode): Remove "set target-async 1" > from example. > (Background Execution): Move target-async docs... > (Asynchronous and non-stop modes): ... here. Rewrite to > MI form. > > * gdb.mi/mi-cli.exp: Don't check "$async". The changes to the manual are OK. Thanks. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-10-22 18:26 ` [PATCH v4 9/9] enable target-async Tom Tromey 2013-10-22 20:15 ` Eli Zaretskii @ 2013-11-11 19:54 ` Pedro Alves 2013-11-12 20:53 ` Pedro Alves 2 siblings, 0 replies; 53+ messages in thread From: Pedro Alves @ 2013-11-11 19:54 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches A small note: Still looking at this one, but meanwhile, since we're adding back a maint command to force-disable async, I'd prefer seeing this patch not actually toggle the default, but leave that to a final one-liner patch. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-10-22 18:26 ` [PATCH v4 9/9] enable target-async Tom Tromey 2013-10-22 20:15 ` Eli Zaretskii 2013-11-11 19:54 ` Pedro Alves @ 2013-11-12 20:53 ` Pedro Alves 2013-11-15 0:45 ` Tom Tromey 2 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-11-12 20:53 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches Hi Tom, still not a full review, but I thought I'd send out a couple comments as I have them. On 10/22/2013 06:59 PM, Tom Tromey wrote: > /* Never display the default GDB prompt in MI case. */ > > static int > mi_interpreter_prompt_p (void *data) > { Looks quite odd for a predicate function to actually have side effects. I guess this is the hack you mentioned? I think this is missing a comment explaining what is going on. It's not obvious at all to me. > + if (!interp_quiet_p (NULL)) > + { > + if (!target_is_async_p () > + || (!sync_execution > + && (!target_async_permitted > + || iterate_over_threads (thread_command_not_mi, > + NULL) == NULL))) > + { > + fputs_unfiltered ("(gdb) \n", raw_stdout); > + gdb_flush (raw_stdout); > + } > + } > + > return 0; > } > @@ -1837,7 +1851,7 @@ mi_cmd_list_target_features (char *command, char **argv, int argc) > struct ui_out *uiout = current_uiout; > > cleanup = make_cleanup_ui_out_list_begin_end (uiout, "features"); > - if (target_can_async_p ()) > + if (mi_target_can_async_p ()) > ui_out_field_string (uiout, NULL, "async"); > if (target_can_execute_reverse) > ui_out_field_string (uiout, NULL, "reverse"); Hmm, not sure this is right. This supposedly returns the set of supported features. But mi_target_can_async_p returns false until the frontend enables target-async. So this change creates a sort of chicken and egg situation. > --- a/gdb/testsuite/gdb.mi/mi-cli.exp > +++ b/gdb/testsuite/gdb.mi/mi-cli.exp > @@ -134,20 +134,7 @@ mi_gdb_test "500-stack-select-frame 0" \ > {500\^done} \ > "-stack-select-frame 0" > > -# When a CLI command is entered in MI session, the respose is different in > -# sync and async modes. In sync mode normal_stop is called when current > -# interpreter is CLI. So: > -# - print_stop_reason prints stop reason in CLI uiout, and we don't show it > -# in MI > -# - The stop position is printed, and appears in MI 'console' channel. > -# > -# In async mode the stop event is processed when we're back to MI interpreter, > -# so the stop reason is printed into MI uiout an. > -if {$async} { > - set reason "end-stepping-range" > -} else { > - set reason "" > -} > +set reason "end-stepping-range" I'm a little confused by this one. Isn't it still necessary for targets that don't do async? -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-11-12 20:53 ` Pedro Alves @ 2013-11-15 0:45 ` Tom Tromey 2013-11-18 15:42 ` Pedro Alves 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-11-15 0:45 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: >> static int >> mi_interpreter_prompt_p (void *data) >> { Pedro> Looks quite odd for a predicate function to actually have Pedro> side effects. I guess this is the hack you mentioned? Pedro> I think this is missing a comment explaining what is Pedro> going on. It's not obvious at all to me. Yeah, this is the biggest hack. I will try to comment it some more. The fundamental issue here I ran into is that MI is very odd about when it prints a prompt. So, the hacks are needed to keep the behavior consistent -- even though, IMNSHO, the behavior doesn't actually make any sense. I don't even see why MI needs a prompt, but of course that can't be addressed until someone wants to roll out MI3. >> cleanup = make_cleanup_ui_out_list_begin_end (uiout, "features"); >> - if (target_can_async_p ()) >> + if (mi_target_can_async_p ()) >> ui_out_field_string (uiout, NULL, "async"); >> if (target_can_execute_reverse) >> ui_out_field_string (uiout, NULL, "reverse"); Pedro> Hmm, not sure this is right. This supposedly returns the set of Pedro> supported features. But mi_target_can_async_p returns false Pedro> until the frontend enables target-async. So this change creates Pedro> a sort of chicken and egg situation. That is what I thought, too, but IIRC if one changes this, then a test will fail. Also it is consistent with what gdb does today: (gdb) -list-target-features ^done,features=[] (gdb) set target-async on &"set target-async on\n" ^done (gdb) -list-target-features ^done,features=["async"] (gdb) Strange but true. Actually I think this is symptomatic of the general issue where MI paid attention to "set target-async", whereas I think in a clean design it would not. >> -# so the stop reason is printed into MI uiout an. >> -if {$async} { >> - set reason "end-stepping-range" >> -} else { >> - set reason "" >> -} >> +set reason "end-stepping-range" Pedro> I'm a little confused by this one. Isn't it still necessary Pedro> for targets that don't do async? Not sure if you remember the story. When I started this project I was working under the belief that "set target-async" was a "please enable a feature" sort of option -- that is, it ought to have no user visible effect other than making the "&" feature available; and as such I could simply enable it always, fix the test suite failures, and deprecate the option. However, it turns out that this model did not fit the reality. MI used the target-async setting not just to put the target into async mode and to enable the "&" feature, but also to change its output style in various spots. There's a thread you can dig up where Marc Khouzam says they changed Eclipse to disable target-async explicitly, just to work around the oddities that ensued. For this test case the check may in fact be irrelevant, since we aren't enabling target-async. However if that is so, we might as well drop it anyway on account of clarity. Or maybe this is intended to support running the test suite with some pre-canned MI sequence to enable target async. I would guess nobody ever does this, since I think when I tried something like this (naively setting target_async_permitted = 1), stuff broke all over. Which is apparently intentional. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-11-15 0:45 ` Tom Tromey @ 2013-11-18 15:42 ` Pedro Alves 2013-12-06 20:44 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-11-18 15:42 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 11/14/2013 09:56 PM, Tom Tromey wrote: >>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: > >>> static int >>> mi_interpreter_prompt_p (void *data) >>> { > > Pedro> Looks quite odd for a predicate function to actually have > Pedro> side effects. I guess this is the hack you mentioned? > Pedro> I think this is missing a comment explaining what is > Pedro> going on. It's not obvious at all to me. > > Yeah, this is the biggest hack. > I will try to comment it some more. > > The fundamental issue here I ran into is that MI is very odd about when > it prints a prompt. > So, the hacks are needed to keep the behavior > consistent -- even though, IMNSHO, the behavior doesn't actually make > any sense. Sorry, I wasn't clear. I understand what as a whole the patch is trying to do; what I don't understand is why a hack was necessary, or its implementation. E.g., what exactly fails if the hack is not in place?; Why this spot for the hack?; What's the predicate used in the hack actually checking? Now that I looked again a little closer, I recalled that GDB in sync mode always outputs a silly extra prompt right after ^running (in response to execution commands), before the target stops, even though GDB is not ready for input then. Guess this hack is related? >>> cleanup = make_cleanup_ui_out_list_begin_end (uiout, "features"); >>> - if (target_can_async_p ()) >>> + if (mi_target_can_async_p ()) >>> ui_out_field_string (uiout, NULL, "async"); >>> if (target_can_execute_reverse) >>> ui_out_field_string (uiout, NULL, "reverse"); > > Pedro> Hmm, not sure this is right. This supposedly returns the set of > Pedro> supported features. But mi_target_can_async_p returns false > Pedro> until the frontend enables target-async. So this change creates > Pedro> a sort of chicken and egg situation. > > That is what I thought, too, but IIRC if one changes this, then a test > will fail. > > Also it is consistent with what gdb does today: > > (gdb) > -list-target-features > ^done,features=[] > (gdb) > set target-async on > &"set target-async on\n" > ^done > (gdb) > -list-target-features > ^done,features=["async"] > (gdb) > I guess we could see it either way. -list-target-features lists target features. So with GDB today, until "set target-async on" is issued, the target doesn't support async. After the series, the target does support async even if MI itself isn't async. E.g., I'd've expected 'interpreter-exec mi "-list-target-features"' issued from the cli to list "async". Given the chicken and egg thing already exists today, this makes me think no frontend is actually looking for this feature... (?) Anyway, fine with me to leave this as you have it for now, and maybe reconsider it after the series is in. > Strange but true. Actually I think this is symptomatic of the general > issue where MI paid attention to "set target-async", whereas I think in > a clean design it would not. Yeah, well. TBC, MI async was not really designed around "set target-async" specifically. Async support goes all the way back to 1999. At the time only the remote target supported async, but it wasn't actually the remote target. You'd do "target async" instead of "target remote" (that's still visible in gdb.base/async.exp). So by then, the frontend always knew what to expect, as it was the one who chose whether to async or not. When we started fixing async bitrot (and fixing lots of things) almost 10 years later, "target async" and "target remote" were merged, and a knob added to enable async-ness (and then GNU/Linux learnt async too). Fundamentally, in a perfect world, MI should have always just printed the prompt whenever GDB is ready for further frontend input. Then, a frontend that is aware of that, and that understands async (that is, is aware that gdb may be ready for further input before the target stops) would be able to cope with sync output too. But, unfortunately, in sync mode, GDB prints the prompt too right after sync execution commands, before the target stops. :-/ So one could say that even without async in the picture, the prompt in sync mode has always been wrong... In async mode, I don't think MI could ever emulate that, as the extra prompt would confuse the frontend. Well, maybe it could, if it had been defined that after ^running there's always an extra prompt that should be ignored. Oh well... > >>> -# so the stop reason is printed into MI uiout an. >>> -if {$async} { >>> - set reason "end-stepping-range" >>> -} else { >>> - set reason "" >>> -} >>> +set reason "end-stepping-range" > > Pedro> I'm a little confused by this one. Isn't it still necessary > Pedro> for targets that don't do async? > > Not sure if you remember the story. > > When I started this project I was working under the belief that "set > target-async" was a "please enable a feature" sort of option -- that is, > it ought to have no user visible effect other than making the "&" > feature available; and as such I could simply enable it always, fix the > test suite failures, and deprecate the option. > > However, it turns out that this model did not fit the reality. MI used > the target-async setting not just to put the target into async mode and > to enable the "&" feature, but also to change its output style in > various spots. > > There's a thread you can dig up where Marc Khouzam says they changed > Eclipse to disable target-async explicitly, just to work around the > oddities that ensued. Yeah, I recall that. https://sourceware.org/ml/gdb-patches/2011-12/msg00810.html The main issue was that ^C doesn't work for interrupting the target in async mode (you have to use -exec-interrupt). But I can imagine that the frontends would get confused by not seeing the extra prompt gdb puts out for (no good reason) after resumption commands in sync mode. > For this test case the check may in fact be irrelevant, since we aren't > enabling target-async. However if that is so, we might as well drop it > anyway on account of clarity. But you're dropping the sync path. How come the test doesn't fail on all other targets but GNU/Linux|Remote after this? > Or maybe this is intended to support running the test suite with some > pre-canned MI sequence to enable target async. I would guess nobody > ever does this, since I think when I tried something like this (naively > setting target_async_permitted = 1), stuff broke all over. Which is > apparently intentional. Now I'm _really_ confused. I (and Vladimir back when he was hacking on async) have done something like that many times in the past. You use a dejagnu board that does something like: set GDBFLAGS "${GDBFLAGS} -ex \"set target-async on\"" That's how async mode has been tested so far. That's why we call mi_detect_async right after starting gdb. Before this series, stuff isn't supposed to be breaking all over with that. Last I tried you'd only get a few regressions (like the bugs this series fixes), and I was assuming that you had done that too, and that at least before patch 9, testing gdb like that would be regression free compared to a default sync run. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-11-18 15:42 ` Pedro Alves @ 2013-12-06 20:44 ` Tom Tromey 2013-12-09 12:01 ` Pedro Alves 2014-02-24 17:38 ` Tom Tromey 0 siblings, 2 replies; 53+ messages in thread From: Tom Tromey @ 2013-12-06 20:44 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: Pedro> Sorry, I wasn't clear. I understand what as a whole the patch is trying Pedro> to do; what I don't understand is why a hack was necessary, or its Pedro> implementation. E.g., what exactly fails if the hack is not Pedro> in place?; Why this spot for the hack?; What's the predicate used in Pedro> the hack actually checking? I don't remember, but I'll back it out and redo the analysis to find out. Pedro> Now that I looked again a little closer, I recalled that GDB in Pedro> sync mode always outputs a silly extra prompt right after Pedro> ^running (in response to execution commands), before the target Pedro> stops, even though GDB is not ready for input then. Guess this Pedro> hack is related? Probably so. To my surprise dealing with the prompt was by far the most difficult part of this series. Doubly sad since the prompt just seems like a mistake to me in the first place. >> set target-async on >> &"set target-async on\n" >> ^done >> (gdb) >> -list-target-features >> ^done,features=["async"] Pedro> I guess we could see it either way. -list-target-features lists Pedro> target features. So with GDB today, until "set target-async on" Pedro> is issued, the target doesn't support async. After the series, Pedro> the target does support async even if MI itself isn't async. Pedro> E.g., I'd've expected 'interpreter-exec mi "-list-target-features"' Pedro> issued from the cli to list "async". Pedro> Given the chicken and egg thing already exists today, this makes Pedro> me think no frontend is actually looking for this feature... (?) Pedro> Anyway, fine with me to leave this as you have it for now, and Pedro> maybe reconsider it after the series is in. Yeah. My overall goal for this series was to eliminate the need for "set target-async on", while at the same time preserving the current MI output, regardless of whether I felt it was correct; unless said change is clearly compatible according to the normal MI rules. My reasoning is that this is the best way to avoid breaking a client. >>>> -# so the stop reason is printed into MI uiout an. >>>> -if {$async} { >>>> - set reason "end-stepping-range" >>>> -} else { >>>> - set reason "" >>>> -} >>>> +set reason "end-stepping-range" >> Pedro> I'm a little confused by this one. Isn't it still necessary Pedro> for targets that don't do async? I re-examined this and I think the answer is much simpler than all the incorrect things I wrote before. git master gdb in the default (target-async off) mode prints: *stopped,frame={addr="0x0000000000400597",func="callee4",args=[],file="../../../binutils-gdb/gdb/testsuite/gdb.mi/basics.c",fullname="/home/tromey/Space/SecondArcher/binutils-gdb/gdb/testsuite/gdb.mi/basics.c",line="26"},thread-id="1",stopped-threads="all",core="0" The gdb from this branch prints: *stopped,reason="end-stepping-range",frame={addr="0x0000000000400597",func="callee4",args=[],file="../../../binutils-gdb/gdb/testsuite/gdb.mi/basics.c",fullname="/home/tromey/gnu/gdb/devel/binutils-gdb/gdb/testsuite/gdb.mi/basics.c",line="26"},thread-id="1",stopped-threads="all",core="3" That is, gdb now emits reason="end-stepping-range". This is a compatible change to the output. This explains the patch: now the !$async case is no longer hit. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-12-06 20:44 ` Tom Tromey @ 2013-12-09 12:01 ` Pedro Alves 2013-12-09 15:57 ` Tom Tromey 2014-02-24 17:38 ` Tom Tromey 1 sibling, 1 reply; 53+ messages in thread From: Pedro Alves @ 2013-12-09 12:01 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 12/06/2013 08:44 PM, Tom Tromey wrote: >>>>> -# so the stop reason is printed into MI uiout an. >>>>> -if {$async} { >>>>> - set reason "end-stepping-range" >>>>> -} else { >>>>> - set reason "" >>>>> -} >>>>> +set reason "end-stepping-range" >>> > Pedro> I'm a little confused by this one. Isn't it still necessary > Pedro> for targets that don't do async? > > I re-examined this and I think the answer is much simpler than all the > incorrect things I wrote before. > > git master gdb in the default (target-async off) mode prints: > > *stopped,frame={addr="0x0000000000400597",func="callee4",args=[],file="../../../binutils-gdb/gdb/testsuite/gdb.mi/basics.c",fullname="/home/tromey/Space/SecondArcher/binutils-gdb/gdb/testsuite/gdb.mi/basics.c",line="26"},thread-id="1",stopped-threads="all",core="0" > > The gdb from this branch prints: > > *stopped,reason="end-stepping-range",frame={addr="0x0000000000400597",func="callee4",args=[],file="../../../binutils-gdb/gdb/testsuite/gdb.mi/basics.c",fullname="/home/tromey/gnu/gdb/devel/binutils-gdb/gdb/testsuite/gdb.mi/basics.c",line="26"},thread-id="1",stopped-threads="all",core="3" > > But that was on GNU/Linux, where the backend will always be async, right? > That is, gdb now emits reason="end-stepping-range". This is a > compatible change to the output. This explains the patch: now the > !$async case is no longer hit. The question is whether that is actually true on all the other backends/targets that _don't_ know how to async. Like e.g., Windows. -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-12-09 12:01 ` Pedro Alves @ 2013-12-09 15:57 ` Tom Tromey 2014-02-21 20:23 ` Tom Tromey 0 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-12-09 15:57 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes: >> That is, gdb now emits reason="end-stepping-range". This is a >> compatible change to the output. This explains the patch: now the >> !$async case is no longer hit. Pedro> The question is whether that is actually true on all the other Pedro> backends/targets that _don't_ know how to async. Like e.g., Pedro> Windows. Thanks, I see what you mean, finally. I'll test it (really everything) in the new special not-async mode and fix the test case accordingly. It's an awful irony that trying to remove target-async has actually apparently made the mode situation worse :-( Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-12-09 15:57 ` Tom Tromey @ 2014-02-21 20:23 ` Tom Tromey 0 siblings, 0 replies; 53+ messages in thread From: Tom Tromey @ 2014-02-21 20:23 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro> The question is whether that is actually true on all the other Pedro> backends/targets that _don't_ know how to async. Like e.g., Pedro> Windows. Tom> Thanks, I see what you mean, finally. I'll test it (really everything) Tom> in the new special not-async mode and fix the test case accordingly. I patched the test case in question (mi-cli.exp) to fix this problem. Then I patched gdb to disable the new "maint set target-async" by default (and to make "set target-async" enable this mode as well ... a hack only needed when doing this kind of test) and re-ran the test suite. This passed. So, I don't think there should be any other bugs that will bite on a non-target-async system. That said, I will test the next revision of the series on AIX, which is truly not-async. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 9/9] enable target-async 2013-12-06 20:44 ` Tom Tromey 2013-12-09 12:01 ` Pedro Alves @ 2014-02-24 17:38 ` Tom Tromey 1 sibling, 0 replies; 53+ messages in thread From: Tom Tromey @ 2014-02-24 17:38 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches Pedro> Sorry, I wasn't clear. I understand what as a whole the patch is trying Pedro> to do; what I don't understand is why a hack was necessary, or its Pedro> implementation. E.g., what exactly fails if the hack is not Pedro> in place?; Why this spot for the hack?; What's the predicate used in Pedro> the hack actually checking? Tom> I don't remember, but I'll back it out and redo the analysis to find Tom> out. Pedro> Now that I looked again a little closer, I recalled that GDB in Pedro> sync mode always outputs a silly extra prompt right after Pedro> ^running (in response to execution commands), before the target Pedro> stops, even though GDB is not ready for input then. Guess this Pedro> hack is related? Tom> Probably so. Yes, this is what is going on. If I remove the prompt-printing code here, then a simple case like mi-start.exp fails because gdb fails to print a prompt after the *stopped notification. Moving the prompt printing elsewhere doesn't really help, either, because other choices mean introducing test regressions somewhere. Tom ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 7/9] make dprintf.exp pass in always-async mode 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey ` (7 preceding siblings ...) 2013-10-22 18:26 ` [PATCH v4 9/9] enable target-async Tom Tromey @ 2013-10-22 19:00 ` Tom Tromey 2013-11-12 0:05 ` Pedro Alves 8 siblings, 1 reply; 53+ messages in thread From: Tom Tromey @ 2013-10-22 19:00 UTC (permalink / raw) To: gdb-patches; +Cc: Tom Tromey When target-async is enabled, dprintf.exp fails. This happens because run_inferior_call causes gdb to forget that it is running in sync_execution mode, so something like a breakpoint condition that makes an inferior call causes gdb to enter fully async mode. This patch fixes the problem by noticing when gdb was in sync_execution mode in run_inferior_call, and taking care to restore this state afterward. Built and regtested on x86-64 Fedora 18. PR cli/15718: * infcall.c: Include event-top.h. (run_inferior_call): Call async_disable_stdin if needed. --- gdb/infcall.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/gdb/infcall.c b/gdb/infcall.c index 19af044..7398913 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -36,6 +36,7 @@ #include "ada-lang.h" #include "gdbthread.h" #include "exceptions.h" +#include "event-top.h" /* If we can't find a function's name from its address, we print this instead. */ @@ -398,6 +399,8 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) TRY_CATCH (e, RETURN_MASK_ALL) { + int was_sync = sync_execution; + proceed (real_pc, GDB_SIGNAL_0, 0); /* Inferior function calls are always synchronous, even if the @@ -407,6 +410,11 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) { wait_for_inferior (); normal_stop (); + /* If gdb was previously in sync execution mode, then ensure + that it remains so. normal_stop calls + async_enable_stdin, so reset it again here. */ + if (was_sync) + async_disable_stdin (); } } -- 1.8.1.4 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v4 7/9] make dprintf.exp pass in always-async mode 2013-10-22 19:00 ` [PATCH v4 7/9] make dprintf.exp pass in always-async mode Tom Tromey @ 2013-11-12 0:05 ` Pedro Alves 0 siblings, 0 replies; 53+ messages in thread From: Pedro Alves @ 2013-11-12 0:05 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches On 10/22/2013 06:59 PM, Tom Tromey wrote: > When target-async is enabled, dprintf.exp fails. > > This happens because run_inferior_call causes gdb to forget that it is > running in sync_execution mode, so something like a breakpoint > condition that makes an inferior call causes gdb to enter fully async > mode. It'd be great if we had a test that exercised this without relying on how dprintf is implemented behind the scenes. > This patch fixes the problem by noticing when gdb was in > sync_execution mode in run_inferior_call, and taking care to restore > this state afterward. > > Built and regtested on x86-64 Fedora 18. > > PR cli/15718: > * infcall.c: Include event-top.h. > (run_inferior_call): Call async_disable_stdin if needed. > --- > gdb/infcall.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/gdb/infcall.c b/gdb/infcall.c > index 19af044..7398913 100644 > --- a/gdb/infcall.c > +++ b/gdb/infcall.c > @@ -36,6 +36,7 @@ > #include "ada-lang.h" > #include "gdbthread.h" > #include "exceptions.h" > +#include "event-top.h" > > /* If we can't find a function's name from its address, > we print this instead. */ > @@ -398,6 +399,8 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) > > TRY_CATCH (e, RETURN_MASK_ALL) > { > + int was_sync = sync_execution; > + > proceed (real_pc, GDB_SIGNAL_0, 0); > > /* Inferior function calls are always synchronous, even if the > @@ -407,6 +410,11 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) > { > wait_for_inferior (); > normal_stop (); > + /* If gdb was previously in sync execution mode, then ensure > + that it remains so. normal_stop calls > + async_enable_stdin, so reset it again here. */ > + if (was_sync) > + async_disable_stdin (); > } > } Hmm, I was a worried about what would happen if the inferior exits, or some other thread hits a breakpoint, or something else like that. Like, what enables back stdin in that case? So an error is always thrown, and we'll hit the INF_REG_EVENT error handling inf-loop.c:inferior_event_handler, which re-enables back stdin and the prompt. So the change is OK with me, but I'd like to have this mentioned in the comment. Thanks, -- Pedro Alves ^ permalink raw reply [flat|nested] 53+ messages in thread
end of thread, other threads:[~2014-02-24 17:38 UTC | newest] Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-10-22 17:59 [PATCH v4 0/9] enable target-async by default Tom Tromey 2013-10-22 17:59 ` [PATCH v4 5/9] PR gdb/13860: make "-exec-foo"'s MI output equal to "foo"'s MI output Tom Tromey 2013-10-22 17:59 ` [PATCH v4 1/9] fix latent bugs in ui-out.c Tom Tromey 2013-10-28 15:20 ` Pedro Alves 2013-10-28 17:36 ` Tom Tromey 2013-10-22 17:59 ` [PATCH v4 3/9] add target method delegation Tom Tromey 2013-10-28 16:05 ` Pedro Alves 2013-10-28 17:51 ` Tom Tromey 2013-10-28 17:53 ` Tom Tromey 2013-10-29 20:55 ` Tom Tromey 2013-11-08 17:44 ` Pedro Alves 2013-12-11 22:03 ` Tom Tromey 2013-12-12 2:46 ` Tom Tromey 2013-12-13 22:07 ` Tom Tromey 2013-12-16 13:07 ` Pedro Alves 2013-12-16 21:21 ` Tom Tromey 2013-12-17 16:17 ` Pedro Alves 2013-12-18 18:29 ` Tom Tromey 2013-12-18 22:06 ` Tom Tromey 2013-12-19 16:03 ` Pedro Alves 2013-12-19 16:15 ` Tom Tromey 2013-12-20 19:24 ` Tom Tromey 2013-11-08 16:34 ` Pedro Alves 2013-10-22 17:59 ` [PATCH v4 4/9] PR gdb/13860: make -interpreter-exec console "list" behave more like "list" Tom Tromey 2013-10-22 17:59 ` [PATCH v4 2/9] add "this" pointers to more target APIs Tom Tromey 2013-10-28 16:04 ` Pedro Alves 2013-10-28 16:37 ` Tom Tromey 2013-10-28 16:44 ` Pedro Alves 2013-10-28 16:52 ` Tom Tromey 2013-11-08 18:04 ` Pedro Alves 2013-11-08 21:53 ` Tom Tromey 2013-11-09 3:35 ` Pedro Alves 2013-12-06 17:40 ` Tom Tromey 2013-12-06 18:35 ` Pedro Alves 2013-12-06 18:23 ` Tom Tromey 2013-12-06 19:06 ` Pedro Alves 2013-10-22 18:11 ` [PATCH v4 8/9] fix py-finish-breakpoint.exp with always-async Tom Tromey 2013-11-11 19:51 ` Pedro Alves 2013-12-09 17:53 ` Tom Tromey 2013-10-22 18:26 ` [PATCH v4 6/9] PR gdb/13860: don't lose '-interpreter-exec console EXECUTION_COMMAND''s output in async mode Tom Tromey 2013-10-22 18:26 ` [PATCH v4 9/9] enable target-async Tom Tromey 2013-10-22 20:15 ` Eli Zaretskii 2013-11-11 19:54 ` Pedro Alves 2013-11-12 20:53 ` Pedro Alves 2013-11-15 0:45 ` Tom Tromey 2013-11-18 15:42 ` Pedro Alves 2013-12-06 20:44 ` Tom Tromey 2013-12-09 12:01 ` Pedro Alves 2013-12-09 15:57 ` Tom Tromey 2014-02-21 20:23 ` Tom Tromey 2014-02-24 17:38 ` Tom Tromey 2013-10-22 19:00 ` [PATCH v4 7/9] make dprintf.exp pass in always-async mode Tom Tromey 2013-11-12 0:05 ` Pedro Alves
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).