[v2,1/7] main: Overhaul target selection

Message ID 20180601063050.19286-2-amitay@ozlabs.org
State Superseded
Headers show
Series
  • Overhaul target selection
Related show

Commit Message

Amitay Isaacs June 1, 2018, 6:30 a.m.
Support explicit multiple target selection using ranges and lists.
For options -p/-c/-t, support the following valid arguments:

  3
  0-7
  1,2,3
  0-5,7,9-11,17,19

For loss of sanity, make sense of the following valid arguments:

  3,3,3,3,3,3
  1-6,2-5
  1,2,3,0-7

Conjunction of -p/-c/-t with -a also works and it's insensitive to order
of options specified.

  -a -c 1,2     processors 0-max; chips 1-2; threads 0-max
  -p 0 -c 0 -a  processors 0; chips 0; threads 0-max
  -a -c 1 -t 1  processors 0-max; chips 1; threads 1

Signed-off-by: Amitay Isaacs <amitay@ozlabs.org>
---
 src/main.c | 171 ++++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 131 insertions(+), 40 deletions(-)

Comments

Alistair Popple June 4, 2018, 4:21 a.m. | #1
Thanks Amitay. A couple more comments below:

> +	while ((tok = strtok_r(tmp, ",", &saveptr)) != NULL) {
> +		char *a, *b, *saveptr2 = NULL;
> +		int from, to;

These should be unsigned to avoid somewhat contrived errors like this:

./pdbg -p 2147483649,4294967295 getscom 0xf000f
Segmentation fault

> +
> +
> +		a = strtok_r(tok, "-", &saveptr2);
> +		if (a == NULL) {
> +			return false;
> +		} else {
> +			from = atoi(a);
> +			if (from >= max) {
> +				return false;
> +			}
> +		}
> +
> +		b = strtok_r(NULL, "-", &saveptr2);
> +		if (b == NULL) {
> +			to = from;
> +		} else {
> +			to = atoi(b);

I think it would be preferable to detect errors here. At the moment the following works:

./pdbg -p 0-0xa ...

but:

./pdbg -p 1-0xa ...

doesn't. Which is not a problem in that I don't think people will want to
specify things in hex (altough perhaps they do and is there any cost to
supporting it?), but an error would be less confusing than guessing the users
intent.

> +			if (to >= max) {
> +				return false;
> +			}
> +		}
> +
> +		if (from > to)
> +			return false;

Should we add an assert(to < max) here? I can see at present that the above code
bails if to/from is >= max but future changes could alter that and lead to an
overflow in the below.

> +		for (i=from; i<=to; i++)

Also very minor style nit-pick - most of the exsiting code has whitespace
between operators (ie. for (i = from; i <= to; i++)), so it would be good if we
could maintain that. Thanks!

- Alistair

> +			list[i] = 1;
> +
> +		tmp = NULL;
> +	};
> +
> +	n = 0;
> +	for (i=0; i<max; i++) {
> +		if (list[i] == 1)
> +			n++;
> +	}
> +
> +	*count = n;
> +	return true;
> +}
> +
>  static bool parse_options(int argc, char *argv[])
>  {
>  	int c;
>  	bool opt_error = true;
> -	static int current_processor = INT_MAX, current_chip = INT_MAX, current_thread = INT_MAX;
> +	int p_list[MAX_PROCESSORS];
> +	int c_list[MAX_CHIPS];
> +	int t_list[MAX_THREADS];
> +	int p_count = 0, c_count = 0, t_count = 0;
> +	int i, j, k;
>  	struct option long_opts[] = {
>  		{"all",			no_argument,		NULL,	'a'},
>  		{"backend",		required_argument,	NULL,	'b'},
> @@ -195,50 +256,45 @@ static bool parse_options(int argc, char *argv[])
>  		switch(c) {
>  		case 'a':
>  			opt_error = false;
> -			for (current_processor = 0; current_processor < MAX_PROCESSORS; current_processor++) {
> -				processorsel[current_processor] = &chipsel[current_processor][0];
> -				for (current_chip = 0; current_chip < MAX_CHIPS; current_chip++) {
> -					chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
> -					for (current_thread = 0; current_thread < MAX_THREADS; current_thread++)
> -						threadsel[current_processor][current_chip][current_thread] = 1;
> -				}
> +
> +			if (p_count == 0) {
> +				p_count = MAX_PROCESSORS;
> +				for (i=0; i<MAX_PROCESSORS; i++)
> +					p_list[i] = 1;
> +			}
> +
> +			if (c_count == 0) {
> +				c_count = MAX_CHIPS;
> +				for (i=0; i<MAX_CHIPS; i++)
> +					c_list[i] = 1;
> +			}
> +
> +			if (t_count == 0) {
> +				t_count = MAX_THREADS;
> +				for (i=0; i<MAX_THREADS; i++)
> +					t_list[i] = 1;
>  			}
>  			break;
>  
>  		case 'p':
> -			errno = 0;
> -			current_processor = strtoul(optarg, &endptr, 0);
> -			opt_error = (errno || *endptr != '\0');
> -			if (!opt_error) {
> -				if (current_processor >= MAX_PROCESSORS)
> -					opt_error = true;
> -				else
> -					processorsel[current_processor] = &chipsel[current_processor][0];
> -			}
> +			if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count))
> +				fprintf(stderr, "Failed to parse '-p %s'\n", optarg);
> +			else
> +				opt_error = false;
>  			break;
>  
>  		case 'c':
> -			errno = 0;
> -			current_chip = strtoul(optarg, &endptr, 0);
> -			opt_error = (errno || *endptr != '\0');
> -			if (!opt_error) {
> -				if (current_chip >= MAX_CHIPS)
> -					opt_error = true;
> -				else
> -					chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
> -			}
> +			if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count))
> +				fprintf(stderr, "Failed to parse '-c %s'\n", optarg);
> +			else
> +				opt_error = false;
>  			break;
>  
>  		case 't':
> -			errno = 0;
> -			current_thread = strtoul(optarg, &endptr, 0);
> -			opt_error = (errno || *endptr != '\0');
> -			if (!opt_error) {
> -				if (current_thread >= MAX_THREADS)
> -					opt_error = true;
> -				else
> -					threadsel[current_processor][current_chip][current_thread] = 1;
> -			}
> +			if (!parse_list(optarg, MAX_THREADS, t_list, &t_count))
> +				fprintf(stderr, "Failed to parse '-t %s'\n", optarg);
> +			else
> +				opt_error = false;
>  			break;
>  
>  		case 'b':
> @@ -292,10 +348,45 @@ static bool parse_options(int argc, char *argv[])
>  		}
>  	} while (c != EOF && !opt_error);
>  
> -	if (opt_error)
> +	if (opt_error) {
>  		print_usage(basename(argv[0]));
> +		return false;
> +	}
> +
> +	if ((c_count > 0 || t_count > 0) && p_count == 0) {
> +		fprintf(stderr, "No processor(s) selected\n");
> +		fprintf(stderr, "Use -p or -a to select processor(s)\n");
> +		return false;
> +	}
> +
> +	if (t_count > 0 && c_count == 0)  {
> +		fprintf(stderr, "No chip(s) selected\n");
> +		fprintf(stderr, "Use -c or -a to select chip(s)\n");
> +		return false;
> +	}
> +
> +	for (i=0; i<MAX_PROCESSORS; i++) {
> +		if (p_list[i] == 0)
> +			continue;
> +
> +		processorsel[i] = &chipsel[i][0];
> +
> +		for (j=0; j<MAX_CHIPS; j++) {
> +			if (c_list[j] == 0)
> +				continue;
> +
> +			chipsel[i][j] = &threadsel[i][j][0];
> +
> +			for (k=0; k<MAX_THREADS; k++) {
> +				if (t_list[k] == 0)
> +					continue;
> +
> +				threadsel[i][j][k] = 1;
> +			}
> +		}
> +	}
>  
> -	return !opt_error;
> +	return true;
>  }
>  
>  void target_select(struct pdbg_target *target)
>
Amitay Isaacs June 4, 2018, 8:30 a.m. | #2
On Mon, 2018-06-04 at 14:21 +1000, Alistair Popple wrote:
> Thanks Amitay. A couple more comments below:
> 
> > +	while ((tok = strtok_r(tmp, ",", &saveptr)) != NULL) {
> > +		char *a, *b, *saveptr2 = NULL;
> > +		int from, to;
> 
> These should be unsigned to avoid somewhat contrived errors like
> this:
> 
> ./pdbg -p 2147483649,4294967295 getscom 0xf000f
> Segmentation fault

Tests, tests, we need tests. :-)

