[RFC] WIP: Refactor commands and command argument parsing

Message ID 20180530042617.16753-1-alistair@popple.id.au
State RFC
Headers show
Series
  • [RFC] WIP: Refactor commands and command argument parsing
Related show

Commit Message

Alistair Popple May 30, 2018, 4:26 a.m.
---
 Makefile.am  |   2 +-
 src/cfam.c   |  52 ++++++----------------
 src/main.c   |  49 ++++++---------------
 src/opt.c    | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 src/opt.h    |  96 +++++++++++++++++++++++++++++++++++++++++
 src/reg.c    | 137 +++++++++++++++++++---------------------------------------
 src/ring.c   |  32 +++-----------
 src/scom.c   |  51 +++++-----------------
 src/thread.c |  35 ++++++---------
 9 files changed, 334 insertions(+), 259 deletions(-)
 create mode 100644 src/opt.c
 create mode 100644 src/opt.h

Comments

Balbir Singh May 30, 2018, 8:57 a.m. | #1
On Wed, May 30, 2018 at 2:26 PM, Alistair Popple <alistair@popple.id.au> wrote:
> ---
>  Makefile.am  |   2 +-
>  src/cfam.c   |  52 ++++++----------------
>  src/main.c   |  49 ++++++---------------
>  src/opt.c    | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/opt.h    |  96 +++++++++++++++++++++++++++++++++++++++++
>  src/reg.c    | 137 +++++++++++++++++++---------------------------------------
>  src/ring.c   |  32 +++-----------
>  src/scom.c   |  51 +++++-----------------
>  src/thread.c |  35 ++++++---------
>  9 files changed, 334 insertions(+), 259 deletions(-)
>  create mode 100644 src/opt.c
>  create mode 100644 src/opt.h

Without a changelog it's really hard to review the
intention/improvement. Having said that print_cmds_usage() is ugliest
function I've seen written in all of pdbg (please find a better way to
do this). There's a bit of type dependency in parsing, but I guess
it's the lack of proper type inference semantics.

Balbir Singh.
Alistair Popple May 31, 2018, 1:56 a.m. | #2
On Wednesday, 30 May 2018 6:57:40 PM AEST Balbir Singh wrote:
> On Wed, May 30, 2018 at 2:26 PM, Alistair Popple <alistair@popple.id.au> wrote:
> > ---
> >  Makefile.am  |   2 +-
> >  src/cfam.c   |  52 ++++++----------------
> >  src/main.c   |  49 ++++++---------------
> >  src/opt.c    | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  src/opt.h    |  96 +++++++++++++++++++++++++++++++++++++++++
> >  src/reg.c    | 137 +++++++++++++++++++---------------------------------------
> >  src/ring.c   |  32 +++-----------
> >  src/scom.c   |  51 +++++-----------------
> >  src/thread.c |  35 ++++++---------
> >  9 files changed, 334 insertions(+), 259 deletions(-)
> >  create mode 100644 src/opt.c
> >  create mode 100644 src/opt.h
> 
> Without a changelog it's really hard to review the
> intention/improvement. Having said that print_cmds_usage() is ugliest
> function I've seen written in all of pdbg (please find a better way to
> do this).

Oh be fair ... I found this yesterday:

	while ((!((val & HTM_MEM_SIZE_SMALL) && i>>20 == 16)) && (!(!(val & HTM_MEM_SIZE_SMALL) && i>>20 == 512))) {

Surely that must be in the running :-)

But yeah, that is something I will cleanup before posting a non-WIP version.

> There's a bit of type dependency in parsing, but I guess
> it's the lack of proper type inference semantics.

Sorry, I should've included a better changelog.

The intent here is to centralise command argument parsing so that we don't have
code like this copied everywhere:

