[Pdbg] [PATCH v2 8/9] main: Overhaul target selection logic
Amitay Isaacs
amitay at ozlabs.org
Thu May 24 12:19:40 AEST 2018
On Thu, 2018-05-24 at 11:35 +1000, Balbir Singh wrote:
> On Wed, May 23, 2018 at 2:31 PM, Amitay Isaacs <amitay at ozlabs.org>
> wrote:
> > This should now fix the logic for -a working in conjunction with
> > -p/-c/-t to select all threads.
> >
> > -a select all threads
> > -a -p 0 select all threads on processor 0
> > -p 0 -a select all threads on processor 0
> > -p 0 -c 1 -a select all threads on processor 0, chip 1
> >
> > Signed-off-by: Amitay Isaacs <amitay at ozlabs.org>
> > ---
> > src/main.c | 89 ++++++++++++++++++++++++++++++++++++++++++------
> > ------
> > 1 file changed, 69 insertions(+), 20 deletions(-)
> >
> > diff --git a/src/main.c b/src/main.c
> > index 65bf6c5..71f82fe 100644
> > --- a/src/main.c
> > +++ b/src/main.c
> > @@ -135,7 +135,7 @@ static void print_usage(char *pname)
> > printf("\t-c, --chip=<0-%d>\n", MAX_CHIPS-1);
> > printf("\t-t, --thread=<0-%d>\n", MAX_THREADS-1);
> > printf("\t-a, --all\n");
> > - printf("\t\tRun command on all possible
> > processors/chips/threads (default)\n");
> > + printf("\t\tRun command on all possible
> > processors/chips/threads\n");
> > printf("\t-b, --backend=backend\n");
> > printf("\t\tfsi:\tAn experimental backend that uses\n");
> > printf("\t\t\tbit-banging to access the host processor\n");
> > @@ -170,7 +170,9 @@ 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;
> > + static int start_p = INT_MAX, start_c = INT_MAX, start_t =
> > INT_MAX;
> > + static int end_p = INT_MAX, end_c = INT_MAX, end_t =
> > INT_MAX;
>
> This is a bit confusing, why is start_p INT_MAX, effectively we need
> a
> range, why does the range
> start at INT_MAX and not 0, we need to reset it to 0 down below?
This is undefined value. If there is no -p/-c/-t given then it's set
to INT_MAX. This is to know if any options were given or not.
>
> > + static int cur_p, cur_c, cur_t;
> > struct option long_opts[] = {
> > {"all", no_argument, NUL
> > L, 'a'},
> > {"backend", required_argument, NUL
> > L, 'b'},
> > @@ -195,49 +197,53 @@ 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_p
> > rocessor][current_chip][current_thread] = 1;
> > - }
> > + if (start_p == INT_MAX) {
> > + start_p = 0;
> > + end_p = MAX_PROCESSORS-1;
> > + }
> > + if (start_c == INT_MAX) {
> > + start_c = 0;
> > + end_c = MAX_CHIPS-1;
> > + }
> > + if (start_t == INT_MAX) {
> > + start_t = 0;
> > + end_t = MAX_THREADS-1;
>
> Why not make these the starting default values above?
>
> > }
> > break;
> >
> > case 'p':
> > errno = 0;
> > - current_processor = strtoul(optarg,
> > &endptr, 0);
> > + cur_p = strtoul(optarg, &endptr, 0);
> > opt_error = (errno || *endptr != '\0');
> > if (!opt_error) {
> > - if (current_processor >=
> > MAX_PROCESSORS)
> > + if (cur_p >= MAX_PROCESSORS)
> > opt_error = true;
> > else
> > - processorsel[current_proces
> > sor] = &chipsel[current_processor][0];
> > + start_p = end_p = cur_p;
> > }
> > break;
> >
> > case 'c':
> > errno = 0;
> > - current_chip = strtoul(optarg, &endptr, 0);
> > + cur_c = strtoul(optarg, &endptr, 0);
> > opt_error = (errno || *endptr != '\0');
> > if (!opt_error) {
> > - if (current_chip >= MAX_CHIPS)
> > + if (cur_c >= MAX_CHIPS)
> > opt_error = true;
> > else
> > - chipsel[current_processor][
> > current_chip] = &threadsel[current_processor][current_chip][0];
> > + start_c = end_c = cur_c;
> > }
> > break;
> >
> > case 't':
> > errno = 0;
> > - current_thread = strtoul(optarg, &endptr,
> > 0);
> > + cur_t = strtoul(optarg, &endptr, 0);
> > opt_error = (errno || *endptr != '\0');
> > if (!opt_error) {
> > - if (current_thread >= MAX_THREADS)
> > + if (cur_t >= MAX_THREADS)
> > opt_error = true;
> > else
> > - threadsel[current_processor
> > ][current_chip][current_thread] = 1;
> > + start_t = end_t = cur_t;
> > }
> > break;
> >
> > @@ -292,10 +298,53 @@ static bool parse_options(int argc, char
> > *argv[])
> > }
> > } while (c != EOF && !opt_error);
> >
> > - if (opt_error)
> > + if (opt_error) {
> > print_usage(argv[0]);
> > + return opt_error;
> > + }
> > +
> > + if (start_c != INT_MAX && start_p == INT_MAX) {
> > + printf("No processor(s) selected\n");
> > + printf("Use -p to select processor(s)\n");
>
> These should all go to stderr? I think this can be simplified and
> needs to
> be revisited. I think this patch also subtly breaks other things, in
> the past we could do
>
> pdbg -c0 -t 0 -t 1, now thats broken, IIUC
Ah, yes.. I didn't consider selection of multiple (not all) targets.
Amitay.
--
When you have been wronged, a poor memory is your best response.
More information about the Pdbg
mailing list