> 
> > +
> > +
> > +		a = strtok_r(tok, "-", &saveptr2);
> > +		if (a == NULL) {
> > +			return false;
> > +		} else {
> > +			from = atoi(a);
> > +			if (from >= max) {
> > +				return false;
> > +			}
> > +		}
> > +
> > +		b = strtok_r(NULL, "-", &saveptr2);
> > +		if (b == NULL) {
> > +			to = from;
> > +		} else {
> > +			to = atoi(b);
> 
> I think it would be preferable to detect errors here. At the moment
> the following works:
> 
> ./pdbg -p 0-0xa ...
> 
> but:
> 
> ./pdbg -p 1-0xa ...
> 
> doesn't. Which is not a problem in that I don't think people will
> want to
> specify things in hex (altough perhaps they do and is there any cost
> to
> supporting it?), but an error would be less confusing than guessing
> the users
> intent.

Will switch to using stroul instead of atoi.

> 
> > +			if (to >= max) {
> > +				return false;
> > +			}
> > +		}
> > +
> > +		if (from > to)
> > +			return false;
> 
> Should we add an assert(to < max) here? I can see at present that the
> above code
> bails if to/from is >= max but future changes could alter that and
> lead to an
> overflow in the below.

I think we should never assert on user input.  Assert is good to catch
programming errors.  For invalid user-input, we should print
appropriate error message.