int handle_spr(int optind, int argc, char *argv[])
{
        char *endptr;
        uint64_t spr;

        if (optind + 1 >= argc) {
                printf("%s: command '%s' requires a GPR\n", argv[0], argv[optind]);
                return -1;
        }

        errno = 0;
        spr = strtoull(argv[optind + 1], &endptr, 0);
        if (errno || *endptr != '\0') {
                printf("%s: command '%s' couldn't parse GPR '%s'\n",
                                argv[0], argv[optind], argv[optind + 1]);
                return -1;
        }

Which is not only prone to copy-and-paste bugs (have you spotted the error in
the above?) but also a pain to maintain/change due to the multiple copies. We
could certainly refactor that somewhat to reduce code duplication and that's the
rabbit hole I started down to end up with:

int parse_spr(char *arg)
{
...
}

int getspr(int spr) {
...
};
DEFINE_CMD1(getspr, spr);

Where `spr` specifies the parser for converting the "char *" command argument
into a "int spr" is `parse_spr`. Note that it is in fact type safe as well -
changing parse_spr() to return a long int instead of an int for example will
result in a grokable compiler error.

- Alistair

> Balbir Singh.
>
Balbir Singh May 31, 2018, 2:45 p.m. | #3
On Thu, May 31, 2018 at 11:56 AM, Alistair Popple <alistair@popple.id.au> wrote:
> On Wednesday, 30 May 2018 6:57:40 PM AEST Balbir Singh wrote:
>> On Wed, May 30, 2018 at 2:26 PM, Alistair Popple <alistair@popple.id.au> wrote:
>> > ---
>> >  Makefile.am  |   2 +-
>> >  src/cfam.c   |  52 ++++++----------------
>> >  src/main.c   |  49 ++++++---------------
>> >  src/opt.c    | 139 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> >  src/opt.h    |  96 +++++++++++++++++++++++++++++++++++++++++
>> >  src/reg.c    | 137 +++++++++++++++++++---------------------------------------
>> >  src/ring.c   |  32 +++-----------
>> >  src/scom.c   |  51 +++++-----------------
>> >  src/thread.c |  35 ++++++---------
>> >  9 files changed, 334 insertions(+), 259 deletions(-)
>> >  create mode 100644 src/opt.c
>> >  create mode 100644 src/opt.h
>>
>> Without a changelog it's really hard to review the
>> intention/improvement. Having said that print_cmds_usage() is ugliest
>> function I've seen written in all of pdbg (please find a better way to
>> do this).
>
> Oh be fair ... I found this yesterday:
>
>         while ((!((val & HTM_MEM_SIZE_SMALL) && i>>20 == 16)) && (!(!(val & HTM_MEM_SIZE_SMALL) && i>>20 == 512))) {
>
> Surely that must be in the running :-)
>

Surely that wins, sigh!

