[PATCH]: c4e746075e server: rtos: don't fake step for hwthread rtos.

[<ge...@op...>]

This is an automated email from Gerrit.

"Evgeniy Naydanov <evg...@sy...>" just uploaded a new patch set to Gerrit, which you can find at https://review.openocd.org/c/openocd/+/9177

-- gerrit

commit c4e746075e71467c38db03c7c0fe8bfb0eb23444
Author: Tim Newsome <ti...@si...>
Date:   Wed Aug 14 11:56:44 2019 -0700

    server: rtos: don't fake step for hwthread rtos.
    
    This is a cherry-pick of:
    Link: https://github.com/riscv-collab/riscv-openocd/commit/efce094b409179acbaa7726c112a10fcf8343503
    
    Fake step is a hack introduced to make things work with real RTOSs that
    have a concept of a current thread. The hwthread rtos always has access
    to all threads, so doesn't need it.
    
    This fixes a bug when running my MulticoreRegTest against HiFive
    Unleashed where OpenOCD would return the registers of the wrong thread
    after gdb stepped a hart.
    
    Change-Id: I64f538a133fb078c05a0c6b8121388b0b9d7f1b8
    Signed-off-by: Tim Newsome <ti...@si...>

diff --git a/src/rtos/hwthread.c b/src/rtos/hwthread.c
index b2a45ade9f..f7617ead7a 100644
--- a/src/rtos/hwthread.c
+++ b/src/rtos/hwthread.c
@@ -28,6 +28,7 @@ static int hwthread_read_buffer(struct rtos *rtos, target_addr_t address,
 		uint32_t size, uint8_t *buffer);
 static int hwthread_write_buffer(struct rtos *rtos, target_addr_t address,
 		uint32_t size, const uint8_t *buffer);
+static bool hwthread_needs_fake_step(struct target *target, int64_t thread_id);
 
 #define HW_THREAD_NAME_STR_SIZE (32)
 
@@ -59,6 +60,7 @@ const struct rtos_type hwthread_rtos = {
 	.set_reg = hwthread_set_reg,
 	.read_buffer = hwthread_read_buffer,
 	.write_buffer = hwthread_write_buffer,
+	.needs_fake_step = hwthread_needs_fake_step
 };
 
 struct hwthread_params {
@@ -446,3 +448,8 @@ static int hwthread_write_buffer(struct rtos *rtos, target_addr_t address,
 
 	return target_write_buffer(curr, address, size, buffer);
 }
+
+bool hwthread_needs_fake_step(struct target *target, int64_t thread_id)
+{
+	return false;
+}
diff --git a/src/rtos/rtos.c b/src/rtos/rtos.c
index 2ccccf1b0d..dc2b83ee82 100644
--- a/src/rtos/rtos.c
+++ b/src/rtos/rtos.c
@@ -730,3 +730,10 @@ int rtos_write_buffer(struct target *target, target_addr_t address,
 		return target->rtos->type->write_buffer(target->rtos, address, size, buffer);
 	return ERROR_NOT_IMPLEMENTED;
 }
+
+bool rtos_needs_fake_step(struct target *target, int64_t thread_id)
+{
+	if (target->rtos->type->needs_fake_step)
+		return target->rtos->type->needs_fake_step(target, thread_id);
+	return target->rtos->current_thread != thread_id;
+}
diff --git a/src/rtos/rtos.h b/src/rtos/rtos.h
index dbaa7e8ce8..c0a39771aa 100644
--- a/src/rtos/rtos.h
+++ b/src/rtos/rtos.h
@@ -77,6 +77,13 @@ struct rtos_type {
 			uint8_t *buffer);
 	int (*write_buffer)(struct rtos *rtos, target_addr_t address, uint32_t size,
 			const uint8_t *buffer);
+	/**
+	 * Possibly work around an annoying gdb behaviour: when the current thread
+	 * is changed in gdb, it assumes that the target can follow and also make
+	 * the thread current. This is an assumption that cannot hold for a real
+	 * target running a multi-threading OS. If an RTOS can do this, override
+	 * needs_fake_step(). */
+	bool (*needs_fake_step)(struct target *target, int64_t thread_id);
 };
 
 struct stack_register_offset {
@@ -135,6 +142,7 @@ int rtos_read_buffer(struct target *target, target_addr_t address,
 		uint32_t size, uint8_t *buffer);
 int rtos_write_buffer(struct target *target, target_addr_t address,
 		uint32_t size, const uint8_t *buffer);
+bool rtos_needs_fake_step(struct target *target, int64_t thread_id);
 
 // Keep in alphabetic order this list of rtos
 extern const struct rtos_type chibios_rtos;
diff --git a/src/server/gdb_server.c b/src/server/gdb_server.c
index 080e3360ae..0cef8a8ba5 100644
--- a/src/server/gdb_server.c
+++ b/src/server/gdb_server.c
@@ -3075,8 +3075,21 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p
 		}
 
 		if (target->rtos) {
-			/* FIXME: why is this necessary? rtos state should be up-to-date here already! */
-			rtos_update_threads(target);
+			/* Sometimes this results in picking a different thread than
+			 * gdb just requested to step. Then we fake it, and now there's
+			 * a different thread selected than gdb expects, so register
+			 * accesses go to the wrong one!
+			 * E.g.:
+			 * Hg1$
+			 * P8=72101ce197869329$		# write r8 on thread 1
+			 * g$
+			 * vCont?$
+			 * vCont;s:1;c$				# rtos_update_threads changes to other thread
+			 * g$
+			 * qXfer:threads:read::0,fff$
+			 * P8=cc060607eb89ca7f$		# write r8 on other thread
+			 * g$
+			 * */
 
 			target->rtos->gdb_target_for_threadid(connection, thread_id, &ct);
 
@@ -3084,8 +3097,7 @@ static bool gdb_handle_vcont_packet(struct connection *connection, const char *p
 			 * check if the thread to be stepped is the current rtos thread
 			 * if not, we must fake the step
 			 */
-			if (target->rtos->current_thread != thread_id)
-				fake_step = true;
+			fake_step = rtos_needs_fake_step(target, thread_id);
 		}
 
 		if (parse[0] == ';') {

--