From 5c26fd7ab8916d25cb876487d39ec45e3c936d55 Mon Sep 17 00:00:00 2001 From: Antonio Borneo Date: Mon, 31 Jan 2022 10:08:29 +0100 Subject: [PATCH] gdb_server: simplify logic to enable/disable gdb_log_callback() GDB client cannot always display generic messages from OpenOCD. The callback gdb_log_callback() is continuously added and removed to follow the GDB status and thus enabling/disabling sending the OpenOCD output to GDB. While this is a nice stress test for log_{add,remove}_callback(), it is also a waste of computational resources that could impact the speed of OpenOCD during GDB user interactions. Add a connection-level flag to enable/disable the log callback and simply change the flag instead of adding/removing the callback. Use an enum for the flag instead of a bool. This improves code readability and allows setting other states, e.g. keep-alive through asynchronous notification https://review.openocd.org/4828/ Change-Id: I072d3c6928dedfd0cef0abe7acf9bdd4b89dbf5b Signed-off-by: Antonio Borneo Reviewed-on: https://review.openocd.org/c/openocd/+/6839 Reviewed-by: Tomas Vanek Tested-by: jenkins --- src/server/gdb_server.c | 42 ++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c index b76a3ef7d..b47e312a3 100644 --- a/src/server/gdb_server.c +++ b/src/server/gdb_server.c @@ -61,6 +61,13 @@ * found in most modern embedded processors. */ +enum gdb_output_flag { + /* GDB doesn't accept 'O' packets */ + GDB_OUTPUT_NO, + /* GDB accepts 'O' packets */ + GDB_OUTPUT_ALL, +}; + struct target_desc_format { char *tdesc; uint32_t tdesc_length; @@ -97,6 +104,8 @@ struct gdb_connection { struct target_desc_format target_desc; /* temporarily used for thread list support */ char *thread_list; + /* flag to mask the output from gdb_log_callback() */ + enum gdb_output_flag output_flag; }; #if 0 @@ -478,7 +487,7 @@ static int gdb_put_packet_inner(struct connection *connection, break; else if (reply == '-') { /* Stop sending output packets for now */ - log_remove_callback(gdb_log_callback, connection); + gdb_con->output_flag = GDB_OUTPUT_NO; LOG_WARNING("negative reply, retrying"); } else if (reply == 0x3) { gdb_con->ctrl_c = true; @@ -489,7 +498,7 @@ static int gdb_put_packet_inner(struct connection *connection, break; else if (reply == '-') { /* Stop sending output packets for now */ - log_remove_callback(gdb_log_callback, connection); + gdb_con->output_flag = GDB_OUTPUT_NO; LOG_WARNING("negative reply, retrying"); } else if (reply == '$') { LOG_ERROR("GDB missing ack(1) - assumed good"); @@ -932,7 +941,7 @@ static void gdb_frontend_halted(struct target *target, struct connection *connec */ if (gdb_connection->frontend_state == TARGET_RUNNING) { /* stop forwarding log packets! */ - log_remove_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_NO; /* check fileio first */ if (target_get_gdb_fileio_info(target, target->fileio_info) == ERROR_OK) @@ -992,6 +1001,7 @@ static int gdb_new_connection(struct connection *connection) gdb_connection->target_desc.tdesc = NULL; gdb_connection->target_desc.tdesc_length = 0; gdb_connection->thread_list = NULL; + gdb_connection->output_flag = GDB_OUTPUT_NO; /* send ACK to GDB for debug request */ gdb_write(connection, "+", 1); @@ -1076,6 +1086,8 @@ static int gdb_new_connection(struct connection *connection) * register callback to be informed about target events */ target_register_event_callback(gdb_target_callback_event_handler, connection); + log_add_callback(gdb_log_callback, connection); + return ERROR_OK; } @@ -2728,7 +2740,7 @@ static int gdb_query_packet(struct connection *connection, cmd[len] = 0; /* We want to print all debug output to GDB connection */ - log_add_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_ALL; target_call_timer_callbacks_now(); /* some commands need to know the GDB connection, make note of current * GDB connection. */ @@ -2736,7 +2748,7 @@ static int gdb_query_packet(struct connection *connection, command_run_line(cmd_ctx, cmd); current_gdb_connection = NULL; target_call_timer_callbacks_now(); - log_remove_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_NO; free(cmd); } gdb_put_packet(connection, "OK", 2); @@ -2917,7 +2929,7 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p if (parse[0] == 'c') { gdb_running_type = 'c'; LOG_DEBUG("target %s continue", target_name(target)); - log_add_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_ALL; retval = target_resume(target, 1, 0, 0, 0); if (retval == ERROR_TARGET_NOT_HALTED) LOG_INFO("target %s was not halted when resume was requested", target_name(target)); @@ -3006,7 +3018,7 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p } LOG_DEBUG("target %s single-step thread %"PRIx64, target_name(ct), thread_id); - log_add_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_ALL; target_call_event_callbacks(ct, TARGET_EVENT_GDB_START); /* @@ -3027,7 +3039,7 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p "T05thread:%016"PRIx64";", thread_id); gdb_put_packet(connection, sig_reply, sig_reply_len); - log_remove_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_NO; return true; } @@ -3039,7 +3051,7 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p LOG_DEBUG("stepi ignored. GDB will now fetch the register state " "from the target."); gdb_sig_halted(connection); - log_remove_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_NO; } else gdb_connection->frontend_state = TARGET_RUNNING; return true; @@ -3057,7 +3069,7 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p /* send back signal information */ gdb_signal_reply(ct, connection); /* stop forwarding log packets! */ - log_remove_callback(gdb_log_callback, connection); + gdb_connection->output_flag = GDB_OUTPUT_NO; } else gdb_connection->frontend_state = TARGET_RUNNING; } else { @@ -3387,6 +3399,10 @@ static void gdb_log_callback(void *priv, const char *file, unsigned line, struct connection *connection = priv; struct gdb_connection *gdb_con = connection->priv; + if (gdb_con->output_flag == GDB_OUTPUT_NO) + /* No out allowed */ + return; + if (gdb_con->busy) { /* do not reply this using the O packet */ return; @@ -3489,7 +3505,7 @@ static int gdb_input_inner(struct connection *connection) case 's': { gdb_thread_packet(connection, packet, packet_size); - log_add_callback(gdb_log_callback, connection); + gdb_con->output_flag = GDB_OUTPUT_ALL; if (gdb_con->mem_write_error) { LOG_ERROR("Memory write failure!"); @@ -3532,7 +3548,7 @@ static int gdb_input_inner(struct connection *connection) gdb_sig_halted(connection); /* stop forwarding log packets! */ - log_remove_callback(gdb_log_callback, connection); + gdb_con->output_flag = GDB_OUTPUT_NO; } else { /* We're running/stepping, in which case we can * forward log output until the target is halted @@ -3604,7 +3620,7 @@ static int gdb_input_inner(struct connection *connection) * Fretcode,errno,Ctrl-C flag;call-specific attachment */ gdb_con->frontend_state = TARGET_RUNNING; - log_add_callback(gdb_log_callback, connection); + gdb_con->output_flag = GDB_OUTPUT_ALL; gdb_fileio_response_packet(connection, packet, packet_size); break;