> But yeah, that is something I will cleanup before posting a non-WIP version.
>
>> There's a bit of type dependency in parsing, but I guess
>> it's the lack of proper type inference semantics.
>
> Sorry, I should've included a better changelog.
>
> The intent here is to centralise command argument parsing so that we don't have
> code like this copied everywhere:
>
> int handle_spr(int optind, int argc, char *argv[])
> {
>         char *endptr;
>         uint64_t spr;
>
>         if (optind + 1 >= argc) {
>                 printf("%s: command '%s' requires a GPR\n", argv[0], argv[optind]);
>                 return -1;
>         }
>
>         errno = 0;
>         spr = strtoull(argv[optind + 1], &endptr, 0);
>         if (errno || *endptr != '\0') {
>                 printf("%s: command '%s' couldn't parse GPR '%s'\n",
>                                 argv[0], argv[optind], argv[optind + 1]);
>                 return -1;
>         }
>
> Which is not only prone to copy-and-paste bugs (have you spotted the error in
> the above?) but also a pain to maintain/change due to the multiple copies. We
> could certainly refactor that somewhat to reduce code duplication and that's the
> rabbit hole I started down to end up with:
>
> int parse_spr(char *arg)
> {
> ...
> }
>
> int getspr(int spr) {
> ...
> };
> DEFINE_CMD1(getspr, spr);
>
> Where `spr` specifies the parser for converting the "char *" command argument
> into a "int spr" is `parse_spr`. Note that it is in fact type safe as well -
> changing parse_spr() to return a long int instead of an int for example will
> result in a grokable compiler error.
>

That's much better, yeah we probably just need a type converter from
string to type T, I suppose. I still don't like the ACTION1/2, seems
irrelavant to the cause.

Balbir

Patch

diff --git a/Makefile.am b/Makefile.am
index 99b0ee4..0fc8c7e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -28,7 +28,7 @@  BUILT_SOURCES = $(DT) $(DT_headers)
 
 pdbg_SOURCES = \
 	src/main.c src/cfam.c src/scom.c src/reg.c src/mem.c src/thread.c \
-	src/ring.c src/htm.c src/progress.c src/options_@ARCH@.c
+	src/ring.c src/htm.c src/progress.c src/options_@ARCH@.c src/opt.c
 
 pdbg_LDADD = $(DT_objects) libpdbg.la libfdt.la \
 	-L.libs -lrt
diff --git a/src/cfam.c b/src/cfam.c
index 269123e..429e2b9 100644
--- a/src/cfam.c
+++ b/src/cfam.c
@@ -20,8 +20,9 @@ 
 #include <inttypes.h>
 
 #include "main.h"
+#include "opt.h"
 
-static int getcfam(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *unused)
+static int getcfam_cb(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *unused)
 {
 	uint32_t value;
 
@@ -33,7 +34,7 @@  static int getcfam(struct pdbg_target *target, uint32_t index, uint64_t *addr, u
 	return 1;
 }
 
-static int putcfam(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *data)
+static int putcfam_cb(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *data)
 {
 	if (fsi_write(target, *addr, *data))
 		return 0;
@@ -41,44 +42,15 @@  static int putcfam(struct pdbg_target *target, uint32_t index, uint64_t *addr, u
 	return 1;
 }
 
-int handle_cfams(int optind, int argc, char *argv[])
+static int getcfam(uint64_t addr)
 {
-	uint64_t addr;
-	char *endptr;
-
-	if (optind + 1 >= argc) {
-		printf("%s: command '%s' requires an address\n", argv[0], argv[optind]);
-		return -1;
-	}
-
-	errno = 0;
-	addr = strtoull(argv[optind + 1], &endptr, 0);
-	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse address '%s'\n",
-				argv[0], argv[optind], argv[optind + 1]);
-		return -1;
-	}
-
-	if (strcmp(argv[optind], "putcfam") == 0) {
-		uint64_t data;
-
-		if (optind + 2 >= argc) {
-			printf("%s: command '%s' requires data\n", argv[0], argv[optind]);
-			return -1;
-		}
-
-		errno = 0;
-		data = strtoull(argv[optind + 2], &endptr, 0);
-		if (errno || *endptr != '\0') {
-			printf("%s: command '%s' couldn't parse data '%s'\n",
-				argv[0], argv[optind], argv[optind + 1]);
-			return -1;
-		}
-
-		return for_each_target("fsi", putcfam, &addr, &data);
-	}
-
-	return for_each_target("fsi", getcfam, &addr, NULL);
+	return for_each_target("fsi", getcfam_cb, &addr, NULL);
 }
+DEFINE_ACTION1(getcfam, uint64_t, "Read system cfam", "<address>");
 
-
+static int putcfam(uint64_t addr, uint64_t data)
+{
+	return for_each_target("fsi", putcfam_cb, &addr, &data);
+}
+/* TODO: Support [<mask>] */
+DEFINE_ACTION2(putcfam, uint64_t, uint64_t, "Write system cfam", "<address>", "<value>");
diff --git a/src/main.c b/src/main.c
index baeb57e..53483dc 100644
--- a/src/main.c
+++ b/src/main.c
@@ -42,6 +42,7 @@ 
 #include "thread.h"
 #include "htm.h"
 #include "options.h"
+#include "opt.h"
 
 #define PR_ERROR(x, args...) \
 	pdbg_log(PDBG_ERROR, x, ##args)
@@ -88,18 +89,6 @@  struct action {
 };
 
 static struct action actions[] = {
-	{ "getgpr",  "<gpr>", "Read General Purpose Register (GPR)", &handle_getgpr },
-	{ "putgpr",  "<gpr> <value>", "Write General Purpose Register (GPR)", &handle_putgpr },
-	{ "getnia",  "", "Get Next Instruction Address (NIA)", &handle_getnia },
-	{ "putnia",  "<value>", "Write Next Instrution Address (NIA)", &handle_putnia },
-	{ "getspr",  "<spr>", "Get Special Purpose Register (SPR)", &handle_getspr },
-	{ "putspr",  "<spr> <value>", "Write Special Purpose Register (SPR)", &handle_putspr },
-	{ "getmsr",  "", "Get Machine State Register (MSR)", &handle_getmsr },
-	{ "putmsr",  "<value>", "Write Machine State Register (MSR)", &handle_putmsr },
-	{ "getring", "<addr> <len>", "Read a ring. Length must be correct", &handle_getring },
-	{ "start",   "", "Start thread", &thread_start },
-	{ "step",    "<count>", "Set a thread <count> instructions", &thread_step },
-	{ "stop",    "", "Stop thread", &thread_stop },
 	{ "htm_start", "", "[deprecated use 'htm nest start'] Start Nest HTM", &run_htm_start },
 	{ "htm_stop", "", "[deprecated use 'htm nest stop'] Stop Nest HTM", &run_htm_stop },
 	{ "htm_status", "", "[deprecated use 'htm nest status'] Print the status of HTM", &run_htm_status },
@@ -110,22 +99,12 @@  static struct action actions[] = {
 	{ "htm", "core|nest start|stop|status|reset|dump|trace|analyse", "Hardware Trace Macro", &run_htm },
 	{ "release", "", "Should be called after pdbg work is finished, to release special wakeups and other resources.", &handle_release},
 	{ "probe", "", "", &handle_probe },
-	{ "getcfam", "<address>", "Read system cfam", &handle_cfams },
-	{ "putcfam", "<address> <value> [<mask>]", "Write system cfam", &handle_cfams },
-	{ "getscom", "<address>", "Read system scom", &handle_scoms },
-	{ "putscom", "<address> <value> [<mask>]", "Write system scom", &handle_scoms },
 	{ "getmem",  "<address> <count>", "Read system memory", &handle_mem },
 	{ "putmem",  "<address>", "Write to system memory", &handle_mem },
-	{ "threadstatus", "", "Print the status of a thread", &thread_status_print },
-	{ "sreset",  "", "Reset", &thread_sreset },
-	{ "regs",  "", "State", &thread_state },
 };
 
-
 static void print_usage(char *pname)
 {
-	int i;
-
 	printf("Usage: %s [options] command ...\n\n", pname);
 	printf(" Options:\n");
 	printf("\t-p, --processor=processor-id\n");
@@ -156,8 +135,8 @@  static void print_usage(char *pname)
 	printf("\t-h, --help\n");
 	printf("\n");
 	printf(" Commands:\n");
-	for (i = 0; i < ARRAY_SIZE(actions); i++)
-		printf("  %-15s %-27s  %s\n", actions[i].name, actions[i].args, actions[i].desc);
+	printf("%s\n", actions[0].name);
+	print_cmds_usage();
 }
 
 static bool parse_options(int argc, char *argv[])
@@ -582,7 +561,9 @@  static int handle_release(int optind, int argc, char *argv[])
 
 int main(int argc, char *argv[])
 {
-	int i, rc = 0;
+	int rc = 0;
+	void **args;
+	pdbg_cmd_t *cmd;
 
 	backend = default_backend();
 	device_node = default_target(backend);
@@ -614,19 +595,15 @@  int main(int argc, char *argv[])
 
 	atexit(atexit_release);
 
-	for (i = 0; i < ARRAY_SIZE(actions); i++) {
-		if (strcmp(argv[optind], actions[i].name) == 0) {
-			rc = actions[i].fn(optind, argc, argv);
-			goto found_action;
-		}
+	cmd = parse_cmds(&argv[optind], argc - optind, &args);
+	if (!cmd) {
+		PR_ERROR("Unsupported command: %s\n", argv[optind]);
+		print_usage(basename(argv[0]));
+		return 1;
 	}
 
-	PR_ERROR("Unsupported command: %s\n", argv[optind]);
-	print_usage(basename(argv[0]));
-	return 1;
-
-found_action:
-	if (rc <= 0) {
+	/* Run the command */
+	if (cmd(args) <= 0) {
                 printf("No valid targets found or specified. Try adding -p/-c/-t options to specify a target.\n");
                 printf("Alternatively run '%s -a probe' to get a list of all valid targets\n",
 		       basename(argv[0]));
diff --git a/src/opt.c b/src/opt.c
new file mode 100644
index 0000000..ca2c40f
--- /dev/null
+++ b/src/opt.c
@@ -0,0 +1,139 @@ 
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <assert.h>
+#include <string.h>
+#include <errno.h>
+
+#include "opt.h"
+
+/* Filled in by the DEFINE_ACTION macros above */
+extern struct pdbg_action *__start_pdbg_actions[];
+extern struct pdbg_action *__stop_pdbg_actions;
+
+/* Global parsers. These should allocate memory for the required type and return
+ * a pointer to it or NULL on error. */
+uint64_t *parse_uint64_t(char *argv)
+{
+	uint64_t *n = malloc(sizeof(*n));
+	char *endptr;
+
+	errno = 0;
+	*n = strtoull(argv, &endptr, 0);
+	if (errno || *endptr != '\0')
+		return NULL;
+
+	return n;
+}
+
+uint32_t *parse_uint32_t(char *argv)
+{
+	uint32_t *n = malloc(sizeof(*n));
+	char *endptr;
+
+	errno = 0;
+	*n = strtoul(argv, &endptr, 0);
+	if (errno || *endptr != '\0')
+		return NULL;
+
+	return n;
+}
+
+extern struct pdbg_action *pdbg_action_test_p;
+pdbg_cmd_t *parse_cmds(char *argv[], int argc, void **args[])
+{
+	int i, arg_count;
+	struct pdbg_cmd *cmd = NULL;
+	pdbg_arg_t **arg;
+	char *cmd_name = argv[0];
+	char **cmd_args = &argv[1];
+	void **results;
+	struct pdbg_action **p, *action;
+
+	for (p = __start_pdbg_actions; p < &__stop_pdbg_actions; p++) {
+		action = *p;
+		if (!strcmp(cmd_name, action->name)) {
+			/* Found our action */
+			cmd = action->cmd;
+			break;
+		}
+	}
+
+	/* Did not find a valid command to execute */
+	if (!cmd) {
+		printf("Invalid command %s specified\n", cmd_name);
+		return NULL;
+	}
+
+	arg = cmd->args;
+
+	for (arg_count = 0; arg[arg_count]; arg_count++) {}
+
+	/* Did we get the right number of commands */
+	if (arg_count != argc - 1) {
+		printf("Incorrect number of arguments for %s specified.\n", cmd_name);
+		printf("Expected %d argument got %d:\n", arg_count, argc - 1);
+		printf("Eg. %s ", cmd_name);
+		for (i = 0; i < arg_count; i++)
+			printf("%s ", action->args_desc[i]);
+		printf("\n");
+		return NULL;
+	}
+
+	/* Allocate space for parsing results */
+	results = malloc(arg_count*sizeof(void *));
+	assert(results);
+
+	for (i = 0; i < arg_count; i++) {
+		results[i] = arg[i](cmd_args[i]);
+
+		if (!results[i]) {
+			/* Were we able to parse the argument? Note the parsers
+			 * provide the error. */
+			printf("Invalid arguments specified\n");
+			return NULL;
+		}
+	}
+
+	*args = results;
+
+	return cmd->cmdp;
+}
+
+void print_cmds_usage(void)
+{
+	char args[29] = "";
+	struct pdbg_action **p, *action;
+	int i, len = 28;
+
+	for (p = __start_pdbg_actions; p < &__stop_pdbg_actions; p++) {
+		action = *p;
+
+		/* I really hate C sometimes. Surely there is a better way than
+		 * this, but it is late. */
+		for (i = 0; action->cmd->args[i]; i++) {
+			len = strlen(args);
+			if (len) {
+				/* Overwrite \0 with a space */
+				args[len] = ' ';
+
+				/* Put the \0 back */
+				args[len+1] = '\0';
+			}
+
+			/* Recalculate the remaining length not including \0 */
+			len = 28 - len + 1;
+
+			/* If you hit this it's because your commands argument
+			 * description is longer than 15 chars */
+			assert(len > 0);
+
+			/* Add another argument description */
+			strncat(args, action->args_desc[i], len - 1);
+		}
+
+		printf("  %-15s %-27s  %s\n", action->name, args, action->desc);
+		args[0] = '\0';
+		len = 28;
+	}
+}
diff --git a/src/opt.h b/src/opt.h
new file mode 100644
index 0000000..b28f6f3
--- /dev/null
+++ b/src/opt.h
@@ -0,0 +1,96 @@ 
+/* Copyright 2017 IBM Corp.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * 	http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+ * implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "compiler.h"
+
+typedef void * (pdbg_arg_t)(char *argv);
+typedef int (pdbg_cmd_t)(void *[]);
+
+struct pdbg_cmd {
+	pdbg_cmd_t *cmdp;
+	pdbg_arg_t *args[];
+};
+
+struct pdbg_action {
+	char *name;
+	struct pdbg_cmd *cmd;
+	char *desc;
+	char *args_desc[3];
+};
+
+/* TODO: Still haven't figured out the recursive fun we need here, or if it's
+ * even possible */
+//#define DEFINE_CMD1(arg1, ...)
+//	typeof(*arg1(""))
+#define ARG_TYPE(arg)							\
+	typeof(*parse_##arg(""))
+
+#define DEFINE_CMD0(cmd)					\
+	int cmd(void);						\
+	int call_##cmd(void *args[]) {				\
+		return cmd();					\
+	}							\
+	struct pdbg_cmd pdbg_cmd_##cmd = {			\
+		.cmdp = call_##cmd,				\
+		.args = {NULL},					\
+	}
+
+#define DEFINE_CMD1(cmd, arg1)						\
+	int cmd(ARG_TYPE(arg1));					\
+	int call_##cmd(void *args[]) {					\
+		return cmd(*(ARG_TYPE(arg1) *) args[0]);		\
+	}								\
+	struct pdbg_cmd pdbg_cmd_##cmd = {				\
+		.cmdp = call_##cmd,					\
+		.args = {(pdbg_arg_t *) &parse_##arg1, NULL},		\
+	}
+
+#define DEFINE_CMD2(cmd, arg1, arg2)					\
+	int cmd(ARG_TYPE(arg1), ARG_TYPE(arg2));			\
+	int call_##cmd(void *args[]) {					\
+		return cmd(*(ARG_TYPE(arg1) *) args[0],			\
+			   *(ARG_TYPE(arg2) *) args[1]);		\
+	}								\
+	struct pdbg_cmd pdbg_cmd_##cmd = {				\
+		.cmdp = call_##cmd,					\
+		.args = { (pdbg_arg_t *) &parse_##arg1,			\
+			  (pdbg_arg_t *) &parse_##arg2, NULL},		\
+	}
+
+#define DEFINE_ACTION0(cmd, desc)					\
+	DEFINE_CMD0(cmd);						\
+	const struct pdbg_action __used pdbg_action_##cmd =		\
+	{ #cmd, &pdbg_cmd_##cmd, desc, {NULL} };			\
+	const struct pdbg_action __used __section("pdbg_actions") *pdbg_action_##cmd##_p = &pdbg_action_##cmd
+
+#define DEFINE_ACTION1(cmd, arg1, desc, arg1_desc)	\
+	DEFINE_CMD1(cmd, arg1);				\
+	const struct pdbg_action pdbg_action_##cmd =	\
+	{ #cmd, &pdbg_cmd_##cmd, desc, {arg1_desc} };	\
+	const struct pdbg_action __used __section("pdbg_actions") *pdbg_action_##cmd##_p = &pdbg_action_##cmd
+
+#define DEFINE_ACTION2(cmd, arg1, arg2, desc, arg1_desc, arg2_desc)	\
+	DEFINE_CMD2(cmd, arg1, arg2);					\
+	const struct pdbg_action pdbg_action_##cmd =			\
+	{ #cmd, &pdbg_cmd_##cmd, desc, {arg1_desc, arg2_desc} };			\
+	const struct pdbg_action __used __section("pdbg_actions") *pdbg_action_##cmd##_p = &pdbg_action_##cmd
+
+pdbg_cmd_t *parse_cmds(char *argv[], int argc, void **args[]);
+void print_cmds_usage(void);
+
+/* Argument parsers */
+uint64_t *parse_uint64_t(char *argv);
+uint32_t *parse_uint32_t(char *argv);
diff --git a/src/reg.c b/src/reg.c
index 932ae43..3c71e48 100644
--- a/src/reg.c
+++ b/src/reg.c
@@ -18,10 +18,12 @@ 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include <assert.h>
 
 #include <libpdbg.h>
 
 #include "main.h"
+#include "opt.h"
 
 #define REG_MEM -3
 #define REG_MSR -2
@@ -91,157 +93,106 @@  static int getprocreg(struct pdbg_target *target, uint32_t index, uint64_t *reg,
 	return !rc;
 }
 
-static int parse_gpr(int optind, int argc, char *argv[])
+static uint64_t *parse_gpr(char *argv)
 {
 	char *endptr;
-	int gpr;
+	uint64_t *gpr;
 
-	if (optind + 1 >= argc) {
-		printf("%s: command '%s' requires a GPR\n", argv[0], argv[optind]);
-		return -1;
-	}
+	gpr = malloc(sizeof(*gpr));
+	assert(gpr);
 
 	errno = 0;
-	gpr = strtoul(argv[optind + 1], &endptr, 0);
+	*gpr = strtoul(argv, &endptr, 0);
 	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse GPR '%s'\n",
-		       argv[0], argv[optind], argv[optind + 1]);
-		return -1;
+		printf("couldn't parse GPR '%s'\n", argv);
+		return NULL;
 	}
 
-	if (gpr > 31) {
+	if (*gpr > 31) {
 		printf("A GPR must be between zero and 31 inclusive\n");
-		return -1;
+		return NULL;
 	}
 
 	return gpr;
 }
 
-static int parse_data(int optind, int argc, char *argv[], uint64_t *data)
+static uint64_t *parse_spr(char *argv)
 {
 	char *endptr;
+	uint64_t *spr;
 
-	if (optind + 2 >= argc) {
-		printf("%s: command '%s' requires data\n", argv[0], argv[optind]);
-		return -1;
-	}
+	spr = malloc(sizeof(*spr));
+	assert(spr);
 
 	errno = 0;
-	*data = strtoull(argv[optind + 2], &endptr, 0);
+	*spr = strtoull(argv, &endptr, 0);
 	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse data '%s'\n",
-		       argv[0], argv[optind], argv[optind + 1]);
-		return -1;
+		printf("couldn't parse SPR '%s'\n", argv);
+		return NULL;
 	}
 
-	return 0;
+	if (*spr > 0x3ff) {
+		printf("A SPR must be between zero and 1023 inclusive\n");
+		return NULL;
+	}
+
+	return spr;
 }
 
-int handle_getgpr(int optind, int argc, char *argv[])
+static int getgpr(uint64_t gpr)
 {
-	uint64_t gpr;
-
-	if ((gpr = parse_gpr(optind, argc, argv)) < 0)
-		return -1;
-
 	return for_each_target("thread", getprocreg, &gpr, NULL);
 }
+DEFINE_ACTION1(getgpr, gpr, "Read General Purpose Register (GPR)", "<gpr>");
 
-int handle_putgpr(int optind, int argc, char *argv[])
+static int putgpr(uint64_t gpr, uint64_t data)
 {
-	uint64_t gpr, data;
-
-	if ((gpr = parse_gpr(optind, argc, argv)) < 0)
-		return -1;
-
-	if (parse_data(optind, argc, argv, &data))
-		return -1;
-
 	return for_each_target("thread", putprocreg, &gpr, &data);
 }
+DEFINE_ACTION2(putgpr, gpr, uint64_t, "Write General Purpose Register (GPR)", "<gpr>", "<value>");
 
-int handle_getnia(int optind, int argc, char *argv[])
+static int getnia(void)
 {
 	uint64_t reg = REG_NIA;
 
 	return for_each_target("thread", getprocreg, &reg, NULL);
 }
+DEFINE_ACTION0(getnia, "Get Next Instruction Address (NIA)");
 
-int handle_putnia(int optind, int argc, char *argv[])
+static int putnia(uint64_t nia)
 {
-	uint64_t reg = REG_NIA, data;
-
-	if (parse_data(optind, argc, argv, &data))
-		return -1;
-
-	return for_each_target("thread", putprocreg, &reg, &data);
-}
-
-static int parse_spr(int optind, int argc, char *argv[])
-{
-	char *endptr;
-	int gpr;
-
-	if (optind + 1 >= argc) {
-		printf("%s: command '%s' requires a SPR\n", argv[0], argv[optind]);
-		return -1;
-	}
-
-	errno = 0;
-	gpr = strtoul(argv[optind + 1], &endptr, 0);
-	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse SPR '%s'\n",
-		       argv[0], argv[optind], argv[optind + 1]);
-		return -1;
-	}
-
-	if (gpr > 0x3ff) {
-		printf("A SPR must be between zero and 1023 inclusive\n");
-		return -1;
-	}
+	uint64_t reg = REG_NIA;
 
-	return gpr;
+	return for_each_target("thread", putprocreg, &reg, &nia);
 }
+DEFINE_ACTION1(putnia, uint64_t, "Write Next Instrution Address (NIA)", "<value>");
 
-int handle_getspr(int optind, int argc, char *argv[])
+static int getspr(uint64_t spr)
 {
-	uint64_t spr;
-
-	if ((spr = parse_spr(optind, argc, argv)) < 0)
-		return -1;
-
 	spr += REG_R31;
-
 	return for_each_target("thread", getprocreg, &spr, NULL);
 }
+DEFINE_ACTION1(getspr, spr, "Get Special Purpose Register (SPR)", "<spr>");
 
-int handle_putspr(int optind, int argc, char *argv[])
+static int putspr(uint64_t spr, uint64_t data)
 {
-	uint64_t spr, data;
-
-	if ((spr = parse_spr(optind, argc, argv)) < 0)
-		return -1;
-
 	spr += REG_R31;
-	if (parse_data(optind, argc, argv, &data))
-		return -1;
-
 	return for_each_target("thread", putprocreg, &spr, &data);
 }
+DEFINE_ACTION2(putspr, spr, uint64_t, "Write Special Purpose Register (SPR)", "<spr>", "<value>");
 
-int handle_getmsr(int optind, int argc, char *argv[])
+static int getmsr(void)
 {
 	uint64_t msr = REG_MSR;
 
 	return for_each_target("thread", getprocreg, &msr, NULL);
 }
+DEFINE_ACTION0(getmsr, "Get Machine State Register (MSR)");
 
-int handle_putmsr(int optind, int argc, char *argv[])
+static int putmsr(uint64_t msr)
 {
-	uint64_t msr = REG_MSR, data;
-
-	if (parse_data(optind, argc, argv, &data))
-		return -1;
+	uint64_t reg_msr = REG_MSR;
 
-	return for_each_target("thread", putprocreg, &msr, &data);
+	return for_each_target("thread", putprocreg, &reg_msr, &msr);
 }
+DEFINE_ACTION1(putmsr, uint64_t, "Write Machine State Register (MSR)", "<value>");
diff --git a/src/ring.c b/src/ring.c
index b0c9376..604ab73 100644
--- a/src/ring.c
+++ b/src/ring.c
@@ -23,8 +23,9 @@ 
 #include <operations.h>
 
 #include "main.h"
+#include "opt.h"
 
-static int pdbg_getring(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *len)
+static int pdbg_getring_cb(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *len)
 {
 	uint32_t *result;
 	int i, words;
@@ -51,31 +52,8 @@  static int pdbg_getring(struct pdbg_target *target, uint32_t index, uint64_t *ad
 	return 1;
 }
 
-int handle_getring(int optind, int argc, char *argv[])
+static int pdbg_getring(uint64_t ring_addr, uint64_t ring_len)
 {
-	uint64_t ring_addr, ring_len;
-	char *endptr;
-
-	if (optind + 2 >= argc) {
-		printf("%s: command '%s' requires two arguments (address and length)\n",
-		       argv[0], argv[optind]);
-		return -1;
-	}
-
-	errno = 0;
-	ring_addr = strtoull(argv[optind + 1], &endptr, 0);
-	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse ring address '%s'\n",
-		       argv[0], argv[optind], argv[optind + 1]);
-		return -1;
-	}
-
-	ring_len = strtoull(argv[optind + 2], &endptr, 0);
-	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse ring length '%s'\n",
-		       argv[0], argv[optind], argv[optind + 2]);
-		return -1;
-	}
-
-	return for_each_target("chiplet", pdbg_getring, &ring_addr, &ring_len);
+	return for_each_target("chiplet", pdbg_getring_cb, &ring_addr, &ring_len);
 }