> 
> > +		for (i=from; i<=to; i++)
> 
> Also very minor style nit-pick - most of the exsiting code has
> whitespace
> between operators (ie. for (i = from; i <= to; i++)), so it would be
> good if we
> could maintain that. Thanks!

Sure.

> 
> - Alistair
> 
> > +			list[i] = 1;
> > +
> > +		tmp = NULL;
> > +	};
> > +
> > +	n = 0;
> > +	for (i=0; i<max; i++) {
> > +		if (list[i] == 1)
> > +			n++;
> > +	}
> > +
> > +	*count = n;
> > +	return true;
> > +}
> > +
> >  static bool parse_options(int argc, char *argv[])
> >  {
> >  	int c;
> >  	bool opt_error = true;
> > -	static int current_processor = INT_MAX, current_chip =
> > INT_MAX, current_thread = INT_MAX;
> > +	int p_list[MAX_PROCESSORS];
> > +	int c_list[MAX_CHIPS];
> > +	int t_list[MAX_THREADS];
> > +	int p_count = 0, c_count = 0, t_count = 0;
> > +	int i, j, k;
> >  	struct option long_opts[] = {
> >  		{"all",			no_argument,	
> > 	NULL,	'a'},
> >  		{"backend",		required_argument,	
> > NULL,	'b'},
> > @@ -195,50 +256,45 @@ static bool parse_options(int argc, char
> > *argv[])
> >  		switch(c) {
> >  		case 'a':
> >  			opt_error = false;
> > -			for (current_processor = 0;
> > current_processor < MAX_PROCESSORS; current_processor++) {
> > -				processorsel[current_processor] =
> > &chipsel[current_processor][0];
> > -				for (current_chip = 0;
> > current_chip < MAX_CHIPS; current_chip++) {
> > -					chipsel[current_processor]
> > [current_chip] = &threadsel[current_processor][current_chip][0];
> > -					for (current_thread = 0;
> > current_thread < MAX_THREADS; current_thread++)
> > -						threadsel[current_
> > processor][current_chip][current_thread] = 1;
> > -				}
> > +
> > +			if (p_count == 0) {
> > +				p_count = MAX_PROCESSORS;
> > +				for (i=0; i<MAX_PROCESSORS; i++)
> > +					p_list[i] = 1;
> > +			}
> > +
> > +			if (c_count == 0) {
> > +				c_count = MAX_CHIPS;
> > +				for (i=0; i<MAX_CHIPS; i++)
> > +					c_list[i] = 1;
> > +			}
> > +
> > +			if (t_count == 0) {
> > +				t_count = MAX_THREADS;
> > +				for (i=0; i<MAX_THREADS; i++)
> > +					t_list[i] = 1;
> >  			}
> >  			break;
> >  
> >  		case 'p':
> > -			errno = 0;
> > -			current_processor = strtoul(optarg,
> > &endptr, 0);
> > -			opt_error = (errno || *endptr != '\0');
> > -			if (!opt_error) {
> > -				if (current_processor >=
> > MAX_PROCESSORS)
> > -					opt_error = true;
> > -				else
> > -					processorsel[current_proce
> > ssor] = &chipsel[current_processor][0];
> > -			}
> > +			if (!parse_list(optarg, MAX_PROCESSORS,
> > p_list, &p_count))
> > +				fprintf(stderr, "Failed to parse
> > '-p %s'\n", optarg);
> > +			else
> > +				opt_error = false;
> >  			break;
> >  
> >  		case 'c':
> > -			errno = 0;
> > -			current_chip = strtoul(optarg, &endptr,
> > 0);
> > -			opt_error = (errno || *endptr != '\0');
> > -			if (!opt_error) {
> > -				if (current_chip >= MAX_CHIPS)
> > -					opt_error = true;
> > -				else
> > -					chipsel[current_processor]
> > [current_chip] = &threadsel[current_processor][current_chip][0];
> > -			}
> > +			if (!parse_list(optarg, MAX_CHIPS, c_list,
> > &c_count))
> > +				fprintf(stderr, "Failed to parse
> > '-c %s'\n", optarg);
> > +			else
> > +				opt_error = false;
> >  			break;
> >  
> >  		case 't':
> > -			errno = 0;
> > -			current_thread = strtoul(optarg, &endptr,
> > 0);
> > -			opt_error = (errno || *endptr != '\0');
> > -			if (!opt_error) {
> > -				if (current_thread >= MAX_THREADS)
> > -					opt_error = true;
> > -				else
> > -					threadsel[current_processo
> > r][current_chip][current_thread] = 1;
> > -			}
> > +			if (!parse_list(optarg, MAX_THREADS,
> > t_list, &t_count))
> > +				fprintf(stderr, "Failed to parse
> > '-t %s'\n", optarg);
> > +			else
> > +				opt_error = false;
> >  			break;
> >  
> >  		case 'b':
> > @@ -292,10 +348,45 @@ static bool parse_options(int argc, char
> > *argv[])
> >  		}
> >  	} while (c != EOF && !opt_error);
> >  
> > -	if (opt_error)
> > +	if (opt_error) {
> >  		print_usage(basename(argv[0]));
> > +		return false;
> > +	}
> > +
> > +	if ((c_count > 0 || t_count > 0) && p_count == 0) {
> > +		fprintf(stderr, "No processor(s) selected\n");
> > +		fprintf(stderr, "Use -p or -a to select
> > processor(s)\n");
> > +		return false;
> > +	}
> > +
> > +	if (t_count > 0 && c_count == 0)  {
> > +		fprintf(stderr, "No chip(s) selected\n");
> > +		fprintf(stderr, "Use -c or -a to select
> > chip(s)\n");
> > +		return false;
> > +	}
> > +
> > +	for (i=0; i<MAX_PROCESSORS; i++) {
> > +		if (p_list[i] == 0)
> > +			continue;
> > +
> > +		processorsel[i] = &chipsel[i][0];
> > +
> > +		for (j=0; j<MAX_CHIPS; j++) {
> > +			if (c_list[j] == 0)
> > +				continue;
> > +
> > +			chipsel[i][j] = &threadsel[i][j][0];
> > +
> > +			for (k=0; k<MAX_THREADS; k++) {
> > +				if (t_list[k] == 0)
> > +					continue;
> > +
> > +				threadsel[i][j][k] = 1;
> > +			}
> > +		}
> > +	}
> >  
> > -	return !opt_error;
> > +	return true;
> >  }
> >  
> >  void target_select(struct pdbg_target *target)
> > 
> 
> 

Amitay.

Patch

diff --git a/src/main.c b/src/main.c
index 2200a01..457cda2 100644
--- a/src/main.c
+++ b/src/main.c
@@ -131,9 +131,9 @@  static void print_usage(char *pname)
 
 	printf("Usage: %s [options] command ...\n\n", pname);
 	printf(" Options:\n");
-	printf("\t-p, --processor=processor-id\n");
-	printf("\t-c, --chip=core-id\n");
-	printf("\t-t, --thread=thread\n");
+	printf("\t-p, --processor=<0-%d>|<range>|<list>\n", MAX_PROCESSORS);
+	printf("\t-c, --chip=<0-%d>|<range>|<list>\n", MAX_CHIPS);
+	printf("\t-t, --thread=<0-%d>|<range>|<list>\n", MAX_THREADS);
 	printf("\t-a, --all\n");
 	printf("\t\tRun command on all possible processors/chips/threads (default)\n");
 	printf("\t-b, --backend=backend\n");
@@ -166,11 +166,72 @@  static void print_usage(char *pname)
 	}
 }
 
+/* Parse argument of the form 0-5,7,9-11,15,17 */
+static bool parse_list(const char *arg, int max, int *list, int *count)
+{
+	char str[strlen(arg)+1];
+	char *tok, *tmp, *saveptr = NULL;
+	int i, n;
+
+	strcpy(str, arg);
+
+	for (i=0; i<max; i++) {
+		list[i] = 0;
+	}
+
+	tmp = str;
+	while ((tok = strtok_r(tmp, ",", &saveptr)) != NULL) {
+		char *a, *b, *saveptr2 = NULL;
+		int from, to;
+
+		a = strtok_r(tok, "-", &saveptr2);
+		if (a == NULL) {
+			return false;
+		} else {
+			from = atoi(a);
+			if (from >= max) {
+				return false;
+			}
+		}
+
+		b = strtok_r(NULL, "-", &saveptr2);
+		if (b == NULL) {
+			to = from;
+		} else {
+			to = atoi(b);
+			if (to >= max) {
+				return false;
+			}
+		}
+
+		if (from > to)
+			return false;
+
+		for (i=from; i<=to; i++)
+			list[i] = 1;
+
+		tmp = NULL;
+	};
+
+	n = 0;
+	for (i=0; i<max; i++) {
+		if (list[i] == 1)
+			n++;
+	}
+
+	*count = n;
+	return true;
+}
+
 static bool parse_options(int argc, char *argv[])
 {
 	int c;
 	bool opt_error = true;
-	static int current_processor = INT_MAX, current_chip = INT_MAX, current_thread = INT_MAX;
+	int p_list[MAX_PROCESSORS];
+	int c_list[MAX_CHIPS];
+	int t_list[MAX_THREADS];
+	int p_count = 0, c_count = 0, t_count = 0;
+	int i, j, k;
 	struct option long_opts[] = {
 		{"all",			no_argument,		NULL,	'a'},
 		{"backend",		required_argument,	NULL,	'b'},
@@ -195,50 +256,45 @@  static bool parse_options(int argc, char *argv[])
 		switch(c) {
 		case 'a':
 			opt_error = false;
-			for (current_processor = 0; current_processor < MAX_PROCESSORS; current_processor++) {
-				processorsel[current_processor] = &chipsel[current_processor][0];
-				for (current_chip = 0; current_chip < MAX_CHIPS; current_chip++) {
-					chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
-					for (current_thread = 0; current_thread < MAX_THREADS; current_thread++)
-						threadsel[current_processor][current_chip][current_thread] = 1;
-				}
+
+			if (p_count == 0) {
+				p_count = MAX_PROCESSORS;
+				for (i=0; i<MAX_PROCESSORS; i++)
+					p_list[i] = 1;
+			}
+
+			if (c_count == 0) {
+				c_count = MAX_CHIPS;
+				for (i=0; i<MAX_CHIPS; i++)
+					c_list[i] = 1;
+			}
+
+			if (t_count == 0) {
+				t_count = MAX_THREADS;
+				for (i=0; i<MAX_THREADS; i++)
+					t_list[i] = 1;
 			}
 			break;
 
 		case 'p':
-			errno = 0;
-			current_processor = strtoul(optarg, &endptr, 0);
-			opt_error = (errno || *endptr != '\0');
-			if (!opt_error) {
-				if (current_processor >= MAX_PROCESSORS)
-					opt_error = true;
-				else
-					processorsel[current_processor] = &chipsel[current_processor][0];
-			}
+			if (!parse_list(optarg, MAX_PROCESSORS, p_list, &p_count))
+				fprintf(stderr, "Failed to parse '-p %s'\n", optarg);
+			else
+				opt_error = false;
 			break;
 
 		case 'c':
-			errno = 0;
-			current_chip = strtoul(optarg, &endptr, 0);
-			opt_error = (errno || *endptr != '\0');
-			if (!opt_error) {
-				if (current_chip >= MAX_CHIPS)
-					opt_error = true;
-				else
-					chipsel[current_processor][current_chip] = &threadsel[current_processor][current_chip][0];
-			}
+			if (!parse_list(optarg, MAX_CHIPS, c_list, &c_count))
+				fprintf(stderr, "Failed to parse '-c %s'\n", optarg);
+			else
+				opt_error = false;
 			break;
 
 		case 't':
-			errno = 0;
-			current_thread = strtoul(optarg, &endptr, 0);
-			opt_error = (errno || *endptr != '\0');
-			if (!opt_error) {
-				if (current_thread >= MAX_THREADS)
-					opt_error = true;
-				else
-					threadsel[current_processor][current_chip][current_thread] = 1;
-			}
+			if (!parse_list(optarg, MAX_THREADS, t_list, &t_count))
+				fprintf(stderr, "Failed to parse '-t %s'\n", optarg);
+			else
+				opt_error = false;
 			break;
 
 		case 'b':
@@ -292,10 +348,45 @@  static bool parse_options(int argc, char *argv[])
 		}
 	} while (c != EOF && !opt_error);
 
-	if (opt_error)
+	if (opt_error) {
 		print_usage(basename(argv[0]));
+		return false;
+	}
+
+	if ((c_count > 0 || t_count > 0) && p_count == 0) {
+		fprintf(stderr, "No processor(s) selected\n");
+		fprintf(stderr, "Use -p or -a to select processor(s)\n");
+		return false;
+	}
+
+	if (t_count > 0 && c_count == 0)  {
+		fprintf(stderr, "No chip(s) selected\n");
+		fprintf(stderr, "Use -c or -a to select chip(s)\n");
+		return false;
+	}
+
+	for (i=0; i<MAX_PROCESSORS; i++) {
+		if (p_list[i] == 0)
+			continue;
+
+		processorsel[i] = &chipsel[i][0];
+
+		for (j=0; j<MAX_CHIPS; j++) {
+			if (c_list[j] == 0)
+				continue;
+
+			chipsel[i][j] = &threadsel[i][j][0];
+
+			for (k=0; k<MAX_THREADS; k++) {
+				if (t_list[k] == 0)
+					continue;
+
+				threadsel[i][j][k] = 1;
+			}
+		}
+	}
 
-	return !opt_error;
+	return true;
 }
 
 void target_select(struct pdbg_target *target)