From 388a708404618466bbe51b7198de7a64f0a5da9f Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 9 Sep 2015 18:23:24 +0100 Subject: [PATCH] Convert infcalls to thread_fsm mechanism This removes infcall-specific special casing from normal_stop, simplifying it. Like the "finish" command's, the FSM is responsible for storing the function's return value. gdb/ChangeLog: 2015-09-09 Pedro Alves * infcall.c: Include thread_fsm.h. (struct call_return_meta_info): New. (get_call_return_value): New function, factored out from call_function_by_hand_dummy. (struct call_thread_fsm): New. (call_thread_fsm_ops): New global. (new_call_thread_fsm, call_thread_fsm_should_stop) (call_thread_fsm_should_notify_stop): New functions. (run_inferior_call): Add 'sm' parameter. Associate the FSM with the thread. (call_function_by_hand_dummy): Create a new call_thread_fsm instance, associate it with the thread, and wait for the FSM to finish. If finished successfully, fetch the function's result value out of the FSM. * infrun.c (fetch_inferior_event): If the FSM says the stop shouldn't be notified, don't call normal_stop. (maybe_remove_breakpoints): New function, factored out from ... (normal_stop): ... here. Simplify. * infrun.h (maybe_remove_breakpoints): Declare. * thread-fsm.c (thread_fsm_should_notify_stop): New function. (thread-fsm.h) : New field. (thread_fsm_should_notify_stop): Declare. --- gdb/ChangeLog | 25 ++++ gdb/infcall.c | 305 ++++++++++++++++++++++++++++++++++++----------- gdb/infrun.c | 159 +++++++++++------------- gdb/infrun.h | 4 + gdb/thread-fsm.c | 10 ++ gdb/thread-fsm.h | 6 + 6 files changed, 349 insertions(+), 160 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index df98753740..f6fa8ec302 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,3 +1,28 @@ +2015-09-09 Pedro Alves + + * infcall.c: Include thread_fsm.h. + (struct call_return_meta_info): New. + (get_call_return_value): New function, factored out from + call_function_by_hand_dummy. + (struct call_thread_fsm): New. + (call_thread_fsm_ops): New global. + (new_call_thread_fsm, call_thread_fsm_should_stop) + (call_thread_fsm_should_notify_stop): New functions. + (run_inferior_call): Add 'sm' parameter. Associate the FSM with + the thread. + (call_function_by_hand_dummy): Create a new call_thread_fsm + instance, associate it with the thread, and wait for the FSM to + finish. If finished successfully, fetch the function's result + value out of the FSM. + * infrun.c (fetch_inferior_event): If the FSM says the stop + shouldn't be notified, don't call normal_stop. + (maybe_remove_breakpoints): New function, factored out from ... + (normal_stop): ... here. Simplify. + * infrun.h (maybe_remove_breakpoints): Declare. + * thread-fsm.c (thread_fsm_should_notify_stop): New function. + (thread-fsm.h) : New field. + (thread_fsm_should_notify_stop): Declare. + 2015-09-09 Pedro Alves * Makefile.in (COMMON_OBS): Add thread-fsm.o. diff --git a/gdb/infcall.c b/gdb/infcall.c index 734d6b5219..f1de10e7c3 100644 --- a/gdb/infcall.c +++ b/gdb/infcall.c @@ -38,6 +38,7 @@ #include "observer.h" #include "top.h" #include "interps.h" +#include "thread-fsm.h" /* If we can't find a function's name from its address, we print this instead. */ @@ -374,6 +375,174 @@ get_function_name (CORE_ADDR funaddr, char *buf, int buf_size) } } +/* All the meta data necessary to extract the call's return value. */ + +struct call_return_meta_info +{ + /* The caller frame's architecture. */ + struct gdbarch *gdbarch; + + /* The called function. */ + struct value *function; + + /* The return value's type. */ + struct type *value_type; + + /* Are we returning a value using a structure return or a normal + value return? */ + int struct_return_p; + + /* If using a structure return, this is the structure's address. */ + CORE_ADDR struct_addr; + + /* Whether stack temporaries are enabled. */ + int stack_temporaries_enabled; +}; + +/* Extract the called function's return value. */ + +static struct value * +get_call_return_value (struct call_return_meta_info *ri) +{ + struct value *retval = NULL; + int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid); + + if (TYPE_CODE (ri->value_type) == TYPE_CODE_VOID) + retval = allocate_value (ri->value_type); + else if (ri->struct_return_p) + { + if (stack_temporaries) + { + retval = value_from_contents_and_address (ri->value_type, NULL, + ri->struct_addr); + push_thread_stack_temporary (inferior_ptid, retval); + } + else + { + retval = allocate_value (ri->value_type); + read_value_memory (retval, 0, 1, ri->struct_addr, + value_contents_raw (retval), + TYPE_LENGTH (ri->value_type)); + } + } + else + { + retval = allocate_value (ri->value_type); + gdbarch_return_value (ri->gdbarch, ri->function, ri->value_type, + get_current_regcache (), + value_contents_raw (retval), NULL); + if (stack_temporaries && class_or_union_p (ri->value_type)) + { + /* Values of class type returned in registers are copied onto + the stack and their lval_type set to lval_memory. This is + required because further evaluation of the expression + could potentially invoke methods on the return value + requiring GDB to evaluate the "this" pointer. To evaluate + the this pointer, GDB needs the memory address of the + value. */ + value_force_lval (retval, ri->struct_addr); + push_thread_stack_temporary (inferior_ptid, retval); + } + } + + gdb_assert (retval != NULL); + return retval; +} + +/* Data for the FSM that manages an infcall. It's main job is to + record the called function's return value. */ + +struct call_thread_fsm +{ + /* The base class. */ + struct thread_fsm thread_fsm; + + /* All the info necessary to be able to extract the return + value. */ + struct call_return_meta_info return_meta_info; + + /* The called function's return value. This is extracted from the + target before the dummy frame is popped. */ + struct value *return_value; +}; + +static int call_thread_fsm_should_stop (struct thread_fsm *self); +static int call_thread_fsm_should_notify_stop (struct thread_fsm *self); + +/* call_thread_fsm's vtable. */ + +static struct thread_fsm_ops call_thread_fsm_ops = +{ + NULL, /*dtor */ + NULL, /* clean_up */ + call_thread_fsm_should_stop, + NULL, /* return_value */ + NULL, /* async_reply_reason*/ + call_thread_fsm_should_notify_stop, +}; + +/* Allocate a new call_thread_fsm object. */ + +static struct call_thread_fsm * +new_call_thread_fsm (struct gdbarch *gdbarch, struct value *function, + struct type *value_type, + int struct_return_p, CORE_ADDR struct_addr) +{ + struct call_thread_fsm *sm; + + sm = XCNEW (struct call_thread_fsm); + thread_fsm_ctor (&sm->thread_fsm, &call_thread_fsm_ops); + + sm->return_meta_info.gdbarch = gdbarch; + sm->return_meta_info.function = function; + sm->return_meta_info.value_type = value_type; + sm->return_meta_info.struct_return_p = struct_return_p; + sm->return_meta_info.struct_addr = struct_addr; + + return sm; +} + +/* Implementation of should_stop method for infcalls. */ + +static int +call_thread_fsm_should_stop (struct thread_fsm *self) +{ + struct call_thread_fsm *f = (struct call_thread_fsm *) self; + + if (stop_stack_dummy == STOP_STACK_DUMMY) + { + /* Done. */ + thread_fsm_set_finished (self); + + /* Stash the return value before the dummy frame is popped and + registers are restored to what they were before the + call.. */ + f->return_value = get_call_return_value (&f->return_meta_info); + + /* Break out of wait_sync_command_done. */ + async_enable_stdin (); + } + + return 1; +} + +/* Implementation of should_notify_stop method for infcalls. */ + +static int +call_thread_fsm_should_notify_stop (struct thread_fsm *self) +{ + if (thread_fsm_finished_p (self)) + { + /* Infcall succeeded. Be silent and proceed with evaluating the + expression. */ + return 0; + } + + /* Something wrong happened. E.g., an unexpected breakpoint + triggered, or a signal was intercepted. Notify the stop. */ + return 1; +} + /* Subroutine of call_function_by_hand to simplify it. Start up the inferior and wait for it to stop. Return the exception if there's an error, or an exception with @@ -383,7 +552,8 @@ get_function_name (CORE_ADDR funaddr, char *buf, int buf_size) thrown errors. The caller should rethrow if there's an error. */ static struct gdb_exception -run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) +run_inferior_call (struct call_thread_fsm *sm, + struct thread_info *call_thread, CORE_ADDR real_pc) { struct gdb_exception caught_error = exception_none; int saved_in_infcall = call_thread->control.in_infcall; @@ -402,6 +572,11 @@ run_inferior_call (struct thread_info *call_thread, CORE_ADDR real_pc) clear_proceed_status (0); + /* Associate the FSM with the thread after clear_proceed_status + (otherwise it'd clear this FSM), and before anything throws, so + we don't leak it (and any resources it manages). */ + call_thread->thread_fsm = &sm->thread_fsm; + disable_watchpoints_before_interactive_call_start (); /* We want to print return value, please... */ @@ -620,8 +795,6 @@ call_function_by_hand_dummy (struct value *function, struct gdb_exception e; char name_buf[RAW_FUNCTION_ADDRESS_SIZE]; int stack_temporaries = thread_stack_temporaries_enabled_p (inferior_ptid); - struct dummy_frame_context_saver *context_saver; - struct cleanup *context_saver_cleanup; if (TYPE_CODE (ftype) == TYPE_CODE_PTR) ftype = check_typedef (TYPE_TARGET_TYPE (ftype)); @@ -1008,13 +1181,6 @@ call_function_by_hand_dummy (struct value *function, register_dummy_frame_dtor (dummy_id, inferior_ptid, dummy_dtor, dummy_dtor_data); - /* dummy_frame_context_saver_setup must be called last so that its - saving of inferior registers gets called first (before possible - DUMMY_DTOR destructor). */ - context_saver = dummy_frame_context_saver_setup (dummy_id, inferior_ptid); - context_saver_cleanup = make_cleanup (dummy_frame_context_saver_cleanup, - context_saver); - /* Register a clean-up for unwind_on_terminating_exception_breakpoint. */ terminate_bp_cleanup = make_cleanup (cleanup_delete_std_terminate_breakpoint, NULL); @@ -1027,6 +1193,12 @@ call_function_by_hand_dummy (struct value *function, in a block so that it's only in scope during the time it's valid. */ { struct thread_info *tp = inferior_thread (); + struct thread_fsm *saved_sm; + struct call_thread_fsm *sm; + + /* Save the current FSM. We'll override it. */ + saved_sm = tp->thread_fsm; + tp->thread_fsm = NULL; /* Save this thread's ptid, we need it later but the thread may have exited. */ @@ -1034,10 +1206,57 @@ call_function_by_hand_dummy (struct value *function, /* Run the inferior until it stops. */ - e = run_inferior_call (tp, real_pc); - } + /* Create the FSM used to manage the infcall. It tells infrun to + not report the stop to the user, and captures the return value + before the dummy frame is popped. run_inferior_call registers + it with the thread ASAP. */ + sm = new_call_thread_fsm (gdbarch, function, + values_type, + struct_return || hidden_first_param_p, + struct_addr); + + e = run_inferior_call (sm, tp, real_pc); + + observer_notify_inferior_call_post (call_thread_ptid, funaddr); + + tp = find_thread_ptid (call_thread_ptid); + if (tp != NULL) + { + /* The FSM should still be the same. */ + gdb_assert (tp->thread_fsm == &sm->thread_fsm); + + if (thread_fsm_finished_p (tp->thread_fsm)) + { + struct value *retval; + + /* The inferior call is successful. Pop the dummy frame, + which runs its destructors and restores the inferior's + suspend state, and restore the inferior control + state. */ + dummy_frame_pop (dummy_id, call_thread_ptid); + restore_infcall_control_state (inf_status); + + /* Get the return value. */ + retval = sm->return_value; + + /* Clean up / destroy the call FSM, and restore the + original one. */ + thread_fsm_clean_up (tp->thread_fsm); + thread_fsm_delete (tp->thread_fsm); + tp->thread_fsm = saved_sm; - observer_notify_inferior_call_post (call_thread_ptid, funaddr); + maybe_remove_breakpoints (); + + do_cleanups (terminate_bp_cleanup); + gdb_assert (retval != NULL); + return retval; + } + + /* Didn't complete. Restore previous state machine, and + handle the error. */ + tp->thread_fsm = saved_sm; + } + } /* Rethrow an error if we got one trying to run the inferior. */ @@ -1119,7 +1338,6 @@ When the function is done executing, GDB will silently stop."), name); } - if (stopped_by_random_signal || stop_stack_dummy != STOP_STACK_DUMMY) { /* Make a copy as NAME may be in an objfile freed by dummy_frame_pop. */ char *name = xstrdup (get_function_name (funaddr, @@ -1221,65 +1439,10 @@ When the function is done executing, GDB will silently stop."), name); } - /* The above code errors out, so ... */ - internal_error (__FILE__, __LINE__, _("... should not be here")); } - do_cleanups (terminate_bp_cleanup); - - /* If we get here the called FUNCTION ran to completion, - and the dummy frame has already been popped. */ - - { - struct value *retval = NULL; - - /* Inferior call is successful. Restore the inferior status. - At this stage, leave the RETBUF alone. */ - restore_infcall_control_state (inf_status); - - if (TYPE_CODE (values_type) == TYPE_CODE_VOID) - retval = allocate_value (values_type); - else if (struct_return || hidden_first_param_p) - { - if (stack_temporaries) - { - retval = value_from_contents_and_address (values_type, NULL, - struct_addr); - push_thread_stack_temporary (inferior_ptid, retval); - } - else - { - retval = allocate_value (values_type); - read_value_memory (retval, 0, 1, struct_addr, - value_contents_raw (retval), - TYPE_LENGTH (values_type)); - } - } - else - { - retval = allocate_value (values_type); - gdbarch_return_value (gdbarch, function, values_type, - dummy_frame_context_saver_get_regs (context_saver), - value_contents_raw (retval), NULL); - if (stack_temporaries && class_or_union_p (values_type)) - { - /* Values of class type returned in registers are copied onto - the stack and their lval_type set to lval_memory. This is - required because further evaluation of the expression - could potentially invoke methods on the return value - requiring GDB to evaluate the "this" pointer. To evaluate - the this pointer, GDB needs the memory address of the - value. */ - value_force_lval (retval, struct_addr); - push_thread_stack_temporary (inferior_ptid, retval); - } - } - - do_cleanups (context_saver_cleanup); - - gdb_assert (retval); - return retval; - } + /* The above code errors out, so ... */ + gdb_assert_not_reached ("... should not be here"); } diff --git a/gdb/infrun.c b/gdb/infrun.c index 73ac090c91..6324fbda25 100644 --- a/gdb/infrun.c +++ b/gdb/infrun.c @@ -3834,6 +3834,7 @@ fetch_inferior_event (void *client_data) struct inferior *inf = find_inferior_ptid (ecs->ptid); int should_stop = 1; struct thread_info *thr = ecs->event_thread; + int should_notify_stop = 1; delete_just_stopped_threads_infrun_breakpoints (); @@ -3853,12 +3854,21 @@ fetch_inferior_event (void *client_data) { clean_up_just_stopped_threads_fsms (ecs); - /* We may not find an inferior if this was a process exit. */ - if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY) - normal_stop (); + if (thr != NULL && thr->thread_fsm != NULL) + { + should_notify_stop + = thread_fsm_should_notify_stop (thr->thread_fsm); + } + + if (should_notify_stop) + { + /* We may not find an inferior if this was a process exit. */ + if (inf == NULL || inf->control.stop_soon == NO_STOP_QUIETLY) + normal_stop (); - inferior_event_handler (INF_EXEC_COMPLETE, NULL); - cmd_done = 1; + inferior_event_handler (INF_EXEC_COMPLETE, NULL); + cmd_done = 1; + } } } @@ -7810,6 +7820,23 @@ print_stop_event (struct ui_out *uiout) } } +/* See infrun.h. */ + +void +maybe_remove_breakpoints (void) +{ + if (!breakpoints_should_be_inserted_now () && target_has_execution) + { + if (remove_breakpoints ()) + { + target_terminal_ours_for_output (); + printf_filtered (_("Cannot remove breakpoints because " + "program is no longer writable.\nFurther " + "execution is probably impossible.\n")); + } + } +} + /* Here to return control to GDB when the inferior stops for real. Print appropriate messages, remove breakpoints, give terminal our modes. @@ -7903,16 +7930,7 @@ normal_stop (void) } /* Note: this depends on the update_thread_list call above. */ - if (!breakpoints_should_be_inserted_now () && target_has_execution) - { - if (remove_breakpoints ()) - { - target_terminal_ours_for_output (); - printf_filtered (_("Cannot remove breakpoints because " - "program is no longer writable.\nFurther " - "execution is probably impossible.\n")); - } - } + maybe_remove_breakpoints (); /* If an auto-display called a function and that got a signal, delete that auto-display to avoid an infinite recursion. */ @@ -7923,87 +7941,50 @@ normal_stop (void) target_terminal_ours (); async_enable_stdin (); - /* Set the current source location. This will also happen if we - display the frame below, but the current SAL will be incorrect - during a user hook-stop function. */ - if (has_stack_frames () && !stop_stack_dummy) - set_current_sal_from_frame (get_current_frame ()); - - /* Let the user/frontend see the threads as stopped, but defer to - call_function_by_hand if the thread finished an infcall - successfully. We may be e.g., evaluating a breakpoint condition. - In that case, the thread had state THREAD_RUNNING before the - infcall, and shall remain marked running, all without informing - the user/frontend about state transition changes. */ - if (target_has_execution - && inferior_thread ()->control.in_infcall - && stop_stack_dummy == STOP_STACK_DUMMY) - discard_cleanups (old_chain); - else - do_cleanups (old_chain); - - /* Look up the hook_stop and run it (CLI internally handles problem - of stop_command's pre-hook not existing). */ - if (stop_command) - catch_errors (hook_stop_stub, stop_command, - "Error while running hook_stop:\n", RETURN_MASK_ALL); + /* Let the user/frontend see the threads as stopped. */ + do_cleanups (old_chain); - if (!has_stack_frames ()) - goto done; + /* Select innermost stack frame - i.e., current frame is frame 0, + and current location is based on that. Handle the case where the + dummy call is returning after being stopped. E.g. the dummy call + previously hit a breakpoint. (If the dummy call returns + normally, we won't reach here.) Do this before the stop hook is + run, so that it doesn't get to see the temporary dummy frame, + which is not where we'll present the stop. */ + if (has_stack_frames ()) + { + if (stop_stack_dummy == STOP_STACK_DUMMY) + { + /* Pop the empty frame that contains the stack dummy. This + also restores inferior state prior to the call (struct + infcall_suspend_state). */ + struct frame_info *frame = get_current_frame (); - if (last.kind == TARGET_WAITKIND_SIGNALLED - || last.kind == TARGET_WAITKIND_EXITED) - goto done; + gdb_assert (get_frame_type (frame) == DUMMY_FRAME); + frame_pop (frame); + /* frame_pop calls reinit_frame_cache as the last thing it + does which means there's now no selected frame. */ + } - /* Select innermost stack frame - i.e., current frame is frame 0, - and current location is based on that. - Don't do this on return from a stack dummy routine, - or if the program has exited. */ - - if (!stop_stack_dummy) - select_frame (get_current_frame ()); - - if (stop_stack_dummy == STOP_STACK_DUMMY) - { - /* Pop the empty frame that contains the stack dummy. - This also restores inferior state prior to the call - (struct infcall_suspend_state). */ - struct frame_info *frame = get_current_frame (); - - gdb_assert (get_frame_type (frame) == DUMMY_FRAME); - frame_pop (frame); - /* frame_pop() calls reinit_frame_cache as the last thing it - does which means there's currently no selected frame. We - don't need to re-establish a selected frame if the dummy call - returns normally, that will be done by - restore_infcall_control_state. However, we do have to handle - the case where the dummy call is returning after being - stopped (e.g. the dummy call previously hit a breakpoint). - We can't know which case we have so just always re-establish - a selected frame here. */ select_frame (get_current_frame ()); - } - -done: - /* Suppress the stop observer if we're in the middle of: + /* Set the current source location. */ + set_current_sal_from_frame (get_current_frame ()); + } - - calling an inferior function, as we pretend we inferior didn't - run at all. The return value of the call is handled by the - expression evaluator, through call_function_by_hand. */ + /* Look up the hook_stop and run it (CLI internally handles problem + of stop_command's pre-hook not existing). */ + if (stop_command) + catch_errors (hook_stop_stub, stop_command, + "Error while running hook_stop:\n", RETURN_MASK_ALL); - if (!target_has_execution - || last.kind == TARGET_WAITKIND_SIGNALLED - || last.kind == TARGET_WAITKIND_EXITED - || last.kind == TARGET_WAITKIND_NO_RESUMED - || !inferior_thread ()->control.in_infcall) - { - if (!ptid_equal (inferior_ptid, null_ptid)) - observer_notify_normal_stop (inferior_thread ()->control.stop_bpstat, - stop_print_frame); - else - observer_notify_normal_stop (NULL, stop_print_frame); - } + /* Notify observers about the stop. This is where the interpreters + print the stop event. */ + if (!ptid_equal (inferior_ptid, null_ptid)) + observer_notify_normal_stop (inferior_thread ()->control.stop_bpstat, + stop_print_frame); + else + observer_notify_normal_stop (NULL, stop_print_frame); annotate_stopped (); diff --git a/gdb/infrun.h b/gdb/infrun.h index 33e729417c..bf2c416c15 100644 --- a/gdb/infrun.h +++ b/gdb/infrun.h @@ -217,4 +217,8 @@ extern void mark_infrun_async_event_handler (void); to get past e.g., a breakpoint. */ extern struct thread_info *step_over_queue_head; +/* Remove breakpoints if possible (usually that means, if everything + is stopped). On failure, print a message. */ +extern void maybe_remove_breakpoints (void); + #endif /* INFRUN_H */ diff --git a/gdb/thread-fsm.c b/gdb/thread-fsm.c index 761ad1cb25..4a27d024a8 100644 --- a/gdb/thread-fsm.c +++ b/gdb/thread-fsm.c @@ -95,3 +95,13 @@ thread_fsm_async_reply_reason (struct thread_fsm *self) return self->ops->async_reply_reason (self); } + +/* See thread-fsm.h. */ + +int +thread_fsm_should_notify_stop (struct thread_fsm *self) +{ + if (self->ops->should_notify_stop != NULL) + return self->ops->should_notify_stop (self); + return 1; +} diff --git a/gdb/thread-fsm.h b/gdb/thread-fsm.h index 031684b27a..c22c51fe7c 100644 --- a/gdb/thread-fsm.h +++ b/gdb/thread-fsm.h @@ -67,6 +67,9 @@ struct thread_fsm_ops /* The async_reply_reason that is broadcast to MI clients if this FSM finishes successfully. */ enum async_reply_reason (*async_reply_reason) (struct thread_fsm *self); + + /* Whether the stop should be notified to the user/frontend. */ + int (*should_notify_stop) (struct thread_fsm *self); }; /* Initialize FSM. */ extern void thread_fsm_ctor (struct thread_fsm *fsm, @@ -95,4 +98,7 @@ extern int thread_fsm_finished_p (struct thread_fsm *fsm); extern enum async_reply_reason thread_fsm_async_reply_reason (struct thread_fsm *fsm); +/* Calls the FSM's should_notify_stop method. */ +extern int thread_fsm_should_notify_stop (struct thread_fsm *self); + #endif /* THREAD_FSM_H */ -- 2.34.1