Improve riscv expose_[csrs|custom] commands (#536)

* Improve riscv expose_[csrs|custom] commands

* Add option to specify custom name for registers.
* Allow to call commands multiple times without loss of previous data.
* Make sure the commands can only be used in the config phase (before "init").
* Validity checks and warnings.
* Change commands to be per target.
* Fix memory leaks.
* Also fix unrelated memory leaks to keep valgrind happy.

Signed-off-by: Samuel Obuch <sobuch@codasip.com>

* fixes after review

* improve error message

doc/openocd.texi

@@ -9641,9 +9641,11 @@ OpenOCD exposes each hart as a separate core.
 
 @subsection RISC-V Debug Configuration Commands
 
-@deffn Command {riscv expose_csrs} n0[-m0][,n1[-m1]]...
+@deffn Command {riscv expose_csrs} n[-m|=name] [...]
-Configure a list of inclusive ranges for CSRs to expose in addition to the
+Configure which CSRs to expose in addition to the standard ones. The CSRs to expose
-standard ones. This must be executed before `init`.
+can be specified as individual register numbers or register ranges (inclusive). For the
+individually listed CSRs, a human-readable name can optionally be set.
+This command must be executed before `init`.
 
 By default OpenOCD attempts to expose only CSRs that are mentioned in a spec,
 and then only if the corresponding extension appears to be implemented. This
@@ -9651,11 +9653,12 @@ command can be used if OpenOCD gets this wrong, or a target implements custom
 CSRs.
 @end deffn
 
-@deffn Command {riscv expose_custom} n0[-m0][,n1[-m1]]...
+@deffn Command {riscv expose_custom} n[-m|=name] [...]
 The RISC-V Debug Specification allows targets to expose custom registers
 through abstract commands. (See Section 3.5.1.1 in that document.) This command
-configures a list of inclusive ranges of those registers to expose. Number 0
+configures individual registers or register ranges (inclusive) that shall be exposed.
-indicates the first custom register, whose abstract command number is 0xc000.
+Number 0 indicates the first custom register, whose abstract command number is 0xc000.
+For individually listed registers, a human-readable name can be optionally provided.
 This command must be executed before `init`.
 @end deffn
 

src/target/riscv/riscv.c

@@ -211,18 +211,6 @@ bool riscv_ebreaku = true;
 
 bool riscv_enable_virtual;
 
-typedef struct {
-	uint16_t low, high;
-} range_t;
-
-/* In addition to the ones in the standard spec, we'll also expose additional
- * CSRs in this list.
- * The list is either NULL, or a series of ranges (inclusive), terminated with
- * 1,0. */
-range_t *expose_csr;
-/* Same, but for custom registers. */
-range_t *expose_custom;
-
 static enum {
 	RO_NORMAL,
 	RO_REVERSED
@@ -482,16 +470,29 @@ static void riscv_free_registers(struct target *target)
 static void riscv_deinit_target(struct target *target)
 {
 	LOG_DEBUG("riscv_deinit_target()");
+
+	riscv_info_t *info = (riscv_info_t *) target->arch_info;
 	struct target_type *tt = get_target_type(target);
-	if (tt) {
+
+	if (tt && info->version_specific)
 		tt->deinit_target(target);
-		riscv_info_t *info = (riscv_info_t *) target->arch_info;
-		free(info->reg_names);
-		free(info);
-	}
 
 	riscv_free_registers(target);
 
+	range_list_t *entry, *tmp;
+	list_for_each_entry_safe(entry, tmp, &info->expose_csr, list) {
+		free(entry->name);
+		free(entry);
+	}
+
+	list_for_each_entry_safe(entry, tmp, &info->expose_custom, list) {
+		free(entry->name);
+		free(entry);
+	}
+
+	free(info->reg_names);
+	free(target->arch_info);
+
 	target->arch_info = NULL;
 }
 
@@ -2454,103 +2455,167 @@ COMMAND_HANDLER(riscv_set_enable_virtual)
 	return ERROR_OK;
 }
 
-void parse_error(const char *string, char c, unsigned position)
+int parse_ranges(struct list_head *ranges, const char *tcl_arg, const char *reg_type, unsigned max_val)
 {
-	char buf[position+2];
+	char *args = strdup(tcl_arg);
-	for (unsigned i = 0; i < position; i++)
+	if (!args)
-		buf[i] = ' ';
+		return ERROR_FAIL;
-	buf[position] = '^';
-	buf[position + 1] = 0;
 
-	LOG_ERROR("Parse error at character %c in:", c);
+	/* For backward compatibility, allow multiple parameters within one TCL argument, separated by ',' */
-	LOG_ERROR("%s", string);
+	char *arg = strtok(args, ",");
-	LOG_ERROR("%s", buf);
+	while (arg) {
-}
-
-int parse_ranges(range_t **ranges, const char **argv)
-{
-	for (unsigned pass = 0; pass < 2; pass++) {
-		unsigned range = 0;
 		unsigned low = 0;
-		bool parse_low = true;
 		unsigned high = 0;
-		for (unsigned i = 0; i == 0 || argv[0][i-1]; i++) {
+		char *name = NULL;
-			char c = argv[0][i];
+
-			if (isspace(c)) {
+		char *dash = strchr(arg, '-');
-				/* Ignore whitespace. */
+		char *equals = strchr(arg, '=');
-				continue;
+		unsigned pos;
+
+		if (!dash && !equals) {
+			/* Expecting single register number. */
+			if (sscanf(arg, "%u%n", &low, &pos) != 1 || pos != strlen(arg)) {
+				LOG_ERROR("Failed to parse single register number from '%s'.", arg);
+				free(args);
+				return ERROR_COMMAND_SYNTAX_ERROR;
 			}
-
+		} else if (dash && !equals) {
-			if (parse_low) {
+			/* Expecting register range - two numbers separated by a dash: ##-## */
-				if (isdigit(c)) {
+			*dash = 0;
-					low *= 10;
+			dash++;
-					low += c - '0';
+			if (sscanf(arg, "%u%n", &low, &pos) != 1 || pos != strlen(arg)) {
-				} else if (c == '-') {
+				LOG_ERROR("Failed to parse single register number from '%s'.", arg);
-					parse_low = false;
+				free(args);
-				} else if (c == ',' || c == 0) {
+				return ERROR_COMMAND_SYNTAX_ERROR;
-					if (pass == 1) {
-						(*ranges)[range].low = low;
-						(*ranges)[range].high = low;
-					}
-					low = 0;
-					range++;
-				} else {
-					parse_error(argv[0], c, i);
-					return ERROR_COMMAND_SYNTAX_ERROR;
-				}
-
-			} else {
-				if (isdigit(c)) {
-					high *= 10;
-					high += c - '0';
-				} else if (c == ',' || c == 0) {
-					parse_low = true;
-					if (pass == 1) {
-						(*ranges)[range].low = low;
-						(*ranges)[range].high = high;
-					}
-					low = 0;
-					high = 0;
-					range++;
-				} else {
-					parse_error(argv[0], c, i);
-					return ERROR_COMMAND_SYNTAX_ERROR;
-				}
 			}
-		}
+			if (sscanf(dash, "%u%n", &high, &pos) != 1 || pos != strlen(dash)) {
-
+				LOG_ERROR("Failed to parse single register number from '%s'.", dash);
-		if (pass == 0) {
+				free(args);
-			free(*ranges);
+				return ERROR_COMMAND_SYNTAX_ERROR;
-			*ranges = calloc(range + 2, sizeof(range_t));
+			}
-			if (!*ranges)
+			if (high < low) {
+				LOG_ERROR("Incorrect range encountered [%u, %u].", low, high);
+				free(args);
 				return ERROR_FAIL;
+			}
+		} else if (!dash && equals) {
+			/* Expecting single register number with textual name specified: ##=name */
+			*equals = 0;
+			equals++;
+			if (sscanf(arg, "%u%n", &low, &pos) != 1 || pos != strlen(arg)) {
+				LOG_ERROR("Failed to parse single register number from '%s'.", arg);
+				free(args);
+				return ERROR_COMMAND_SYNTAX_ERROR;
+			}
+
+			name = calloc(1, strlen(equals) + strlen(reg_type) + 2);
+			if (!name) {
+				free(args);
+				return ERROR_FAIL;
+			}
+
+			/* Register prefix: "csr_" or "custom_" */
+			strcpy(name, reg_type);
+			name[strlen(reg_type)] = '_';
+
+			if (sscanf(equals, "%[_a-zA-Z0-9]%n", name + strlen(reg_type) + 1, &pos) != 1 || pos != strlen(equals)) {
+				LOG_ERROR("Failed to parse register name from '%s'.", equals);
+				free(args);
+				free(name);
+				return ERROR_COMMAND_SYNTAX_ERROR;
+			}
 		} else {
-			(*ranges)[range].low = 1;
+			LOG_ERROR("Invalid argument '%s'.", arg);
-			(*ranges)[range].high = 0;
+			free(args);
+			return ERROR_COMMAND_SYNTAX_ERROR;
 		}
+
+		high = high > low ? high : low;
+
+		if (high > max_val) {
+			LOG_ERROR("Cannot expose %s register number %u, maximum allowed value is %u.", reg_type, high, max_val);
+			free(name);
+			free(args);
+			return ERROR_FAIL;
+		}
+
+		/* Check for overlap, name uniqueness. */
+		range_list_t *entry;
+		list_for_each_entry(entry, ranges, list) {
+			if ((entry->low <= high) && (low <= entry->high)) {
+				if (low == high)
+					LOG_WARNING("Duplicate %s register number - "
+							"Register %u has already been exposed previously", reg_type, low);
+				else
+					LOG_WARNING("Overlapping register ranges - Register range starting from %u overlaps "
+							"with already exposed register/range at %u.", low, entry->low);
+			}
+
+			if (entry->name && name && (strcasecmp(entry->name, name) == 0)) {
+				LOG_ERROR("Duplicate register name \"%s\" found.", name);
+				free(name);
+				free(args);
+				return ERROR_FAIL;
+			}
+		}
+
+		range_list_t *range = calloc(1, sizeof(range_list_t));
+		if (!range) {
+			free(name);
+			free(args);
+			return ERROR_FAIL;
+		}
+
+		range->low = low;
+		range->high = high;
+		range->name = name;
+		list_add(&range->list, ranges);
+
+		arg = strtok(NULL, ",");
 	}
 
+	free(args);
 	return ERROR_OK;
 }
 
 COMMAND_HANDLER(riscv_set_expose_csrs)
 {
-	if (CMD_ARGC != 1) {
+	if (CMD_ARGC == 0) {
-		LOG_ERROR("Command takes exactly 1 parameter");
+		LOG_ERROR("Command expects parameters");
 		return ERROR_COMMAND_SYNTAX_ERROR;
 	}
 
-	return parse_ranges(&expose_csr, CMD_ARGV);
+	struct target *target = get_current_target(CMD_CTX);
+	RISCV_INFO(info);
+	int ret = ERROR_OK;
+
+	for (unsigned i = 0; i < CMD_ARGC; i++) {
+		ret = parse_ranges(&info->expose_csr, CMD_ARGV[i], "csr", 0xfff);
+		if (ret != ERROR_OK)
+			break;
+	}
+
+	return ret;
 }
 
 COMMAND_HANDLER(riscv_set_expose_custom)
 {
-	if (CMD_ARGC != 1) {
+	if (CMD_ARGC == 0) {
-		LOG_ERROR("Command takes exactly 1 parameter");
+		LOG_ERROR("Command expects parameters");
 		return ERROR_COMMAND_SYNTAX_ERROR;
 	}
 
-	return parse_ranges(&expose_custom, CMD_ARGV);
+	struct target *target = get_current_target(CMD_CTX);
+	RISCV_INFO(info);
+	int ret = ERROR_OK;
+
+	for (unsigned i = 0; i < CMD_ARGC; i++) {
+		ret = parse_ranges(&info->expose_custom, CMD_ARGV[i], "custom", 0x3fff);
+		if (ret != ERROR_OK)
+			break;
+	}
+
+	return ret;
 }
 
 COMMAND_HANDLER(riscv_authdata_read)
@@ -2910,8 +2975,8 @@ static const struct command_registration riscv_exec_command_handlers[] = {
 	{
 		.name = "expose_csrs",
 		.handler = riscv_set_expose_csrs,
-		.mode = COMMAND_ANY,
+		.mode = COMMAND_CONFIG,
-		.usage = "n0[-m0][,n1[-m1]]...",
+		.usage = "n0[-m0|=name0][,n1[-m1|=name1]]...",
 		.help = "Configure a list of inclusive ranges for CSRs to expose in "
 				"addition to the standard ones. This must be executed before "
 				"`init`."
@@ -2919,8 +2984,8 @@ static const struct command_registration riscv_exec_command_handlers[] = {
 	{
 		.name = "expose_custom",
 		.handler = riscv_set_expose_custom,
-		.mode = COMMAND_ANY,
+		.mode = COMMAND_CONFIG,
-		.usage = "n0[-m0][,n1[-m1]]...",
+		.usage = "n0[-m0|=name0][,n1[-m1|=name1]]...",
 		.help = "Configure a list of inclusive ranges for custom registers to "
 			"expose. custom0 is accessed as abstract register number 0xc000, "
 			"etc. This must be executed before `init`."
@@ -3154,6 +3219,9 @@ void riscv_info_init(struct target *target, riscv_info_t *r)
 	r->mem_access_progbuf_warn = true;
 	r->mem_access_sysbus_warn = true;
 	r->mem_access_abstract_warn = true;
+
+	INIT_LIST_HEAD(&r->expose_csr);
+	INIT_LIST_HEAD(&r->expose_custom);
 }
 
 static int riscv_resume_go_all_harts(struct target *target)
@@ -3886,13 +3954,10 @@ int riscv_init_registers(struct target *target)
 	target->reg_cache->name = "RISC-V Registers";
 	target->reg_cache->num_regs = GDB_REGNO_COUNT;
 
-	if (expose_custom) {
+	if (!list_empty(&info->expose_custom)) {
-		for (unsigned i = 0; expose_custom[i].low <= expose_custom[i].high; i++) {
+		range_list_t *entry;
-			for (unsigned number = expose_custom[i].low;
+		list_for_each_entry(entry, &info->expose_custom, list)
-					number <= expose_custom[i].high;
+			target->reg_cache->num_regs += entry->high - entry->low + 1;
-					number++)
-				target->reg_cache->num_regs++;
-		}
 	}
 
 	LOG_DEBUG("create register cache for %d registers",
@@ -4052,7 +4117,6 @@ int riscv_init_registers(struct target *target)
 	qsort(csr_info, DIM(csr_info), sizeof(*csr_info), cmp_csr_info);
 	unsigned csr_info_index = 0;
 
-	unsigned custom_range_index = 0;
 	int custom_within_range = 0;
 
 	riscv_reg_info_t *shared_reg_info = calloc(1, sizeof(riscv_reg_info_t));
@@ -4431,14 +4495,21 @@ int riscv_init_registers(struct target *target)
 					break;
 			}
 
-			if (!r->exist && expose_csr) {
+			if (!r->exist && !list_empty(&info->expose_csr)) {
-				for (unsigned i = 0; expose_csr[i].low <= expose_csr[i].high; i++) {
+				range_list_t *entry;
-					if (csr_number >= expose_csr[i].low && csr_number <= expose_csr[i].high) {
+				list_for_each_entry(entry, &info->expose_csr, list)
-						LOG_INFO("Exposing additional CSR %d", csr_number);
+					if ((entry->low <= csr_number) && (csr_number <= entry->high)) {
+						if (entry->name) {
+							*reg_name = 0;
+							r->name = entry->name;
+						}
+
+						LOG_DEBUG("Exposing additional CSR %d (name=%s)",
+								csr_number, entry->name ? entry->name : reg_name);
+
 						r->exist = true;
 						break;
 					}
-				}
 			}
 
 		} else if (number == GDB_REGNO_PRIV) {
@@ -4458,10 +4529,10 @@ int riscv_init_registers(struct target *target)
 
 		} else if (number >= GDB_REGNO_COUNT) {
 			/* Custom registers. */
-			assert(expose_custom);
+			assert(!list_empty(&info->expose_custom));
+
+			range_list_t *range = list_first_entry(&info->expose_custom, range_list_t, list);
 
-			range_t *range = &expose_custom[custom_range_index];
-			assert(range->low <= range->high);
 			unsigned custom_number = range->low + custom_within_range;
 
 			r->group = "custom";
@@ -4473,18 +4544,27 @@ int riscv_init_registers(struct target *target)
 			((riscv_reg_info_t *) r->arch_info)->custom_number = custom_number;
 			sprintf(reg_name, "custom%d", custom_number);
 
+			if (range->name) {
+				*reg_name = 0;
+				r->name = range->name;
+			}
+
+			LOG_DEBUG("Exposing additional custom register %d (name=%s)",
+					number, range->name ? range->name : reg_name);
+
 			custom_within_range++;
 			if (custom_within_range > range->high - range->low) {
 				custom_within_range = 0;
-				custom_range_index++;
+				list_rotate_left(&info->expose_custom);
 			}
 		}
 
-		if (reg_name[0])
+		if (reg_name[0]) {
 			r->name = reg_name;
-		reg_name += strlen(reg_name) + 1;
+			reg_name += strlen(reg_name) + 1;
-		assert(reg_name < info->reg_names + target->reg_cache->num_regs *
+			assert(reg_name < info->reg_names + target->reg_cache->num_regs *
-				max_reg_name_len);
+					max_reg_name_len);
+		}
 		r->value = &info->reg_cache_values[number];
 	}
 

src/target/riscv/riscv.h

@@ -60,6 +60,12 @@ typedef struct {
 	unsigned custom_number;
 } riscv_reg_info_t;
 
+typedef struct {
+	struct list_head list;
+	uint16_t low, high;
+	char *name;
+} range_list_t;
+
 typedef struct {
 	unsigned dtm_version;
 
@@ -197,6 +203,14 @@ typedef struct {
 	bool mem_access_progbuf_warn;
 	bool mem_access_sysbus_warn;
 	bool mem_access_abstract_warn;
+
+	/* In addition to the ones in the standard spec, we'll also expose additional
+	 * CSRs in this list. */
+	struct list_head expose_csr;
+	/* Same, but for custom registers.
+	 * Custom registers are for non-standard extensions and use abstract register numbers
+	 * from range 0xc000 ... 0xffff. */
+	struct list_head expose_custom;
 } riscv_info_t;
 
 typedef struct {