+DEFINE_ACTION2(pdbg_getring, uint64_t, uint64_t, "Read a ring. Length must be correct", "<addr>", "<len>");
diff --git a/src/scom.c b/src/scom.c
index 4c59e2a..3d4bf54 100644
--- a/src/scom.c
+++ b/src/scom.c
@@ -22,8 +22,9 @@ 
 #include <libpdbg.h>
 
 #include "main.h"
+#include "opt.h"
 
-static int getscom(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *unused)
+static int getscom_cb(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *unused)
 {
 	uint64_t value;
 
@@ -35,7 +36,7 @@  static int getscom(struct pdbg_target *target, uint32_t index, uint64_t *addr, u
 	return 1;
 }
 
-static int putscom(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *data)
+static int putscom_cb(struct pdbg_target *target, uint32_t index, uint64_t *addr, uint64_t *data)
 {
 	if (pib_write(target, *addr, *data))
 		return 0;
@@ -43,44 +44,14 @@  static int putscom(struct pdbg_target *target, uint32_t index, uint64_t *addr, u
 	return 1;
 }
 
-
-int handle_scoms(int optind, int argc, char *argv[])
+static int getscom(uint64_t addr)
 {
-	uint64_t addr;
-	char *endptr;
-
-	if (optind + 1 >= argc) {
-		printf("%s: command '%s' requires an address\n", argv[0], argv[optind]);
-		return -1;
-	}
-
-	errno = 0;
-	addr = strtoull(argv[optind + 1], &endptr, 0);
-	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse address '%s'\n",
-				argv[0], argv[optind], argv[optind + 1]);
-		return -1;
-	}
-
-	if (strcmp(argv[optind], "putscom") == 0) {
-		uint64_t data;
-
-		if (optind + 2 >= argc) {
-			printf("%s: command '%s' requires data\n", argv[0], argv[optind]);
-			return -1;
-		}
-
-		errno = 0;
-		data = strtoull(argv[optind + 2], &endptr, 0);
-		if (errno || *endptr != '\0') {
-			printf("%s: command '%s' couldn't parse data '%s'\n",
-				argv[0], argv[optind], argv[optind + 1]);
-			return -1;
-		}
-
-		return for_each_target("pib", putscom, &addr, &data);
-	}
-
-	return for_each_target("pib", getscom, &addr, NULL);
+	return for_each_target("pib", getscom_cb, &addr, NULL);
 }
+DEFINE_ACTION1(getscom, uint64_t, "Read system scom", "<address>");
 
+static int putscom(uint64_t addr, uint64_t data)
+{
+	return for_each_target("pib", putscom_cb, &addr, &data);
+}
+DEFINE_ACTION2(putscom, uint64_t, uint64_t, "Write system scom", "<address>", "<data>");
diff --git a/src/thread.c b/src/thread.c
index e8b54cb..a0fdc51 100644
--- a/src/thread.c
+++ b/src/thread.c
@@ -22,6 +22,7 @@ 
 
 #include "main.h"
 #include "mem.h"
+#include "opt.h"
 
 static int print_thread_status(struct pdbg_target *target, uint32_t index, uint64_t *arg, uint64_t *unused1)
 {
@@ -140,48 +141,37 @@  static int state_thread(struct pdbg_target *thread_target, uint32_t index, uint6
 	return 1;
 }
 
-int thread_start(int optind, int argc, char *argv[])
+static int thread_start(void)
 {
 	return for_each_target("thread", start_thread, NULL, NULL);
 }
+DEFINE_ACTION0(thread_start, "Start thread");
 
-int thread_step(int optind, int argc, char *argv[])
+static int thread_step(uint64_t count)
 {
-	uint64_t count;
-	char *endptr;
-
-	if (optind + 1 >= argc) {
-		printf("%s: command '%s' requires a count\n", argv[0], argv[optind]);
-		return -1;
-	}
-
-	errno = 0;
-	count = strtoull(argv[optind + 1], &endptr, 0);
-	if (errno || *endptr != '\0') {
-		printf("%s: command '%s' couldn't parse count '%s'\n",
-				argv[0], argv[optind], argv[optind + 1]);
-		return -1;
-	}
-
 	return for_each_target("thread", step_thread, &count, NULL);
 }
+DEFINE_ACTION1(thread_step, uint64_t, "Set a thread <count> instructions", "<count>");
 
-int thread_stop(int optind, int argc, char *argv[])
+static int thread_stop(void)
 {
 	return for_each_target("thread", stop_thread, NULL, NULL);
 }
+DEFINE_ACTION0(thread_stop, "Stop thread");
 
-int thread_status_print(int optind, int argc, char *argv[])
+static int thread_status_print(void)
 {
 	return for_each_target("pib", print_proc_thread_status, NULL, NULL);
 }
+DEFINE_ACTION0(thread_status_print, "Print the status of a thread");
 
-int thread_sreset(int optind, int argc, char *argv[])
+static int thread_sreset(void)
 {
 	return for_each_target("thread", sreset_thread, NULL, NULL);
 }
+DEFINE_ACTION0(thread_sreset, "Reset a thread");
 
-int thread_state(int optind, int argc, char *argv[])
+static int thread_state(void)
 {
 	int err;
 
@@ -191,3 +181,4 @@  int thread_state(int optind, int argc, char *argv[])
 
 	return err;
 }
+DEFINE_ACTION0(thread_state, "Dump thread register state possibly including a backtrace");